Skip to content

Release mode processing for the route not being ready. #2948

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
knative-prow-robot merged 12 commits into from
Jan 23, 2019
+205 −41
Merged

Release mode processing for the route not being ready. #2948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d81d89f
add interface
vagababov Jan 18, 2019
d520cc6
Implement release mode checking for the service not yet being ready.
vagababov Jan 19, 2019
39fa158
remove debug logging
vagababov Jan 19, 2019
d6a43a9
Bring back the accidentaly deleted comment.
vagababov Jan 21, 2019
2eeba80
Apply suggestions from the code review.
vagababov Jan 22, 2019
6de2aa2
Merge branch 'master' into 2430-release
vagababov Jan 22, 2019
d5d7cc7
Replace cmp with kmp package.
vagababov Jan 22, 2019
6d45c7e
Update pkg/reconciler/v1alpha1/service/service.go
vagababov Jan 23, 2019
3b2581c
Merge branch 'master' into 2430-release
vagababov Jan 23, 2019
1e5dc8d
Remove the check for type of service
vagababov Jan 23, 2019
c6ade88
diff -> eq
vagababov Jan 23, 2019
a9863af
Merge branch '2430-release' of github.com:vagababov/serving into 2430…
vagababov Jan 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions pkg/reconciler/testing/table.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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)
}
}

Expand Down Expand Up @@ -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)
}
}
}
Expand Down
29 changes: 14 additions & 15 deletions pkg/reconciler/v1alpha1/service/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Copy link
Copy Markdown
Member

@mattmoor mattmoor Jan 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you mean to delete this line? If so, . on the previous line?

Copy link
Copy Markdown
Contributor Author

@vagababov vagababov Jan 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I did not. Probably one of those vscode vim bindings random actions.

} 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",
Expand Down Expand Up @@ -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 {
Expand Down
195 changes: 176 additions & 19 deletions pkg/reconciler/v1alpha1/service/service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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{
Expand Down Expand Up @@ -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"),
Expand Down
8 changes: 8 additions & 0 deletions pkg/reconciler/v1alpha1/testing/functional.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down