From d81d89f2da65eb8f4409ec8d241dd55069ab826c Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Fri, 18 Jan 2019 09:20:31 -0800 Subject: [PATCH 1/9] add interface --- pkg/reconciler/v1alpha1/service/service.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/reconciler/v1alpha1/service/service.go b/pkg/reconciler/v1alpha1/service/service.go index a9ae49339900..fce1369ad9aa 100644 --- a/pkg/reconciler/v1alpha1/service/service.go +++ b/pkg/reconciler/v1alpha1/service/service.go @@ -214,12 +214,17 @@ func (c *Reconciler) reconcile(ctx context.Context, service *v1alpha1.Service) e switch { case service.Spec.RunLatest != nil: runLatestCheckTrafficMigrated(ss, &route.Status, &config.Status) + case service.Spic.Release != nil: + releaseCheckTrafficMigrated(ss, &route.Status, &config.Status) } service.Status.ObservedGeneration = service.Generation return nil } +func releaseCheckTrafficMigrated(ss *v1alpha1.ServiceStatus, rs *v1alpha1.RouteStatus, cs *v1alpha1.ConfigurationStatus) { +} + 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 { From d520cc687bf3c76b7aae603ecbd241d89ba643e9 Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Fri, 18 Jan 2019 17:50:03 -0800 Subject: [PATCH 2/9] Implement release mode checking for the service not yet being ready. When we update the service in the release mode, but the route is not yet updated to the new version, we should mark the service to be in `Unknown` ready state. See: #2430 --- pkg/reconciler/v1alpha1/service/service.go | 57 ++++++-- .../v1alpha1/service/service_test.go | 131 ++++++++++++++++++ pkg/reconciler/v1alpha1/testing/functional.go | 8 ++ 3 files changed, 188 insertions(+), 8 deletions(-) diff --git a/pkg/reconciler/v1alpha1/service/service.go b/pkg/reconciler/v1alpha1/service/service.go index fce1369ad9aa..9116ee926e0a 100644 --- a/pkg/reconciler/v1alpha1/service/service.go +++ b/pkg/reconciler/v1alpha1/service/service.go @@ -22,6 +22,7 @@ import ( "reflect" "time" + "github.com/google/go-cmp/cmp" "github.com/knative/pkg/controller" "github.com/knative/pkg/kmp" "github.com/knative/pkg/logging" @@ -143,7 +144,7 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { // If we didn't change anything then don't call updateStatus. // 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", @@ -213,25 +214,65 @@ func (c *Reconciler) reconcile(ctx context.Context, service *v1alpha1.Service) e // Apply additional logic, once the generic data has been propagated. switch { case service.Spec.RunLatest != nil: - runLatestCheckTrafficMigrated(ss, &route.Status, &config.Status) - case service.Spic.Release != nil: - releaseCheckTrafficMigrated(ss, &route.Status, &config.Status) + c.runLatestCheckTrafficMigrated(service, &route.Status, &config.Status) + case service.Spec.Release != nil: + c.releaseCheckTrafficMigrated(service, &route.Status, &config.Status) } service.Status.ObservedGeneration = service.Generation return nil } -func releaseCheckTrafficMigrated(ss *v1alpha1.ServiceStatus, rs *v1alpha1.RouteStatus, cs *v1alpha1.ConfigurationStatus) { +func (c *Reconciler) releaseCheckTrafficMigrated(s *v1alpha1.Service, rs *v1alpha1.RouteStatus, cs *v1alpha1.ConfigurationStatus) { + c.Logger.Error("##### checking release") + // If the service's RouteReady condition is in a happy state. + if rc := s.Status.GetCondition(v1alpha1.ServiceConditionRoutesReady); rc != nil && rc.Status == corev1.ConditionTrue { + // Case 1. Either all the traffic is going to the `current` or there is no `candidate` at all. + // Note as implemented, if we have 2 revisions in the Spec, but route is ready to serve + // only `current`, but `rolloutPercent` is 0, then we'll presume that the service is ready, + // since it is _ready_ to serve the desired configuration. + rp := s.Spec.Release.RolloutPercent + revs := s.Spec.Release.Revisions + if rp == 0 || len(revs) == 1 { + if len(rs.Traffic) == 0 || rs.Traffic[0].RevisionName != revs[0] { + c.Logger.Errorf("### %s: traffic not yet migrated to %s", s.Name, revs[0]) + s.Status.MarkRouteNotYetReady() + } + } else { + // Case 2. We have two candidates and traffic is in 0-99 range. + if len(rs.Traffic) < 2 { + c.Logger.Errorf("### %s: traffic does not have enough entries yet", s.Name) + s.Status.MarkRouteNotYetReady() + return + } + want := map[string]int{ + revs[0]: 100 - rp, + revs[1]: rp, + } + // See `MakeRoute`: `current` is the first one, `candidate` the second. + got := map[string]int{ + rs.Traffic[0].RevisionName: rs.Traffic[0].Percent, + rs.Traffic[1].RevisionName: rs.Traffic[1].Percent, + } + + // Not happy with `cmp` here, but it seems the best way for now. + if !cmp.Equal(got, want) { + c.Logger.Errorf("### %s: desired vs actual route config: diff(+service, -traffic): %+s", + s.Name, cmp.Diff(got, want)) + s.Status.MarkRouteNotYetReady() + } + } + } } -func runLatestCheckTrafficMigrated(ss *v1alpha1.ServiceStatus, rs *v1alpha1.RouteStatus, cs *v1alpha1.ConfigurationStatus) { +func (c *Reconciler) runLatestCheckTrafficMigrated(s *v1alpha1.Service, 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 { + if rc := s.Status.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() + c.Logger.Debugf("%s: traffic is %#v, want to point at: %s", s.Name, rs.Traffic, cs.LatestReadyRevisionName) + s.Status.MarkRouteNotYetReady() } } } diff --git a/pkg/reconciler/v1alpha1/service/service_test.go b/pkg/reconciler/v1alpha1/service/service_test.go index 196415d72d9c..9580a8d412ca 100644 --- a/pkg/reconciler/v1alpha1/service/service_test.go +++ b/pkg/reconciler/v1alpha1/service/service_test.go @@ -176,6 +176,137 @@ func TestReconcile(t *testing.T) { Eventf(corev1.EventTypeNormal, "Created", "Created Route %q", "release"), Eventf(corev1.EventTypeNormal, "Updated", "Updated Service %q", "release"), }, + }, { + Name: "release - update service, route not ready", + Objects: []runtime.Object{ + 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{ diff --git a/pkg/reconciler/v1alpha1/testing/functional.go b/pkg/reconciler/v1alpha1/testing/functional.go index 5f41ea37fb31..a9a7a5c288a0 100644 --- a/pkg/reconciler/v1alpha1/testing/functional.go +++ b/pkg/reconciler/v1alpha1/testing/functional.go @@ -419,6 +419,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) { From 39fa1587604abd1ac66cb417c8bd1150650f3b9a Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Fri, 18 Jan 2019 19:06:26 -0800 Subject: [PATCH 3/9] remove debug logging --- pkg/reconciler/v1alpha1/service/service.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/reconciler/v1alpha1/service/service.go b/pkg/reconciler/v1alpha1/service/service.go index 9116ee926e0a..0a07834a9c7e 100644 --- a/pkg/reconciler/v1alpha1/service/service.go +++ b/pkg/reconciler/v1alpha1/service/service.go @@ -224,7 +224,6 @@ func (c *Reconciler) reconcile(ctx context.Context, service *v1alpha1.Service) e } func (c *Reconciler) releaseCheckTrafficMigrated(s *v1alpha1.Service, rs *v1alpha1.RouteStatus, cs *v1alpha1.ConfigurationStatus) { - c.Logger.Error("##### checking release") // If the service's RouteReady condition is in a happy state. if rc := s.Status.GetCondition(v1alpha1.ServiceConditionRoutesReady); rc != nil && rc.Status == corev1.ConditionTrue { // Case 1. Either all the traffic is going to the `current` or there is no `candidate` at all. @@ -235,13 +234,13 @@ func (c *Reconciler) releaseCheckTrafficMigrated(s *v1alpha1.Service, rs *v1alph revs := s.Spec.Release.Revisions if rp == 0 || len(revs) == 1 { if len(rs.Traffic) == 0 || rs.Traffic[0].RevisionName != revs[0] { - c.Logger.Errorf("### %s: traffic not yet migrated to %s", s.Name, revs[0]) + c.Logger.Debugf("%s: traffic not yet migrated to %s", s.Name, revs[0]) s.Status.MarkRouteNotYetReady() } } else { // Case 2. We have two candidates and traffic is in 0-99 range. if len(rs.Traffic) < 2 { - c.Logger.Errorf("### %s: traffic does not have enough entries yet", s.Name) + c.Logger.Debugf("%s: traffic does not have enough entries yet", s.Name) s.Status.MarkRouteNotYetReady() return } @@ -257,7 +256,7 @@ func (c *Reconciler) releaseCheckTrafficMigrated(s *v1alpha1.Service, rs *v1alph // Not happy with `cmp` here, but it seems the best way for now. if !cmp.Equal(got, want) { - c.Logger.Errorf("### %s: desired vs actual route config: diff(+service, -traffic): %+s", + c.Logger.Debugf("%s: desired vs actual route config: diff(+service, -traffic): %+s", s.Name, cmp.Diff(got, want)) s.Status.MarkRouteNotYetReady() } From d6a43a9d9334e1cd121ddf2cabe8f628b4087e1d Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Mon, 21 Jan 2019 15:26:21 -0800 Subject: [PATCH 4/9] Bring back the accidentaly deleted comment. --- pkg/reconciler/v1alpha1/service/service.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/reconciler/v1alpha1/service/service.go b/pkg/reconciler/v1alpha1/service/service.go index 0a07834a9c7e..82cf7c04b3d7 100644 --- a/pkg/reconciler/v1alpha1/service/service.go +++ b/pkg/reconciler/v1alpha1/service/service.go @@ -144,6 +144,7 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { // If we didn't change anything then don't call updateStatus. // 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)) From 2eeba80bb8f6489153cd36bb28a6ee5fc61932d2 Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Tue, 22 Jan 2019 15:18:46 -0800 Subject: [PATCH 5/9] Apply suggestions from the code review. --- pkg/reconciler/testing/table.go | 14 ++-- pkg/reconciler/v1alpha1/service/service.go | 71 ++++--------------- .../v1alpha1/service/service_test.go | 64 ++++++++++++----- 3 files changed, 65 insertions(+), 84 deletions(-) diff --git a/pkg/reconciler/testing/table.go b/pkg/reconciler/testing/table.go index 818338adcd81..ff4c1f78dcc0 100644 --- a/pkg/reconciler/testing/table.go +++ b/pkg/reconciler/testing/table.go @@ -129,7 +129,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 { @@ -163,7 +163,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 { @@ -194,7 +194,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 { @@ -248,7 +248,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 { @@ -265,7 +265,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 { @@ -276,7 +276,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) } } @@ -307,7 +307,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 82cf7c04b3d7..9edd62d2818a 100644 --- a/pkg/reconciler/v1alpha1/service/service.go +++ b/pkg/reconciler/v1alpha1/service/service.go @@ -212,69 +212,24 @@ 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: - c.runLatestCheckTrafficMigrated(service, &route.Status, &config.Status) - case service.Spec.Release != nil: - c.releaseCheckTrafficMigrated(service, &route.Status, &config.Status) - } - service.Status.ObservedGeneration = service.Generation - - return nil -} - -func (c *Reconciler) releaseCheckTrafficMigrated(s *v1alpha1.Service, rs *v1alpha1.RouteStatus, cs *v1alpha1.ConfigurationStatus) { - // If the service's RouteReady condition is in a happy state. - if rc := s.Status.GetCondition(v1alpha1.ServiceConditionRoutesReady); rc != nil && rc.Status == corev1.ConditionTrue { - // Case 1. Either all the traffic is going to the `current` or there is no `candidate` at all. - // Note as implemented, if we have 2 revisions in the Spec, but route is ready to serve - // only `current`, but `rolloutPercent` is 0, then we'll presume that the service is ready, - // since it is _ready_ to serve the desired configuration. - rp := s.Spec.Release.RolloutPercent - revs := s.Spec.Release.Revisions - if rp == 0 || len(revs) == 1 { - if len(rs.Traffic) == 0 || rs.Traffic[0].RevisionName != revs[0] { - c.Logger.Debugf("%s: traffic not yet migrated to %s", s.Name, revs[0]) - s.Status.MarkRouteNotYetReady() - } - } else { - // Case 2. We have two candidates and traffic is in 0-99 range. - if len(rs.Traffic) < 2 { - c.Logger.Debugf("%s: traffic does not have enough entries yet", s.Name) - s.Status.MarkRouteNotYetReady() - return - } - want := map[string]int{ - revs[0]: 100 - rp, - revs[1]: rp, + if service.Spec.RunLatest != nil || service.Spec.Release != nil { + if rc := service.Status.GetCondition(v1alpha1.ServiceConditionRoutesReady); rc != nil && rc.Status == corev1.ConditionTrue { + want := route.DeepCopy().Spec.Traffic + for idx := range want { + if want[idx].ConfigurationName == config.Name { + want[idx].RevisionName = config.Status.LatestReadyRevisionName + want[idx].ConfigurationName = "" + } } - // See `MakeRoute`: `current` is the first one, `candidate` the second. - got := map[string]int{ - rs.Traffic[0].RevisionName: rs.Traffic[0].Percent, - rs.Traffic[1].RevisionName: rs.Traffic[1].Percent, - } - - // Not happy with `cmp` here, but it seems the best way for now. - if !cmp.Equal(got, want) { - c.Logger.Debugf("%s: desired vs actual route config: diff(+service, -traffic): %+s", - s.Name, cmp.Diff(got, want)) - s.Status.MarkRouteNotYetReady() + c.Logger.Errorf("###\n\n%#v\n\n%#v\nDIFF: %s", want, route.Status.Traffic, cmp.Diff(want, route.Status.Traffic)) + if got := route.Status.Traffic; !cmp.Equal(got, want) { + service.Status.MarkRouteNotYetReady() } } } -} + service.Status.ObservedGeneration = service.Generation -func (c *Reconciler) runLatestCheckTrafficMigrated(s *v1alpha1.Service, rs *v1alpha1.RouteStatus, cs *v1alpha1.ConfigurationStatus) { - // If the service's RouteReady is in favorable condition. - if rc := s.Status.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 { - c.Logger.Debugf("%s: traffic is %#v, want to point at: %s", s.Name, rs.Traffic, cs.LatestReadyRevisionName) - s.Status.MarkRouteNotYetReady() - } - } + return nil } func (c *Reconciler) updateStatus(desired *v1alpha1.Service) (*v1alpha1.Service, error) { diff --git a/pkg/reconciler/v1alpha1/service/service_test.go b/pkg/reconciler/v1alpha1/service/service_test.go index 9580a8d412ca..88b67505d9ca 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{ @@ -310,38 +320,54 @@ func TestReconcile(t *testing.T) { }, { Name: "release - route and config ready, propagate ready, percentage set", 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, - WithDomain, WithDomainInternal, WithAddress, WithInitRouteConditions, + 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"), From d5d7cc7be2736a25f346394613b64d0c1c9b0c4f Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Tue, 22 Jan 2019 15:24:54 -0800 Subject: [PATCH 6/9] Replace cmp with kmp package. --- pkg/reconciler/v1alpha1/service/service.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/reconciler/v1alpha1/service/service.go b/pkg/reconciler/v1alpha1/service/service.go index d2523879a392..8ecffae3be7c 100644 --- a/pkg/reconciler/v1alpha1/service/service.go +++ b/pkg/reconciler/v1alpha1/service/service.go @@ -22,7 +22,6 @@ import ( "reflect" "time" - "github.com/google/go-cmp/cmp" "github.com/knative/pkg/controller" "github.com/knative/pkg/kmp" "github.com/knative/pkg/logging" @@ -223,15 +222,14 @@ func (c *Reconciler) reconcile(ctx context.Context, service *v1alpha1.Service) e if service.Spec.RunLatest != nil || service.Spec.Release != nil { if rc := service.Status.GetCondition(v1alpha1.ServiceConditionRoutesReady); rc != nil && rc.Status == corev1.ConditionTrue { - want := route.DeepCopy().Spec.Traffic + want, got := route.DeepCopy().Spec.Traffic, route.Status.Traffic for idx := range want { if want[idx].ConfigurationName == config.Name { want[idx].RevisionName = config.Status.LatestReadyRevisionName want[idx].ConfigurationName = "" } } - c.Logger.Errorf("###\n\n%#v\n\n%#v\nDIFF: %s", want, route.Status.Traffic, cmp.Diff(want, route.Status.Traffic)) - if got := route.Status.Traffic; !cmp.Equal(got, want) { + if diff, err := kmp.SafeEqual(got, want); !diff || err != nil { service.Status.MarkRouteNotYetReady() } } From 6d45c7e7a239c8357e984836204a53df044d91ff Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Tue, 22 Jan 2019 21:14:45 -0800 Subject: [PATCH 7/9] Update pkg/reconciler/v1alpha1/service/service.go --- pkg/reconciler/v1alpha1/service/service.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/reconciler/v1alpha1/service/service.go b/pkg/reconciler/v1alpha1/service/service.go index 8ecffae3be7c..f2a6e124906a 100644 --- a/pkg/reconciler/v1alpha1/service/service.go +++ b/pkg/reconciler/v1alpha1/service/service.go @@ -229,7 +229,7 @@ func (c *Reconciler) reconcile(ctx context.Context, service *v1alpha1.Service) e want[idx].ConfigurationName = "" } } - if diff, err := kmp.SafeEqual(got, want); !diff || err != nil { + if eq, err := kmp.SafeEqual(got, want); !eq || err != nil { service.Status.MarkRouteNotYetReady() } } From 1e5dc8d17f086e8ae4fa089a96245e6674e47b39 Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Wed, 23 Jan 2019 09:32:58 -0800 Subject: [PATCH 8/9] Remove the check for type of service Since reconcile() is only invoked, when we're not in manual mode, hence the type check is useless here. Also move the DeepCopy() one word right, saving us a few microseconds. --- pkg/reconciler/v1alpha1/service/service.go | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/pkg/reconciler/v1alpha1/service/service.go b/pkg/reconciler/v1alpha1/service/service.go index 8ecffae3be7c..d748d3cb101d 100644 --- a/pkg/reconciler/v1alpha1/service/service.go +++ b/pkg/reconciler/v1alpha1/service/service.go @@ -220,19 +220,18 @@ func (c *Reconciler) reconcile(ctx context.Context, service *v1alpha1.Service) e ss := &service.Status ss.PropagateRouteStatus(&route.Status) - if service.Spec.RunLatest != nil || service.Spec.Release != nil { - if rc := service.Status.GetCondition(v1alpha1.ServiceConditionRoutesReady); rc != nil && rc.Status == corev1.ConditionTrue { - want, got := route.DeepCopy().Spec.Traffic, route.Status.Traffic - for idx := range want { - if want[idx].ConfigurationName == config.Name { - want[idx].RevisionName = config.Status.LatestReadyRevisionName - want[idx].ConfigurationName = "" - } - } - if diff, err := kmp.SafeEqual(got, want); !diff || err != nil { - service.Status.MarkRouteNotYetReady() + // `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 + for idx := range want { + if want[idx].ConfigurationName == config.Name { + want[idx].RevisionName = config.Status.LatestReadyRevisionName + want[idx].ConfigurationName = "" } } + if diff, err := kmp.SafeEqual(got, want); !diff || err != nil { + service.Status.MarkRouteNotYetReady() + } } service.Status.ObservedGeneration = service.Generation From c6ade889ccc2d323feffc895ebb1c4de84fb8dcf Mon Sep 17 00:00:00 2001 From: Victor Agababov Date: Wed, 23 Jan 2019 09:35:31 -0800 Subject: [PATCH 9/9] diff -> eq --- pkg/reconciler/v1alpha1/service/service.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/reconciler/v1alpha1/service/service.go b/pkg/reconciler/v1alpha1/service/service.go index d748d3cb101d..5f9e341d25c5 100644 --- a/pkg/reconciler/v1alpha1/service/service.go +++ b/pkg/reconciler/v1alpha1/service/service.go @@ -223,13 +223,14 @@ func (c *Reconciler) reconcile(ctx context.Context, service *v1alpha1.Service) e // `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 diff, err := kmp.SafeEqual(got, want); !diff || err != nil { + if eq, err := kmp.SafeEqual(got, want); !eq || err != nil { service.Status.MarkRouteNotYetReady() } }