Fix problem of Configuration becomes ready prematurally

taragu · Nov 25, 2019 · 2a07865 · 2a07865
1 parent 78673fa
commit 2a07865
Show file tree

Hide file tree

Showing 6 changed files with 107 additions and 23 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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) {