Release mode processing for the route not being ready. by vagababov · Pull Request #2948 · knative/serving

vagababov · 2019-01-19T01:52:59Z

Proposed Changes

check for the route's traffic in the release mode to match the desired shape.
if it doesn't match -- mark service to be in Unknown state
Tested manually and with ITests.
Added unit tests for the new functionality.

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: knative#2430

vagababov · 2019-01-19T01:53:28Z

/cc @mattmoor
/cc @dgerd

mattmoor · 2019-01-21T20:04:47Z

 		// 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.
+


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

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

mattmoor · 2019-01-21T20:09:19Z

+			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()
+			}


I feel like there's an uncovered path through here when I go from:

revisions: ["rev1", "rev2"] percent: 0

to:

revisions: ["rev1", "rev3"] percent: 0

We only check that "current" matches, not that "candidate" matches.

Well, that's why I left the comment. Since this does not affect the traffic — should it matter?
If you think it does, then it's just reorganization of code.

mattmoor · 2019-01-21T20:16:09Z

-		runLatestCheckTrafficMigrated(ss, &route.Status, &config.Status)
+		c.runLatestCheckTrafficMigrated(service, &route.Status, &config.Status)
+	case service.Spec.Release != nil:
+		c.releaseCheckTrafficMigrated(service, &route.Status, &config.Status)


Couldn't we replace this whole switch (and both functions) with:

if rc := service.Status.GetCondition(v1alpha1.ServiceConditionRoutesReady); rc != nil && rc.Status == corev1.ConditionTrue { want := route.Spec.Traffic.DeepCopy() for idx := range want { if want[idx].ConfigurationName == config.Name { want[idx].RevisionName = config.LatestReadyConfigurationName want[idx].ConfigurationName = "" } } if got := route.Status.Traffic; !cmp.Equal(got, want) { s.Status.MarkRouteNotYetReady() } }

Don't know. Have to think more about this. May be.

As is this definitely makes old tests fail. New tests still pass.
I can try to tweak it a bit.

FWIW cmp.Equal can panic. If you go with this approach, consider: https://github.com/knative/pkg/blob/2a1c043ce3d778a4017e9c988b7a7dd0644312ad/kmp/diff.go#L53

Had to update other tests, since they were reflecting just part of the reality;

implemented Matt's suggestion;

implemented Jon's suggestion.

🍷

vagababov · 2019-01-22T23:27:21Z

/test pull-knative-serving-go-coverage

vagababov · 2019-01-22T23:27:29Z

PTAL>

dgerd · 2019-01-23T00:25:43Z

-	switch {
-	case service.Spec.RunLatest != nil:
-		runLatestCheckTrafficMigrated(ss, &route.Status, &config.Status)
+	if service.Spec.RunLatest != nil || service.Spec.Release != nil {


I think I would prefer service.Spec.Manual == nil as it is the only mode that we don't reconcile. Even though pinned is deprecated I don't think there is a strong reason to explicitly exclude it here, and checking that we are not manual enables any future modes to just work.

Well there's still pinned, it's deprecated, but supported.

Yeah, but it's a nop for Pinned. I think I have a mild preference for Manual != nil as well because if we were to add a new mode, this would either do the right thing or the author would have to adjust this logic for the Service to become Ready.

Doesn't matter at all, since we don't reconcile manual anyway.

So we don't reconcile if we're in manual, so I removed the check altogether.

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.

…-release

knative-metrics-robot · 2019-01-23T17:40:07Z

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/service/service.go	92.0%	92.2%	0.2

dgerd · 2019-01-23T18:14:43Z

/lgtm

mattmoor · 2019-01-23T20:14:12Z

/approve

knative-prow-robot · 2019-01-23T20:14:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, vagababov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~pkg/reconciler/OWNERS~~ [mattmoor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vagababov added 2 commits January 18, 2019 09:20

add interface

d81d89f

Implement release mode checking for the service not yet being ready.

d520cc6

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: knative#2430

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 19, 2019

knative-prow-robot requested review from jonjohnsonjr and tcnghia January 19, 2019 01:53

knative-prow-robot requested review from dgerd and mattmoor January 19, 2019 01:53

remove debug logging

39fa158

mattmoor reviewed Jan 21, 2019

View reviewed changes

vagababov added 4 commits January 21, 2019 15:26

Bring back the accidentaly deleted comment.

d6a43a9

Apply suggestions from the code review.

2eeba80

Merge branch 'master' into 2430-release

6de2aa2

Replace cmp with kmp package.

d5d7cc7

jonjohnsonjr reviewed Jan 23, 2019

View reviewed changes

Comment thread pkg/reconciler/v1alpha1/service/service.go Outdated

dgerd reviewed Jan 23, 2019

View reviewed changes

vagababov and others added 5 commits January 22, 2019 21:14

Update pkg/reconciler/v1alpha1/service/service.go

6d45c7e

Merge branch 'master' into 2430-release

3b2581c

Remove the check for type of service

1e5dc8d

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.

diff -> eq

c6ade88

Merge branch '2430-release' of github.com:vagababov/serving into 2430…

a9863af

…-release

knative-prow-robot assigned dgerd Jan 23, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 23, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2019

knative-prow-robot merged commit af08456 into knative:master Jan 23, 2019

vagababov deleted the 2430-release branch January 24, 2019 23:33

dgerd mentioned this pull request Feb 14, 2019

Add dgerd to list of serving-api-approvers #3228

Merged

Conversation

vagababov commented Jan 19, 2019

Proposed Changes

Uh oh!

vagababov commented Jan 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vagababov Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vagababov commented Jan 22, 2019

Uh oh!

vagababov commented Jan 22, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knative-metrics-robot commented Jan 23, 2019

Uh oh!

dgerd commented Jan 23, 2019

Uh oh!

mattmoor commented Jan 23, 2019

Uh oh!

knative-prow-robot commented Jan 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vagababov Jan 22, 2019 •

edited

Loading