Calculate traffic split when N-1 revisions are specified #1483

vyasgun · 2021-10-18T05:46:16Z

Description

Changes

Determine traffic split when N-1 revisions are specified
In case @latest is specified, cannot determine the split unless it sums to 100

Reference

Fixes #

knative-prow-robot

@vyasgun: 0 warnings.

In response to this:

Description

Changes

Determine traffic split when N-1 revisions are specified

In case @latest is specified, cannot determine the split unless it sums to 100

Reference

Fixes #

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.

codecov · 2021-10-18T05:50:26Z

Codecov Report

Merging #1483 (30801ee) into main (62b1f73) will increase coverage by 0.25%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1483      +/-   ##
==========================================
+ Coverage   79.05%   79.30%   +0.25%     
==========================================
  Files         162      162              
  Lines        8454     8495      +41     
==========================================
+ Hits         6683     6737      +54     
+ Misses       1085     1076       -9     
+ Partials      686      682       -4

Impacted Files	Coverage Δ
pkg/kn/commands/service/update.go	`81.42% <100.00%> (+0.83%)`	⬆️
pkg/kn/traffic/compute.go	`100.00% <100.00%> (ø)`
pkg/kn/commands/source/ping/update.go	`84.00% <0.00%> (-0.62%)`	⬇️
pkg/kn/plugin/stat.go	`60.00% <0.00%> (ø)`
pkg/serving/v1/client.go	`82.77% <0.00%> (ø)`
pkg/eventing/v1/client.go	`87.67% <0.00%> (ø)`
pkg/serving/v1alpha1/client.go	`86.76% <0.00%> (ø)`
pkg/kn/commands/domain/update.go	`65.71% <0.00%> (ø)`
pkg/kn/commands/trigger/update.go	`77.19% <0.00%> (ø)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62b1f73...30801ee. Read the comment docs.

dsimansk · 2021-10-18T16:21:45Z

/retest

dsimansk · 2021-10-18T16:22:31Z

@vyasgun the test should be OK now, minus go-coverage that's still failing but it isn't required to pass.

vyasgun · 2021-10-19T05:06:22Z

thanks, @dsimansk!

dsimansk · 2021-10-19T11:51:55Z

pkg/kn/traffic/compute.go

+					return nil
+				}
+			}
+			return errorTrafficDistribution()


Can this error be tested as well pls?

dsimansk · 2021-10-19T11:53:39Z

pkg/kn/traffic/compute.go

+		}
+		if sum < 100 && revPercents == revisionCount-1 {
+			if latestNameFound {
+				return errorTrafficDistribution()


Maybe the error could accept a reason param? Just an idea to better explain the failure.

dsimansk · 2021-10-19T11:58:36Z

In addition to the above comments E2E tests with the change would be great.

And there should be an example added to showcase the usage. E.g. https://github.com/knative/client/blob/main/docs/cmd/kn_service_update.md#examples

maximilien

Thanks for contribution and for adding lots of tests. However, no integration tests. Perhaps adding some could reduce your unit tests and also make it easier to verify this feature? Might worth a try.

/ok-to-test

maximilien · 2021-10-19T18:15:41Z

pkg/kn/traffic/compute.go

@@ -260,18 +271,42 @@ func verifyInput(trafficFlags *flags.Traffic) error {
 		sum += percentInt
 	}

+	revPercents := len(trafficFlags.RevisionsPercentages)


Maybe leave a comment here since not obvious that the length of the array is the revPercents

maximilien · 2021-10-19T20:47:35Z

pkg/kn/traffic/compute.go

 // verifies if user has repeated @latest field in --tag or --traffic flags
 // verifyInput checks:
 // - if user has repeated @latest field in --tag or --traffic flags
 // - if provided traffic portion are integers
-func verifyInput(trafficFlags *flags.Traffic) error {
+func verifyInput(trafficFlags *flags.Traffic, revisions []servingv1.Revision) error {


I would split this function into smaller private functions. That might make it easier to parse and comprehend. For instance there’s logical helper functions that could be extracted around each of the for loops… as an example

maximilien · 2021-10-19T20:48:58Z

pkg/kn/traffic/compute_test.go

-			[]string{"@latest"},
-			[]string{"latest"},
-			[]int64{100},
+			name:             "assign 'latest' tag to @latest revision",


Add some comment to explain the “matrix” of tests you are doing with the big structure below. This would explain why you have x numbers of tests and what the differences between the tests

Thank you for the review. This test matrix was already present. The change is to include the field names which weren't there before (modified because of the new field existingRevisions being added to the struct)

maximilien · 2021-10-19T20:49:48Z

pkg/kn/traffic/compute_test.go

-			append(newServiceTraffic([]servingv1.TrafficTarget{}), newTarget("", "", 100, true)),
-			[]string{"--tag", "@latest="},
-			"expecting the value format in value1=value2, given @latest=",
+			name:            "repeatedly tagging to @latest revision not allowed",


Same comment as above to add some summary comment to explain the different tests in the array…

dsimansk · 2021-10-20T11:31:23Z

pkg/kn/commands/service/update.go

+  kn service update gitopstest --env KEY1=VALUE1 --target=/user/knfiles/test.json
+
+
+  # Split 50% traffic to stable, 40% traffic to staging and the


I'd move it above the "offline" mode example to make it appear as a follow-up to the above examples.

Moved it, thanks

vyasgun · 2021-10-25T09:47:16Z

/retest

dsimansk · 2021-10-26T13:17:47Z

Thanks, looks good to me now. :)

/lgtm

docs/cmd/kn_service_update.md

pkg/kn/commands/service/update.go

Co-authored-by: Navid Shaikh <[email protected]>

knative-metrics-robot · 2021-10-26T18:56:25Z

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

File	Old Coverage	New Coverage	Delta
pkg/kn/commands/service/update.go	86.9%	87.5%	0.6

dsimansk · 2021-10-29T18:02:39Z

Since all the review comments are addressed, lets get it in. :)

Thanks!
/approve
/lgtm

knative-prow-robot · 2021-10-29T18:02:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk, vyasgun

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~~ [dsimansk]

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

dsimansk · 2021-10-29T19:54:24Z

/retest

vyasgun · 2021-10-30T09:34:53Z

/retest

Calculate traffic split when N-1 revisions are specified

609b050

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 18, 2021

knative-prow-robot reviewed Oct 18, 2021

View reviewed changes

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 18, 2021

Added test cases for traffic split

d978374

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 18, 2021

dsimansk reviewed Oct 19, 2021

View reviewed changes

pkg/kn/traffic/compute.go Outdated

return nil

}

}

return errorTrafficDistribution()

Copy link

Contributor

dsimansk Oct 19, 2021

Choose a reason for hiding this comment

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

Can this error be tested as well pls?

dsimansk reviewed Oct 19, 2021

View reviewed changes

maximilien suggested changes Oct 19, 2021

View reviewed changes

knative-prow-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Oct 19, 2021

knative-prow-robot assigned maximilien Oct 19, 2021

enhanced error messages and added example

681ab32

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 20, 2021

dsimansk reviewed Oct 20, 2021

View reviewed changes

added e2e test

5c4211d

refactored verifyInput function

5c3472d

vyasgun requested review from dsimansk and maximilien October 26, 2021 11:26

knative-prow-robot assigned dsimansk Oct 26, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2021

navidshaikh requested changes Oct 26, 2021

View reviewed changes

docs/cmd/kn_service_update.md Outdated Show resolved Hide resolved

pkg/kn/commands/service/update.go Outdated Show resolved Hide resolved

knative-prow-robot assigned navidshaikh Oct 26, 2021

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2021

vyasgun and others added 3 commits October 26, 2021 20:44

Update docs/cmd/kn_service_update.md

31eca29

Co-authored-by: Navid Shaikh <[email protected]>

Update pkg/kn/commands/service/update.go

8629a3e

Co-authored-by: Navid Shaikh <[email protected]>

added unit test for revision list error

30801ee

vyasgun force-pushed the pr/traffic-split branch from 1e04682 to 30801ee Compare October 26, 2021 18:53

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2021

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 29, 2021

knative-prow-robot merged commit d8d0ee1 into knative:main Oct 29, 2021

vyasgun mentioned this pull request Nov 29, 2021

Improve traffic split UX #1365

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calculate traffic split when N-1 revisions are specified #1483

Calculate traffic split when N-1 revisions are specified #1483

vyasgun commented Oct 18, 2021

knative-prow-robot left a comment

codecov bot commented Oct 18, 2021 •

edited

Loading

dsimansk commented Oct 18, 2021

dsimansk commented Oct 18, 2021

vyasgun commented Oct 19, 2021

dsimansk Oct 19, 2021

dsimansk Oct 19, 2021

dsimansk commented Oct 19, 2021 •

edited

Loading

maximilien left a comment

maximilien Oct 19, 2021

maximilien Oct 19, 2021

maximilien Oct 19, 2021

vyasgun Oct 20, 2021

maximilien Oct 19, 2021

dsimansk Oct 20, 2021

vyasgun Oct 20, 2021

vyasgun commented Oct 25, 2021

dsimansk commented Oct 26, 2021

knative-metrics-robot commented Oct 26, 2021

dsimansk commented Oct 29, 2021

knative-prow-robot commented Oct 29, 2021

dsimansk commented Oct 29, 2021

vyasgun commented Oct 30, 2021

		kn service update gitopstest --env KEY1=VALUE1 --target=/user/knfiles/test.json


		# Split 50% traffic to stable, 40% traffic to staging and the

Calculate traffic split when N-1 revisions are specified #1483

Calculate traffic split when N-1 revisions are specified #1483

Conversation

vyasgun commented Oct 18, 2021

Description

Changes

Reference

knative-prow-robot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

codecov bot commented Oct 18, 2021 • edited Loading

Codecov Report

dsimansk commented Oct 18, 2021

dsimansk commented Oct 18, 2021

vyasgun commented Oct 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsimansk commented Oct 19, 2021 • edited Loading

maximilien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyasgun commented Oct 25, 2021

dsimansk commented Oct 26, 2021

knative-metrics-robot commented Oct 26, 2021

dsimansk commented Oct 29, 2021

knative-prow-robot commented Oct 29, 2021

dsimansk commented Oct 29, 2021

vyasgun commented Oct 30, 2021

codecov bot commented Oct 18, 2021 •

edited

Loading

dsimansk commented Oct 19, 2021 •

edited

Loading