From 598c203e5c6c51f6d10bf9cb873a50ae6b1f7d61 Mon Sep 17 00:00:00 2001 From: Tara Gu Date: Mon, 22 Jul 2019 15:01:50 -0400 Subject: [PATCH] Refactor garbage collection from configuration reconciler --- cmd/controller/main.go | 2 + hack/update-codegen.sh | 2 +- pkg/reconciler/configuration/configuration.go | 76 +--- .../configuration/configuration_test.go | 291 -------------- pkg/reconciler/configuration/controller.go | 7 - .../{configuration => gc}/config/doc.go | 0 .../{configuration => gc}/config/store.go | 3 +- .../config/store_test.go | 0 .../config/testdata/config-gc.yaml | 0 .../config/zz_generated.deepcopy.go | 0 pkg/reconciler/gc/controller.go | 82 ++++ pkg/reconciler/gc/gc.go | 153 ++++++++ pkg/reconciler/gc/gc_test.go | 361 ++++++++++++++++++ 13 files changed, 602 insertions(+), 375 deletions(-) rename pkg/reconciler/{configuration => gc}/config/doc.go (100%) rename pkg/reconciler/{configuration => gc}/config/store.go (94%) rename pkg/reconciler/{configuration => gc}/config/store_test.go (100%) rename pkg/reconciler/{configuration => gc}/config/testdata/config-gc.yaml (100%) rename pkg/reconciler/{configuration => gc}/config/zz_generated.deepcopy.go (100%) create mode 100644 pkg/reconciler/gc/controller.go create mode 100644 pkg/reconciler/gc/gc.go create mode 100644 pkg/reconciler/gc/gc_test.go diff --git a/cmd/controller/main.go b/cmd/controller/main.go index 3b1027000dda..2961bed353bb 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -24,6 +24,7 @@ import ( "knative.dev/serving/pkg/reconciler/route" "knative.dev/serving/pkg/reconciler/serverlessservice" "knative.dev/serving/pkg/reconciler/service" + "knative.dev/serving/pkg/reconciler/gc" // This defines the shared main for injected controllers. "knative.dev/pkg/injection/sharedmain" @@ -37,5 +38,6 @@ func main() { route.NewController, serverlessservice.NewController, service.NewController, + gc.NewController, ) } diff --git a/hack/update-codegen.sh b/hack/update-codegen.sh index 31ccfca8bf18..9fcf8dbe1025 100755 --- a/hack/update-codegen.sh +++ b/hack/update-codegen.sh @@ -68,7 +68,7 @@ ${GOPATH}/bin/deepcopy-gen \ -i knative.dev/serving/pkg/apis/config \ -i knative.dev/serving/pkg/reconciler/ingress/config \ -i knative.dev/serving/pkg/reconciler/certificate/config \ - -i knative.dev/serving/pkg/reconciler/configuration/config \ + -i knative.dev/serving/pkg/reconciler/gc/config \ -i knative.dev/serving/pkg/reconciler/revision/config \ -i knative.dev/serving/pkg/reconciler/route/config \ -i knative.dev/serving/pkg/tracing/config \ diff --git a/pkg/reconciler/configuration/configuration.go b/pkg/reconciler/configuration/configuration.go index ffc3cfd973f2..f81c90e9793b 100644 --- a/pkg/reconciler/configuration/configuration.go +++ b/pkg/reconciler/configuration/configuration.go @@ -20,8 +20,6 @@ import ( "context" "fmt" "reflect" - "sort" - "time" "go.uber.org/zap" corev1 "k8s.io/api/core/v1" @@ -37,7 +35,6 @@ import ( "knative.dev/serving/pkg/apis/serving/v1beta1" listers "knative.dev/serving/pkg/client/listers/serving/v1alpha1" "knative.dev/serving/pkg/reconciler" - configns "knative.dev/serving/pkg/reconciler/configuration/config" "knative.dev/serving/pkg/reconciler/configuration/resources" ) @@ -67,8 +64,6 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { } logger := logging.FromContext(ctx) - ctx = c.configStore.ToContext(ctx) - // Get the Configuration resource with this namespace/name. original, err := c.configurationLister.Configurations(namespace).Get(name) if errors.IsNotFound(err) { @@ -203,7 +198,7 @@ func (c *Reconciler) reconcile(ctx context.Context, config *v1alpha1.Configurati return err } - return c.gcRevisions(ctx, config) + return nil } // CheckNameAvailability checks that if the named Revision specified by the Configuration @@ -292,72 +287,3 @@ func (c *Reconciler) updateStatus(desired *v1alpha1.Configuration) (*v1alpha1.Co existing.Status = desired.Status return c.ServingClientSet.ServingV1alpha1().Configurations(desired.Namespace).UpdateStatus(existing) } - -func (c *Reconciler) gcRevisions(ctx context.Context, config *v1alpha1.Configuration) error { - cfg := configns.FromContext(ctx).RevisionGC - logger := logging.FromContext(ctx) - - selector := labels.Set{serving.ConfigurationLabelKey: config.Name}.AsSelector() - revs, err := c.revisionLister.Revisions(config.Namespace).List(selector) - if err != nil { - return err - } - - gcSkipOffset := cfg.StaleRevisionMinimumGenerations - - if gcSkipOffset >= int64(len(revs)) { - return nil - } - - // Sort by creation timestamp descending - sort.Slice(revs, func(i, j int) bool { - return revs[j].CreationTimestamp.Before(&revs[i].CreationTimestamp) - }) - - for _, rev := range revs[gcSkipOffset:] { - if isRevisionStale(ctx, rev, config) { - err := c.ServingClientSet.ServingV1alpha1().Revisions(rev.Namespace).Delete(rev.Name, &metav1.DeleteOptions{}) - if err != nil { - logger.Errorf("Failed to delete stale revision: %v", err) - return err - } - } - } - return nil -} - -func isRevisionStale(ctx context.Context, rev *v1alpha1.Revision, config *v1alpha1.Configuration) bool { - cfg := configns.FromContext(ctx).RevisionGC - logger := logging.FromContext(ctx) - - if config.Status.LatestReadyRevisionName == rev.Name { - return false - } - - curTime := time.Now() - if rev.ObjectMeta.CreationTimestamp.Add(cfg.StaleRevisionCreateDelay).After(curTime) { - // Revision was created sooner than staleRevisionCreateDelay. Ignore it. - return false - } - - lastPin, err := rev.GetLastPinned() - if err != nil { - if err.(v1alpha1.LastPinnedParseError).Type != v1alpha1.AnnotationParseErrorTypeMissing { - logger.Errorf("Failed to determine revision last pinned: %v", err) - } else { - // Revision was never pinned and its RevisionConditionReady is not true after staleRevisionCreateDelay. - // It usually happens when ksvc was deployed with wrong configuration. - rc := rev.Status.GetCondition(v1beta1.RevisionConditionReady) - if rc == nil || rc.Status != corev1.ConditionTrue { - return true - } - } - return false - } - - ret := lastPin.Add(cfg.StaleRevisionTimeout).Before(curTime) - if ret { - logger.Infof("Detected stale revision %v with creation time %v and lastPinned time %v.", rev.ObjectMeta.Name, rev.ObjectMeta.CreationTimestamp, lastPin) - } - return ret -} diff --git a/pkg/reconciler/configuration/configuration_test.go b/pkg/reconciler/configuration/configuration_test.go index 7ffd3f94155d..c4126026c3ba 100644 --- a/pkg/reconciler/configuration/configuration_test.go +++ b/pkg/reconciler/configuration/configuration_test.go @@ -18,7 +18,6 @@ package configuration import ( "context" - "fmt" "testing" "time" @@ -29,7 +28,6 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" clientgotesting "k8s.io/client-go/testing" duckv1beta1 "knative.dev/pkg/apis/duck/v1beta1" "knative.dev/pkg/configmap" @@ -38,9 +36,7 @@ import ( "knative.dev/pkg/ptr" "knative.dev/serving/pkg/apis/serving/v1alpha1" "knative.dev/serving/pkg/apis/serving/v1beta1" - "knative.dev/serving/pkg/gc" "knative.dev/serving/pkg/reconciler" - "knative.dev/serving/pkg/reconciler/configuration/config" "knative.dev/serving/pkg/reconciler/configuration/resources" . "knative.dev/pkg/reconciler/testing" @@ -404,153 +400,6 @@ func TestReconcile(t *testing.T) { Base: reconciler.NewBase(ctx, controllerAgentName, cmw), configurationLister: listers.GetConfigurationLister(), revisionLister: listers.GetRevisionLister(), - configStore: &testConfigStore{ - config: ReconcilerTestConfig(), - }, - } - })) -} - -func TestGCReconcile(t *testing.T) { - now := time.Now() - tenMinutesAgo := now.Add(-10 * time.Minute) - - old := now.Add(-11 * time.Minute) - older := now.Add(-12 * time.Minute) - oldest := now.Add(-13 * time.Minute) - - table := TableTest{{ - Name: "delete oldest, keep two", - Objects: []runtime.Object{ - cfg("keep-two", "foo", 5556, - WithLatestCreated("5556"), - WithLatestReady("5556"), - WithObservedGen), - rev("keep-two", "foo", 5554, MarkRevisionReady, - WithRevName("5554"), - WithCreationTimestamp(oldest), - WithLastPinned(tenMinutesAgo)), - rev("keep-two", "foo", 5555, MarkRevisionReady, - WithRevName("5555"), - WithCreationTimestamp(older), - WithLastPinned(tenMinutesAgo)), - rev("keep-two", "foo", 5556, MarkRevisionReady, - WithRevName("5556"), - WithCreationTimestamp(old), - WithLastPinned(tenMinutesAgo)), - }, - WantDeletes: []clientgotesting.DeleteActionImpl{{ - ActionImpl: clientgotesting.ActionImpl{ - Namespace: "foo", - Verb: "delete", - Resource: schema.GroupVersionResource{ - Group: "serving.knative.dev", - Version: "v1alpha1", - Resource: "revisions", - }, - }, - Name: "5554", - }}, - Key: "foo/keep-two", - }, { - Name: "keep oldest when no lastPinned", - Objects: []runtime.Object{ - cfg("keep-no-last-pinned", "foo", 5556, - WithLatestCreated("5556"), - WithLatestReady("5556"), - WithObservedGen), - // No lastPinned so we will keep this. - rev("keep-no-last-pinned", "foo", 5554, MarkRevisionReady, - WithRevName("5554"), - WithCreationTimestamp(oldest)), - rev("keep-no-last-pinned", "foo", 5555, MarkRevisionReady, - WithRevName("5555"), - WithCreationTimestamp(older), - WithLastPinned(tenMinutesAgo)), - rev("keep-no-last-pinned", "foo", 5556, MarkRevisionReady, - WithRevName("5556"), - WithCreationTimestamp(old), - WithLastPinned(tenMinutesAgo)), - }, - Key: "foo/keep-no-last-pinned", - }, { - Name: "keep recent lastPinned", - Objects: []runtime.Object{ - cfg("keep-recent-last-pinned", "foo", 5556, - WithLatestCreated("5556"), - WithLatestReady("5556"), - WithObservedGen), - rev("keep-recent-last-pinned", "foo", 5554, MarkRevisionReady, - WithRevName("5554"), - WithCreationTimestamp(oldest), - // This is an indication that things are still routing here. - WithLastPinned(now)), - rev("keep-recent-last-pinned", "foo", 5555, MarkRevisionReady, - WithRevName("5555"), - WithCreationTimestamp(older), - WithLastPinned(tenMinutesAgo)), - rev("keep-recent-last-pinned", "foo", 5556, MarkRevisionReady, - WithRevName("5556"), - WithCreationTimestamp(old), - WithLastPinned(tenMinutesAgo)), - }, - Key: "foo/keep-recent-last-pinned", - }, { - Name: "keep LatestReadyRevision", - Objects: []runtime.Object{ - // Create a revision where the LatestReady is 5554, but LatestCreated is 5556. - // We should keep LatestReady even if it is old. - cfg("keep-two", "foo", 5556, - WithLatestReady("5554"), - // This comes after 'WithLatestReady' so the - // Configuration's 'Ready' Status is 'Unknown' - WithLatestCreated("5556"), - WithObservedGen), - rev("keep-two", "foo", 5554, MarkRevisionReady, - WithRevName("5554"), - WithCreationTimestamp(oldest), - WithLastPinned(tenMinutesAgo)), - rev("keep-two", "foo", 5555, // Not Ready - WithRevName("5555"), - WithCreationTimestamp(older), - WithLastPinned(tenMinutesAgo)), - rev("keep-two", "foo", 5556, // Not Ready - WithRevName("5556"), - WithCreationTimestamp(old), - WithLastPinned(tenMinutesAgo)), - }, - Key: "foo/keep-two", - }, { - Name: "keep stale revision because of minimum generations", - Objects: []runtime.Object{ - cfg("keep-all", "foo", 5554, - // Don't set the latest ready revision here - // since those by default are always retained - WithLatestCreated("keep-all"), - WithObservedGen), - rev("keep-all", "foo", 5554, - WithRevName("keep-all"), - WithCreationTimestamp(oldest), - WithLastPinned(tenMinutesAgo)), - }, - Key: "foo/keep-all", - }} - - defer logtesting.ClearAll() - table.Test(t, MakeFactory(func(ctx context.Context, listers *Listers, cmw configmap.Watcher) controller.Reconciler { - return &Reconciler{ - Base: reconciler.NewBase(ctx, controllerAgentName, cmw), - configurationLister: listers.GetConfigurationLister(), - revisionLister: listers.GetRevisionLister(), - configStore: &testConfigStore{ - config: &config.Config{ - RevisionGC: &gc.Config{ - StaleRevisionCreateDelay: 5 * time.Minute, - StaleRevisionTimeout: 5 * time.Minute, - StaleRevisionMinimumGenerations: 2, - }, - }, - }, } })) } @@ -583,143 +432,3 @@ func rev(name, namespace string, generation int64, ro ...RevisionOption) *v1alph } return r } - -type testConfigStore struct { - config *config.Config -} - -func (t *testConfigStore) ToContext(ctx context.Context) context.Context { - return config.ToContext(ctx, t.config) -} - -var _ reconciler.ConfigStore = (*testConfigStore)(nil) - -func ReconcilerTestConfig() *config.Config { - return &config.Config{ - RevisionGC: &gc.Config{ - StaleRevisionCreateDelay: 5 * time.Minute, - StaleRevisionTimeout: 5 * time.Minute, - }, - } -} - -func TestIsRevisionStale(t *testing.T) { - curTime := time.Now() - staleTime := curTime.Add(-10 * time.Minute) - - tests := []struct { - name string - rev *v1alpha1.Revision - latestRev string - want bool - }{{ - name: "fresh revision that was never pinned", - rev: &v1alpha1.Revision{ - ObjectMeta: metav1.ObjectMeta{ - Name: "myrev", - CreationTimestamp: metav1.NewTime(curTime), - }, - }, - want: false, - }, { - name: "stale revision that was never pinned w/ Ready status", - rev: &v1alpha1.Revision{ - ObjectMeta: metav1.ObjectMeta{ - Name: "myrev", - CreationTimestamp: metav1.NewTime(staleTime), - }, - Status: v1alpha1.RevisionStatus{ - Status: duckv1beta1.Status{ - Conditions: duckv1beta1.Conditions{{ - Type: v1alpha1.RevisionConditionReady, - Status: "True", - }}, - }, - }, - }, - want: false, - }, { - name: "stale revision that was never pinned w/o Ready status", - rev: &v1alpha1.Revision{ - ObjectMeta: metav1.ObjectMeta{ - Name: "myrev", - CreationTimestamp: metav1.NewTime(staleTime), - }, - Status: v1alpha1.RevisionStatus{ - Status: duckv1beta1.Status{ - Conditions: duckv1beta1.Conditions{{ - Type: v1alpha1.RevisionConditionReady, - Status: "Unknown", - }}, - }, - }, - }, - want: true, - }, { - name: "stale revision that was previously pinned", - rev: &v1alpha1.Revision{ - ObjectMeta: metav1.ObjectMeta{ - Name: "myrev", - CreationTimestamp: metav1.NewTime(staleTime), - Annotations: map[string]string{ - "serving.knative.dev/lastPinned": fmt.Sprintf("%d", staleTime.Unix()), - }, - }, - }, - want: true, - }, { - name: "fresh revision that was previously pinned", - rev: &v1alpha1.Revision{ - ObjectMeta: metav1.ObjectMeta{ - Name: "myrev", - CreationTimestamp: metav1.NewTime(staleTime), - Annotations: map[string]string{ - "serving.knative.dev/lastPinned": fmt.Sprintf("%d", curTime.Unix()), - }, - }, - }, - want: false, - }, { - name: "stale latest ready revision", - rev: &v1alpha1.Revision{ - ObjectMeta: metav1.ObjectMeta{ - Name: "myrev", - CreationTimestamp: metav1.NewTime(staleTime), - Annotations: map[string]string{ - "serving.knative.dev/lastPinned": fmt.Sprintf("%d", staleTime.Unix()), - }, - }, - }, - latestRev: "myrev", - want: false, - }} - - cfgStore := testConfigStore{ - config: &config.Config{ - RevisionGC: &gc.Config{ - StaleRevisionCreateDelay: 5 * time.Minute, - StaleRevisionTimeout: 5 * time.Minute, - StaleRevisionMinimumGenerations: 2, - }, - }, - } - ctx := cfgStore.ToContext(context.Background()) - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - cfg := &v1alpha1.Configuration{ - Status: v1alpha1.ConfigurationStatus{ - ConfigurationStatusFields: v1alpha1.ConfigurationStatusFields{ - LatestReadyRevisionName: test.latestRev, - }, - }, - } - - got := isRevisionStale(ctx, test.rev, cfg) - - if got != test.want { - t.Errorf("IsRevisionStale want %v got %v", test.want, got) - } - }) - } -} diff --git a/pkg/reconciler/configuration/controller.go b/pkg/reconciler/configuration/controller.go index adecb4266dc1..b35a2246b09f 100644 --- a/pkg/reconciler/configuration/controller.go +++ b/pkg/reconciler/configuration/controller.go @@ -21,13 +21,11 @@ import ( configurationinformer "knative.dev/serving/pkg/client/injection/informers/serving/v1alpha1/configuration" revisioninformer "knative.dev/serving/pkg/client/injection/informers/serving/v1alpha1/revision" - "k8s.io/client-go/tools/cache" "knative.dev/pkg/configmap" "knative.dev/pkg/controller" "knative.dev/serving/pkg/apis/serving/v1alpha1" "knative.dev/serving/pkg/reconciler" - configns "knative.dev/serving/pkg/reconciler/configuration/config" ) const controllerAgentName = "configuration-controller" @@ -56,10 +54,5 @@ func NewController( Handler: controller.HandleAll(impl.EnqueueControllerOf), }) - c.Logger.Info("Setting up ConfigMap receivers") - configStore := configns.NewStore(c.Logger.Named("config-store"), controller.GetResyncPeriod(ctx)) - configStore.WatchConfigs(c.ConfigMapWatcher) - c.configStore = configStore - return impl } diff --git a/pkg/reconciler/configuration/config/doc.go b/pkg/reconciler/gc/config/doc.go similarity index 100% rename from pkg/reconciler/configuration/config/doc.go rename to pkg/reconciler/gc/config/doc.go diff --git a/pkg/reconciler/configuration/config/store.go b/pkg/reconciler/gc/config/store.go similarity index 94% rename from pkg/reconciler/configuration/config/store.go rename to pkg/reconciler/gc/config/store.go index fe50fad0ec1f..30c51f22a996 100644 --- a/pkg/reconciler/configuration/config/store.go +++ b/pkg/reconciler/gc/config/store.go @@ -54,7 +54,7 @@ func (s *Store) Load() *Config { } } -func NewStore(logger configmap.Logger, minRevisionTimeout time.Duration) *Store { +func NewStore(logger configmap.Logger, minRevisionTimeout time.Duration, onAfterStore ...func(name string, value interface{})) *Store { return &Store{ UntypedStore: configmap.NewUntypedStore( "configuration", @@ -62,6 +62,7 @@ func NewStore(logger configmap.Logger, minRevisionTimeout time.Duration) *Store configmap.Constructors{ gc.ConfigName: gc.NewConfigFromConfigMapFunc(logger, minRevisionTimeout), }, + onAfterStore..., ), } } diff --git a/pkg/reconciler/configuration/config/store_test.go b/pkg/reconciler/gc/config/store_test.go similarity index 100% rename from pkg/reconciler/configuration/config/store_test.go rename to pkg/reconciler/gc/config/store_test.go diff --git a/pkg/reconciler/configuration/config/testdata/config-gc.yaml b/pkg/reconciler/gc/config/testdata/config-gc.yaml similarity index 100% rename from pkg/reconciler/configuration/config/testdata/config-gc.yaml rename to pkg/reconciler/gc/config/testdata/config-gc.yaml diff --git a/pkg/reconciler/configuration/config/zz_generated.deepcopy.go b/pkg/reconciler/gc/config/zz_generated.deepcopy.go similarity index 100% rename from pkg/reconciler/configuration/config/zz_generated.deepcopy.go rename to pkg/reconciler/gc/config/zz_generated.deepcopy.go diff --git a/pkg/reconciler/gc/controller.go b/pkg/reconciler/gc/controller.go new file mode 100644 index 000000000000..f1e8e17081a5 --- /dev/null +++ b/pkg/reconciler/gc/controller.go @@ -0,0 +1,82 @@ +/* +Copyright 2019 The Knative Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package gc + +import ( + "context" + + "k8s.io/client-go/tools/cache" + "knative.dev/pkg/configmap" + "knative.dev/pkg/controller" + "knative.dev/serving/pkg/apis/serving/v1alpha1" + configurationinformer "knative.dev/serving/pkg/client/injection/informers/serving/v1alpha1/configuration" + revisioninformer "knative.dev/serving/pkg/client/injection/informers/serving/v1alpha1/revision" + gcconfig "knative.dev/serving/pkg/gc" + pkgreconciler "knative.dev/serving/pkg/reconciler" + configns "knative.dev/serving/pkg/reconciler/gc/config" +) + +const ( + controllerAgentName = "revision-gc-controller" +) + +// NewController creates a new Garbage Collection controller +func NewController( + ctx context.Context, + cmw configmap.Watcher, +) *controller.Impl { + + configurationInformer := configurationinformer.Get(ctx) + revisionInformer := revisioninformer.Get(ctx) + + c := &reconciler{ + Base: pkgreconciler.NewBase(ctx, controllerAgentName, cmw), + configurationLister: configurationInformer.Lister(), + revisionLister: revisionInformer.Lister(), + } + impl := controller.NewImpl(c, c.Logger, "Garbage Collection") + + c.Logger.Info("Setting up event handlers") + + // Since the gc controller came from the configuration controller, having event handlers + // on both configuration and revision matches the existing behaviors of the configuration + // controller. This is to minimize risk heading into v1. + // TODO (taragu): probably one or both of these event handlers are not needed + configurationInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue)) + + revisionInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{ + FilterFunc: controller.Filter(v1alpha1.SchemeGroupVersion.WithKind("Configuration")), + Handler: controller.HandleAll(impl.EnqueueControllerOf), + }) + + c.Logger.Info("Setting up ConfigMap receivers with resync func") + configsToResync := []interface{}{ + &gcconfig.Config{}, + } + resync := configmap.TypeFilter(configsToResync...)(func(string, interface{}) { + // Triggers syncs on all revisions when configuration + // changes + impl.GlobalResync(revisionInformer.Informer()) + }) + + c.Logger.Info("Setting up ConfigMap receivers") + configStore := configns.NewStore(c.Logger.Named("config-store"), controller.GetResyncPeriod(ctx), resync) + configStore.WatchConfigs(c.ConfigMapWatcher) + c.configStore = configStore + + return impl +} diff --git a/pkg/reconciler/gc/gc.go b/pkg/reconciler/gc/gc.go new file mode 100644 index 000000000000..16f1503ab495 --- /dev/null +++ b/pkg/reconciler/gc/gc.go @@ -0,0 +1,153 @@ +/* +Copyright 2019 The Knative Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package gc + +import ( + "context" + "fmt" + "sort" + "time" + + "go.uber.org/zap" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/client-go/tools/cache" + "knative.dev/pkg/controller" + "knative.dev/pkg/logging" + "knative.dev/serving/pkg/apis/serving" + "knative.dev/serving/pkg/apis/serving/v1alpha1" + "knative.dev/serving/pkg/apis/serving/v1beta1" + listers "knative.dev/serving/pkg/client/listers/serving/v1alpha1" + pkgreconciler "knative.dev/serving/pkg/reconciler" + configns "knative.dev/serving/pkg/reconciler/gc/config" +) + +// reconciler implements controller.Reconciler for Garbage Collection resources. +type reconciler struct { + *pkgreconciler.Base + + // listers index properties about resources + configurationLister listers.ConfigurationLister + revisionLister listers.RevisionLister + + configStore pkgreconciler.ConfigStore +} + +// Check that our reconciler implements controller.Reconciler +var _ controller.Reconciler = (*reconciler)(nil) + +// Reconcile compares the actual state with the desired, and attempts to +// converge the two. It then updates the Status block of the Garbage Collection +// resource with the current status of the resource. +func (c *reconciler) Reconcile(ctx context.Context, key string) error { + // Convert the namespace/name string into a distinct namespace and name. + namespace, name, err := cache.SplitMetaNamespaceKey(key) + if err != nil { + c.Logger.Errorf("invalid resource key %q: %v", key, err) + return nil + } + logger := logging.FromContext(ctx) + + ctx = c.configStore.ToContext(ctx) + + // Get the Configuration resource with this namespace/name. + config, err := c.configurationLister.Configurations(namespace).Get(name) + if errors.IsNotFound(err) { + // The resource no longer exists, in which case we stop processing. + logger.Errorf("Configuration %q in work queue no longer exists", key) + return nil + } else if err != nil { + return err + } + + reconcileErr := c.reconcile(ctx, config) + if reconcileErr != nil { + c.Recorder.Event(config, corev1.EventTypeWarning, "InternalError", reconcileErr.Error()) + } + return reconcileErr +} + +func (c *reconciler) reconcile(ctx context.Context, config *v1alpha1.Configuration) error { + cfg := configns.FromContext(ctx).RevisionGC + logger := logging.FromContext(ctx) + + selector := labels.Set{serving.ConfigurationLabelKey: config.Name}.AsSelector() + revs, err := c.revisionLister.Revisions(config.Namespace).List(selector) + if err != nil { + return err + } + + gcSkipOffset := cfg.StaleRevisionMinimumGenerations + + if gcSkipOffset >= int64(len(revs)) { + return nil + } + + // Sort by creation timestamp descending + sort.Slice(revs, func(i, j int) bool { + return revs[j].CreationTimestamp.Before(&revs[i].CreationTimestamp) + }) + + for _, rev := range revs[gcSkipOffset:] { + if isRevisionStale(ctx, rev, config) { + err := c.ServingClientSet.ServingV1alpha1().Revisions(rev.Namespace).Delete(rev.Name, &metav1.DeleteOptions{}) + if err != nil { + logger.Errorw(fmt.Sprintf("Failed to delete stale revision %q", rev.Name), zap.Error(err)) + continue + } + } + } + return nil +} + +func isRevisionStale(ctx context.Context, rev *v1alpha1.Revision, config *v1alpha1.Configuration) bool { + if config.Status.LatestReadyRevisionName == rev.Name { + return false + } + + cfg := configns.FromContext(ctx).RevisionGC + logger := logging.FromContext(ctx) + + curTime := time.Now() + if rev.ObjectMeta.CreationTimestamp.Add(cfg.StaleRevisionCreateDelay).After(curTime) { + // Revision was created sooner than staleRevisionCreateDelay. Ignore it. + return false + } + + lastPin, err := rev.GetLastPinned() + if err != nil { + if err.(v1alpha1.LastPinnedParseError).Type != v1alpha1.AnnotationParseErrorTypeMissing { + logger.Errorw("Failed to determine revision last pinned", zap.Error(err)) + } else { + // Revision was never pinned and its RevisionConditionReady is not true after staleRevisionCreateDelay. + // It usually happens when ksvc was deployed with wrong configuration. + rc := rev.Status.GetCondition(v1beta1.RevisionConditionReady) + if rc == nil || rc.Status != corev1.ConditionTrue { + return true + } + } + return false + } + + ret := lastPin.Add(cfg.StaleRevisionTimeout).Before(curTime) + if ret { + logger.Infof("Detected stale revision %v with creation time %v and lastPinned time %v.", rev.ObjectMeta.Name, rev.ObjectMeta.CreationTimestamp, lastPin) + } + return ret +} diff --git a/pkg/reconciler/gc/gc_test.go b/pkg/reconciler/gc/gc_test.go new file mode 100644 index 000000000000..08030b5ccea3 --- /dev/null +++ b/pkg/reconciler/gc/gc_test.go @@ -0,0 +1,361 @@ +/* +Copyright 2019 The Knative Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package gc + +import ( + "context" + "fmt" + "testing" + "time" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + clientgotesting "k8s.io/client-go/testing" + duckv1beta1 "knative.dev/pkg/apis/duck/v1beta1" + "knative.dev/pkg/configmap" + "knative.dev/pkg/controller" + logtesting "knative.dev/pkg/logging/testing" + "knative.dev/pkg/ptr" + . "knative.dev/pkg/reconciler/testing" + "knative.dev/serving/pkg/apis/serving/v1alpha1" + "knative.dev/serving/pkg/apis/serving/v1beta1" + _ "knative.dev/serving/pkg/client/injection/informers/serving/v1alpha1/configuration/fake" + _ "knative.dev/serving/pkg/client/injection/informers/serving/v1alpha1/revision/fake" + gcconfig "knative.dev/serving/pkg/gc" + pkgreconciler "knative.dev/serving/pkg/reconciler" + "knative.dev/serving/pkg/reconciler/configuration/resources" + "knative.dev/serving/pkg/reconciler/gc/config" + . "knative.dev/serving/pkg/reconciler/testing/v1alpha1" + . "knative.dev/serving/pkg/testing/v1alpha1" +) + +var revisionSpec = v1alpha1.RevisionSpec{ + RevisionSpec: v1beta1.RevisionSpec{ + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + }}, + }, + TimeoutSeconds: ptr.Int64(60), + }, +} + +func TestGCReconcile(t *testing.T) { + now := time.Now() + tenMinutesAgo := now.Add(-10 * time.Minute) + + old := now.Add(-11 * time.Minute) + older := now.Add(-12 * time.Minute) + oldest := now.Add(-13 * time.Minute) + + table := TableTest{{ + Name: "delete oldest, keep two", + Objects: []runtime.Object{ + cfg("keep-two", "foo", 5556, + WithLatestCreated("5556"), + WithLatestReady("5556"), + WithObservedGen), + rev("keep-two", "foo", 5554, MarkRevisionReady, + WithRevName("5554"), + WithCreationTimestamp(oldest), + WithLastPinned(tenMinutesAgo)), + rev("keep-two", "foo", 5555, MarkRevisionReady, + WithRevName("5555"), + WithCreationTimestamp(older), + WithLastPinned(tenMinutesAgo)), + rev("keep-two", "foo", 5556, MarkRevisionReady, + WithRevName("5556"), + WithCreationTimestamp(old), + WithLastPinned(tenMinutesAgo)), + }, + WantDeletes: []clientgotesting.DeleteActionImpl{{ + ActionImpl: clientgotesting.ActionImpl{ + Namespace: "foo", + Verb: "delete", + Resource: schema.GroupVersionResource{ + Group: "serving.knative.dev", + Version: "v1alpha1", + Resource: "revisions", + }, + }, + Name: "5554", + }}, + Key: "foo/keep-two", + }, { + Name: "keep oldest when no lastPinned", + Objects: []runtime.Object{ + cfg("keep-no-last-pinned", "foo", 5556, + WithLatestCreated("5556"), + WithLatestReady("5556"), + WithObservedGen), + // No lastPinned so we will keep this. + rev("keep-no-last-pinned", "foo", 5554, MarkRevisionReady, + WithRevName("5554"), + WithCreationTimestamp(oldest)), + rev("keep-no-last-pinned", "foo", 5555, MarkRevisionReady, + WithRevName("5555"), + WithCreationTimestamp(older), + WithLastPinned(tenMinutesAgo)), + rev("keep-no-last-pinned", "foo", 5556, MarkRevisionReady, + WithRevName("5556"), + WithCreationTimestamp(old), + WithLastPinned(tenMinutesAgo)), + }, + Key: "foo/keep-no-last-pinned", + }, { + Name: "keep recent lastPinned", + Objects: []runtime.Object{ + cfg("keep-recent-last-pinned", "foo", 5556, + WithLatestCreated("5556"), + WithLatestReady("5556"), + WithObservedGen), + rev("keep-recent-last-pinned", "foo", 5554, MarkRevisionReady, + WithRevName("5554"), + WithCreationTimestamp(oldest), + // This is an indication that things are still routing here. + WithLastPinned(now)), + rev("keep-recent-last-pinned", "foo", 5555, MarkRevisionReady, + WithRevName("5555"), + WithCreationTimestamp(older), + WithLastPinned(tenMinutesAgo)), + rev("keep-recent-last-pinned", "foo", 5556, MarkRevisionReady, + WithRevName("5556"), + WithCreationTimestamp(old), + WithLastPinned(tenMinutesAgo)), + }, + Key: "foo/keep-recent-last-pinned", + }, { + Name: "keep LatestReadyRevision", + Objects: []runtime.Object{ + // Create a revision where the LatestReady is 5554, but LatestCreated is 5556. + // We should keep LatestReady even if it is old. + cfg("keep-two", "foo", 5556, + WithLatestReady("5554"), + // This comes after 'WithLatestReady' so the + // Configuration's 'Ready' Status is 'Unknown' + WithLatestCreated("5556"), + WithObservedGen), + rev("keep-two", "foo", 5554, MarkRevisionReady, + WithRevName("5554"), + WithCreationTimestamp(oldest), + WithLastPinned(tenMinutesAgo)), + rev("keep-two", "foo", 5555, // Not Ready + WithRevName("5555"), + WithCreationTimestamp(older), + WithLastPinned(tenMinutesAgo)), + rev("keep-two", "foo", 5556, // Not Ready + WithRevName("5556"), + WithCreationTimestamp(old), + WithLastPinned(tenMinutesAgo)), + }, + Key: "foo/keep-two", + }, { + Name: "keep stale revision because of minimum generations", + Objects: []runtime.Object{ + cfg("keep-all", "foo", 5554, + // Don't set the latest ready revision here + // since those by default are always retained + WithLatestCreated("keep-all"), + WithObservedGen), + rev("keep-all", "foo", 5554, + WithRevName("keep-all"), + WithCreationTimestamp(oldest), + WithLastPinned(tenMinutesAgo)), + }, + Key: "foo/keep-all", + }} + + defer logtesting.ClearAll() + table.Test(t, MakeFactory(func(ctx context.Context, listers *Listers, cmw configmap.Watcher) controller.Reconciler { + return &reconciler{ + Base: pkgreconciler.NewBase(ctx, controllerAgentName, cmw), + configurationLister: listers.GetConfigurationLister(), + revisionLister: listers.GetRevisionLister(), + configStore: &testConfigStore{ + config: &config.Config{ + RevisionGC: &gcconfig.Config{ + StaleRevisionCreateDelay: 5 * time.Minute, + StaleRevisionTimeout: 5 * time.Minute, + StaleRevisionMinimumGenerations: 2, + }, + }, + }, + } + })) +} + +func TestIsRevisionStale(t *testing.T) { + curTime := time.Now() + staleTime := curTime.Add(-10 * time.Minute) + + tests := []struct { + name string + rev *v1alpha1.Revision + latestRev string + want bool + }{{ + name: "fresh revision that was never pinned", + rev: &v1alpha1.Revision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "myrev", + CreationTimestamp: metav1.NewTime(curTime), + }, + }, + want: false, + }, { + name: "stale revision that was never pinned w/ Ready status", + rev: &v1alpha1.Revision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "myrev", + CreationTimestamp: metav1.NewTime(staleTime), + }, + Status: v1alpha1.RevisionStatus{ + Status: duckv1beta1.Status{ + Conditions: duckv1beta1.Conditions{{ + Type: v1alpha1.RevisionConditionReady, + Status: "True", + }}, + }, + }, + }, + want: false, + }, { + name: "stale revision that was never pinned w/o Ready status", + rev: &v1alpha1.Revision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "myrev", + CreationTimestamp: metav1.NewTime(staleTime), + }, + Status: v1alpha1.RevisionStatus{ + Status: duckv1beta1.Status{ + Conditions: duckv1beta1.Conditions{{ + Type: v1alpha1.RevisionConditionReady, + Status: "Unknown", + }}, + }, + }, + }, + want: true, + }, { + name: "stale revision that was previously pinned", + rev: &v1alpha1.Revision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "myrev", + CreationTimestamp: metav1.NewTime(staleTime), + Annotations: map[string]string{ + "serving.knative.dev/lastPinned": fmt.Sprintf("%d", staleTime.Unix()), + }, + }, + }, + want: true, + }, { + name: "fresh revision that was previously pinned", + rev: &v1alpha1.Revision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "myrev", + CreationTimestamp: metav1.NewTime(staleTime), + Annotations: map[string]string{ + "serving.knative.dev/lastPinned": fmt.Sprintf("%d", curTime.Unix()), + }, + }, + }, + want: false, + }, { + name: "stale latest ready revision", + rev: &v1alpha1.Revision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "myrev", + CreationTimestamp: metav1.NewTime(staleTime), + Annotations: map[string]string{ + "serving.knative.dev/lastPinned": fmt.Sprintf("%d", staleTime.Unix()), + }, + }, + }, + latestRev: "myrev", + want: false, + }} + + cfgStore := testConfigStore{ + config: &config.Config{ + RevisionGC: &gcconfig.Config{ + StaleRevisionCreateDelay: 5 * time.Minute, + StaleRevisionTimeout: 5 * time.Minute, + StaleRevisionMinimumGenerations: 2, + }, + }, + } + ctx := cfgStore.ToContext(context.Background()) + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + cfg := &v1alpha1.Configuration{ + Status: v1alpha1.ConfigurationStatus{ + ConfigurationStatusFields: v1alpha1.ConfigurationStatusFields{ + LatestReadyRevisionName: test.latestRev, + }, + }, + } + + got := isRevisionStale(ctx, test.rev, cfg) + + if got != test.want { + t.Errorf("IsRevisionStale want %v got %v", test.want, got) + } + }) + } +} + +func cfg(name, namespace string, generation int64, co ...ConfigOption) *v1alpha1.Configuration { + c := &v1alpha1.Configuration{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + Generation: generation, + }, + Spec: v1alpha1.ConfigurationSpec{ + Template: &v1alpha1.RevisionTemplateSpec{ + Spec: *revisionSpec.DeepCopy(), + }, + }, + } + for _, opt := range co { + opt(c) + } + c.SetDefaults(context.Background()) + return c +} + +func rev(name, namespace string, generation int64, ro ...RevisionOption) *v1alpha1.Revision { + r := resources.MakeRevision(cfg(name, namespace, generation)) + r.SetDefaults(v1beta1.WithUpgradeViaDefaulting(context.Background())) + for _, opt := range ro { + opt(r) + } + return r +} + +type testConfigStore struct { + config *config.Config +} + +func (t *testConfigStore) ToContext(ctx context.Context) context.Context { + return config.ToContext(ctx, t.config) +} + +var _ pkgreconciler.ConfigStore = (*testConfigStore)(nil)