From fb8d9ec042d4a0dc54507804139b460ca3c24b04 Mon Sep 17 00:00:00 2001 From: Tara Gu Date: Tue, 21 Jan 2020 16:48:49 -0500 Subject: [PATCH 1/4] Continue to set latest ready revision if error encountered Currently in the config reconciler, if the LRR does not exist, we don't continue the rest of the logic to set the LRR. This PR fixes it by logging the error and continuing. --- pkg/reconciler/configuration/configuration.go | 41 ++++++++++--------- .../configuration/configuration_test.go | 29 +++++++++++++ 2 files changed, 51 insertions(+), 19 deletions(-) diff --git a/pkg/reconciler/configuration/configuration.go b/pkg/reconciler/configuration/configuration.go index 5405c9a6f38d..c3806a4006b6 100644 --- a/pkg/reconciler/configuration/configuration.go +++ b/pkg/reconciler/configuration/configuration.go @@ -127,7 +127,7 @@ func (c *Reconciler) ReconcileKind(ctx context.Context, config *v1.Configuration // findAndSetLatestReadyRevision finds the last ready revision and sets LatestReadyRevisionName to it. func (c *Reconciler) findAndSetLatestReadyRevision(ctx context.Context, config *v1.Configuration) error { - sortedRevisions, err := c.getSortedCreatedRevisions(config) + sortedRevisions, err := c.getSortedCreatedRevisions(ctx, config) if err != nil { return err } @@ -148,33 +148,36 @@ func (c *Reconciler) findAndSetLatestReadyRevision(ctx context.Context, config * // getSortedCreatedRevisions returns the list of created revisions sorted in descending // generation order between the generation of the latest ready revision and config's generation (both inclusive). -func (c *Reconciler) getSortedCreatedRevisions(config *v1.Configuration) ([]*v1.Revision, error) { +func (c *Reconciler) getSortedCreatedRevisions(ctx context.Context, config *v1.Configuration) ([]*v1.Revision, error) { + logger := logging.FromContext(ctx) lister := c.revisionLister.Revisions(config.Namespace) configSelector := labels.SelectorFromSet(labels.Set{ serving.ConfigurationLabelKey: config.Name, }) if config.Status.LatestReadyRevisionName != "" { lrr, err := lister.Get(config.Status.LatestReadyRevisionName) + // Record the error and continue because we still want to set the LRR to the correct revision if err != nil { - return nil, err - } - start := lrr.Generation - var generations []string - for i := start; i <= int64(config.Generation); i++ { - generations = append(generations, strconv.FormatInt(i, 10)) - } + logger.Errorf("Error getting latest ready revision %q: %v", config.Status.LatestReadyRevisionName, err) + } else { + start := lrr.Generation + var generations []string + for i := start; i <= int64(config.Generation); i++ { + generations = append(generations, strconv.FormatInt(i, 10)) + } - // Add an "In" filter so that the configurations we get back from List have generation - // in range (config's latest ready generation, config's generation] - generationKey := serving.ConfigurationGenerationLabelKey - inReq, err := labels.NewRequirement(generationKey, - selection.In, - generations, - ) - if err != nil { - return nil, err + // Add an "In" filter so that the configurations we get back from List have generation + // in range (config's latest ready generation, config's generation] + generationKey := serving.ConfigurationGenerationLabelKey + inReq, err := labels.NewRequirement(generationKey, + selection.In, + generations, + ) + if err != nil { + return nil, err + } + configSelector = configSelector.Add(*inReq) } - configSelector = configSelector.Add(*inReq) } list, err := lister.List(configSelector) diff --git a/pkg/reconciler/configuration/configuration_test.go b/pkg/reconciler/configuration/configuration_test.go index 7a2f2f8de0c1..c8828dbd294a 100644 --- a/pkg/reconciler/configuration/configuration_test.go +++ b/pkg/reconciler/configuration/configuration_test.go @@ -468,6 +468,35 @@ func TestReconcile(t *testing.T) { Eventf(corev1.EventTypeNormal, "LatestReadyUpdate", "LatestReadyRevisionName updated to %q", "revnotready-00002"), }, Key: "foo/revnotready", + }, { + Name: "current LRR doesn't exist, LCR is ready", + Objects: []runtime.Object{ + cfg("lrrnotexist", "foo", 2, + WithLatestCreated("lrrnotexist-00002"), + WithLatestReady("lrrnotexist-00001"), WithObservedGen, func(cfg *v1alpha1.Configuration) { + cfg.Spec.GetTemplate().Name = "lrrnotexist-00002" + }, + ), + rev("lrrnotexist", "foo", 1, + WithRevName("lrrnotexist-00000"), + WithCreationTimestamp(now), MarkRevisionReady), + rev("lrrnotexist", "foo", 2, + WithRevName("lrrnotexist-00002"), + WithCreationTimestamp(now), MarkRevisionReady), + }, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: cfg("lrrnotexist", "foo", 2, + WithLatestCreated("lrrnotexist-00002"), + WithLatestReady("lrrnotexist-00002"), + WithObservedGen, func(cfg *v1alpha1.Configuration) { + cfg.Spec.GetTemplate().Name = "lrrnotexist-00002" + }, + ), + }}, + WantEvents: []string{ + Eventf(corev1.EventTypeNormal, "LatestReadyUpdate", "LatestReadyRevisionName updated to %q", "lrrnotexist-00002"), + }, + Key: "foo/lrrnotexist", }} table.Test(t, MakeFactory(func(ctx context.Context, listers *Listers, cmw configmap.Watcher) controller.Reconciler { From 72e1cb6a01fa4f1bfa899919e5d130b0e32c5bef Mon Sep 17 00:00:00 2001 From: Tara Gu Date: Wed, 5 Feb 2020 10:28:50 -0500 Subject: [PATCH 2/4] Review --- pkg/reconciler/configuration/configuration.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/reconciler/configuration/configuration.go b/pkg/reconciler/configuration/configuration.go index c3806a4006b6..65356e9285a1 100644 --- a/pkg/reconciler/configuration/configuration.go +++ b/pkg/reconciler/configuration/configuration.go @@ -156,7 +156,7 @@ func (c *Reconciler) getSortedCreatedRevisions(ctx context.Context, config *v1.C }) if config.Status.LatestReadyRevisionName != "" { lrr, err := lister.Get(config.Status.LatestReadyRevisionName) - // Record the error and continue because we still want to set the LRR to the correct revision + // Record the error and continue because we still want to set the LRR to the correct revision. if err != nil { logger.Errorf("Error getting latest ready revision %q: %v", config.Status.LatestReadyRevisionName, err) } else { @@ -173,10 +173,9 @@ func (c *Reconciler) getSortedCreatedRevisions(ctx context.Context, config *v1.C selection.In, generations, ) - if err != nil { - return nil, err + if err == nil { + configSelector = configSelector.Add(*inReq) } - configSelector = configSelector.Add(*inReq) } } From 317b902d52b2fa08395e72034503528f7dbc6dc0 Mon Sep 17 00:00:00 2001 From: Tara Gu Date: Thu, 20 Feb 2020 12:06:31 -0500 Subject: [PATCH 3/4] Fix rebase --- pkg/reconciler/configuration/configuration_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/reconciler/configuration/configuration_test.go b/pkg/reconciler/configuration/configuration_test.go index c8828dbd294a..7bd8575a0762 100644 --- a/pkg/reconciler/configuration/configuration_test.go +++ b/pkg/reconciler/configuration/configuration_test.go @@ -473,7 +473,7 @@ func TestReconcile(t *testing.T) { Objects: []runtime.Object{ cfg("lrrnotexist", "foo", 2, WithLatestCreated("lrrnotexist-00002"), - WithLatestReady("lrrnotexist-00001"), WithObservedGen, func(cfg *v1alpha1.Configuration) { + WithLatestReady("lrrnotexist-00001"), WithObservedGen, func(cfg *v1.Configuration) { cfg.Spec.GetTemplate().Name = "lrrnotexist-00002" }, ), @@ -488,7 +488,7 @@ func TestReconcile(t *testing.T) { Object: cfg("lrrnotexist", "foo", 2, WithLatestCreated("lrrnotexist-00002"), WithLatestReady("lrrnotexist-00002"), - WithObservedGen, func(cfg *v1alpha1.Configuration) { + WithObservedGen, func(cfg *v1.Configuration) { cfg.Spec.GetTemplate().Name = "lrrnotexist-00002" }, ), From 40791d877c9f56c5b55ed90abcc1480f4c3f32f6 Mon Sep 17 00:00:00 2001 From: Tara Gu Date: Fri, 21 Feb 2020 09:18:07 -0500 Subject: [PATCH 4/4] Review --- pkg/reconciler/configuration/configuration.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/reconciler/configuration/configuration.go b/pkg/reconciler/configuration/configuration.go index 65356e9285a1..decce9eb764d 100644 --- a/pkg/reconciler/configuration/configuration.go +++ b/pkg/reconciler/configuration/configuration.go @@ -158,6 +158,9 @@ func (c *Reconciler) getSortedCreatedRevisions(ctx context.Context, config *v1.C lrr, err := lister.Get(config.Status.LatestReadyRevisionName) // Record the error and continue because we still want to set the LRR to the correct revision. if err != nil { + // If the user deletes the LatestReadyRevision then this may return an error due to the + // dangling reference. Proceed to calculate the next-latest ready revision so that the + // caller can synthesize a new Revision at the current generation to replace the one deleted. logger.Errorf("Error getting latest ready revision %q: %v", config.Status.LatestReadyRevisionName, err) } else { start := lrr.Generation