Implementation of the new v2 garbage collector

pkg/gc/config.go

@@ -55,7 +55,7 @@ type Config struct {
 	// and exempt from GC.Note that GCMaxStaleRevision may override this if set.
 	// Set Disabled (-1) to disable/ignore duration and always consider active.
 	RetainSinceLastActiveTime time.Duration
-	// Minimum number of stale revisions to keep before considering for GC.
+	// Minimum number of non-active revisions to keep before considering for GC.
 	MinNonActiveRevisions int64
 	// Maximum number of non-active revisions to keep before considering for GC.
 	// regardless of creation or staleness time-bounds.

pkg/reconciler/gc/reconciler_test.go

@@ -36,6 +36,7 @@ import (
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
 	servingclient "knative.dev/serving/pkg/client/injection/client/fake"
 	configreconciler "knative.dev/serving/pkg/client/injection/reconciler/serving/v1/configuration"
+	"knative.dev/serving/pkg/gc"
 	gcconfig "knative.dev/serving/pkg/gc"
 	"knative.dev/serving/pkg/reconciler/configuration/resources"
 	"knative.dev/serving/pkg/reconciler/gc/config"
@@ -69,9 +70,16 @@ func TestGCReconcile(t *testing.T) {
 		ConfigStore: &testConfigStore{
 			config: &config.Config{
 				RevisionGC: &gcconfig.Config{
+					// v1 settings
 					StaleRevisionCreateDelay:        5 * time.Minute,
 					StaleRevisionTimeout:            5 * time.Minute,
 					StaleRevisionMinimumGenerations: 2,
+
+					// v2 settings
+					RetainSinceCreateTime:     5 * time.Minute,
+					RetainSinceLastActiveTime: 5 * time.Minute,
+					MinNonActiveRevisions:     1,
+					MaxNonActiveRevisions:     gc.Disabled,
 				},
 			},
 		}}
@@ -86,16 +94,16 @@ func TestGCReconcile(t *testing.T) {
 				WithConfigObservedGen),
 			rev("keep-two", "foo", 5554, MarkRevisionReady,
 				WithRevName("5554"),
-				WithCreationTimestamp(oldest),
-				WithLastPinned(oldest)),
+				WithRoutingState(v1.RoutingStateReserve),
+				WithRoutingStateModified(oldest)),
 			rev("keep-two", "foo", 5555, MarkRevisionReady,
 				WithRevName("5555"),
-				WithCreationTimestamp(older),
-				WithLastPinned(older)),
+				WithRoutingState(v1.RoutingStateReserve),
+				WithRoutingStateModified(older)),
 			rev("keep-two", "foo", 5556, MarkRevisionReady,
 				WithRevName("5556"),
-				WithCreationTimestamp(old),
-				WithLastPinned(old)),
+				WithRoutingState(v1.RoutingStateActive),
+				WithRoutingStateModified(old)),
 		},
 		WantDeletes: []clientgotesting.DeleteActionImpl{{
 			ActionImpl: clientgotesting.ActionImpl{

pkg/reconciler/gc/v2/gc.go

@@ -31,6 +31,7 @@ import (
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
 	clientset "knative.dev/serving/pkg/client/clientset/versioned"
 	listers "knative.dev/serving/pkg/client/listers/serving/v1"
+	"knative.dev/serving/pkg/gc"
 	configns "knative.dev/serving/pkg/reconciler/gc/config"
 )
 
@@ -49,63 +50,98 @@ func Collect(
 		return err
 	}
 
-	gcSkipOffset := cfg.StaleRevisionMinimumGenerations
-
-	if gcSkipOffset >= int64(len(revs)) {
+	min, max := int(cfg.MinNonActiveRevisions), int(cfg.MaxNonActiveRevisions)
+	if len(revs) <= min ||
+		max == gc.Disabled && cfg.RetainSinceCreateTime == gc.Disabled && cfg.RetainSinceLastActiveTime == gc.Disabled {
 		return nil
 	}
 
-	// Sort by last active descending
+	// Filter out active revs
+	revs = nonactiveRevisions(revs, config)
+
+	// Sort by last active ascending (oldest first)
 	sort.Slice(revs, func(i, j int) bool {
 		a, b := revisionLastActiveTime(revs[i]), revisionLastActiveTime(revs[j])
-		return a.After(b)
+		return a.Before(b)
 	})
 
-	for _, rev := range revs[gcSkipOffset:] {
-		if strings.EqualFold(rev.ObjectMeta.Annotations[serving.RevisionPreservedAnnotationKey], "true") {
+	// Delete stale revisions while more than min remain
+	remaining := len(revs)
+	nonstale := make([]*v1.Revision, 0, remaining)
+	for _, rev := range revs {
+		switch {
+		case remaining <= min:
+			return nil
+
+		case !isRevisionStale(cfg, rev, logger):
+			nonstale = append(nonstale, rev)
 			continue
-		}
-		if isRevisionStale(ctx, rev, config) {
-			err := client.ServingV1().Revisions(rev.Namespace).Delete(rev.Name, &metav1.DeleteOptions{})
-			if err != nil {
-				logger.With(zap.Error(err)).Errorf("Failed to delete stale revision %q", rev.Name)
-				continue
+
+		default:
+			remaining--
+			logger.Info("Deleting stale revision: ", rev.ObjectMeta.Name)
+			if err := client.ServingV1().Revisions(rev.Namespace).Delete(rev.Name, &metav1.DeleteOptions{}); err != nil {
+				logger.With(zap.Error(err)).Error("Failed to GC revision: ", rev.Name)
 			}
 		}
 	}
+
+	if max == gc.Disabled || len(nonstale) <= max {
+		return nil
+	}
+
+	// Delete extra revisions past max
+	for _, rev := range nonstale[:len(nonstale)-max] {
+		logger.Infof("Maximum(%d) reached. Deleting oldest non-active revision %q", max, rev.ObjectMeta.Name)
+		if err := client.ServingV1().Revisions(rev.Namespace).Delete(rev.Name, &metav1.DeleteOptions{}); err != nil {
+			logger.With(zap.Error(err)).Error("Failed to GC revision: ", rev.Name)
+		}
+	}
 	return nil
 }
 
-func isRevisionStale(ctx context.Context, rev *v1.Revision, config *v1.Configuration) bool {
+func nonactiveRevisions(revs []*v1.Revision, config *v1.Configuration) []*v1.Revision {
+	nonactive := make([]*v1.Revision, 0, len(revs))
+	for _, rev := range revs {
+		if !isRevisionActive(rev, config) {
+			nonactive = append(nonactive, rev)
+		}
+	}
+	return nonactive
+}
+
+func isRevisionActive(rev *v1.Revision, config *v1.Configuration) bool {
 	if config.Status.LatestReadyRevisionName == rev.Name {
-		return false
+		return true // never delete latest ready, even if config is not active.
 	}
-	cfg := configns.FromContext(ctx).RevisionGC
-	logger := logging.FromContext(ctx)
-	curTime := time.Now()
-	createTime := rev.ObjectMeta.CreationTimestamp
 
-	if createTime.Add(cfg.StaleRevisionCreateDelay).After(curTime) {
-		// Revision was created sooner than staleRevisionCreateDelay. Ignore it.
-		return false
+	if strings.EqualFold(rev.Annotations[serving.RevisionPreservedAnnotationKey], "true") {
+		return true
 	}
+	// Anything that the labeler hasn't explicitly labelled as inactive.
+	// Revisions which do not yet have any annotation are not eligible for deletion.
+	return rev.GetRoutingState() != v1.RoutingStateReserve
+}
 
-	lastActive := revisionLastActiveTime(rev)
+func isRevisionStale(cfg *gc.Config, rev *v1.Revision, logger *zap.SugaredLogger) bool {
+	sinceCreate, sinceActive := cfg.RetainSinceCreateTime, cfg.RetainSinceLastActiveTime
+	if sinceCreate == gc.Disabled && sinceActive == gc.Disabled {
+		return false // Time checks are both disabled. Not stale.
+	}
 
-	// TODO(whaught): this is carried over from v1, but I'm not sure why we can't delete a ready revision
-	// that isn't referenced? Maybe because of labeler failure - can we replace this with 'pending' routing state check?
-	if lastActive.Equal(createTime.Time) {
-		// Revision was never active and it's not ready after staleRevisionCreateDelay.
-		// It usually happens when ksvc was deployed with wrong configuration.
-		return !rev.Status.GetCondition(v1.RevisionConditionReady).IsTrue()
+	createTime := rev.ObjectMeta.CreationTimestamp.Time
+	if sinceCreate != gc.Disabled && time.Since(createTime) < sinceCreate {
+		return false // Revision was created sooner than RetainSinceCreateTime. Not stale.
 	}
 
-	ret := lastActive.Add(cfg.StaleRevisionTimeout).Before(curTime)
-	if ret {
-		logger.Infof("Detected stale revision %v with creation time %v and last active time %v.",
-			rev.ObjectMeta.Name, rev.ObjectMeta.CreationTimestamp, lastActive)
+	active := revisionLastActiveTime(rev)
+	if sinceActive != gc.Disabled && time.Since(active) < sinceActive {
+		return false // Revision was recently active. Not stale.
 	}
-	return ret
+
+	logger.Infof("Detected stale revision %q with creation time %v and last active time %v.",
+		rev.ObjectMeta.Name, createTime, active)
+	return true
 }
 
 // revisionLastActiveTime returns if present: