feature(service): Implements traffic splitting and tagging targets

[navidshaikh]

Fix #210

Introduced 3 flags for working with traffic: --traffic, --tag, --untag.
Use @latest identifier to reference latest revision for --traffic and --tag flags.

Added first cut, please give it a try by building the client from this PR,
see if there are use cases you need and not covered here.

PR is WIP and requires following to be complete:

• Add check to verify if given revision exists (open Invalid revision names are accepted in the revision template serving#5094)
• Better error handling
• Better error reporting
• Code comments
• Unit tests
• e2e tests
• Fix marked TODOs
• client side verification for traffic percent sum to 100

[duglin]

started to play with it... one initial comment:
the use of --traffic-latest is kind of awkward since all other traffic settings are under --traffic. Since all revision names must start with service-, how about we just use --traffic latest:50 since we can assume that latest can never be a valid revision name... no - in it.

[duglin]

Unless we drop the requirement of all revision names starting with service- for the CLI - which we did discuss but I don't remember the results of that chat.

[navidshaikh]

@duglin : Thanks for testing it.

the use of --traffic-latest is kind of awkward since all other traffic settings are under --traffic.

The way I'd use that flag as easy way to re-present the desired state on CLI, for eg:
I want to rollout 10% traffic to my new app while keeping rest on known version

kn service update myapp --image myapp:latest --traffic-latest 10 --traffic stable:90

helps make identifying latestRevision reference in traffic block by a flag than an assumed value.

Since all revision names must start with service-, how about we just use --traffic latest:50 since we can assume that latest can never be a valid revision name... no - in it.

Personally, -1 for any assumptions, I'd rather want to write exact revision names on CLIs. BTW, did you notice you can also do

kn service update myapp --traffic echo-v1:20,echo-v2:20 --traffic echo-v3:60

If one doesn't want to repeat --traffic flag, could just append the values comma separated, also works in combination (was simple but useful switch to use right method from cobra).

[duglin]

oh wait, using --traffic latest;50 won't work due to us being able to use tag names, not just revision names in there.

I still think:

kn service update myapp --traffic echo-v1:20,echo-v2:20,@latest:60

is more friendly than:

kn service update myapp --traffic echo-v1:20,echo-v2:20 --traffic-latest 60

But oh well. Still need to play more with tag names and multiple "latest" blocks....

[maximilien]

kn service update myapp --traffic echo-v1:20,echo-v2:20,@latest:60`

is more friendly than:

kn service update myapp --traffic echo-v1:20,echo-v2:20 --traffic-latest 60`

What’s the ` at the end of your command here? Typo?

[maximilien]

If one doesn't want to repeat --traffic flag, could just append the values comma separated, also works in combination (was simple but useful switch to use right method from cobra).

👍🏽👍🏽

[rhuss]

wrt. to --tag-latest and a special tag name like @latest, e..g

kn service update myapp --traffic echo-v1:20,echo-v2:20,@latest:60

vs

kn service update myapp --traffic echo-v1:20,echo-v2:20 --traffic-latest:60

I'm clearly for the @latest variant as this would save us to options (--traffic-latest and --tag-latest). kn service update already is 15 options, more to come (like for annotations and labels), so adding an additional 5 options is a heavy addition to the cognitive load. Saving 2 options here is a clear benefit (we could even save 3 if we would remove --untag-revision and use a value convention on --tag-revision.

So my suggestion would be:

• --tag
• --untag
• --traffic

with latest handling done by a marker value. This looks like a nice and concise UX.

[navidshaikh]

What’s the ` at the end of your command here? Typo?

@maximilien : Typo, fixed it.

[navidshaikh]

For discussion on having dedicated flag to reference latest revision, I'd prefer to use separate flags (--traffic-latest, --tag-latest), but it seems there is more consensus on using an assumed special word for referencing same.

We'll need bring docs for this, IMO we had that 'special word' represented by flag --traffic-latest with cobra native docs for this, but then there is point raise about number of flags.

My take on re-presenting the YAMLs via CLI UX is to give users convenient flags that are easy to remember and use, I don't think number of flags should be a concern if they are used for specific operations, like these flags will be used for specific operations and we'll have 2 for traffic and 3 for tagging, that number sounds fine to me.

[navidshaikh]

If we're to use special word and omit the --traffic-latest and --tag-latest, k8s has restrictions for resource names as as described here ref, excerpts:

By convention, the names of Kubernetes resources should be up to maximum length of 253
characters and consist of lower case alphanumeric characters, -, and ., but certain resources
have more specific restrictions.

Something to note here is that it allows only lower case characters, so using a special assumed word in upper case for eg: LATEST without any special characters like @ is safe IMO, helps avoid interference with scripting languages kn could be used with.

Would LATEST word be a convenient choice? WDYT ?

[duglin]

it's actually because @ isn't allowed as a name that we SHOULD use it :-) Then there's no chance over overlap with a user's name. The presence of the @latest in there should be detected by the CLI and converted to the appropriate yaml sent to the server, meaning the server should never see it.

[rhuss]

@duglin LATEST is equally forbidden and there is no chance of interference with shell special characters nor that it can be used as a regular revision name. The same here: LATEST will never be sent but has the special meaning for the client to specify the 'latest'.

I have no super strong opinion, but think LATEST (all upper case) might make more sense (except when the user gets confused with the casing, in which case a special prefix character like @, ., or : might make sense.

[rhuss]

Let's make a vote for these three options

• --tag-latest
• LATEST
• @latest

[rhuss]

Vote for --tag-latest

[rhuss]

Vote for LATEST as special revision name

[rhuss]

Vote for @latest or @LATEST (i.e. in any case) as special name.

[navidshaikh]

Another question is around whether we want to have --tag-revision or whether its not already clear from the context (tags can be only on revisions), that we could reduce that to --tag and --untag

@rhuss : I like your suggestion.

I am +1 to change the current --tag-revision and --untag-revision to --tag and --untag, since tag should be referred in terms of traffic block of service, while for general tagging we have label. Update coming..

[duglin]

+1 to removing the unnecessary -revision suffix on the --tag option

[navidshaikh]

@duglin @rhuss : Updated the PR per suggestions, please git it a spin. If the UX surface looks good, I'll go ahead by adding rest of the items in the checklist.
/cc @sixolet @maximilien @cppforlife

[duglin]

yep - I know LATEST is disallowed too, it's just less clear to a newbie that it's special. I think a special char (like @) makes it stand-out more that this is special and it won't look like all of the others when they look at the status section. "@" almost looks like a pointer reference to me - which is kind of what it is. Plus it's easier to type :-) But, yeah, either can work.

[navidshaikh]

/test pull-knative-client-integration-tests-latest-release

[knative-metrics-robot]

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/service_update.go	76.2%	71.4%	-4.8
pkg/kn/traffic/compute.go	Do not exist	100.0%

[duglin]

It can't and its not safe to guess right? --traffic supports referencing tag or revision.
If user wants to have multiple targets referring to same revision, user must tag them.

✗ kn service update svc --tag svc-5r2dp=pepsi,svc-5r2dp=coke --traffic pepsi=50,coke=50
Waiting for service 'svc' to become ready ... OK
Service 'svc' updated in namespace 'default'.

ok, so this is what I'm seeing:

$ ./kn service update echo --tag echo-v1=coke,echo-v1=pepsi --traffic coke=50,pepsi=50

does appear to work, so that's good. However, my first attempt at it I tried to just setup the traffic block w/o any tags and just have 2 50% sections:

$ ./kn service update echo --traffic echo-v1=50,echo-v1=50
admission webhook "webhook.serving.knative.dev" denied the request: mutation failed: Traffic targets sum to 50, want 100: spec.traffic

Should I be able to do this?

[zhanggbj]

@duglin About the second item as below, I think we disallow duplicated revision reference. I have added a check and it will report an error from the client instead of the server, the code should have been included in this PR request.

$ ./kn service update echo --traffic echo-v1=50,echo-v1=50
admission webhook "webhook.serving.knative.dev" denied the request: mutation failed: Traffic targets sum to 50, want 100: spec.traffic

Now you will get something as below,

$ ./kn service update echo --traffic echo-v1=50,echo-v1=50
repetition of revision reference echo-v1 is not allowed, use only once with --traffic flag

[navidshaikh]

Should I be able to do this?

If the desired state expect to have same revision in multiple targets, user must tag them for kn to identify targets uniquely for subsequent operations.

[jimcurtis]

Clean code. I like how you handled precedence. LGTM

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jimcurtis, maximilien, navidshaikh

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [maximilien,navidshaikh]

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

[navidshaikh]

@sixolet @maximilien @cppforlife: I've rebased and squashed, PTAL.

[sixolet]

/lgtm

[maximilien]

/lgtm

[maximilien]

@navidshaikh and @zhanggbj I think it’s not merging because you need to rebase, conflict with: docs/cmd/kn_service_update.md

[knative-metrics-robot]

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/service_update.go	76.2%	71.4%	-4.8
pkg/kn/traffic/compute.go	Do not exist	100.0%

[knative-metrics-robot]

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/service_update.go	76.2%	71.4%	-4.8
pkg/kn/traffic/compute.go	Do not exist	100.0%

[sixolet]

/lgtm