Traffic split auto redirection should only consider the active revisions #1617

vyasgun · 2022-03-07T10:14:30Z

Description

Changes

Automatic redirection of traffic should only take the active revisions into consideration
Modified unit test according to the change

gvyas-mac:client gvyas$ ./kn revision list -s cloudevents-player
NAME                       SERVICE              TRAFFIC   TAGS   GENERATION   AGE   CONDITIONS   READY   REASON
cloudevents-player-00005   cloudevents-player   100%             5            76m   3 OK / 4     True    
cloudevents-player-00004   cloudevents-player                    4            84m   3 OK / 4     True    
cloudevents-player-00003   cloudevents-player                    3            85m   3 OK / 4     True    
cloudevents-player-00002   cloudevents-player                    2            27d   3 OK / 4     True    
cloudevents-player-00001   cloudevents-player                    1            27d   3 OK / 4     True

gvyas-mac:client gvyas$ ./kn service update cloudevents-player --env ABC=xyz --traffic 70
Updating Service 'cloudevents-player' in namespace 'default':

  0.065s The Route is still working to reflect the latest desired specification.
  0.110s ...
  3.198s Traffic is not yet migrated to the latest revision.
  3.262s Ingress has not yet been reconciled.
  3.309s Waiting for load balancer to be ready
  3.458s Ready to serve.

Service 'cloudevents-player' updated to latest revision 'cloudevents-player-00006' is available at URL:
http://cloudevents-player.default.10.111.180.117.sslip.io
gvyas-mac:client gvyas$ ./kn revision list -s cloudevents-player
NAME                       SERVICE              TRAFFIC   TAGS   GENERATION   AGE   CONDITIONS   READY   REASON
cloudevents-player-00006   cloudevents-player   70%              6            6s    4 OK / 4     True    
cloudevents-player-00005   cloudevents-player   30%              5            77m   3 OK / 4     True    
cloudevents-player-00004   cloudevents-player                    4            86m   3 OK / 4     True    
cloudevents-player-00003   cloudevents-player                    3            86m   3 OK / 4     True    
cloudevents-player-00002   cloudevents-player                    2            27d   3 OK / 4     True    
cloudevents-player-00001   cloudevents-player                    1            27d   3 OK / 4     True

Reference

Fixes #1609

knative-prow-robot

@vyasgun: 0 warnings.

In response to this:

Description

Changes

Automatic redirection of traffic should only take the active revisions into consideration
Modified unit test according to the change

gvyas-mac:client gvyas$ ./kn revision list -s cloudevents-player
NAME                       SERVICE              TRAFFIC   TAGS   GENERATION   AGE   CONDITIONS   READY   REASON
cloudevents-player-00005   cloudevents-player   100%             5            76m   3 OK / 4     True    
cloudevents-player-00004   cloudevents-player                    4            84m   3 OK / 4     True    
cloudevents-player-00003   cloudevents-player                    3            85m   3 OK / 4     True    
cloudevents-player-00002   cloudevents-player                    2            27d   3 OK / 4     True    
cloudevents-player-00001   cloudevents-player                    1            27d   3 OK / 4     True

gvyas-mac:client gvyas$ ./kn service update cloudevents-player --env ABC=xyz --traffic 70
Updating Service 'cloudevents-player' in namespace 'default':

 0.065s The Route is still working to reflect the latest desired specification.
 0.110s ...
 3.198s Traffic is not yet migrated to the latest revision.
 3.262s Ingress has not yet been reconciled.
 3.309s Waiting for load balancer to be ready
 3.458s Ready to serve.

Service 'cloudevents-player' updated to latest revision 'cloudevents-player-00006' is available at URL:
http://cloudevents-player.default.10.111.180.117.sslip.io
gvyas-mac:client gvyas$ ./kn revision list -s cloudevents-player
NAME                       SERVICE              TRAFFIC   TAGS   GENERATION   AGE   CONDITIONS   READY   REASON
cloudevents-player-00006   cloudevents-player   70%              6            6s    4 OK / 4     True    
cloudevents-player-00005   cloudevents-player   30%              5            77m   3 OK / 4     True    
cloudevents-player-00004   cloudevents-player                    4            86m   3 OK / 4     True    
cloudevents-player-00003   cloudevents-player                    3            86m   3 OK / 4     True    
cloudevents-player-00002   cloudevents-player                    2            27d   3 OK / 4     True    
cloudevents-player-00001   cloudevents-player                    1            27d   3 OK / 4     True

Reference

Fixes #1609

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 · 2022-03-07T10:19:22Z

Codecov Report

Merging #1617 (47109ee) into main (af05208) will increase coverage by 0.17%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1617      +/-   ##
==========================================
+ Coverage   79.80%   79.98%   +0.17%     
==========================================
  Files         163      171       +8     
  Lines        8824     9098     +274     
==========================================
+ Hits         7042     7277     +235     
- Misses       1089     1109      +20     
- Partials      693      712      +19

Impacted Files	Coverage Δ
pkg/kn/traffic/compute.go	`100.00% <100.00%> (ø)`
pkg/kn/commands/types.go	`87.36% <0.00%> (-1.27%)`	⬇️
pkg/kn/commands/eventtype/delete.go	`82.60% <0.00%> (ø)`
pkg/eventing/v1beta1/client.go	`84.00% <0.00%> (ø)`
pkg/kn/commands/eventtype/describe.go	`80.95% <0.00%> (ø)`
pkg/kn/commands/eventtype/eventtype.go	`100.00% <0.00%> (ø)`
pkg/kn/commands/eventtype/list.go	`82.14% <0.00%> (ø)`
pkg/kn/commands/flags/eventtype.go	`100.00% <0.00%> (ø)`
pkg/eventing/v1beta1/client_mock.go	`95.23% <0.00%> (ø)`
pkg/kn/commands/eventtype/create.go	`88.88% <0.00%> (ø)`
... and 2 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 af05208...47109ee. Read the comment docs.

vyasgun · 2022-03-07T10:48:17Z

/retest

dsimansk · 2022-03-07T11:25:04Z

@vyasgun seems like an error related to the fix?
https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_client/1617/pull-knative-client-integration-tests-latest-release/1500785404868038656#1:build-log.txt%3A1162

dsimansk · 2022-03-08T10:37:14Z

/approve
/lgtm

knative-prow-robot · 2022-03-08T10:37:24Z

[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

rhuss

Looks good to me (just one minor cosmetic comment)

rhuss · 2022-03-08T10:26:21Z

pkg/kn/traffic/compute.go

-	// number of existing revisions
-	var existingRevisionCount = len(revisions)
+	// number of existing targets
+	var existingRevisionCount = len(targets)


would it make sense to rename the variable to targetsCount ?

Yes, I'll change it

rhuss · 2022-03-08T10:57:56Z

Also, tested it locally, and works as expected. Thanks @vyasgun !

vyasgun · 2022-03-08T11:14:37Z

Taking a look at the failing tests

dsimansk · 2022-03-08T11:53:28Z

/lgtm

vyasgun added 2 commits March 7, 2022 15:38

Traffic split should consider only the active revisions

025d51d

Modified unit test for traffic split change

84b582d

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 7, 2022

knative-prow-robot reviewed Mar 7, 2022

View reviewed changes

dsimansk mentioned this pull request Mar 8, 2022

Update changelog for 1.3 release #1619

Merged

Fix e2e test case

fc76277

knative-prow-robot assigned dsimansk Mar 8, 2022

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 8, 2022

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 8, 2022

rhuss reviewed Mar 8, 2022

View reviewed changes

e2e test fix

47109ee

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 8, 2022

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 8, 2022

knative-prow-robot merged commit 7be13f0 into knative:main Mar 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Traffic split auto redirection should only consider the active revisions #1617

Traffic split auto redirection should only consider the active revisions #1617

vyasgun commented Mar 7, 2022

knative-prow-robot left a comment

codecov bot commented Mar 7, 2022 •

edited

Loading

vyasgun commented Mar 7, 2022

dsimansk commented Mar 7, 2022

dsimansk commented Mar 8, 2022

knative-prow-robot commented Mar 8, 2022

rhuss left a comment

rhuss Mar 8, 2022

vyasgun Mar 8, 2022

rhuss commented Mar 8, 2022

vyasgun commented Mar 8, 2022

dsimansk commented Mar 8, 2022

Traffic split auto redirection should only consider the active revisions #1617

Traffic split auto redirection should only consider the active revisions #1617

Conversation

vyasgun commented Mar 7, 2022

Description

Changes

Reference

knative-prow-robot left a comment

Choose a reason for hiding this comment

Description

Changes

Reference

codecov bot commented Mar 7, 2022 • edited Loading

Codecov Report

vyasgun commented Mar 7, 2022

dsimansk commented Mar 7, 2022

dsimansk commented Mar 8, 2022

knative-prow-robot commented Mar 8, 2022

rhuss left a comment

Choose a reason for hiding this comment

rhuss Mar 8, 2022

Choose a reason for hiding this comment

vyasgun Mar 8, 2022

Choose a reason for hiding this comment

rhuss commented Mar 8, 2022

vyasgun commented Mar 8, 2022

dsimansk commented Mar 8, 2022

codecov bot commented Mar 7, 2022 •

edited

Loading