From 7bfc8df6a968e9d6a052799fe358f41b253bed28 Mon Sep 17 00:00:00 2001 From: Dan Gerdesmeier Date: Sun, 15 Sep 2019 19:30:24 +0000 Subject: [PATCH 1/2] Reconcile Configuration first when using named Revisions This change fixes a problem where RouteReady will periodically go from Unknown to False and then to True when reconciling a Service that uses the "BYO Revision Name" functionality. If the Route reconciles before a Revision has been created it will report a False status and cause clients watching the status to report failure. Since Revision names can only be referenced before they created when a user brings their own name this change serializes the Configuration reconciliation to occur and stabilize before doing the Route reconciliation. Fixes #4173 --- pkg/reconciler/configuration/configuration.go | 5 ++- .../configuration/configuration_test.go | 8 ++--- pkg/reconciler/service/service.go | 6 ++++ pkg/reconciler/service/service_test.go | 20 ++++++++++++ pkg/testing/v1alpha1/service.go | 32 +++++++++++++++++++ 5 files changed, 66 insertions(+), 5 deletions(-) diff --git a/pkg/reconciler/configuration/configuration.go b/pkg/reconciler/configuration/configuration.go index 4912963d41ae..e727aa735ec1 100644 --- a/pkg/reconciler/configuration/configuration.go +++ b/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. diff --git a/pkg/reconciler/configuration/configuration_test.go b/pkg/reconciler/configuration/configuration_test.go index 366e25df919d..2f88827c4274 100644 --- a/pkg/reconciler/configuration/configuration_test.go +++ b/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", diff --git a/pkg/reconciler/service/service.go b/pkg/reconciler/service/service.go index 5cd3c8a54a2d..472f11f3e8ae 100644 --- a/pkg/reconciler/service/service.go +++ b/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) diff --git a/pkg/reconciler/service/service_test.go b/pkg/reconciler/service/service_test.go index c4c7028a678a..d5db77b7fad0 100644 --- a/pkg/reconciler/service/service_test.go +++ b/pkg/reconciler/service/service_test.go @@ -63,6 +63,26 @@ func TestReconcile(t *testing.T) { Service("delete-pending", "foo", WithServiceDeletionTimestamp), }, Key: "foo/delete-pending", + }, { + 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{ diff --git a/pkg/testing/v1alpha1/service.go b/pkg/testing/v1alpha1/service.go index 2b8b741fc3ff..e6c63b22e073 100644 --- a/pkg/testing/v1alpha1/service.go +++ b/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{ From 25b1a2db75a96d45ae91e59d4416429d5362dfdd Mon Sep 17 00:00:00 2001 From: Dan Gerdesmeier Date: Mon, 16 Sep 2019 01:08:07 +0000 Subject: [PATCH 2/2] Add another testcase for byo revision --- pkg/reconciler/service/service_test.go | 17 +++++++++++++++++ pkg/testing/v1alpha1/service.go | 6 ++++++ 2 files changed, 23 insertions(+) diff --git a/pkg/reconciler/service/service_test.go b/pkg/reconciler/service/service_test.go index d5db77b7fad0..051ef468ca11 100644 --- a/pkg/reconciler/service/service_test.go +++ b/pkg/reconciler/service/service_test.go @@ -63,6 +63,23 @@ 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{ diff --git a/pkg/testing/v1alpha1/service.go b/pkg/testing/v1alpha1/service.go index e6c63b22e073..556817fc4404 100644 --- a/pkg/testing/v1alpha1/service.go +++ b/pkg/testing/v1alpha1/service.go @@ -442,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.