From 2a0786573e2e96c06cf54172596f7394aa022e8b Mon Sep 17 00:00:00 2001 From: Tara Gu Date: Mon, 25 Nov 2019 10:50:23 -0500 Subject: [PATCH] Fix problem of Configuration becomes ready prematurally --- .../v1alpha1/configuration_lifecycle.go | 17 +++++-- .../v1alpha1/configuration_lifecycle_test.go | 6 +++ pkg/reconciler/configuration/configuration.go | 8 +++- .../configuration/configuration_test.go | 45 +++++++++++++++++-- pkg/reconciler/service/service_test.go | 40 +++++++++++------ pkg/testing/v1alpha1/configuration.go | 14 ++++++ 6 files changed, 107 insertions(+), 23 deletions(-) diff --git a/pkg/apis/serving/v1alpha1/configuration_lifecycle.go b/pkg/apis/serving/v1alpha1/configuration_lifecycle.go index c0b4a6653515..ca6d400d8bdd 100644 --- a/pkg/apis/serving/v1alpha1/configuration_lifecycle.go +++ b/pkg/apis/serving/v1alpha1/configuration_lifecycle.go @@ -79,18 +79,27 @@ func (cs *ConfigurationStatus) InitializeConditions() { func (cs *ConfigurationStatus) SetLatestCreatedRevisionName(name string) { cs.LatestCreatedRevisionName = name if cs.LatestReadyRevisionName != name { - confCondSet.Manage(cs).MarkUnknown( - ConfigurationConditionReady, - "", - "") + cs.MarkConfigurationUnknown() } } func (cs *ConfigurationStatus) SetLatestReadyRevisionName(name string) { cs.LatestReadyRevisionName = name +} + +// MarkConfigurationReady sets the Ready state of a configuration to True +func (cs *ConfigurationStatus) MarkConfigurationReady() { confCondSet.Manage(cs).MarkTrue(ConfigurationConditionReady) } +// MarkConfigurationUnknown sets the Ready state of a configuration to Unknown +func (cs *ConfigurationStatus) MarkConfigurationUnknown() { + confCondSet.Manage(cs).MarkUnknown( + ConfigurationConditionReady, + "", + "") +} + func (cs *ConfigurationStatus) MarkLatestCreatedFailed(name, message string) { confCondSet.Manage(cs).MarkFalse( ConfigurationConditionReady, diff --git a/pkg/apis/serving/v1alpha1/configuration_lifecycle_test.go b/pkg/apis/serving/v1alpha1/configuration_lifecycle_test.go index abd2d519624d..31684cd448b5 100644 --- a/pkg/apis/serving/v1alpha1/configuration_lifecycle_test.go +++ b/pkg/apis/serving/v1alpha1/configuration_lifecycle_test.go @@ -224,6 +224,7 @@ func TestTypicalFlow(t *testing.T) { apitestv1.CheckConditionOngoing(r.duck(), ConfigurationConditionReady, t) r.SetLatestReadyRevisionName("foo") + r.MarkConfigurationReady() apitestv1.CheckConditionSucceeded(r.duck(), ConfigurationConditionReady, t) // Verify a second call to SetLatestCreatedRevisionName doesn't change the status from Ready @@ -235,6 +236,7 @@ func TestTypicalFlow(t *testing.T) { apitestv1.CheckConditionOngoing(r.duck(), ConfigurationConditionReady, t) r.SetLatestReadyRevisionName("bar") + r.MarkConfigurationReady() apitestv1.CheckConditionSucceeded(r.duck(), ConfigurationConditionReady, t) r.MarkResourceNotConvertible(ConvertErrorf("build", "something something not allowed.").(*CannotConvertError)) @@ -272,6 +274,7 @@ func TestFailingFirstRevisionWithRecovery(t *testing.T) { // When the new revision becomes ready, then Ready becomes true as well. r.SetLatestReadyRevisionName("bar") + r.MarkConfigurationReady() apitestv1.CheckConditionSucceeded(r.duck(), ConfigurationConditionReady, t) } @@ -284,6 +287,7 @@ func TestFailingSecondRevision(t *testing.T) { apitestv1.CheckConditionOngoing(r.duck(), ConfigurationConditionReady, t) r.SetLatestReadyRevisionName("foo") + r.MarkConfigurationReady() apitestv1.CheckConditionSucceeded(r.duck(), ConfigurationConditionReady, t) r.SetLatestCreatedRevisionName("bar") @@ -307,6 +311,7 @@ func TestLatestRevisionDeletedThenFixed(t *testing.T) { apitestv1.CheckConditionOngoing(r.duck(), ConfigurationConditionReady, t) r.SetLatestReadyRevisionName("foo") + r.MarkConfigurationReady() apitestv1.CheckConditionSucceeded(r.duck(), ConfigurationConditionReady, t) // When the latest revision is deleted, the Configuration became Failed. @@ -322,6 +327,7 @@ func TestLatestRevisionDeletedThenFixed(t *testing.T) { apitestv1.CheckConditionOngoing(r.duck(), ConfigurationConditionReady, t) r.SetLatestReadyRevisionName("bar") + r.MarkConfigurationReady() apitestv1.CheckConditionSucceeded(r.duck(), ConfigurationConditionReady, t) } diff --git a/pkg/reconciler/configuration/configuration.go b/pkg/reconciler/configuration/configuration.go index 3e5a8f64aa3c..49644421549b 100644 --- a/pkg/reconciler/configuration/configuration.go +++ b/pkg/reconciler/configuration/configuration.go @@ -187,14 +187,14 @@ func (c *Reconciler) reconcile(ctx context.Context, config *v1alpha1.Configurati return fmt.Errorf("unrecognized condition status: %v on revision %q", rc.Status, revName) } - if err = c.findAndSetLatestReadyRevision(config); err != nil { + if err = c.findAndSetLatestReadyRevision(config, rc != nil && rc.Status == corev1.ConditionTrue); err != nil { return fmt.Errorf("failed to find and set latest ready revision: %w", err) } return nil } // findAndSetLatestReadyRevision finds the last ready revision and sets LatestReadyRevisionName to it. -func (c *Reconciler) findAndSetLatestReadyRevision(config *v1alpha1.Configuration) error { +func (c *Reconciler) findAndSetLatestReadyRevision(config *v1alpha1.Configuration, lcrReady bool) error { sortedRevisions, err := c.getSortedCreatedRevisions(config) if err != nil { return err @@ -203,6 +203,10 @@ func (c *Reconciler) findAndSetLatestReadyRevision(config *v1alpha1.Configuratio if rev.Status.IsReady() { old, new := config.Status.LatestReadyRevisionName, rev.Name config.Status.SetLatestReadyRevisionName(rev.Name) + // Only mark config ready if latest created revision is ready + if lcrReady { + config.Status.MarkConfigurationReady() + } if old != new { c.Recorder.Eventf(config, corev1.EventTypeNormal, "LatestReadyUpdate", "LatestReadyRevisionName updated to %q", rev.Name) diff --git a/pkg/reconciler/configuration/configuration_test.go b/pkg/reconciler/configuration/configuration_test.go index a0a1cd4f0ddd..4587b27d8348 100644 --- a/pkg/reconciler/configuration/configuration_test.go +++ b/pkg/reconciler/configuration/configuration_test.go @@ -234,7 +234,8 @@ func TestReconcile(t *testing.T) { // When we see the LatestCreatedRevision become Ready, then we // update the latest ready revision. WithLatestReady("matching-revision-done-00001"), - WithLatestCreated("matching-revision-done-00001")), + WithLatestCreated("matching-revision-done-00001"), + WithConfigurationReady()), }}, WantEvents: []string{ Eventf(corev1.EventTypeNormal, "ConfigurationReady", "Configuration becomes ready"), @@ -246,7 +247,8 @@ func TestReconcile(t *testing.T) { Name: "reconcile revision matching generation (ready: true, idempotent)", Objects: []runtime.Object{ cfg("matching-revision-done-idempotent", "foo", 5566, - WithObservedGen, WithLatestCreated("matching-revision"), WithLatestReady("matching-revision")), + WithObservedGen, WithLatestCreated("matching-revision"), WithLatestReady("matching-revision"), + WithConfigurationReady()), rev("matching-revision-done-idempotent", "foo", 5566, WithCreationTimestamp(now), MarkRevisionReady, WithRevName("matching-revision")), }, @@ -358,6 +360,7 @@ func TestReconcile(t *testing.T) { Object: cfg("revision-recovers", "foo", 1337, WithLatestCreated("revision-recovers-00001"), WithLatestReady("revision-recovers-00001"), + WithConfigurationReady(), WithObservedGen, // When a LatestReadyRevision recovers from failure, // then we should go back to Ready. @@ -374,10 +377,10 @@ func TestReconcile(t *testing.T) { // when no fix is present cfg("double-trouble", "foo", 1, WithLatestCreated("double-trouble-00001"), - WithLatestReady("double-trouble-00001"), WithObservedGen), + WithLatestReady("double-trouble-00001"), WithConfigurationReady(), WithObservedGen), cfg("first-trouble", "foo", 1, WithLatestCreated("first-trouble-00001"), - WithLatestReady("first-trouble-00001"), WithObservedGen), + WithLatestReady("first-trouble-00001"), WithConfigurationReady(), WithObservedGen), rev("first-trouble", "foo", 1, WithRevName("first-trouble-00001"), @@ -410,6 +413,7 @@ func TestReconcile(t *testing.T) { Object: cfg("threerevs", "foo", 3, WithLatestCreated("threerevs-00003"), WithLatestReady("threerevs-00002"), + WithConfigurationUnknown(), WithObservedGen, func(cfg *v1alpha1.Configuration) { cfg.Spec.GetTemplate().Name = "threerevs-00003" }, @@ -419,6 +423,39 @@ func TestReconcile(t *testing.T) { Eventf(corev1.EventTypeNormal, "LatestReadyUpdate", "LatestReadyRevisionName updated to %q", "threerevs-00002"), }, Key: "foo/threerevs", + }, { + Name: "revision not ready, the latest ready should be updated, but the configuration should still be ready==Unknown", + Objects: []runtime.Object{ + cfg("revnotready", "foo", 3, + WithLatestCreated("revnotready-00002"), + WithLatestReady("revnotready-00001"), WithObservedGen, func(cfg *v1alpha1.Configuration) { + cfg.Spec.GetTemplate().Name = "revnotready-00003" + }, + ), + rev("revnotready", "foo", 1, + WithRevName("revnotready-00001"), + WithCreationTimestamp(now), MarkRevisionReady), + rev("revnotready", "foo", 2, + WithRevName("revnotready-00002"), + WithCreationTimestamp(now), MarkRevisionReady), + rev("revnotready", "foo", 3, + WithRevName("revnotready-00003"), + WithCreationTimestamp(now)), + }, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: cfg("revnotready", "foo", 3, + WithLatestCreated("revnotready-00003"), + WithLatestReady("revnotready-00002"), + WithConfigurationUnknown(), + WithObservedGen, func(cfg *v1alpha1.Configuration) { + cfg.Spec.GetTemplate().Name = "revnotready-00003" + }, + ), + }}, + WantEvents: []string{ + Eventf(corev1.EventTypeNormal, "LatestReadyUpdate", "LatestReadyRevisionName updated to %q", "revnotready-00002"), + }, + Key: "foo/revnotready", }} table.Test(t, MakeFactory(func(ctx context.Context, listers *Listers, cmw configmap.Watcher) controller.Reconciler { diff --git a/pkg/reconciler/service/service_test.go b/pkg/reconciler/service/service_test.go index b471d02d3f23..3ab365f1716b 100644 --- a/pkg/reconciler/service/service_test.go +++ b/pkg/reconciler/service/service_test.go @@ -210,7 +210,8 @@ func TestReconcile(t *testing.T) { config("pinned3", "foo", WithReleaseRollout("pinned3-00001"), WithGeneration(1), WithObservedGen, WithLatestCreated("pinned3-00001"), - WithLatestReady("pinned3-00001")), + WithLatestReady("pinned3-00001"), + WithConfigurationReady()), route("pinned3", "foo", WithReleaseRollout("pinned3-00001"), WithURL, WithAddress, WithInitRouteConditions, WithStatusTraffic(v1alpha1.TrafficTarget{ @@ -324,7 +325,8 @@ func TestReconcile(t *testing.T) { Objects: []runtime.Object{ Service("release-nr", "foo", WithReleaseRollout("release-nr-00002"), WithInitSvcConditions), config("release-nr", "foo", WithReleaseRollout("release-nr-00002"), - WithCreatedAndReady("release-nr-00002", "release-nr-00002")), + WithCreatedAndReady("release-nr-00002", "release-nr-00002"), + WithConfigurationReady()), // NB: route points to the previous revision. route("release-nr", "foo", WithReleaseRollout("release-nr-00002"), RouteReady, WithURL, WithAddress, WithInitRouteConditions, @@ -363,7 +365,8 @@ func TestReconcile(t *testing.T) { Objects: []runtime.Object{ Service("release-nr", "foo", WithReleaseRollout("release-nr-00002", "release-nr-00003"), WithInitSvcConditions), config("release-nr", "foo", WithReleaseRollout("release-nr-00002", "release-nr-00003"), - WithCreatedAndReady("release-nr-00003", "release-nr-00003")), + WithCreatedAndReady("release-nr-00003", "release-nr-00003"), + WithConfigurationReady()), // NB: route points to the previous revision. route("release-nr", "foo", WithReleaseRollout("release-nr-00002", "release-nr-00003"), RouteReady, WithURL, WithAddress, WithInitRouteConditions, @@ -405,7 +408,8 @@ func TestReconcile(t *testing.T) { WithInitSvcConditions), config("release-nr-ts", "foo", WithReleaseRolloutAndPercentage(42, "release-nr-ts-00002", "release-nr-ts-00003"), - WithCreatedAndReady("release-nr-ts-00003", "release-nr-ts-00003")), + WithCreatedAndReady("release-nr-ts-00003", "release-nr-ts-00003"), + WithConfigurationReady()), route("release-nr-ts", "foo", WithReleaseRolloutAndPercentage(42, "release-nr-ts-00002", "release-nr-ts-00003"), RouteReady, WithURL, WithAddress, WithInitRouteConditions, @@ -457,7 +461,8 @@ func TestReconcile(t *testing.T) { WithInitSvcConditions), config("release-nr-ts2", "foo", WithReleaseRolloutAndPercentage(58, "release-nr-ts2-00002", "release-nr-ts2-00003"), - WithCreatedAndReady("release-nr-ts2-00003", "release-nr-ts2-00003")), + WithCreatedAndReady("release-nr-ts2-00003", "release-nr-ts2-00003"), + WithConfigurationReady()), route("release-nr-ts2", "foo", WithReleaseRolloutAndPercentage(58, "release-nr-ts2-00002", "release-nr-ts2-00003"), RouteReady, WithURL, WithAddress, WithInitRouteConditions, @@ -526,7 +531,8 @@ func TestReconcile(t *testing.T) { WithGeneration(1), WithObservedGen, // These turn a Configuration to Ready=true WithLatestCreated("release-ready-lr-00001"), - WithLatestReady("release-ready-lr-00001")), + WithLatestReady("release-ready-lr-00001"), + WithConfigurationReady()), }, Key: "foo/release-ready-lr", WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ @@ -595,7 +601,8 @@ func TestReconcile(t *testing.T) { WithGeneration(2), WithObservedGen, // These turn a Configuration to Ready=true WithLatestCreated("release-ready-lr-00002"), - WithLatestReady("release-ready-lr-00002")), + WithLatestReady("release-ready-lr-00002"), + WithConfigurationReady()), }, Key: "foo/release-ready-lr", WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ @@ -671,7 +678,8 @@ func TestReconcile(t *testing.T) { config("release-ready", "foo", WithRunLatestRollout, WithGeneration(2), WithObservedGen, // These turn a Configuration to Ready=true - WithLatestCreated("release-ready-00002"), WithLatestReady("release-ready-00002")), + WithLatestCreated("release-ready-00002"), WithLatestReady("release-ready-00002"), + WithConfigurationReady()), }, Key: "foo/release-ready", WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ @@ -1092,7 +1100,8 @@ func TestReconcile(t *testing.T) { config("all-ready", "foo", WithRunLatestRollout, WithGeneration(1), WithObservedGen, // These turn a Configuration to Ready=true - WithLatestCreated("all-ready-00001"), WithLatestReady("all-ready-00001")), + WithLatestCreated("all-ready-00001"), WithLatestReady("all-ready-00001"), + WithConfigurationReady()), }, Key: "foo/all-ready", WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ @@ -1137,7 +1146,8 @@ func TestReconcile(t *testing.T) { config("all-ready", "foo", WithRunLatestRollout, WithGeneration(1), WithObservedGen, WithGeneration(2), // These turn a Configuration to Ready=true - WithLatestCreated("all-ready-00001"), WithLatestReady("all-ready-00001")), + WithLatestCreated("all-ready-00001"), WithLatestReady("all-ready-00001"), + WithConfigurationReady()), }, Key: "foo/all-ready", WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ @@ -1180,7 +1190,8 @@ func TestReconcile(t *testing.T) { config("config-only-ready", "foo", WithRunLatestRollout, WithGeneration(2 /*will generate revision -00002*/), WithObservedGen, // These turn a Configuration to Ready=true - WithLatestCreated("config-only-ready-00002"), WithLatestReady("config-only-ready-00002")), + WithLatestCreated("config-only-ready-00002"), WithLatestReady("config-only-ready-00002"), + WithConfigurationReady()), }, Key: "foo/config-only-ready", WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ @@ -1221,6 +1232,7 @@ func TestReconcile(t *testing.T) { }), MarkTrafficAssigned, MarkIngressReady), config("config-fails", "foo", WithRunLatestRollout, WithGeneration(2), WithLatestReady("config-fails-00001"), WithLatestCreated("config-fails-00002"), + WithConfigurationReady(), MarkLatestCreatedFailed("blah"), WithObservedGen), }, Key: "foo/config-fails", @@ -1280,7 +1292,8 @@ func TestReconcile(t *testing.T) { RouteFailed("Propagate me, please", "")), config("route-fails", "foo", WithRunLatestRollout, WithGeneration(1), WithObservedGen, // These turn a Configuration to Ready=true - WithLatestCreated("route-fails-00001"), WithLatestReady("route-fails-00001")), + WithLatestCreated("route-fails-00001"), WithLatestReady("route-fails-00001"), + WithConfigurationReady()), }, Key: "foo/route-fails", WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ @@ -1352,7 +1365,8 @@ func TestReconcile(t *testing.T) { }), MarkTrafficAssigned, MarkIngressReady), config("new-owner", "foo", WithRunLatestRollout, WithGeneration(1), WithObservedGen, // These turn a Configuration to Ready=true - WithLatestCreated("new-owner-00001"), WithLatestReady("new-owner-00001")), + WithLatestCreated("new-owner-00001"), WithLatestReady("new-owner-00001"), + WithConfigurationReady()), }, Key: "foo/new-owner", WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ diff --git a/pkg/testing/v1alpha1/configuration.go b/pkg/testing/v1alpha1/configuration.go index b59aac77e1c4..1d930469c46e 100644 --- a/pkg/testing/v1alpha1/configuration.go +++ b/pkg/testing/v1alpha1/configuration.go @@ -82,6 +82,20 @@ func WithLatestReady(name string) ConfigOption { } } +// WithConfigurationReady sets the ConfigurationConditionReady condition to True +func WithConfigurationReady() ConfigOption { + return func(cfg *v1alpha1.Configuration) { + cfg.Status.MarkConfigurationReady() + } +} + +// WithConfigurationUnknown sets the ConfigurationConditionReady condition to Unknown +func WithConfigurationUnknown() ConfigOption { + return func(cfg *v1alpha1.Configuration) { + cfg.Status.MarkConfigurationUnknown() + } +} + // MarkRevisionCreationFailed calls .Status.MarkRevisionCreationFailed. func MarkRevisionCreationFailed(msg string) ConfigOption { return func(cfg *v1alpha1.Configuration) {