Normative example for release mode by sixolet · Pull Request #2044 · knative/serving

sixolet · 2018-09-17T23:42:54Z

Fixes #

Proposed Changes

Describe release mode in a normative example.

Edited the old "manual mode" normative example, as our concept of what "manual mode" is has evolved.

Refers to the proposal in: #1865 (comment)

Release Note

mikehelmick · 2018-09-18T04:42:47Z

+but can be accessed for testing, verification, etc under the
+`next.my-service...` name.
+
+An admission hook prevents changing `revisions` without also setting


I don't think we should prevent this.

It's possible to completely overwrite the revisions array and effect a sharp traffic change.

evankanderson

This looks good, though I think you were going to add some text on handling rollbacks and using annotations to track images.

evankanderson · 2018-09-18T20:37:11Z

+current         100%     def   2018-01-18 20:34    user1        a6f92d1
+                         abc   2018-01-17 10:32    user1        33643fc
+
+$ knative release begin ghi


Not sure how this happened, but --service got lost here and on the next two commands.

Should be good now.

sixolet · 2018-09-19T23:02:56Z

Ok, responded to comments, and another editing pass. Passive voice hides what the user does vs what a controller does 😝

jonjohnsonjr

Some small nits

cppforlife · 2018-09-26T17:47:22Z

what do people think of moving configuration at the top level under spec?

sixolet · 2018-10-05T00:37:37Z

what do people think of moving configuration at the top level under spec?

Seems fine, but not for this PR? We can handle that separately for v1apha2, since it's backcompat-breaking.

Seems like Mike is for it too: #1865 (comment)

sixolet · 2018-10-05T00:38:03Z

Ping I think this is ready.

dgerd · 2018-10-16T18:13:54Z

+
+$ knative revisions list --service my-service
+Route           Traffic  Id    Date                Deployer      Git SHA
+current,latest  100%     def   2018-01-18 20:34    user1         a6f92d1


Is the expectation here that changing to the release strategy created the route names 'current' and 'latest', or is it expected that these names were created by a previous release strategy (i.e. runLatest)?

Yes. Changing to release strategy makes these route subnames.

dgerd · 2018-10-16T18:14:55Z

+$ knative revisions list --service my-service
+Route           Traffic  Id    Date                Deployer     Git SHA
+next,latest     5%       ghi   2018-01-19 12:16    user1        a6f92d1
+current         100%     def   2018-01-18 20:34    user1        a6f92d1


Shouldn't this be 95% since we have 5% going to 'ghi'?

dgerd · 2018-10-16T18:25:24Z

 $ knative deploy --service my-service --env HELLO="blurg"
 [...]
+Deployed revision ghi to https://latest.my-service.default.mydomain.com
+You can begin rolling out this revision with [knative rollout begin ghi]


You have knative rollout here, but the commands below are against knative release.

dgerd · 2018-10-16T18:28:18Z

+$ knative release --service my-service percent 50
 [...]
-$ knative rollout finish
+$ knative release --service my-service finish


Just to confirm, the strategy for deploy will stay the same even after a finish? For example, a new deploy will move the latest route to the new revision with 0% traffic?

Confirmed. In release mode adding a new revision moves latest but not current or next, and does not change the allocated percentage if there is a split.

dgerd · 2018-10-16T19:58:16Z

+  name: my-service
+spec:
+  release:
+    revisions: [abc]


This seems equivalent to rolloutPercent: 0 with both revisions in the list. However, setting rolloutPercent would preserve the ability to easily visualize that this is rolled back from 'next'. Thoughts?

That's a good point, but that leaves you in the mid-release state. I think a rollback shouldn't do that, since you've determined you should not release to next. Better to wait until you get a new latest to release to.

dgerd · 2018-10-16T20:00:34Z

+  revision is now the current revision.

-![Manual rollout](images/manual_rollout.png)
+![Release mode](images/manual_rollout.png)


'manual' here seems confusing with the addition of 'manual' type that removes the Service from control of the underlying Configuration and Route.

It was just the name of the image left over. Changed taht.

mattmoor · 2018-11-01T18:44:52Z

@sixolet I noticed this in the backlog, and am curious what's outstanding to land it?

dgerd · 2018-11-07T19:20:02Z

-$ knative rollout next percent 5
+$ knative revisions list --service my-service
+Route           Traffic  Id    Date                Deployer     Git SHA
+next,latest     0%       ghi   2018-01-19 12:16    user1        a6f92d1


'next' was changed to 'candidate' so might be worth a s/next/candidate/g

Otherwise this lgtm to close out.

sixolet · 2018-11-28T01:58:01Z

Got around to making the s/next/candidate/ finally.

dgerd · 2018-11-28T17:25:37Z

/lgtm

jonjohnsonjr · 2018-11-30T21:58:18Z

/assign @evankanderson

for approval

knative-prow-robot · 2018-11-30T21:58:19Z

@jonjohnsonjr: GitHub didn't allow me to assign the following users: for, approval.

Note that only knative members and repo collaborators can be assigned.
For more information please see the contributor guide

Details

In response to this:

/assign @evankanderson for approval

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

evankanderson

/lgtm
/approve

knative-prow-robot · 2018-12-05T00:28:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, sixolet

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:

~~OWNERS~~ [evankanderson]

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

evankanderson · 2018-12-17T22:15:06Z

It looks like there is a conflict now, but I (or anyone else) can /lgtm after you've resolved the conflict and pushed.

evankanderson

/lgtm

knative-prow-robot requested review from evankanderson and vaikas September 17, 2018 23:42

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 17, 2018

sixolet changed the title ~~Normative~~ Normative example for release mode Sep 17, 2018

mikehelmick reviewed Sep 18, 2018

View reviewed changes

mattmoor assigned mikehelmick, cppforlife and jonjohnsonjr Sep 19, 2018

evankanderson reviewed Sep 19, 2018

View reviewed changes

jonjohnsonjr reviewed Sep 21, 2018

View reviewed changes

mattmoor assigned dgerd Oct 8, 2018

dgerd reviewed Oct 16, 2018

View reviewed changes

dgerd reviewed Nov 7, 2018

View reviewed changes

sixolet added 11 commits November 27, 2018 17:53

Change normative example from pinned to release mode

309427d

Proofread and expand

c4e2626

Talk about a safety belt

cab1e7e

consistency in revision names

5db063b

correct last route spec

73a3537

Respond to some comments; add rollback

b03d207

Add the nonce to the revision

d19bf58

Respond to review comments

a5025ab

Fix typos and grammar

050de61

Respond to comments

56cb0e9

s/next/candidate/

0e9a0d4

sixolet force-pushed the normative branch from 0b1b3f1 to 0e9a0d4 Compare November 28, 2018 01:53

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2018

knative-prow-robot assigned evankanderson Nov 30, 2018

evankanderson reviewed Dec 5, 2018

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 5, 2018

Merge from head

ef7bf30

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 21, 2018

evankanderson reviewed Dec 21, 2018

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 21, 2018

knative-prow-robot merged commit f495c96 into knative:master Dec 21, 2018

dgerd mentioned this pull request Feb 14, 2019

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

Merged

Conversation

sixolet commented Sep 17, 2018

Proposed Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evankanderson left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sixolet commented Sep 19, 2018

Uh oh!

jonjohnsonjr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cppforlife commented Sep 26, 2018

Uh oh!

sixolet commented Oct 5, 2018

Uh oh!

sixolet commented Oct 5, 2018

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!

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!

mattmoor commented Nov 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sixolet commented Nov 28, 2018

Uh oh!

dgerd commented Nov 28, 2018

Uh oh!

jonjohnsonjr commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knative-prow-robot commented Nov 30, 2018

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Dec 5, 2018

Uh oh!

evankanderson commented Dec 17, 2018

Uh oh!

evankanderson left a comment

jonjohnsonjr commented Nov 30, 2018 •

edited

Loading