From fcc30939d8962929aa51bc94dfa9ae2ac60ab8ab Mon Sep 17 00:00:00 2001 From: Kenjiro Nakayama Date: Mon, 3 Jun 2019 23:23:47 +0900 Subject: [PATCH 1/2] Skip GC proces when revision is still used by route Currently GC process only skips `LatestReadyRevision`. Hence, even if route uses non-LatestReadyRevision, GC removes the revision. It causes service outage with `RevisionMissing` error suddenly. This patch changes to stop GC when revision is still used by route. --- cmd/controller/main.go | 1 + pkg/reconciler/configuration/configuration.go | 19 ++++++++++-- .../configuration/configuration_test.go | 29 ++++++++++++++++++- pkg/reconciler/configuration/controller.go | 2 ++ pkg/reconciler/configuration/queueing_test.go | 1 + 5 files changed, 49 insertions(+), 3 deletions(-) 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..44c6782908ad 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,19 @@ func (c *Reconciler) gcRevisions(ctx context.Context, config *v1alpha1.Configura return nil } + route, err := c.routeLister.Routes(config.Namespace).Get(config.Name) + 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 +334,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 +348,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 From f82bd9b46728032166f2359ff06c3805a5740928 Mon Sep 17 00:00:00 2001 From: Kenjiro Nakayama Date: Tue, 4 Jun 2019 18:18:40 +0900 Subject: [PATCH 2/2] Get route name based on label value instead of same name --- pkg/reconciler/configuration/configuration.go | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/pkg/reconciler/configuration/configuration.go b/pkg/reconciler/configuration/configuration.go index 44c6782908ad..36ddee1a9b32 100644 --- a/pkg/reconciler/configuration/configuration.go +++ b/pkg/reconciler/configuration/configuration.go @@ -311,10 +311,17 @@ func (c *Reconciler) gcRevisions(ctx context.Context, config *v1alpha1.Configura return nil } - route, err := c.routeLister.Routes(config.Namespace).Get(config.Name) - if err != nil && !errors.IsNotFound(err) { - // Go ahead GC process if route not found. - return err + 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