Reconcile Configuration first when using named Revisions

pkg/reconciler/configuration/configuration.go

@@ -127,6 +127,10 @@ func (c *Reconciler) reconcile(ctx context.Context, config *v1alpha1.Configurati
 		return err
 	}
 
+	// Bump observed generation to denote that we have processed this
+	// generation regardless of success or failure.
+	config.Status.ObservedGeneration = config.Generation
+
 	// First, fetch the revision that should exist for the current generation.
 	lcr, err := c.latestCreatedRevision(config)
 	if errors.IsNotFound(err) {
@@ -158,7 +162,6 @@ func (c *Reconciler) reconcile(ctx context.Context, config *v1alpha1.Configurati
 
 	// Second, set this to be the latest revision that we have created.
 	config.Status.SetLatestCreatedRevisionName(revName)
-	config.Status.ObservedGeneration = config.Generation
 
 	// Last, determine whether we should set LatestReadyRevisionName to our
 	// LatestCreatedRevision based on its readiness.

pkg/reconciler/configuration/configuration_test.go

@@ -166,7 +166,7 @@ func TestReconcile(t *testing.T) {
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
 			Object: cfg("byo-name-wrong-gen-wrong-spec", "foo", 1234, func(cfg *v1alpha1.Configuration) {
 				cfg.Spec.GetTemplate().Name = "byo-name-wrong-gen-wrong-spec-foo"
-			}, MarkRevisionCreationFailed(`revisions.serving.knative.dev "byo-name-wrong-gen-wrong-spec-foo" already exists`)),
+			}, MarkRevisionCreationFailed(`revisions.serving.knative.dev "byo-name-wrong-gen-wrong-spec-foo" already exists`), WithObservedGen),
 		}},
 		Key: "foo/byo-name-wrong-gen-wrong-spec",
 	}, {
@@ -184,7 +184,7 @@ func TestReconcile(t *testing.T) {
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
 			Object: cfg("byo-rev-not-owned", "foo", 1234, func(cfg *v1alpha1.Configuration) {
 				cfg.Spec.GetTemplate().Name = "byo-rev-not-owned-foo"
-			}, MarkRevisionCreationFailed(`revisions.serving.knative.dev "byo-rev-not-owned-foo" already exists`)),
+			}, MarkRevisionCreationFailed(`revisions.serving.knative.dev "byo-rev-not-owned-foo" already exists`), WithObservedGen),
 		}},
 		Key: "foo/byo-rev-not-owned",
 	}, {
@@ -200,7 +200,7 @@ func TestReconcile(t *testing.T) {
 		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
 			Object: cfg("validation-failure", "foo", 1234, WithConfigContainerConcurrency(-1),
 				// Expect Revision creation to fail with the following error.
-				MarkRevisionCreationFailed("expected 0 <= -1 <= 1000: spec.containerConcurrency")),
+				MarkRevisionCreationFailed("expected 0 <= -1 <= 1000: spec.containerConcurrency"), WithObservedGen),
 		}},
 		WantEvents: []string{
 			Eventf(corev1.EventTypeWarning, "CreationFailed", "Failed to create Revision for Configuration %q: %v",
@@ -311,7 +311,7 @@ func TestReconcile(t *testing.T) {
 			Object: cfg("create-revision-failure", "foo", 99998,
 				// When we fail to create a Revision is should be surfaced in
 				// the Configuration status.
-				MarkRevisionCreationFailed("inducing failure for create revisions")),
+				MarkRevisionCreationFailed("inducing failure for create revisions"), WithObservedGen),
 		}},
 		WantEvents: []string{
 			Eventf(corev1.EventTypeWarning, "CreationFailed", "Failed to create Revision for Configuration %q: %v",

pkg/reconciler/service/service.go

@@ -152,6 +152,12 @@ func (c *Reconciler) reconcile(ctx context.Context, service *v1alpha1.Service) e
 		// The Configuration hasn't yet reconciled our latest changes to
 		// its desired state, so its conditions are outdated.
 		service.Status.MarkConfigurationNotReconciled()
+
+		// If BYO-Revision name is used we must serialize reconciling the Configuration
+		// and Route. Wait for observed generation to match before continuing.
+		if config.Spec.GetTemplate().Name != "" {
+			return nil
+		}
 	} else {
 		// Update our Status based on the state of our underlying Configuration.
 		service.Status.PropagateConfigurationStatus(&config.Status)

pkg/reconciler/service/service_test.go

@@ -63,6 +63,43 @@ func TestReconcile(t *testing.T) {
 			Service("delete-pending", "foo", WithServiceDeletionTimestamp),
 		},
 		Key: "foo/delete-pending",
+	}, {
+		Name: "inline - byo rev name used in traffic serialize",
+		Objects: []runtime.Object{
+			Service("byo-rev", "foo", WithInlineNamedRevision),
+			config("byo-rev", "foo", WithInlineNamedRevision, WithGeneration(2)),
+		},
+		// Route should not be created until config progresses
+		WantCreates: []runtime.Object{},
+		Key:         "foo/byo-rev",
+		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
+			Object: Service("byo-rev", "foo", WithInlineNamedRevision,
+				// Route conditions should be at init state while Config should be OutOfDate
+				WithInitSvcConditions, WithOutOfDateConfig),
+		}},
+		WantEvents: []string{
+			Eventf(corev1.EventTypeNormal, "Updated", "Updated Service %q", "byo-rev"),
+		},
+	}, {
+		Name: "inline - byo rev name used in traffic",
+		Objects: []runtime.Object{
+			Service("byo-rev", "foo", WithInlineNamedRevision),
+		},
+		Key: "foo/byo-rev",
+		WantCreates: []runtime.Object{
+			config("byo-rev", "foo", WithInlineNamedRevision),
+			route("byo-rev", "foo", WithInlineNamedRevision),
+		},
+		WantStatusUpdates: []clientgotesting.UpdateActionImpl{{
+			Object: Service("byo-rev", "foo", WithInlineNamedRevision,
+				// The first reconciliation will initialize the status conditions.
+				WithInitSvcConditions),
+		}},
+		WantEvents: []string{
+			Eventf(corev1.EventTypeNormal, "Created", "Created Configuration %q", "byo-rev"),
+			Eventf(corev1.EventTypeNormal, "Created", "Created Route %q", "byo-rev"),
+			Eventf(corev1.EventTypeNormal, "Updated", "Updated Service %q", "byo-rev"),
+		},
 	}, {
 		Name: "inline - create route and service",
 		Objects: []runtime.Object{

pkg/testing/v1alpha1/service.go

@@ -115,6 +115,38 @@ func WithInlineRollout(s *v1alpha1.Service) {
 	}
 }
 
+// WithInlineNamedRevision configures the Service to use BYO Revision in the
+// template spec and reference that same revision name in the route spec.
+func WithInlineNamedRevision(s *v1alpha1.Service) {
+	s.Spec = v1alpha1.ServiceSpec{
+		ConfigurationSpec: v1alpha1.ConfigurationSpec{
+			Template: &v1alpha1.RevisionTemplateSpec{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: s.Name + "-byo",
+				},
+				Spec: v1alpha1.RevisionSpec{
+					RevisionSpec: v1.RevisionSpec{
+						PodSpec: corev1.PodSpec{
+							Containers: []corev1.Container{{
+								Image: "busybox",
+							}},
+						},
+						TimeoutSeconds: ptr.Int64(60),
+					},
+				},
+			},
+		},
+		RouteSpec: v1alpha1.RouteSpec{
+			Traffic: []v1alpha1.TrafficTarget{{
+				TrafficTarget: v1.TrafficTarget{
+					RevisionName: s.Name + "-byo",
+					Percent:      ptr.Int64(100),
+				},
+			}},
+		},
+	}
+}
+
 // WithRunLatestRollout configures the Service to use a "runLatest" rollout.
 func WithRunLatestRollout(s *v1alpha1.Service) {
 	s.Spec = v1alpha1.ServiceSpec{
@@ -410,6 +442,12 @@ func WithFailedRoute(reason, message string) ServiceOption {
 	}
 }
 
+// WithOutOfDateConfig reflects the Configuration's readiness in the Service
+// resource.
+func WithOutOfDateConfig(s *v1alpha1.Service) {
+	s.Status.MarkConfigurationNotReconciled()
+}
+
 // WithReadyConfig reflects the Configuration's readiness in the Service
 // resource.  This must coincide with the setting of Latest{Created,Ready}
 // to the provided revision name.