Specify names on service update and generate names client-side. by sixolet · Pull Request #282 · knative/client

sixolet · 2019-07-19T23:36:42Z

Proposed Changes

--revision-name flag allows setting revision names in service update. If you don't provide the service name as a prefix, it'll get auto-added for you.
--generate-revision-name flag (default TRUE) makes revision names client side by default.

Whether to include the generation is up for discussion. Otherwise this is substantially similar to the server-side generateName(), up to and including our lack of vowels.

Release Note

knative-prow-robot · 2019-07-19T23:36:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [sixolet]

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

knative-prow-robot

@sixolet: 6 warnings.

Details

In response to this:

Fixes #277 #276

Proposed Changes

--revision-name flag allows setting revision names in service update. If you don't provide the service name as a prefix, it'll get auto-added for you.

--generate-revision-name flag (default TRUE) makes revision names client side by default.

Whether to include the generation is up for discussion. Otherwise this is substantially similar to the server-side generateName(), up to and including our lack of vowels.

Release Note

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.

sixolet · 2019-07-20T01:08:17Z

Cool, now that I've dealt with the old-api cases I feel happier about this.

sixolet · 2019-07-21T21:58:54Z

/retest

rhuss

Looks good to me with some minor comments. However, I'm still wondering about the usefulness of --generate-revision-name. Why is a random client-side generation better/different than server-side random generation ? The generated name is nowhere reused in the client, and you would need to re-read the generated revision in any case after the revision has been created. But probably I missed something.

If --generate-revision-name is needed, I wonder whether we could combine this with --revision-name to reduce the number of options. E.g. we could say --revision-name=%s-%r{5}-%g e.g. using placeholder which would not be allowed as names (so easily detectable). That way you can flexibly build up your revision names with downstream values (random chars, generation numbers, maybe even label or annotation. values).

In this example -->

%s --> service name
%r{5} --> five random chars
%g --> next generation id
$l{mylabel} --> value of label
$a{myanno} --> value of an annotation

That way we could keep a simple but flexible format for the user to specify (and not only those two predefined formats). The other benefit would be to be very explicit (and no hidden and not necessarily inuitive 'prefix with service name if not already present')

rhuss · 2019-08-07T10:23:24Z

 }

-func (p *ConfigurationEditFlags) AddUpdateFlags(command *cobra.Command) {
+func (p *ConfigurationEditFlags) addSharedFlags(command *cobra.Command) {


Can you add a comment to explain that "Shared" means that these flags are common between both create() and update() ?

Let's get this PR in before #345 so that #345 can add its flags in here too!

@duglin We add traffic flags separately on service update command. IMO they don't need to show up in ConfigurationEditFlags as updating traffic block is not configuration-edit operation and only traffic flags change don't result into a new revision.

From a UX perspective, I'd like to be able to setup the traffic during "service create" - that's all I'm asking for.

rhuss

looks good to me (maybe add one test more for checking the template based generation).

I wonder whether we could add that if --revision-name is given but empty (not sure whether this can be checked with cobra but should be), then we set the name also empty (or don't provided), leading to a server side-generated name.

rhuss · 2019-08-09T12:35:55Z

If I understand the motivation of client-side generation as described in https://github.com/knative/client/blob/master/conventions/client.md#change-non-code-attributes then it's about the potential risk of a race condition so that latestCreatedRevisionName could not be used for obtaining the image digest.

I doubt a bit that this could be the case as you always have an optimistic lock on the service resource, so when two clients simultaneously try to update the service the first one wins, and the second one has to re-fetch the resource (with the updated latestCreatedRevisionName). I'm not sure whether this update of the latestCreatedRevisionName is done in an admission controller or async.

In the latter case, there is still a slight chance for a race, but is this really that severe?

Two clients have to operate at the same time
The digest behind the image tag must have been changed. This is likely the case for latest. It could be the case for a version like tag (1.2) but although possible and sometimes done, I consider this a very unlikely event.

So if there should be a race for latest which this might result in an hick-up of a development workflow (as latest is not used in production).

I don't question the algorithm in the document at all, it has been the result of a lengthy (and probably painful) discussion. I just want to point out that I personally would take that risk and just reuse lastesCreatedRevisionName always for finding the active revision, regardless of who has created the name (since we are in an update or change situation we will create a new revision anyway, no need for a trigger in this case)

sixolet · 2019-08-09T18:43:04Z

latestCreatedRevisionName is updated async, which is the race opportunity, yeah.

The chance of the race is low, but the consequences can be high (running code you specifically did not intend to run)

sixolet · 2019-08-09T23:07:15Z

/retest

duglin · 2019-08-09T23:18:26Z

+	p.markFlagMakesRevision("concurrency-limit")
 	command.Flags().Int32VarP(&p.Port, "port", "p", 0, "The port where application listens on.")
+	p.markFlagMakesRevision("port")
+	command.Flags().StringVar(&p.RevisionName, "revision-name", "{{.Service}}-{{.Random 5}}-{{.Generation}}",


this seems excessively long for on-screen help text :-)

It inspired #351 . I think it's better to describe clearly.

only the first time, after that when people do --help it'll just annoy them

Hum. Roland suggested I add the part about the empty string. I think the part about the template is valuable, as it is not discoverable otherwise.

We could make it so, but we don't have a good place for that yet. I'd like to defer shortening it until we have a good place for other doc on options.

rhuss

Looks good, but I miss the --generate-revision-name parameter as indicated in the PR initial comment. Which btw should be named --no-generate-revision-name as the default is true.

sixolet · 2019-08-10T15:34:36Z

If you pass the empty string, as the revision name, it's equivalent

…

On Aug 10, 2019, at 1:44 AM, Roland Huß ***@***.***> wrote: @rhuss commented on this pull request. Looks good, but I miss the --generate-revision-name parameter as indicated in the PR initial comment. Which btw should be named --no-generate-revision-name as the default is true. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

rhuss · 2019-08-10T16:24:13Z

If you pass the empty string, as the revision name, it's equivalent

Cool. So the last thing which needs clarification whether we enforce a {{.Service}}- prefix or allow everything and wait for the server-side error. I liked that this was added implicitly when not given.

duglin · 2019-08-13T20:29:09Z

Seeing something odd:

$ ./kn service create echo --image duglin/echo --revision-name echo-v1
the service is using too old of an API format for the operation

But this works:

$ ./kn service create echo --image duglin/echo
Service 'echo' successfully created in namespace 'default'.
Waiting for service 'echo' to become ready ... OK

Service URL:
http://echo-default.kndev.us-south.containers.appdomain.cloud

$ ./kn service update echo --revision-name echo-v1
Waiting for service 'echo' to become ready ... OK
Service 'echo' updated in namespace 'default'.

I'm pretty sure I have both alpha1 and beta1 installed in my cluster.

sixolet · 2019-08-13T23:00:53Z

@duglin that's due to a related thing that I think should be fixed in a different PR: we default to creating services in the old api format, so a newly created service "is using the old api format" until it's created and then the server converts it.

duglin · 2019-08-14T00:41:51Z

ok, so when #361 is merged then all should be good, right?
btw - rebase needed on this one.

duglin · 2019-08-14T00:43:35Z

  -p, --port int32               The port where application listens on.
      --requests-cpu string      The requested CPU (e.g., 250m).
      --requests-memory string   The requested memory (e.g., 64Mi).
+      --revision-name string     The revision name to set. Must start with the service name and a dash as a prefix. Empty revision name will result in the server generating a name for the revision. Accepts golang templates, allowing {{.Service}} for the service name, {{.Generation}} for the generation, and {{.Random [n]}} for n random consonants. (default "{{.Service}}-{{.Random 5}}-{{.Generation}}")


Do we describe what "Generation" is in our docs some place?

duglin · 2019-08-14T00:52:30Z

/lgtm

Couple of comments/question though:

I'm assuming once Always create in new format rather than old #361 goes in you'll create a PR to handle the kn service create variant, right?
any reason we're not updating the docs in https://github.com/knative/client/blob/master/docs/cmd/kn_service_update.md with info about this?

Rebase needed.

sixolet · 2019-08-14T20:29:59Z

There we go. Rebased.

knative-metrics-robot · 2019-08-14T20:31:23Z

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

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/service/configuration_edit_flags.go	79.5%	82.8%	3.2
pkg/serving/config_changes.go	81.0%	80.7%	-0.3
pkg/serving/service.go	0.0%	90.6%	90.6

duglin · 2019-08-14T22:30:10Z

/lgtm

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

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 19, 2019

knative-prow-robot reviewed Jul 19, 2019

View reviewed changes

knative-prow-robot requested review from cppforlife and evankanderson July 19, 2019 23:36

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

sixolet added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jul 20, 2019

sixolet added this to the v0.8.0 milestone Jul 22, 2019

sixolet requested a review from rhuss July 29, 2019 21:46

rhuss self-assigned this Jul 30, 2019

rhuss suggested changes Aug 7, 2019

View reviewed changes

rhuss mentioned this pull request Aug 8, 2019

Service and Revision labels #342

Merged

duglin reviewed Aug 8, 2019

View reviewed changes

Comment thread pkg/serving/service.go

duglin mentioned this pull request Aug 8, 2019

feature(service): Implements traffic splitting and tagging targets #345

Merged

7 tasks

sixolet force-pushed the allow-naming branch from 5baceca to 632ada7 Compare August 8, 2019 22:57

rhuss reviewed Aug 9, 2019

View reviewed changes

Comment thread pkg/kn/commands/service/service_update_test.go Outdated

Comment thread pkg/serving/service.go

sixolet force-pushed the allow-naming branch from aa4718b to f5ee248 Compare August 9, 2019 23:01

duglin reviewed Aug 9, 2019

View reviewed changes

Comment thread pkg/kn/commands/service/configuration_edit_flags.go Outdated

duglin reviewed Aug 9, 2019

View reviewed changes

Comment thread docs/cmd/kn_service_update.md

rhuss reviewed Aug 10, 2019

View reviewed changes

duglin reviewed Aug 14, 2019

View reviewed changes

knative-prow-robot assigned duglin Aug 14, 2019

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

sixolet added 14 commits August 14, 2019 12:22

Groundwork for naming revisions automatically and deliberately

6dcb613

Tests for name updates

8080e3b

Add godoc comments

7e35c5b

Changelog entry for naming flags

aa84fb5

Error when trying to BYO revision name to an old-format service

97ab0a4

fix tests again

dfdcc7e

Template the names

476cc76

Polsh, remove unused flags, generate helptext, add better helptext

681ac3d

Decapitalize error msg

bda6abf

Respond to Roland comments

4a9e90f

Forgot to add test file

5d6618f

Be a good citizen, add more tests, try to get coverage happy.

657a61e

true dat

ad67c22

Re-add implicit service prefix if one was not added.

07897d1

sixolet force-pushed the allow-naming branch from 27b2c65 to 07897d1 Compare August 14, 2019 20:29

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 14, 2019

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

knative-prow-robot merged commit ffe80b9 into knative:master Aug 14, 2019

sixolet deleted the allow-naming branch August 15, 2019 22:30

navidshaikh mentioned this pull request Aug 29, 2019

Allow bring-your-own revision name #276

Closed

Conversation

sixolet commented Jul 19, 2019

Proposed Changes

Uh oh!

knative-prow-robot commented Jul 19, 2019

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sixolet commented Jul 20, 2019

Uh oh!

sixolet commented Jul 21, 2019

Uh oh!

rhuss 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!

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!

Uh oh!

Uh oh!

rhuss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rhuss commented Aug 9, 2019

Uh oh!

sixolet commented Aug 9, 2019

Uh oh!

sixolet commented Aug 9, 2019

Uh oh!

duglin Aug 9, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rhuss left a comment

Choose a reason for hiding this comment

Uh oh!

sixolet commented Aug 10, 2019 via email

Uh oh!

rhuss commented Aug 10, 2019

Uh oh!

duglin commented Aug 13, 2019

Uh oh!

sixolet commented Aug 13, 2019

Uh oh!

duglin commented Aug 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin commented Aug 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

duglin Aug 9, 2019 •

edited

Loading

duglin commented Aug 14, 2019 •

edited

Loading