diff --git a/pkg/gc/config.go b/pkg/gc/config.go index 72c7a299b2a6..e71326e9fe48 100644 --- a/pkg/gc/config.go +++ b/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. diff --git a/pkg/reconciler/gc/reconciler_test.go b/pkg/reconciler/gc/reconciler_test.go index 83263e24c8b1..826ad1e58a3b 100644 --- a/pkg/reconciler/gc/reconciler_test.go +++ b/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{ diff --git a/pkg/reconciler/gc/v2/gc.go b/pkg/reconciler/gc/v2/gc.go index 2d27262ab5a1..d3d389b696be 100644 --- a/pkg/reconciler/gc/v2/gc.go +++ b/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: diff --git a/pkg/reconciler/gc/v2/gc_test.go b/pkg/reconciler/gc/v2/gc_test.go index f5e417598f4f..37fd099a400b 100644 --- a/pkg/reconciler/gc/v2/gc_test.go +++ b/pkg/reconciler/gc/v2/gc_test.go @@ -40,6 +40,7 @@ import ( _ "knative.dev/serving/pkg/client/injection/informers/serving/v1/configuration/fake" _ "knative.dev/serving/pkg/client/injection/informers/serving/v1/revision/fake" + . "knative.dev/pkg/logging/testing" . "knative.dev/pkg/reconciler/testing" . "knative.dev/serving/pkg/testing/v1" ) @@ -53,24 +54,20 @@ var revisionSpec = v1.RevisionSpec{ TimeoutSeconds: ptr.Int64(60), } -func TestCollect(t *testing.T) { - now := time.Now() - nineMinutesAgo := now.Add(-9 * time.Minute) - tenMinutesAgo := now.Add(-10 * time.Minute) - - old := now.Add(-11 * time.Minute) - older := now.Add(-12 * time.Minute) - oldest := now.Add(-13 * time.Minute) - +func TestCollectMin(t *testing.T) { cfgMap := &config.Config{ RevisionGC: &gcconfig.Config{ - StaleRevisionCreateDelay: 5 * time.Minute, - StaleRevisionTimeout: 5 * time.Minute, - StaleRevisionMinimumGenerations: 2, + RetainSinceCreateTime: 5 * time.Minute, + RetainSinceLastActiveTime: 5 * time.Minute, + MinNonActiveRevisions: 1, + MaxNonActiveRevisions: -1, // assert no changes to min case }, } - ctx, _ := SetupFakeContext(t) - ctx = config.ToContext(ctx, cfgMap) + + now := time.Now() + old := now.Add(-11 * time.Minute) + older := now.Add(-12 * time.Minute) + oldest := now.Add(-13 * time.Minute) table := []struct { name string @@ -78,24 +75,27 @@ func TestCollect(t *testing.T) { revs []*v1.Revision wantDeletes []clientgotesting.DeleteActionImpl }{{ - name: "delete oldest, keep two lastPinned", + name: "delete oldest, keep one recent, one active", cfg: cfg("keep-two", "foo", 5556, WithLatestCreated("5556"), WithLatestReady("5556"), WithConfigObservedGen), revs: []*v1.Revision{ - rev(ctx, "keep-two", "foo", 5554, MarkRevisionReady, + // Stale, oldest should be deleted + rev("keep-two", "foo", 5554, MarkRevisionReady, WithRevName("5554"), - WithCreationTimestamp(oldest), - WithLastPinned(oldest)), - rev(ctx, "keep-two", "foo", 5555, MarkRevisionReady, + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(oldest)), + // Stale, but MinNonActiveRevisions is 1 + rev("keep-two", "foo", 5555, MarkRevisionReady, WithRevName("5555"), - WithCreationTimestamp(older), - WithLastPinned(older)), - rev(ctx, "keep-two", "foo", 5556, MarkRevisionReady, + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(older)), + // Actively referenced by Configuration + rev("keep-two", "foo", 5556, MarkRevisionReady, WithRevName("5556"), - WithCreationTimestamp(old), - WithLastPinned(old)), + WithRoutingState(v1.RoutingStateActive), + WithRoutingStateModified(old)), }, wantDeletes: []clientgotesting.DeleteActionImpl{{ ActionImpl: clientgotesting.ActionImpl{ @@ -110,23 +110,23 @@ func TestCollect(t *testing.T) { Name: "5554", }}, }, { - name: "delete oldest, keep two routingStateModified", - cfg: cfg("keep-two", "foo", 5556, - WithLatestCreated("5556"), - WithLatestReady("5556"), - WithConfigObservedGen), + name: "no latest ready, one active", + cfg: cfg("keep-two", "foo", 5556, WithConfigObservedGen), revs: []*v1.Revision{ - rev(ctx, "keep-two", "foo", 5554, MarkRevisionReady, + // Stale, oldest should be deleted + rev("keep-two", "foo", 5554, MarkRevisionReady, WithRevName("5554"), - WithCreationTimestamp(oldest), + WithRoutingState(v1.RoutingStateReserve), WithRoutingStateModified(oldest)), - rev(ctx, "keep-two", "foo", 5555, MarkRevisionReady, + // Stale, but MinNonActiveRevisions is 1 + rev("keep-two", "foo", 5555, MarkRevisionReady, WithRevName("5555"), - WithCreationTimestamp(older), + WithRoutingState(v1.RoutingStateReserve), WithRoutingStateModified(older)), - rev(ctx, "keep-two", "foo", 5556, MarkRevisionReady, + // Actively referenced by Configuration + rev("keep-two", "foo", 5556, MarkRevisionReady, WithRevName("5556"), - WithCreationTimestamp(old), + WithRoutingState(v1.RoutingStateActive), WithRoutingStateModified(old)), }, wantDeletes: []clientgotesting.DeleteActionImpl{{ @@ -142,107 +142,238 @@ func TestCollect(t *testing.T) { Name: "5554", }}, }, { - name: "keep oldest when no lastPinned", - cfg: cfg("keep-no-last-pinned", "foo", 5556, + name: "keep oldest when none Reserved", + cfg: cfg("none-reserved", "foo", 5556, WithLatestCreated("5556"), WithLatestReady("5556"), WithConfigObservedGen), revs: []*v1.Revision{ - // No lastPinned so we will keep this. - rev(ctx, "keep-no-last-pinned", "foo", 5554, MarkRevisionReady, + rev("none-reserved", "foo", 5554, MarkRevisionReady, WithRevName("5554"), + WithRoutingState(v1.RoutingStatePending), WithCreationTimestamp(oldest)), - rev(ctx, "keep-no-last-pinned", "foo", 5555, MarkRevisionReady, + rev("none-reserved", "foo", 5555, MarkRevisionReady, WithRevName("5555"), - WithCreationTimestamp(oldest), - WithLastPinned(older)), - rev(ctx, "keep-no-last-pinned", "foo", 5556, MarkRevisionReady, + WithRoutingState(v1.RoutingStateUnset), + WithCreationTimestamp(older)), + rev("none-reserved", "foo", 5556, MarkRevisionReady, WithRevName("5556"), - WithCreationTimestamp(oldest), - WithLastPinned(old)), + WithRoutingState(v1.RoutingStateActive), + WithCreationTimestamp(old)), }, }, { - name: "keep recent lastPinned", - cfg: cfg("keep-recent-last-pinned", "foo", 5556, - WithLatestCreated("5556"), - WithLatestReady("5556"), - WithConfigObservedGen), + name: "none stale", + cfg: cfg("none-stale", "foo", 5556, WithConfigObservedGen), revs: []*v1.Revision{ - rev(ctx, "keep-recent-last-pinned", "foo", 5554, MarkRevisionReady, + rev("none-stale", "foo", 5554, MarkRevisionReady, WithRevName("5554"), - WithCreationTimestamp(oldest), - // This is an indication that things are still routing here. - WithLastPinned(now)), - rev(ctx, "keep-recent-last-pinned", "foo", 5555, MarkRevisionReady, + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(now)), + rev("none-stale", "foo", 5555, MarkRevisionReady, WithRevName("5555"), - WithCreationTimestamp(older), - WithLastPinned(nineMinutesAgo)), - rev(ctx, "keep-recent-last-pinned", "foo", 5556, MarkRevisionReady, + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(now)), + rev("none-stale", "foo", 5556, MarkRevisionReady, WithRevName("5556"), - WithCreationTimestamp(old), - WithLastPinned(tenMinutesAgo)), + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(now)), }, }, { - name: "keep LatestReadyRevision", - cfg: cfg("keep-two", "foo", 5556, - WithLatestReady("5554"), - // This comes after 'WithLatestReady' so the - // Configuration's 'Ready' Status is 'Unknown' + name: "keep oldest because of the preserve annotation", + cfg: cfg("keep-oldest", "foo", 5556, WithConfigObservedGen), + revs: []*v1.Revision{ + rev("keep-oldest", "foo", 5554, MarkRevisionReady, + WithRevName("5554"), + WithRoutingStateModified(oldest), + WithRoutingState(v1.RoutingStateReserve), + WithRevisionPreserveAnnotation()), + rev("keep-oldest", "foo", 5555, MarkRevisionReady, + WithRevName("5555"), + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(older)), + rev("keep-oldest", "foo", 5556, MarkRevisionReady, + WithRevName("5556"), + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(old)), + }, + wantDeletes: []clientgotesting.DeleteActionImpl{{ + ActionImpl: clientgotesting.ActionImpl{ + Namespace: "foo", + Verb: "delete", + Resource: schema.GroupVersionResource{ + Group: "serving.knative.dev", + Version: "v1", + Resource: "revisions", + }, + }, + Name: "5555", + }}, + }} + + for _, test := range table { + t.Run(test.name, func(t *testing.T) { + runTest(t, cfgMap, test.revs, test.cfg, test.wantDeletes) + }) + } +} + +func TestCollectMax(t *testing.T) { + cfgMap := &config.Config{ + RevisionGC: &gcconfig.Config{ + RetainSinceCreateTime: 1 * time.Hour, + RetainSinceLastActiveTime: 1 * time.Hour, + MinNonActiveRevisions: 1, + MaxNonActiveRevisions: 2, + }, + } + + now := time.Now() + old := now.Add(-11 * time.Minute) + older := now.Add(-12 * time.Minute) + oldest := now.Add(-13 * time.Minute) + + table := []struct { + name string + cfg *v1.Configuration + revs []*v1.Revision + wantDeletes []clientgotesting.DeleteActionImpl + }{{ + name: "at max", + cfg: cfg("at max", "foo", 5556, WithLatestCreated("5556"), + WithLatestReady("5556"), WithConfigObservedGen), revs: []*v1.Revision{ - // Create a revision where the LatestReady is 5554, but LatestCreated is 5556. - // We should keep LatestReady even if it is old. - rev(ctx, "keep-two", "foo", 5554, MarkRevisionReady, + // Under max + rev("at max", "foo", 5554, MarkRevisionReady, WithRevName("5554"), - WithCreationTimestamp(oldest), - WithLastPinned(oldest)), - rev(ctx, "keep-two", "foo", 5555, // Not Ready + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(older)), + // Under max + rev("at max", "foo", 5555, MarkRevisionReady, WithRevName("5555"), - WithCreationTimestamp(older), - WithLastPinned(older)), - rev(ctx, "keep-two", "foo", 5556, // Not Ready + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(older)), + // Actively referenced by Configuration + rev("at max", "foo", 5556, MarkRevisionReady, WithRevName("5556"), - WithCreationTimestamp(old), - WithLastPinned(old)), + WithRoutingState(v1.RoutingStateActive), + WithRoutingStateModified(old)), }, }, { - name: "keep stale revision because of minimum generations", - cfg: cfg("keep-all", "foo", 5554, - // Don't set the latest ready revision here - // since those by default are always retained - WithLatestCreated("keep-all"), + name: "delete oldest, keep three max", + cfg: cfg("delete oldest", "foo", 5556, + WithLatestCreated("5556"), + WithLatestReady("5556"), WithConfigObservedGen), revs: []*v1.Revision{ - rev(ctx, "keep-all", "foo", 5554, - WithRevName("keep-all"), - WithCreationTimestamp(oldest), - WithLastPinned(old)), + // Stale and over the max + rev("delete oldest", "foo", 5553, MarkRevisionReady, + WithRevName("5553"), + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(oldest)), + // Stale but under max + rev("delete oldest", "foo", 5554, MarkRevisionReady, + WithRevName("5554"), + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(older)), + // Stale but under max + rev("delete oldest", "foo", 5555, MarkRevisionReady, + WithRevName("5555"), + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(older)), + // Actively referenced by Configuration + rev("keep-two", "foo", 5556, MarkRevisionReady, + WithRevName("5556"), + WithRoutingState(v1.RoutingStateActive), + WithRoutingStateModified(old)), }, + wantDeletes: []clientgotesting.DeleteActionImpl{{ + ActionImpl: clientgotesting.ActionImpl{ + Namespace: "foo", + Verb: "delete", + Resource: schema.GroupVersionResource{ + Group: "serving.knative.dev", + Version: "v1", + Resource: "revisions", + }, + }, + Name: "5553", + }}, }, { - name: "keep oldest because of the preserve annotation", - cfg: cfg("keep-oldest", "foo", 5556, + name: "over max, all active", + cfg: cfg("keep-two", "foo", 5556, WithLatestCreated("5556"), WithLatestReady("5556"), WithConfigObservedGen), revs: []*v1.Revision{ - rev(ctx, "keep-oldest", "foo", 5554, MarkRevisionReady, + rev("keep-two", "foo", 5553, MarkRevisionReady, + WithRevName("5553"), + WithRoutingState(v1.RoutingStateActive)), + rev("keep-two", "foo", 5554, MarkRevisionReady, WithRevName("5554"), - WithCreationTimestamp(oldest), - WithLastPinned(oldest), - WithRevisionPreserveAnnotation()), - rev(ctx, "keep-oldest", "foo", 5555, MarkRevisionReady, + WithRoutingState(v1.RoutingStateActive)), + rev("keep-two", "foo", 5555, MarkRevisionReady, WithRevName("5555"), - WithCreationTimestamp(older), - WithLastPinned(older)), - rev(ctx, "keep-oldest", "foo", 5556, MarkRevisionReady, + WithRoutingState(v1.RoutingStateActive)), + rev("keep-two", "foo", 5556, MarkRevisionReady, WithRevName("5556"), - WithCreationTimestamp(old), - WithLastPinned(old)), - rev(ctx, "keep-oldest", "foo", 5557, MarkRevisionReady, - WithRevName("5557"), - WithCreationTimestamp(tenMinutesAgo), - WithLastPinned(tenMinutesAgo)), + WithRoutingState(v1.RoutingStateActive)), + }, + }} + + for _, test := range table { + t.Run(test.name, func(t *testing.T) { + runTest(t, cfgMap, test.revs, test.cfg, test.wantDeletes) + }) + } +} + +func TestCollectSettings(t *testing.T) { + now := time.Now() + old := now.Add(-11 * time.Minute) + older := now.Add(-12 * time.Minute) + oldest := now.Add(-13 * time.Minute) + + cfg := cfg("settings-test", "foo", 5556, + WithLatestCreated("5556"), + WithLatestReady("5556"), + WithConfigObservedGen) + + revs := []*v1.Revision{ + rev("settings-test", "foo", 5554, MarkRevisionReady, + WithRevName("5554"), + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(oldest)), + rev("settings-test", "foo", 5555, MarkRevisionReady, + WithRevName("5555"), + WithRoutingState(v1.RoutingStateReserve), + WithRoutingStateModified(older)), + rev("settings-test", "foo", 5556, MarkRevisionReady, + WithRevName("5556"), + WithRoutingState(v1.RoutingStateActive), + WithRoutingStateModified(old)), + } + + table := []struct { + name string + gcConfig gcconfig.Config + wantDeletes []clientgotesting.DeleteActionImpl + }{{ + name: "all disabled", + gcConfig: gcconfig.Config{ + RetainSinceCreateTime: time.Duration(gcconfig.Disabled), + RetainSinceLastActiveTime: time.Duration(gcconfig.Disabled), + MinNonActiveRevisions: 1, + MaxNonActiveRevisions: gcconfig.Disabled, + }, + }, { + name: "staleness disabled", + gcConfig: gcconfig.Config{ + RetainSinceCreateTime: time.Duration(gcconfig.Disabled), + RetainSinceLastActiveTime: time.Duration(gcconfig.Disabled), + MinNonActiveRevisions: 0, + MaxNonActiveRevisions: 1, }, wantDeletes: []clientgotesting.DeleteActionImpl{{ ActionImpl: clientgotesting.ActionImpl{ @@ -254,46 +385,79 @@ func TestCollect(t *testing.T) { Resource: "revisions", }, }, - Name: "5555", + Name: "5554", + }}, + }, { + name: "max disabled", + gcConfig: gcconfig.Config{ + RetainSinceCreateTime: time.Duration(gcconfig.Disabled), + RetainSinceLastActiveTime: 1 * time.Minute, + MinNonActiveRevisions: 1, + MaxNonActiveRevisions: gcconfig.Disabled, + }, + wantDeletes: []clientgotesting.DeleteActionImpl{{ + ActionImpl: clientgotesting.ActionImpl{ + Namespace: "foo", + Verb: "delete", + Resource: schema.GroupVersionResource{ + Group: "serving.knative.dev", + Version: "v1", + Resource: "revisions", + }, + }, + Name: "5554", }}, }} for _, test := range table { t.Run(test.name, func(t *testing.T) { - ctx, _ = SetupFakeContext(t) - ctx = config.ToContext(ctx, cfgMap) - client := fakeservingclient.Get(ctx) - - ri := fakerevisioninformer.Get(ctx) - for _, rev := range test.revs { - ri.Informer().GetIndexer().Add(rev) + cfgMap := &config.Config{ + RevisionGC: &test.gcConfig, } + runTest(t, cfgMap, revs, cfg, test.wantDeletes) + }) + } +} - recorderList := ActionRecorderList{client} +func runTest( + t *testing.T, + cfgMap *config.Config, + revs []*v1.Revision, + cfg *v1.Configuration, + wantDeletes []clientgotesting.DeleteActionImpl) { + t.Helper() + ctx, _ := SetupFakeContext(t) + ctx = config.ToContext(ctx, cfgMap) + client := fakeservingclient.Get(ctx) - Collect(ctx, client, ri.Lister(), test.cfg) + ri := fakerevisioninformer.Get(ctx) + for _, rev := range revs { + ri.Informer().GetIndexer().Add(rev) + } - actions, err := recorderList.ActionsByVerb() - if err != nil { - t.Errorf("Error capturing actions by verb: %q", err) - } + recorderList := ActionRecorderList{client} - for i, want := range test.wantDeletes { - if i >= len(actions.Deletes) { - t.Errorf("Missing delete: %#v", want) - continue - } - got := actions.Deletes[i] - if got.GetName() != want.GetName() { - t.Errorf("Unexpected delete[%d]: %#v", i, got) - } - } - if got, want := len(actions.Deletes), len(test.wantDeletes); got > want { - for _, extra := range actions.Deletes[want:] { - t.Errorf("Extra delete: %s/%s", extra.GetNamespace(), extra.GetName()) - } - } - }) + Collect(ctx, client, ri.Lister(), cfg) + + actions, err := recorderList.ActionsByVerb() + if err != nil { + t.Errorf("Error capturing actions by verb: %q", err) + } + + for i, want := range wantDeletes { + if i >= len(actions.Deletes) { + t.Errorf("Missing delete: %#v", want) + continue + } + got := actions.Deletes[i] + if got.GetName() != want.GetName() { + t.Errorf("Unexpected delete[%d]: %#v", i, got) + } + } + if got, want := len(actions.Deletes), len(wantDeletes); got > want { + for _, extra := range actions.Deletes[want:] { + t.Errorf("Extra delete: %s/%s", extra.GetNamespace(), extra.GetName()) + } } } @@ -307,16 +471,7 @@ func TestIsRevisionStale(t *testing.T) { latestRev string want bool }{{ - name: "fresh revision that was never pinned", - rev: &v1.Revision{ - ObjectMeta: metav1.ObjectMeta{ - Name: "myrev", - CreationTimestamp: metav1.NewTime(curTime), - }, - }, - want: false, - }, { - name: "stale revision that was never pinned w/ Ready status", + name: "stale create time", rev: &v1.Revision{ ObjectMeta: metav1.ObjectMeta{ Name: "myrev", @@ -326,18 +481,18 @@ func TestIsRevisionStale(t *testing.T) { Status: duckv1.Status{ Conditions: duckv1.Conditions{{ Type: v1.RevisionConditionReady, - Status: "True", + Status: "Unknown", }}, }, }, }, - want: false, + want: true, }, { - name: "stale revision that was never pinned w/o Ready status", + name: "fresh create time", rev: &v1.Revision{ ObjectMeta: metav1.ObjectMeta{ Name: "myrev", - CreationTimestamp: metav1.NewTime(staleTime), + CreationTimestamp: metav1.NewTime(curTime), }, Status: v1.RevisionStatus{ Status: duckv1.Status{ @@ -348,9 +503,9 @@ func TestIsRevisionStale(t *testing.T) { }, }, }, - want: true, + want: false, }, { - name: "stale revision that was previously pinned", + name: "stale pinned time", rev: &v1.Revision{ ObjectMeta: metav1.ObjectMeta{ Name: "myrev", @@ -362,7 +517,7 @@ func TestIsRevisionStale(t *testing.T) { }, want: true, }, { - name: "fresh revision that was previously pinned", + name: "fresh pinned time", rev: &v1.Revision{ ObjectMeta: metav1.ObjectMeta{ Name: "myrev", @@ -374,42 +529,40 @@ func TestIsRevisionStale(t *testing.T) { }, want: false, }, { - name: "stale latest ready revision", + name: "stale revisionStateModified", rev: &v1.Revision{ ObjectMeta: metav1.ObjectMeta{ Name: "myrev", CreationTimestamp: metav1.NewTime(staleTime), Annotations: map[string]string{ - "serving.knative.dev/lastPinned": fmt.Sprintf("%d", staleTime.Unix()), + "serving.knative.dev/routingStateModified": staleTime.UTC().Format(time.RFC3339), }, }, }, - latestRev: "myrev", - want: false, - }} - - cfgStore := testConfigStore{ - config: &config.Config{ - RevisionGC: &gcconfig.Config{ - StaleRevisionCreateDelay: 5 * time.Minute, - StaleRevisionTimeout: 5 * time.Minute, - StaleRevisionMinimumGenerations: 2, + want: true, + }, { + name: "fresh revisionStateModified", + rev: &v1.Revision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "myrev", + CreationTimestamp: metav1.NewTime(staleTime), + Annotations: map[string]string{ + "serving.knative.dev/routingStateModified": curTime.UTC().Format(time.RFC3339), + }, }, }, + want: false, + }} + + cfg := &gcconfig.Config{ + RetainSinceCreateTime: 5 * time.Minute, + RetainSinceLastActiveTime: 5 * time.Minute, + MinNonActiveRevisions: 2, } - ctx := cfgStore.ToContext(context.Background()) for _, test := range tests { t.Run(test.name, func(t *testing.T) { - cfg := &v1.Configuration{ - Status: v1.ConfigurationStatus{ - ConfigurationStatusFields: v1.ConfigurationStatusFields{ - LatestReadyRevisionName: test.latestRev, - }, - }, - } - - got := isRevisionStale(ctx, test.rev, cfg) + got := isRevisionStale(cfg, test.rev, TestLogger(t)) if got != test.want { t.Errorf("IsRevisionStale want %v got %v", test.want, got) @@ -438,9 +591,9 @@ func cfg(name, namespace string, generation int64, co ...ConfigOption) *v1.Confi return c } -func rev(ctx context.Context, name, namespace string, generation int64, ro ...RevisionOption) *v1.Revision { - config := cfg(name, namespace, generation) - rev := resources.MakeRevision(ctx, config, clock.RealClock{}) +func rev(configName, namespace string, generation int64, ro ...RevisionOption) *v1.Revision { + config := cfg(configName, namespace, generation) + rev := resources.MakeRevision(context.Background(), config, clock.RealClock{}) rev.SetDefaults(context.Background()) for _, opt := range ro {