Delete old GC

config/core/configmaps/gc.yaml

@@ -20,7 +20,7 @@ metadata:
   labels:
     serving.knative.dev/release: devel
   annotations:
-    knative.dev/example-checksum: "4b89cfa0"
+    knative.dev/example-checksum: "e6149382"
 data:
   _example: |
     ################################
@@ -38,31 +38,11 @@ data:
     # this example block and unindented to be in the data block
     # to actually change the configuration.
 
-    # Delay after revision creation before considering it for GC
-    stale-revision-create-delay: "48h"
-
-    # Duration since a route has pointed at the revision before it
-    # should be GC'd.
-    # This minus lastpinned-debounce must be longer than the controller
-    # resync period (10 hours).
-    stale-revision-timeout: "15h"
-
-    # Minimum number of generations of non-active revisions to keep before
-    # considering them for GC.
-    stale-revision-minimum-generations: "20"
-
-    # To avoid constant updates, we allow an existing annotation to be stale by this
-    # amount before we update the timestamp.
-    stale-revision-lastpinned-debounce: "5h"
-
 
     # ---------------------------------------
-    # V2 Garbage Collector Settings
+    # Garbage Collector Settings
     # ---------------------------------------
     #
-    # These settings are enabled via the "responsive-revision-gc" feature flag.
-    # ALPHA NOTE: This feature is still experimental and under active development.
-    #
     # Active
     #   * Revisions which are referenced by a Route are considered active.
     #   * Individual revisions may be marked with the annotation

pkg/apis/config/features.go

@@ -49,7 +49,6 @@ func defaultFeaturesConfig() *Features {
 		PodSpecRuntimeClassName: Disabled,
 		PodSpecSecurityContext:  Disabled,
 		PodSpecTolerations:      Disabled,
-		ResponsiveRevisionGC:    Enabled,
 		TagHeaderBasedRouting:   Disabled,
 	}
 }
@@ -67,7 +66,6 @@ func NewFeaturesConfigFromMap(data map[string]string) (*Features, error) {
 		asFlag("kubernetes.podspec-runtimeclassname", &nc.PodSpecRuntimeClassName),
 		asFlag("kubernetes.podspec-securitycontext", &nc.PodSpecSecurityContext),
 		asFlag("kubernetes.podspec-tolerations", &nc.PodSpecTolerations),
-		asFlag("responsive-revision-gc", &nc.ResponsiveRevisionGC),
 		asFlag("tag-header-based-routing", &nc.TagHeaderBasedRouting)); err != nil {
 		return nil, err
 	}
@@ -89,7 +87,6 @@ type Features struct {
 	PodSpecRuntimeClassName Flag
 	PodSpecSecurityContext  Flag
 	PodSpecTolerations      Flag
-	ResponsiveRevisionGC    Flag
 	TagHeaderBasedRouting   Flag
 }
 

pkg/apis/config/features_test.go

@@ -66,7 +66,6 @@ func TestFeaturesConfiguration(t *testing.T) {
 			PodSpecRuntimeClassName: Enabled,
 			PodSpecSecurityContext:  Enabled,
 			PodSpecTolerations:      Enabled,
-			ResponsiveRevisionGC:    Enabled,
 			TagHeaderBasedRouting:   Enabled,
 		}),
 		data: map[string]string{
@@ -242,24 +241,6 @@ func TestFeaturesConfiguration(t *testing.T) {
 		data: map[string]string{
 			"kubernetes.podspec-tolerations": "Disabled",
 		},
-	}, {
-		name:    "responsive-revision-gc Allowed",
-		wantErr: false,
-		wantFeatures: defaultWith(&Features{
-			ResponsiveRevisionGC: Allowed,
-		}),
-		data: map[string]string{
-			"responsive-revision-gc": "Allowed",
-		},
-	}, {
-		name:    "responsive-revision-gc Enabled",
-		wantErr: false,
-		wantFeatures: defaultWith(&Features{
-			ResponsiveRevisionGC: Enabled,
-		}),
-		data: map[string]string{
-			"responsive-revision-gc": "Enabled",
-		},
 	}, {
 		name:    "security context Allowed",
 		wantErr: false,

pkg/apis/serving/v1/revision_helpers.go

@@ -17,8 +17,6 @@ limitations under the License.
 package v1
 
 import (
-	"fmt"
-	"strconv"
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
@@ -91,16 +89,8 @@ type (
 	}
 
 	// +k8s:deepcopy-gen=false
-
-	// LastPinnedParseError is the error returned when the lastPinned annotation
-	// could not be parsed.
-	LastPinnedParseError AnnotationParseError
 )
 
-func (e LastPinnedParseError) Error() string {
-	return fmt.Sprintf("%v lastPinned value: %q", e.Type, e.Value)
-}
-
 // GetContainer returns a pointer to the relevant corev1.Container field.
 // It is never nil and should be exactly the specified container if len(containers) == 1 or
 // if there are multiple containers it returns the container which has Ports
@@ -163,8 +153,7 @@ func (r *Revision) GetRoutingStateModified() time.Time {
 
 // IsReachable returns whether or not the revision can be reached by a route.
 func (r *Revision) IsReachable() bool {
-	return r.Labels[serving.RouteLabelKey] != "" ||
-		RoutingState(r.Labels[serving.RoutingStateLabelKey]) == RoutingStateActive
+	return RoutingState(r.Labels[serving.RoutingStateLabelKey]) == RoutingStateActive
 }
 
 // GetProtocol returns the app level network protocol.
@@ -183,53 +172,10 @@ func (r *Revision) GetProtocol() (p net.ProtocolType) {
 	return
 }
 
-// SetLastPinned sets the revision's last pinned annotations
-// to be the specified time.
-func (r *Revision) SetLastPinned(t time.Time) {
-	if r.Annotations == nil {
-		r.Annotations = make(map[string]string, 1)
-	}
-
-	r.Annotations[serving.RevisionLastPinnedAnnotationKey] = RevisionLastPinnedString(t)
-}
-
-// GetLastPinned returns the time the revision was last pinned.
-func (r *Revision) GetLastPinned() (time.Time, error) {
-	if r.Annotations == nil {
-		return time.Time{}, LastPinnedParseError{
-			Type: AnnotationParseErrorTypeMissing,
-		}
-	}
-
-	str, ok := r.Annotations[serving.RevisionLastPinnedAnnotationKey]
-	if !ok {
-		// If a revision is past the create delay without an annotation it is stale.
-		return time.Time{}, LastPinnedParseError{
-			Type: AnnotationParseErrorTypeMissing,
-		}
-	}
-
-	secs, err := strconv.ParseInt(str, 10, 64)
-	if err != nil {
-		return time.Time{}, LastPinnedParseError{
-			Type:  AnnotationParseErrorTypeInvalid,
-			Value: str,
-			Err:   err,
-		}
-	}
-
-	return time.Unix(secs, 0), nil
-}
-
 // IsActivationRequired returns true if activation is required.
 func (rs *RevisionStatus) IsActivationRequired() bool {
 	if c := revisionCondSet.Manage(rs).GetCondition(RevisionConditionActive); c != nil {
 		return c.Status != corev1.ConditionTrue
 	}
 	return false
 }
-
-// RevisionLastPinnedString returns a string representation of the specified time.
-func RevisionLastPinnedString(t time.Time) string {
-	return fmt.Sprint(t.Unix())
-}

pkg/apis/serving/v1/revision_helpers_test.go

@@ -106,92 +106,18 @@ func TestIsActivationRequired(t *testing.T) {
 	}
 }
 
-func TestRevisionGetLastPinned(t *testing.T) {
-	cases := []struct {
-		name              string
-		annotations       map[string]string
-		expectTime        time.Time
-		setLastPinnedTime time.Time
-		expectErr         error
-	}{{
-		name: "Nil annotations",
-		expectErr: LastPinnedParseError{
-			Type: AnnotationParseErrorTypeMissing,
-		},
-	}, {
-		name:        "Empty map annotations",
-		annotations: map[string]string{},
-		expectErr: LastPinnedParseError{
-			Type: AnnotationParseErrorTypeMissing,
-		},
-	}, {
-		name:              "Empty map annotations - with set time",
-		annotations:       map[string]string{},
-		setLastPinnedTime: time.Unix(1000, 0),
-		expectTime:        time.Unix(1000, 0),
-	}, {
-		name:        "Invalid time",
-		annotations: map[string]string{serving.RevisionLastPinnedAnnotationKey: "abcd"},
-		expectErr: LastPinnedParseError{
-			Type:  AnnotationParseErrorTypeInvalid,
-			Value: "abcd",
-		},
-	}, {
-		name:        "Valid time",
-		annotations: map[string]string{serving.RevisionLastPinnedAnnotationKey: "10000"},
-		expectTime:  time.Unix(10000, 0),
-	}, {
-		name:              "Valid time empty annotations",
-		setLastPinnedTime: time.Unix(1000, 0),
-		expectTime:        time.Unix(1000, 0),
-	}}
-
-	for _, tc := range cases {
-		t.Run(tc.name, func(t *testing.T) {
-			rev := Revision{
-				ObjectMeta: metav1.ObjectMeta{
-					Annotations: tc.annotations,
-				},
-			}
-
-			if tc.setLastPinnedTime != (time.Time{}) {
-				rev.SetLastPinned(tc.setLastPinnedTime)
-			}
-
-			pt, err := rev.GetLastPinned()
-			failErr := func() {
-				t.Fatalf("Expected error %v got %v", tc.expectErr, err)
-			}
-
-			if tc.expectErr == nil {
-				if err != nil {
-					failErr()
-				}
-			} else {
-				if tc.expectErr.Error() != err.Error() {
-					failErr()
-				}
-			}
-
-			if tc.expectTime != pt {
-				t.Fatalf("Expected pin time %v got %v", tc.expectTime, pt)
-			}
-		})
-	}
-}
-
 func TestRevisionIsReachable(t *testing.T) {
 	tests := []struct {
 		name   string
 		labels map[string]string
 		want   bool
 	}{{
-		name:   "has route annotation",
-		labels: map[string]string{serving.RouteLabelKey: "the-route"},
+		name:   "has serving state label",
+		labels: map[string]string{serving.RoutingStateLabelKey: "active"},
 		want:   true,
 	}, {
 		name:   "empty route annotation",
-		labels: map[string]string{serving.RouteLabelKey: ""},
+		labels: map[string]string{serving.RoutingStateLabelKey: ""},
 		want:   false,
 	}, {
 		name:   "no route annotation",

pkg/gc/config.go

@@ -25,8 +25,6 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	cm "knative.dev/pkg/configmap"
-	"knative.dev/pkg/controller"
-	"knative.dev/pkg/logging"
 )
 
 const (
@@ -42,16 +40,6 @@ const (
 
 // Config defines the tunable parameters for Garbage Collection.
 type Config struct {
-	// Delay duration after a revision create before considering it for GC
-	StaleRevisionCreateDelay time.Duration
-	// Timeout since a revision lastPinned before it should be GC'd
-	// This must be longer than the controller resync period
-	StaleRevisionTimeout time.Duration
-	// Minimum number of generations of revisions to keep before considering for GC.
-	StaleRevisionMinimumGenerations int64
-	// Minimum staleness duration before updating lastPinned
-	StaleRevisionLastpinnedDebounce time.Duration
-
 	// Duration from creation when a Revision should be considered active
 	// and exempt from GC. Note that GCMaxStaleRevision may override this if set.
 	// Set Disabled (-1) to disable/ignore duration and always consider active.
@@ -70,13 +58,6 @@ type Config struct {
 
 func defaultConfig() *Config {
 	return &Config{
-		// V1 GC Settings
-		StaleRevisionCreateDelay:        48 * time.Hour,
-		StaleRevisionTimeout:            15 * time.Hour,
-		StaleRevisionLastpinnedDebounce: 5 * time.Hour,
-		StaleRevisionMinimumGenerations: 20,
-
-		// V2 GC Settings
 		RetainSinceCreateTime:     48 * time.Hour,
 		RetainSinceLastActiveTime: 15 * time.Hour,
 		MinNonActiveRevisions:     20,
@@ -86,19 +67,11 @@ func defaultConfig() *Config {
 
 // NewConfigFromConfigMapFunc creates a Config from the supplied ConfigMap func.
 func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.ConfigMap) (*Config, error) {
-	logger := logging.FromContext(ctx)
-	minRevisionTimeout := controller.GetResyncPeriod(ctx)
 	return func(configMap *corev1.ConfigMap) (*Config, error) {
 		c := defaultConfig()
 
 		var retainCreate, retainActive, max string
 		if err := cm.Parse(configMap.Data,
-			cm.AsDuration("stale-revision-create-delay", &c.StaleRevisionCreateDelay),
-			cm.AsDuration("stale-revision-timeout", &c.StaleRevisionTimeout),
-			cm.AsDuration("stale-revision-lastpinned-debounce", &c.StaleRevisionLastpinnedDebounce),
-			cm.AsInt64("stale-revision-minimum-generations", &c.StaleRevisionMinimumGenerations),
-
-			// v2 settings
 			cm.AsString("retain-since-create-time", &retainCreate),
 			cm.AsString("retain-since-last-active-time", &retainActive),
 			cm.AsInt64("min-non-active-revisions", &c.MinNonActiveRevisions),
@@ -107,17 +80,6 @@ func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.Conf
 			return nil, fmt.Errorf("failed to parse data: %w", err)
 		}
 
-		if c.StaleRevisionMinimumGenerations < 0 {
-			return nil, fmt.Errorf(
-				"stale-revision-minimum-generations must be non-negative, was: %d",
-				c.StaleRevisionMinimumGenerations)
-		}
-		if c.StaleRevisionTimeout-c.StaleRevisionLastpinnedDebounce < minRevisionTimeout {
-			logger.Warnf("Got revision timeout of %v, minimum supported value is %v",
-				c.StaleRevisionTimeout, minRevisionTimeout+c.StaleRevisionLastpinnedDebounce)
-			c.StaleRevisionTimeout = minRevisionTimeout + c.StaleRevisionLastpinnedDebounce
-		}
-
 		// validate V2 settings
 		if err := parseDisabledOrDuration(retainCreate, &c.RetainSinceCreateTime); err != nil {
 			return nil, fmt.Errorf("failed to parse retain-since-create-time: %w", err)
@@ -126,13 +88,13 @@ func NewConfigFromConfigMapFunc(ctx context.Context) func(configMap *corev1.Conf
 			return nil, fmt.Errorf("failed to parse retain-since-last-active-time: %w", err)
 		}
 		if err := parseDisabledOrInt64(max, &c.MaxNonActiveRevisions); err != nil {
-			return nil, fmt.Errorf("failed to parse max-stale-revisions: %w", err)
+			return nil, fmt.Errorf("failed to parse max-non-active-revisions: %w", err)
 		}
 		if c.MinNonActiveRevisions < 0 {
 			return nil, fmt.Errorf("min-non-active-revisions must be non-negative, was: %d", c.MinNonActiveRevisions)
 		}
 		if c.MaxNonActiveRevisions >= 0 && c.MinNonActiveRevisions > c.MaxNonActiveRevisions {
-			return nil, fmt.Errorf("min-non-active-revisions(%d) must be <= max-stale-revisions(%d)", c.MinNonActiveRevisions, c.MaxNonActiveRevisions)
+			return nil, fmt.Errorf("min-non-active-revisions(%d) must be <= max-non-active-revisions(%d)", c.MinNonActiveRevisions, c.MaxNonActiveRevisions)
 		}
 		return c, nil
 	}