diff --git a/pkg/reconciler/testing/table.go b/pkg/reconciler/testing/table.go index 4f4bed21580f..fa70c8551255 100644 --- a/pkg/reconciler/testing/table.go +++ b/pkg/reconciler/testing/table.go @@ -130,7 +130,7 @@ func (r *TableRow) Test(t *testing.T, factory Factory) { } if diff := cmp.Diff(want, obj, ignoreLastTransitionTime, safeDeployDiff, cmpopts.EquateEmpty()); diff != "" { - t.Errorf("Unexpected create (-want +got): %s", diff) + t.Errorf("Unexpected create (-want, +got): %s", diff) } } if got, want := len(actions.Creates), len(r.WantCreates); got > want { @@ -164,7 +164,7 @@ func (r *TableRow) Test(t *testing.T, factory Factory) { objPrevState[objKey(got)] = got if diff := cmp.Diff(want.GetObject(), got, ignoreLastTransitionTime, safeDeployDiff, cmpopts.EquateEmpty()); diff != "" { - t.Errorf("Unexpected update (-want +got): %s", diff) + t.Errorf("Unexpected update (-want, +got): %s", diff) } } if got, want := len(updates), len(r.WantUpdates); got > want { @@ -195,7 +195,7 @@ func (r *TableRow) Test(t *testing.T, factory Factory) { objPrevState[objKey(got)] = got if diff := cmp.Diff(want.GetObject(), got, ignoreLastTransitionTime, safeDeployDiff, cmpopts.EquateEmpty()); diff != "" { - t.Errorf("Unexpected status update (-want +got): %s", diff) + t.Errorf("Unexpected status update (-want, +got): %s", diff) } } if got, want := len(statusUpdates), len(r.WantStatusUpdates); got > want { @@ -249,7 +249,7 @@ func (r *TableRow) Test(t *testing.T, factory Factory) { t.Errorf("Unexpected patch[%d]: %#v", i, got) } if diff := cmp.Diff(string(want.GetPatch()), string(got.GetPatch())); diff != "" { - t.Errorf("Unexpected patch(-want +got): %s", diff) + t.Errorf("Unexpected patch(-want, +got): %s", diff) } } if got, want := len(actions.Patches), len(r.WantPatches); got > want { @@ -266,7 +266,7 @@ func (r *TableRow) Test(t *testing.T, factory Factory) { } if diff := cmp.Diff(want, gotEvents[i]); diff != "" { - t.Errorf("unexpected event(-want +got): %s", diff) + t.Errorf("unexpected event(-want, +got): %s", diff) } } if got, want := len(gotEvents), len(r.WantEvents); got > want { @@ -277,7 +277,7 @@ func (r *TableRow) Test(t *testing.T, factory Factory) { gotStats := statsReporter.GetServiceReadyStats() if diff := cmp.Diff(r.WantServiceReadyStats, gotStats); diff != "" { - t.Errorf("Unexpected service ready stats (-want +got): %s", diff) + t.Errorf("Unexpected service ready stats (-want, +got): %s", diff) } } @@ -308,7 +308,7 @@ func (tt TableTest) Test(t *testing.T, factory Factory) { }) // Validate cached objects do not get soiled after controller loops if diff := cmp.Diff(originObjects, test.Objects, safeDeployDiff, cmpopts.EquateEmpty()); diff != "" { - t.Errorf("Unexpected objects in test %s (-want +got): %v", test.Name, diff) + t.Errorf("Unexpected objects in test %s (-want, +got): %v", test.Name, diff) } } } diff --git a/pkg/reconciler/v1alpha1/service/service.go b/pkg/reconciler/v1alpha1/service/service.go index 3b805dd6b681..5f9e341d25c5 100644 --- a/pkg/reconciler/v1alpha1/service/service.go +++ b/pkg/reconciler/v1alpha1/service/service.go @@ -145,6 +145,7 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { // This is important because the copy we loaded from the informer's // cache may be stale and we don't want to overwrite a prior update // to status with this stale state. + } else if _, uErr := c.updateStatus(service); uErr != nil { logger.Warn("Failed to update service status", zap.Error(uErr)) c.Recorder.Eventf(service, corev1.EventTypeWarning, "UpdateFailed", @@ -219,27 +220,25 @@ func (c *Reconciler) reconcile(ctx context.Context, service *v1alpha1.Service) e ss := &service.Status ss.PropagateRouteStatus(&route.Status) - // Apply additional logic, once the generic data has been propagated. - switch { - case service.Spec.RunLatest != nil: - runLatestCheckTrafficMigrated(ss, &route.Status, &config.Status) + // `manual` is not reconciled. + if rc := service.Status.GetCondition(v1alpha1.ServiceConditionRoutesReady); rc != nil && rc.Status == corev1.ConditionTrue { + want, got := route.Spec.DeepCopy().Traffic, route.Status.Traffic + // Replace `configuration` target with its latest ready revision. + for idx := range want { + if want[idx].ConfigurationName == config.Name { + want[idx].RevisionName = config.Status.LatestReadyRevisionName + want[idx].ConfigurationName = "" + } + } + if eq, err := kmp.SafeEqual(got, want); !eq || err != nil { + service.Status.MarkRouteNotYetReady() + } } service.Status.ObservedGeneration = service.Generation return nil } -func runLatestCheckTrafficMigrated(ss *v1alpha1.ServiceStatus, rs *v1alpha1.RouteStatus, cs *v1alpha1.ConfigurationStatus) { - // If the service's RouteReady is in favorable condition. - if rc := ss.GetCondition(v1alpha1.ServiceConditionRoutesReady); rc != nil && rc.Status == corev1.ConditionTrue { - // In case of RunLatest, verify that traffic has already migrated - // to the latest revision. - if len(rs.Traffic) == 0 || rs.Traffic[0].RevisionName != cs.LatestReadyRevisionName { - ss.MarkRouteNotYetReady() - } - } -} - func (c *Reconciler) updateStatus(desired *v1alpha1.Service) (*v1alpha1.Service, error) { service, err := c.serviceLister.Services(desired.Namespace).Get(desired.Name) if err != nil { diff --git a/pkg/reconciler/v1alpha1/service/service_test.go b/pkg/reconciler/v1alpha1/service/service_test.go index 3a97217bcee5..370970f9e781 100644 --- a/pkg/reconciler/v1alpha1/service/service_test.go +++ b/pkg/reconciler/v1alpha1/service/service_test.go @@ -122,16 +122,21 @@ func TestReconcile(t *testing.T) { }, { Name: "pinned - with ready config and route", Objects: []runtime.Object{ - svc("pinned3", "foo", WithReleaseRollout("pinned3-0001"), + svc("pinned3", "foo", WithReleaseRollout("pinned3-00001"), WithInitSvcConditions), - config("pinned3", "foo", WithReleaseRollout("pinned3-0001"), WithGeneration(1), + config("pinned3", "foo", WithReleaseRollout("pinned3-00001"), WithGeneration(1), WithLatestCreated, WithObservedGen, WithLatestReady), - route("pinned3", "foo", WithReleaseRollout("pinned3-0001"), + route("pinned3", "foo", WithReleaseRollout("pinned3-00001"), WithDomain, WithDomainInternal, WithAddress, WithInitRouteConditions, WithStatusTraffic(v1alpha1.TrafficTarget{ - RevisionName: "pinned3-0001", + Name: "current", + RevisionName: "pinned3-00001", Percent: 100, + }, v1alpha1.TrafficTarget{ + Name: "latest", + RevisionName: "pinned3-00001", + Percent: 0, }), MarkTrafficAssigned, MarkIngressReady), }, Key: "foo/pinned3", @@ -140,14 +145,19 @@ func TestReconcile(t *testing.T) { // from config and route status. Object: svc("pinned3", "foo", // Initial setup conditions. - WithReleaseRollout("pinned3-0001"), + WithReleaseRollout("pinned3-00001"), // The delta induced by configuration object. WithReadyConfig("pinned3-00001"), // The delta induced by route object. WithReadyRoute, WithSvcStatusDomain, WithSvcStatusAddress, WithSvcStatusTraffic(v1alpha1.TrafficTarget{ - RevisionName: "pinned3-0001", + Name: "current", + RevisionName: "pinned3-00001", Percent: 100, + }, v1alpha1.TrafficTarget{ + Name: "latest", + RevisionName: "pinned3-00001", + Percent: 0, })), }}, WantEvents: []string{ @@ -177,40 +187,187 @@ func TestReconcile(t *testing.T) { Eventf(corev1.EventTypeNormal, "Updated", "Updated Service %q", "release"), }, }, { - Name: "release - route and config ready, propagate ready, percentage set", + Name: "release - update service, route not ready", Objects: []runtime.Object{ - svc("release-ready", "foo", WithReleaseRolloutAndPercentage(10, /*candidate traffic percentage*/ - "release-ready-00001", "release-ready-00002"), WithInitSvcConditions), - route("release-ready", "foo", WithReleaseRolloutAndPercentage(10, /*candidate traffic percentage*/ - "release-ready-00001", "release-ready-00002"), RouteReady, + svc("release-nr", "foo", WithReleaseRollout("release-nr-00002"), WithInitSvcConditions), + config("release-nr", "foo", WithReleaseRollout("release-nr-00002"), + WithCreatedAndReady("release-nr-00002", "release-nr-00002")), + // NB: route points to the previous revision. + route("release-nr", "foo", WithReleaseRollout("release-nr-00002"), RouteReady, WithDomain, WithDomainInternal, WithAddress, WithInitRouteConditions, WithStatusTraffic(v1alpha1.TrafficTarget{ + RevisionName: "release-nr-00001", + Percent: 100, + }), MarkTrafficAssigned, MarkIngressReady), + }, + Key: "foo/release-nr", + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: svc("release-nr", "foo", + WithReleaseRollout("release-nr-00002"), + WithReadyConfig("release-nr-00002"), + WithServiceStatusRouteNotReady, WithSvcStatusDomain, WithSvcStatusAddress, + WithSvcStatusTraffic(v1alpha1.TrafficTarget{ + RevisionName: "release-nr-00001", + Percent: 100, + })), + }}, + WantEvents: []string{ + Eventf(corev1.EventTypeNormal, "Updated", "Updated Service %q", "release-nr"), + }, + }, { + Name: "release - update service, route not ready, 2 rev, no split", + Objects: []runtime.Object{ + svc("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")), + // NB: route points to the previous revision. + route("release-nr", "foo", WithReleaseRollout("release-nr-00002", "release-nr-00003"), + RouteReady, WithDomain, WithDomainInternal, WithAddress, WithInitRouteConditions, + WithStatusTraffic(v1alpha1.TrafficTarget{ + RevisionName: "release-nr-00001", + Percent: 100, + }), MarkTrafficAssigned, MarkIngressReady), + }, + Key: "foo/release-nr", + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: svc("release-nr", "foo", + WithReleaseRollout("release-nr-00002", "release-nr-00003"), + WithReadyConfig("release-nr-00003"), + WithServiceStatusRouteNotReady, WithSvcStatusDomain, WithSvcStatusAddress, + WithSvcStatusTraffic(v1alpha1.TrafficTarget{ + RevisionName: "release-nr-00001", + Percent: 100, + })), + }}, + WantEvents: []string{ + Eventf(corev1.EventTypeNormal, "Updated", "Updated Service %q", "release-nr"), + }, + }, { + Name: "release - update service, route not ready, traffic split", + Objects: []runtime.Object{ + svc("release-nr-ts", "foo", + WithReleaseRolloutAndPercentage(42, "release-nr-ts-00002", "release-nr-ts-00003"), + 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")), + route("release-nr-ts", "foo", + WithReleaseRolloutAndPercentage(42, "release-nr-ts-00002", "release-nr-ts-00003"), + RouteReady, WithDomain, WithDomainInternal, WithAddress, WithInitRouteConditions, + WithStatusTraffic(v1alpha1.TrafficTarget{ + RevisionName: "release-nr-ts-00001", + Percent: 58, + }, v1alpha1.TrafficTarget{ + RevisionName: "release-nr-ts-00002", + Percent: 42, + }), MarkTrafficAssigned, MarkIngressReady), + }, + Key: "foo/release-nr-ts", + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: svc("release-nr-ts", "foo", + WithReleaseRolloutAndPercentage(42, "release-nr-ts-00002", "release-nr-ts-00003"), + WithReadyConfig("release-nr-ts-00003"), + WithServiceStatusRouteNotReady, WithSvcStatusDomain, WithSvcStatusAddress, + WithSvcStatusTraffic(v1alpha1.TrafficTarget{ + RevisionName: "release-nr-ts-00001", + Percent: 58, + }, v1alpha1.TrafficTarget{ + RevisionName: "release-nr-ts-00002", + Percent: 42, + })), + }}, + WantEvents: []string{ + Eventf(corev1.EventTypeNormal, "Updated", "Updated Service %q", "release-nr-ts"), + }, + }, { + Name: "release - update service, route not ready, traffic split, percentage changed", + Objects: []runtime.Object{ + svc("release-nr-ts2", "foo", + WithReleaseRolloutAndPercentage(58, "release-nr-ts2-00002", "release-nr-ts2-00003"), + 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")), + route("release-nr-ts2", "foo", + WithReleaseRolloutAndPercentage(58, "release-nr-ts2-00002", "release-nr-ts2-00003"), + RouteReady, WithDomain, WithDomainInternal, WithAddress, WithInitRouteConditions, + // NB: here the revisions match, but percentages, don't. + WithStatusTraffic(v1alpha1.TrafficTarget{ + RevisionName: "release-nr-ts2-00002", + Percent: 58, + }, v1alpha1.TrafficTarget{ + RevisionName: "release-nr-ts2-00003", + Percent: 42, + }), MarkTrafficAssigned, MarkIngressReady), + }, + Key: "foo/release-nr-ts2", + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: svc("release-nr-ts2", "foo", + WithReleaseRolloutAndPercentage(58, "release-nr-ts2-00002", "release-nr-ts2-00003"), + WithReadyConfig("release-nr-ts2-00003"), + WithServiceStatusRouteNotReady, WithSvcStatusDomain, WithSvcStatusAddress, + WithSvcStatusTraffic(v1alpha1.TrafficTarget{ + RevisionName: "release-nr-ts2-00002", + Percent: 58, + }, v1alpha1.TrafficTarget{ + RevisionName: "release-nr-ts2-00003", + Percent: 42, + })), + }}, + WantEvents: []string{ + Eventf(corev1.EventTypeNormal, "Updated", "Updated Service %q", "release-nr-ts2"), + }, + }, { + Name: "release - route and config ready, propagate ready, percentage set", + Objects: []runtime.Object{ + svc("release-ready", "foo", + WithReleaseRolloutAndPercentage(58, /*candidate traffic percentage*/ + "release-ready-00001", "release-ready-00002"), WithInitSvcConditions), + route("release-ready", "foo", + WithReleaseRolloutAndPercentage(58, /*candidate traffic percentage*/ + "release-ready-00001", "release-ready-00002"), + RouteReady, WithDomain, WithDomainInternal, WithAddress, WithInitRouteConditions, + WithStatusTraffic(v1alpha1.TrafficTarget{ + Name: "current", RevisionName: "release-ready-00001", - Percent: 90, + Percent: 42, + }, v1alpha1.TrafficTarget{ + Name: "candidate", + RevisionName: "release-ready-00002", + Percent: 58, }, v1alpha1.TrafficTarget{ + Name: "latest", RevisionName: "release-ready-00002", - Percent: 10, + Percent: 0, }), MarkTrafficAssigned, MarkIngressReady), - config("release-ready", "foo", WithRunLatestRollout, WithGeneration(1), + config("release-ready", "foo", WithRunLatestRollout, WithGeneration(2), // These turn a Configuration to Ready=true WithLatestCreated, WithLatestReady), }, Key: "foo/release-ready", WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: svc("release-ready", "foo", - WithReleaseRolloutAndPercentage(10, /*candidate traffic percentage*/ + WithReleaseRolloutAndPercentage(58, /*candidate traffic percentage*/ "release-ready-00001", "release-ready-00002"), // The delta induced by the config object. - WithReadyConfig("release-ready-00001"), + WithReadyConfig("release-ready-00002"), // The delta induced by route object. WithReadyRoute, WithSvcStatusDomain, WithSvcStatusAddress, WithSvcStatusTraffic(v1alpha1.TrafficTarget{ + Name: "current", RevisionName: "release-ready-00001", - Percent: 90, + Percent: 42, }, v1alpha1.TrafficTarget{ + Name: "candidate", RevisionName: "release-ready-00002", - Percent: 10, - })), + Percent: 58, + }, v1alpha1.TrafficTarget{ + Name: "latest", + RevisionName: "release-ready-00002", + Percent: 0, + }, + ), + ), }}, WantEvents: []string{ Eventf(corev1.EventTypeNormal, "Updated", "Updated Service %q", "release-ready"), diff --git a/pkg/reconciler/v1alpha1/testing/functional.go b/pkg/reconciler/v1alpha1/testing/functional.go index 97d8dfa832ac..f0f4cb4d4eba 100644 --- a/pkg/reconciler/v1alpha1/testing/functional.go +++ b/pkg/reconciler/v1alpha1/testing/functional.go @@ -446,6 +446,14 @@ func WithObservedGen(cfg *v1alpha1.Configuration) { cfg.Status.ObservedGeneration = cfg.Generation } +// WithCreatedAndReady sets the latest{Created,Ready}RevisionName on the Configuration. +func WithCreatedAndReady(created, ready string) ConfigOption { + return func(cfg *v1alpha1.Configuration) { + cfg.Status.SetLatestCreatedRevisionName(created) + cfg.Status.SetLatestReadyRevisionName(ready) + } +} + // WithLatestCreated initializes the .status.latestCreatedRevisionName to be the name // of the latest revision that the Configuration would have created. func WithLatestCreated(cfg *v1alpha1.Configuration) {