diff --git a/cmd/controller/main.go b/cmd/controller/main.go index 9eb3d1e27c1d..14e412e37645 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -107,6 +107,7 @@ func main() { opt, configurationInformer, revisionInformer, + routeInformer, ), revision.NewController( opt, diff --git a/pkg/reconciler/configuration/configuration.go b/pkg/reconciler/configuration/configuration.go index 351dc987d6b7..36ddee1a9b32 100644 --- a/pkg/reconciler/configuration/configuration.go +++ b/pkg/reconciler/configuration/configuration.go @@ -48,6 +48,7 @@ type Reconciler struct { // listers index properties about resources configurationLister listers.ConfigurationLister revisionLister listers.RevisionLister + routeLister listers.RouteLister configStore configStore } @@ -310,13 +311,26 @@ func (c *Reconciler) gcRevisions(ctx context.Context, config *v1alpha1.Configura return nil } + var route *v1alpha1.Route + + routeName, ok := config.GetLabels()[serving.RouteLabelKey] + if !ok { + logger.Warnf("%s does not have %q label", config.Name, serving.RouteLabelKey) + } else { + route, err = c.routeLister.Routes(config.Namespace).Get(routeName) + if err != nil && !errors.IsNotFound(err) { + // Go ahead GC process if route not found. + return err + } + } + // 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) { + if isRevisionStale(ctx, rev, config, route) { err := c.ServingClientSet.ServingV1alpha1().Revisions(rev.Namespace).Delete(rev.Name, &metav1.DeleteOptions{}) if err != nil { logger.Errorf("Failed to delete stale revision: %v", err) @@ -327,7 +341,7 @@ func (c *Reconciler) gcRevisions(ctx context.Context, config *v1alpha1.Configura return nil } -func isRevisionStale(ctx context.Context, rev *v1alpha1.Revision, config *v1alpha1.Configuration) bool { +func isRevisionStale(ctx context.Context, rev *v1alpha1.Revision, config *v1alpha1.Configuration, route *v1alpha1.Route) bool { cfg := configns.FromContext(ctx).RevisionGC logger := logging.FromContext(ctx) @@ -341,6 +355,14 @@ func isRevisionStale(ctx context.Context, rev *v1alpha1.Revision, config *v1alph return false } + if route != nil { + for _, v := range route.Status.Traffic { + if v.RevisionName == rev.Name { + return false + } + } + } + lastPin, err := rev.GetLastPinned() if err != nil { if err.(v1alpha1.LastPinnedParseError).Type != v1alpha1.AnnotationParseErrorTypeMissing { diff --git a/pkg/reconciler/configuration/configuration_test.go b/pkg/reconciler/configuration/configuration_test.go index 50c8b6c77648..796c9e2cbd7f 100644 --- a/pkg/reconciler/configuration/configuration_test.go +++ b/pkg/reconciler/configuration/configuration_test.go @@ -399,6 +399,7 @@ func TestReconcile(t *testing.T) { Base: reconciler.NewBase(opt, controllerAgentName), configurationLister: listers.GetConfigurationLister(), revisionLister: listers.GetRevisionLister(), + routeLister: listers.GetRouteLister(), configStore: &testConfigStore{ config: ReconcilerTestConfig(), }, @@ -537,6 +538,7 @@ func TestGCReconcile(t *testing.T) { Base: reconciler.NewBase(opt, controllerAgentName), configurationLister: listers.GetConfigurationLister(), revisionLister: listers.GetRevisionLister(), + routeLister: listers.GetRouteLister(), configStore: &testConfigStore{ config: &config.Config{ RevisionGC: &gc.Config{ @@ -608,6 +610,7 @@ func TestIsRevisionStale(t *testing.T) { name string rev *v1alpha1.Revision latestRev string + targetRev string want bool }{{ name: "fresh revision that was never pinned", @@ -652,6 +655,19 @@ func TestIsRevisionStale(t *testing.T) { }, }, want: true, + }, { + name: "stale revision that was still used by route", + 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()), + }, + }, + }, + targetRev: "myrev", + want: false, }, { name: "stale revision that was previously pinned", rev: &v1alpha1.Revision{ @@ -712,7 +728,18 @@ func TestIsRevisionStale(t *testing.T) { }, } - got := isRevisionStale(ctx, test.rev, cfg) + route := &v1alpha1.Route{ + Status: v1alpha1.RouteStatus{ + RouteStatusFields: v1alpha1.RouteStatusFields{ + Traffic: []v1alpha1.TrafficTarget{{ + TrafficTarget: v1beta1.TrafficTarget{ + RevisionName: test.targetRev, + }}}, + }, + }, + } + + got := isRevisionStale(ctx, test.rev, cfg, route) 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 22289b1502a5..b85eff320d93 100644 --- a/pkg/reconciler/configuration/controller.go +++ b/pkg/reconciler/configuration/controller.go @@ -40,12 +40,14 @@ func NewController( opt reconciler.Options, configurationInformer servinginformers.ConfigurationInformer, revisionInformer servinginformers.RevisionInformer, + routeInformer servinginformers.RouteInformer, ) *controller.Impl { c := &Reconciler{ Base: reconciler.NewBase(opt, controllerAgentName), configurationLister: configurationInformer.Lister(), revisionLister: revisionInformer.Lister(), + routeLister: routeInformer.Lister(), } impl := controller.NewImpl(c, c.Logger, "Configurations") diff --git a/pkg/reconciler/configuration/queueing_test.go b/pkg/reconciler/configuration/queueing_test.go index cc69a22290b1..7352fc464fb7 100644 --- a/pkg/reconciler/configuration/queueing_test.go +++ b/pkg/reconciler/configuration/queueing_test.go @@ -135,6 +135,7 @@ func newTestController(t *testing.T, stopCh chan struct{}) ( }, servingInformer.Serving().V1alpha1().Configurations(), servingInformer.Serving().V1alpha1().Revisions(), + servingInformer.Serving().V1alpha1().Routes(), ) return