WIP: Adds Traffic Splitting doc

[navidshaikh]

Enlisting the operations, commands and examples.

This is followup to #210 (comment)

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: navidshaikh
To complete the pull request process, please assign sixolet
You can assign the PR to them by writing /assign @sixolet in a comment when ready.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[duglin]

It's not clear to me how this works when there could be more than one traffic block with the specified revisionName. How does this work when there is no revisionName on the block - just latestRevision:true? Asking the user to look at the status section to find the current revisionName in that block will be cumbersome.

[duglin]

And checking the status section is racey

[rhuss]

It's not clear to me how this works when there could be more than one traffic block with the specified revisionName.

You can repeat the same revision name multiple times

How does this work when there is no revisionName on the block - just latestRevision:true

True, that might be an issue. So I would recommend a special revision name for specifying the target referenced with latestResvision. Something which is otherwise an illegal revision name, like "$" or "+".

BTW, a user can always find out all revision names attached to a service via kn service describe

[duglin]

It's not clear to me how this works when there could be more than one traffic block with the specified revisionName.

You can repeat the same revision name multiple times

I wasn't clear... the same revisionName might appear on 5 traffic blocks but I want to set a tag on just one of them....how do I specify that one of the 5?

[navidshaikh]

It's not clear to me how this works when there could be more than one traffic block with the specified revisionName.

As Roland mentioned, just repeat the same revision multiple times.

How does this work when there is no revisionName on the block - just latestRevision:true

I think referencing the entry in traffic block having latestRevsion:true is technically confusing, as with every update to service (with traffic-tag or set-traffic commands) there will be a new revision generated, so which revision should be referenced as latestRevision ? The one which was there before issuing the command OR the one that will be created after issuing the command?
I think here, we want the traffic block entry referencing the most latest revision for service (after issuing the current command), and for that we need to reference that via a word or character, which should translate to latestRevision:true field in traffic block, but not use the latestRevision as is in the CLI (because that could be confusing).

[duglin]

ah I see. I do wonder though if from a user's perspective that's confusing because the main resource we're trying to get them to work on is a "service" and yet we don't ask them to provide that. Yes we may not need it, but will a user need it in their mental model they're dealing with. I don't ever expect to expose my users to "revisions". Just something to think about.

[navidshaikh]

Sure, if we see the kn service set-traffic command, it requires a service name. We could just bring the same experience for kn service tag-target command, i.e. requiring the service name as first positional argument (this also saves the lookup for service name and we can then verify that requested revisions to be tagged belongs to the service requested to operate on).

@rhuss : WDYT about requiring a service name for kn service tag-target command as first positional argument ?

[rhuss]

According to our CLI conventions spec we should always use the name of the noun entity as the first argument, so +1 for using the service name here.

I'm wondering whether we could shorten tag-target to tag as I think target does not add much info in addition (and tag then just means tag within the route)

[navidshaikh]

although there is AGE column but we can ease the lookup by printing the numbers.

👍 I will update the PR for requiring service name for tag-target.

I'm wondering whether we could shorten tag-target to tag as I think target does not add much info in addition (and tag then just means tag within the route)

IMO, tag-target is more explicit specifying its the tag for the traffic target and not the tag for service itself. Also tag word is used to label the resources at other places for example knctl, where knctl revision tag will label the revision object as specified. We also have an open issue #232 on the same line.

[navidshaikh]

I have updated the PR to require serviceName as first argument for tag and untag commands.

[duglin]

Just wanted to point out some key things in case people are unaware:

• The tag property on a traffic block is optional. This means that we CAN NOT identify a traffic block by this value, since not all blocks will have one.
• The revisionName property on a traffic block may not be unique. More than one traffic block can refer to the same revisionName. This means that we CAN NOT identify a traffic block by this value.
• Additionally, since people can use latestRevision: true, there is no consistent revisionName to search on for this block (it'll change each time a new revision is created) - which requires the user to query the service each time they want to modify this traffic block to see if the value changed. Otherwise, per this PR, a new block will be create w/o them realizing it (I think).

IMO this means any operation that tries to modify an existing traffic block probably can't because there's no guaranteed way to find what you're looking for (in some cases) and in others it requires extra work, which is also racey.

[rhuss]

• The tag property on a traffic block is optional. This means that we CAN NOT identify a traffic block by this value, since not all blocks will have one.

True, therefore a user can use revision or tag (or both in case of multiple revisions being present). As mentioned above we might need to add a special name to target the "latestRevision" traffic block.

• The revisionName property on a traffic block may not be unique. More than one traffic block can refer to the same revisionName. This means that we CAN NOT identify a traffic block by this value.

But the combination tag:revision is unique. That's how you would need to address such a block.

[duglin]

only if tag is present - which it may not be because it's optional. I think having a different traffic-block-picking syntax based on the data in the traffic sections will be a bit confusing.
We need a cmd that works consistently.

[navidshaikh]

only if tag is present - which it may not be because it's optional.

Yes, and which gives us opportunity to design UX to remove the ambiguities.

I think having a different traffic-block-picking syntax based on the data in the traffic sections will be a bit confusing.

Yes, and this is why we need to give users a way to not confuse them. We should ask users to create unique identity for ambiguous targets, which is Tag. We ask users to create Tag for a revision if it has to present more than once in traffic block, so that sub-subsequent operations can be performed without ambiguity. Tag is optional, I agree, but we can leverage it for identifying a target uniquely.

There are a bunch of combinations, given that serving traffic block allows fields as optional and aliases, and I think we should work towards making the client UX easy to understand and execute for users, defining the path to reach to a desired state.

[duglin]

Since 'tag' must be unique across all traffic blocks, can't you just make it:
kn service untag-target TagName ?

[navidshaikh]

Yes, we can implement that as first iteration to require only TagName. The subsequent iteration though should support the other options too.

[duglin]

This does not guarantee a unique look-up of the block in question

[rhuss]

wdym ? Because a tag can be named like a revision ? I consider this case super seldom and we should verify this and throw an error, if then the tag and revision differs (as said (a) its very unlikely that a revision and tag will ever have the same name and (b) if this should be the case its very likely that it specifies the same block).

If you mean that when the user specifies a tag or revision which is not present, then of course again an error should be thrown.

So IMO it does guarantee a unique lookup (to 99.99999%) and simplifies the UI considerably.

[duglin]

I mean, since the revisionName is not unique, [revisionName1=Percent] is indeterminate.

[rhuss]

• If the revisionName appears once, the operation is performed
• If the revisionName appears 0 times, an error is thrown it is checked if a revision of this name exists for the given service and the traffic block is created automatically if so. If not, an error is thrown.
• If the revisionName appears more than once, an error is thrown and the user is hinted that she has to use a tag (multiple revisions without tags doesn't make any sense, or ?)

[rhuss]

(just updated my comment above which overlooked a case)

[duglin]

Repeating here:

Some key things in case people are unaware:

• The tag property on a traffic block is optional. This means that we CAN NOT identify a traffic block by this value, since not all blocks will have one.
• The revisionName property on a traffic block may not be unique. More than one traffic block can refer to the same revisionName. This means that we CAN NOT identify a traffic block by this value.
• Additionally, since people can use latestRevision: true, there is no consistent revisionName to search on for this block (it'll change each time a new revision is created) - which requires the user to query the service each time they want to modify this traffic block to see if the value changed. Otherwise, per this PR, a new block will be create w/o them realizing it (I think).

IMO this means any operation that tries to modify an existing traffic block probably can't because there's no guaranteed way to find what you're looking for (in some cases) and in others it requires extra work, which is also racey.

---

I'm saying all of this given the current state of what's defined in Serving. If we want to suggest changes to serving to make things easier then that will change things. But I'm trying to see what can work given what we have today, and I don't see a good way to do any kind of reliable (and non-racey) searching to find a specific traffic target to make any modifications.

That's why I suggested: #210 (comment)

[navidshaikh]

The tag property on a traffic block is optional. This means that we CAN NOT identify a traffic block by this value, since not all blocks will have one.
The revisionName property on a traffic block may not be unique. More than one traffic block can refer to the same revisionName. This means that we CAN NOT identify a traffic block by this value.
Additionally, since people can use latestRevision: true, there is no consistent revisionName to search on for this block (it'll change each time a new revision is created) - which requires the user to query the service each time they want to modify this traffic block to see if the value changed. Otherwise, per this PR, a new block will be create w/o them realizing it (I think).
IMO this means any operation that tries to modify an existing traffic block probably can't because there's no guaranteed way to find what you're looking for (in some cases) and in others it requires extra work, which is also racey.

Replied at #285 (comment)

I'm saying all of this given the current state of what's defined in Serving. If we want to suggest changes to serving to make things easier then that will change things. But I'm trying to see what can work given what we have today, and I don't see a good way to do any kind of reliable (and non-racey) searching to find a specific traffic target to make any modifications.

The other cases where revisionName could be present multiple times is covered (check above comments).

The only case we need to cover is latestRevision:true, which I think, we should refer as placeholder field for referring to a particular (always latest) revision at current state of service, than actual revision reference. (because before and after issuing the command set-traffic or tag-target or even service update, that field's reference will be updated by serving any way)

So we use that field to say, for e.g. whatever the latest revision of this service will be, give that a tag current and 10% traffic.
We can identify that field by a character which can not be revision name, for e.g. $ (as it denotes to last character in regexp, so some similarity as last generated revision for this service).
You could then say

kn service tag-target $:current echo-v2:stable
kn service set-traffic current=10 echo-v2=90

which should form the final spec of the traffic block of the service as

[..]
    traffic:
    - tag: stable
      RevisionName: echo-v2
      percent: 90
    - tag: latest
      latestRevision: true
      percent: 10

@duglin : Does this make sense ?

I think we covered the cases you mentioned.

[duglin]

@navidshaikh what do you think about supporting this type of flow:

$ kn service create myapp --image echo:v1 --revision-name=v1
$ kn service update myapp --image echo:v2 --revision-name=v2 --traffic=v1:90 --traffic=v2:10

I think think type of flow might be popular so forcing them to do it in more commands might be cumbersome.

[navidshaikh]

I think think type of flow might be popular so forcing them to do it in more commands might be cumbersome.

@duglin:
Technically, the operations (flags) on the service update command updates the Configuration of the service, thereby creating a new Revision, while updating the traffic block of the service doesn't update the Configuration and thus doesn't create a new Revision. We should have a separate path for what we're updating.

[duglin]

don't think of from an inner-Kn perspective, think of it from a user perspective. They just created a new revision and want the routing to take effect immediately.

[duglin]

But, if you really want to ... :-) I can do exactly that kn command via a single kubectl command - so it seems natural to me that kn could match that functionality - just in a non-yaml way

[rhuss]

don't think of from an inner-Kn perspective, think of it from a user perspective. They just created a new revision and want the routing to take effect immediately.

I would love to avoid the multiplied --traffic args, which makes it much easier from a user perspective

kn service set-traffic myservice myservice-v1=10% myservice-v2=30% myservice-backup=*

vs

kn service update myservice --traffic myservice-v1=10% --traffic myservice-v2=30% --traffic myservice-backup=*

This would balance the drawback to have a prior call to update the image for me.

[duglin]

the syntax is less important to me than being able to do it all in one cmd :-) so if dropping the flags works, that's fine.

But keep in mind, we may need to allow people to specify it on a kn service create so we'd need consistency there. Not sure yet about the % splitting yet since we just have one revision, but definitely for tagging.

Another thing to remember in all of this... today we have % splitting, we need to make sure whatever design we come up with allows for splitting on something else at some point. See knative/serving#4736

[duglin]

We should mention that in the future people may be able to specify something other than "percent" as the criteria, so we need to design a syntax that doesn't preclude us from supporting that later on.

Since right now I believe all of our proposals have done something of the form ...:% - then as long as the % specification is at the end of the syntax then I think we might be ok. We can then assume that whatever string is after the : is the criteria - even if it's a complex expression. We may need to allow for it to be of the form type:expression (e.g. path:/foo or header:user=john) but I think that's workable. So, nothing needs to be changed w.r.t. any proposal (except the comment above to mention it) - I just wanted to bring this up so people are thinking about it.

[sixolet]

Ping; Navid do you want to re-do this with the decisions we've made, or shall we start a different PR?

[navidshaikh]

@sixolet : I think we'll need a different PR, the doc should be more of doc-style guide than a proposal-style one.

[maximilien]

Nit.. Do we need to repeat the word when it's already bolded and delimits the paragraph :) ? Same for below.

[rhuss]

Would also rephrase to 'RevisionName: Name of a specific revision ...'

[navidshaikh]

sure, we'll have this incorporated in a different PR.

[maximilien]

I would have proposed names for these and small explanation if possible. Since one goal here is to see what other operations people reading this spec can come up with...

[maximilien]

Not a huge fan of the $ here... Maybe there is a precedence in other CLIs or reasoning. Can you elucidate this choice @navidshaikh? Thanks.

Not a huge concern if there is precedence. If not, then *latestRevision or &latestRevision could be perhaps more descriptive?

[rhuss]

$ is a take on regexps where it means 'the end of a line' I guess (like in 'the end of a line of revisions') ;-). A bit made up, we could definitely use something different. However we should choose best something which does not has a meaning for a shell. E.g '*latestRevision' would expand to a filename if in the current directory is a file like 'mylatestRevision' before it's handed over to kn. & will put the process in the background if unescaped. $ alone is safe but when combined with another character it specifies a variable.

I'm afraid regardless of what we chose we won't find something so intuitive that it's obvious without reading the documentation.

I think $ is ok-ish but maybe we find something better ?

[navidshaikh]

one of the suggestion Naomi mentioned during WG meeting is to use flag --traffic-latest which should refer the latestRevision field in traffic block.

We'll need an identifier for (a) traffic (b) tag operations, we can use
--traffic-latest and --tag-latest flags, for eg:

1. --traffic-latest 2
2. --tag-latest current

[maximilien]

Nice start @navidshaikh left some comments.

Another comment, perhaps for @zhanggbj to help out would be to flesh out some more examples and the expected YAML for them either in situ or in a "appendix"-like section. Thoughts?

[zhanggbj]

@maximilien Sure, no problem for me. BTW, @navidshaikh @toVersus and I planned to raise a new doc PR with the voting result and discussions here, just started to break down to pieces so we can collaborate, thx!

[navidshaikh]

Thank you all for review and feedback, since the feedback required re-write of the doc in this PR, here is the new PR #331 with additional description of the flags, operation precedence and more examples, PTAL.

Closing this now.