Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: Adds documentation for traffic splitting support in kn #331

Merged

Conversation

navidshaikh
Copy link
Collaborator

@navidshaikh navidshaikh commented Aug 5, 2019

Proposed Changes

  • Adds Reference doc for traffic splitting operations via kn service update single entry point
  • Defines the operation precedence of the flags involved
  • Adds multiple examples with different combinations of operations

Summary

Following table shows quick summary for traffic splitting flags, their value formats, operation to perform and Repetition column denotes if repeating the particular value of flag is allowed in a kn service update command.

Flag Value(s) Operation Repetition
--traffic RevisionName=Percent Give Percent traffic to RevisionName ✔️
--traffic Tag=Percent Give Percent traffic to the Revision having Tag ✔️
--traffic @latest=Percent Give Percent traffic to the latest ready Revision ✖️
--tag RevisionName=Tag Give Tag to RevisionName ✔️
--tag @latest=Tag Give Tag to the latest ready Revision ✖️
--untag Tag Remove Tag from Revision ✔️

Examples

  1. Tag revisions echo-v1 and echo-v2 as stable and staging respectively:
kn service update svc --tag echo-v1=stable --tag echo-v2=staging
  1. Ramp up/down revision echo-v3 to 20%, adjusting other traffic to accommodate:
kn service update svc --traffic echo-v3=20 --traffic echo-v2=80
  1. Give revision echo-v3 the tag candidate, without otherwise changing any traffic split:
kn service update svc --tag echo-v3=candidate
  1. Give echo-v3 the tag candidate, and 2% of traffic adjusting other traffic to go to revision echo-v2:
kn service update svc --tag echo-v3=candidate --traffic candidate=2 --traffic echo-v2=98
  1. Update the tag for echo-v3 from candidate to current:
kn service update svc --untag candidate --tag echo-v3=current
  1. Remove the tag current from echo-v3:
kn service update svc --untag current
  1. Remove echo-v3 having no tag(s) entirely, adjusting echo-v2 to fill up:
kn service update svc --traffic echo-v2=100
  1. Remove echo-v1 and its tag old from the traffic assignments entirely:
kn service update svc --untag old --traffic echo-v1=0
  1. Tag revision echo-v1 with stable as well as current, and 50-50% traffic split to each:
kn service update svc --tag echo-v1=stable --tag echo-v2=current --traffic stable=50 --traffic current=50
  1. Revert all the traffic to the latest ready revision of service:
kn service update svc --traffic @latest=100
  1. Tag the latest ready revision of service as current:
kn service update svc --tag @latest=current
  1. Update tag for echo-v4 to testing and assign all the traffic to it:
kn service update svc --untag oldv4 --tag echo-v4=testing --traffic testing=100
  1. Replace latest tag of echo-v1 with old tag, give latest to echo-v2:
kn service update svc --untag latest --tag echo-v1=old --tag echo-v2=latest

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 5, 2019
@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2019
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 5, 2019
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
@rhuss
Copy link
Contributor

rhuss commented Aug 6, 2019

looks good to me, however, I would not require to untag a revision before it can be retagged like in:

kn service update svc --untag-revision echo-v3 --tag-revision echo-v3:current

That's because you almost always want a create-or-replace (or "set") semantics when writing tags. Otherwise, you would need to examine whether a revision has been already tagged.

Also, unconditionally setting a tag makes this operation idempotent.

@navidshaikh
Copy link
Collaborator Author

That's because you almost always want a create-or-replace (or "set") semantics when writing tags.

Yup, that's an additional step but its required given that a Revision might be present in multiple targets in the traffic block, for eg: in an existing traffic block say if echo-v1 is tagged as current and new, while doing a create-or-replace for echo-v1 with a new tag, we are not sure then which target to apply the requested operation on.

Otherwise, you would need to examine whether a revision has been already tagged.

Yes and we'd need the resource listing to show these traffic block values.

docs/TrafficSplitting.md Outdated Show resolved Hide resolved
@markito
Copy link

markito commented Aug 6, 2019

Looks good to me as well, looking forward for this feature!

Do I always have to specify weights in way that add up to 100% or if I give "50%" to a given revision the CLI imply that 50% goes to the other revision ?

As I wrote this I already thought about the possible issues when 3+ revisions are available, but similarly I could specify weights for 2 revisions and the difference would go to the 3rd revision.

@navidshaikh
Copy link
Collaborator Author

navidshaikh commented Aug 6, 2019

@markito : Thanks for the review

Do I always have to specify weights in way that add up to 100%

Yes.

or if I give "50%" to a given revision the CLI imply that 50% goes to the other revision ?

No.

Every update to traffic block (for weights) should specify sums which would add up to 100 and that would represent the desired-state of traffic from user.
As we can't safely guess how to distribute the leftover portion among existing traffic targets (more than 2) in the service, as you mentioned.

but similarly I could specify weights for 2 revisions and the difference would go to the 3rd revision.

Yup, in earlier proposal (#285) we discussed about using a character to specify the rest portion using (*) for example kn service update --traffic echo-v1:10 --traffic-latest *, however in this version of proposal we ask users to specify exact sum kn service update --traffic echo-v1:10 --traffic-latest 90. This is to avoid bringing a bunch of rules to users to start using this feature on CLI, we'll add smart handling of traffic weights in upcoming iterations.

Copy link
Contributor

@sixolet sixolet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of copy-editing — aiming to overall try to reduce the assumptions about what the user knows, and what the user has to learn to understand the doc.

docs/TrafficSplitting.md Outdated Show resolved Hide resolved
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
@mattmoor mattmoor removed their request for review August 7, 2019 00:42
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
@navidshaikh navidshaikh force-pushed the pr/traffic-split-doc branch 2 times, most recently from 81e11b6 to c1199b5 Compare August 8, 2019 14:12
@navidshaikh
Copy link
Collaborator Author

Updated the PR with following changes

Removal:

  • --traffic-latest flag
  • --tag-latest flag

Rename:

  • Assignment using = instead of :
  • --tag-revision --> --tag
  • --untag-revision --> --untag

New:

  • @latest identifier to refer to latest ready revision ( Use: --tag @latest=current , --traffic @latest=100 )

docs/TrafficSplitting.md Outdated Show resolved Hide resolved
@navidshaikh navidshaikh force-pushed the pr/traffic-split-doc branch 3 times, most recently from 9630c9a to a06a2e6 Compare August 14, 2019 11:22
@jimcurtis
Copy link

Looks good to me

@navidshaikh navidshaikh force-pushed the pr/traffic-split-doc branch 3 times, most recently from a0a263d to e92d0e1 Compare August 16, 2019 06:12
@navidshaikh
Copy link
Collaborator Author

/test pull-knative-client-go-coverage

@navidshaikh
Copy link
Collaborator Author

@sixolet @maximilien : Can we get this doc PR merged please as we have respective traffic PR merged ?

Copy link
Contributor

@maximilien maximilien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I think that the first three sections don’t do justice to the doc. I would move them out or later in the doc. I would have a brief summary of traffic splitting in terms of the value and the use cases. Briefly mention how Knative support or even mention that later there is a section with details. Then jump into examples and use these examples for explanation of the flags. Especially the ones that are not obvious or combination thereof that are not obvious.

Hope this helps.

docs/TrafficSplitting.md Outdated Show resolved Hide resolved
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
docs/TrafficSplitting.md Outdated Show resolved Hide resolved
@navidshaikh navidshaikh force-pushed the pr/traffic-split-doc branch from e92d0e1 to 721bcc7 Compare August 20, 2019 12:57
Copy link
Contributor

@maximilien maximilien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also the first doc using camel case in the name. Perhaps docs/traffic_splitting.md is better name to follow with the rest (especially generated docs)?

@navidshaikh navidshaikh force-pushed the pr/traffic-split-doc branch from 721bcc7 to 3a93e6f Compare August 21, 2019 07:09
Copy link
Contributor

@maximilien maximilien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 21, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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
Copy link
Collaborator Author

/retest

 - Add summary about operations, their usage and examples
 - Flags precendence explained
 - Summary column explaining if repetition is allowed or not
 - Example of different scenarios
 - Add case where tagOfOneTarget==revisionOfAnotherTarget, prefrence goes to tagOfOneTarget
   as Tags are unique while revions in a traffic block aren't.
 - Updating tag requires using `--untag` flag
 - Update doc to mention the tag update operation and example
 - Add notes about
	- assigning multiple tags for same revision
        - creating a new target in traffic if same revision is tagged with new tag
 - Update CHANGELOG
 - Add an example per sub-section of flag description
@navidshaikh navidshaikh force-pushed the pr/traffic-split-doc branch from 3a93e6f to 36e01db Compare August 22, 2019 08:25
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 22, 2019
@navidshaikh
Copy link
Collaborator Author

@maximilien : There was merge conflict for CHANGELOG file, fixed it. PTAL

@sixolet
Copy link
Contributor

sixolet commented Sep 3, 2019

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 3, 2019
@knative-prow-robot knative-prow-robot merged commit 2985e55 into knative:master Sep 3, 2019
@navidshaikh navidshaikh deleted the pr/traffic-split-doc branch September 5, 2019 10:20
coryrc pushed a commit to coryrc/client that referenced this pull request May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants