feat: Adding support for defining max destination weights in Istio VS #1420
Conversation
agrawroh
force-pushed
the
add-max-weight
branch
from
87a416c
to
7ec3374
Compare
agrawroh
force-pushed
the
add-max-weight
branch
2 times, most recently
from
a1a589c
to
d117d1b
Compare
Signed-off-by: Rohit Agrawal <[email protected]>
agrawroh
force-pushed
the
add-max-weight
branch
from
d117d1b
to
efe7fc2
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
Codecov Report
@@ Coverage Diff @@
## master #1420 +/- ##
==========================================
+ Coverage 81.37% 81.39% +0.02%
==========================================
Files 108 108
Lines 10038 10052 +14
==========================================
+ Hits 8168 8182 +14
Misses 1313 1313
Partials 557 557
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Please update the specification doc: https://github.com/argoproj/argo-rollouts/blob/master/docs/features/specification.md
| @@ -368,6 +368,8 @@ type IstioTrafficRouting struct { | |||
| VirtualService IstioVirtualService `json:"virtualService" protobuf:"bytes,1,opt,name=virtualService"` | |||
| // DestinationRule references an Istio DestinationRule to modify to shape traffic | |||
| DestinationRule *IstioDestinationRule `json:"destinationRule,omitempty" protobuf:"bytes,2,opt,name=destinationRule"` | |||
| // Max weight that will be split between canary and stable services. If unset, it defaults to 100. | |||
| MaxWeight int64 `json:"maxWeight,omitempty" protobuf:"bytes,3,opt,name=maxWeight"` | |||
| @@ -33,6 +33,8 @@ spec: | |||
| virtualService: | |||
| # Reference to a VirtualService which the controller updates with canary weights | |||
| name: rollouts-demo-vsvc | |||
| # Optional if there are only two destinations in your routes or if you want to split 100% traffic between stable and canary services. If specified, this will be used as an upper bound for traffic between canary + stable services. | |||
| maxWeight: 80 | |||
There was a problem hiding this comment.
Feels like we are overusing the example in the getting-started page. Can we move this advanced use to https://argoproj.github.io/argo-rollouts/features/traffic-management/istio/ and leave the getting-started example straightfoward. Thanks.
There was a problem hiding this comment.
Make sense. Will update!
| @@ -33,6 +33,8 @@ spec: | |||
| virtualService: | |||
| # Reference to a VirtualService which the controller updates with canary weights | |||
| name: rollouts-demo-vsvc | |||
| # Optional if there are only two destinations in your routes or if you want to split 100% traffic between stable and canary services. If specified, this will be used as an upper bound for traffic between canary + stable services. | |||
| maxWeight: 80 | |||
There was a problem hiding this comment.
This PR adds maxWeight prop at the level of the virtualService, therefore it will affect all the routes ArgoRollouts manage - which are specified in strategy.canary.trafficRouting.istio.virtualService.routes
While in our use case we'd want only specific routes to have less then 100 weight managed by ArgoRollouts.
Our Virtual Service has such routes
- a route for the traffic originating in Mesh. Here we want e.g. 80% to be split between stable and canary in Mesh, and 20% to go to the legacy system.
- a route for the traffic coming to Mesh Ingress Gateway from the Legacy system. Here we want 100% to be split between stable and canary in Mesh. (we don't want to bounce the traffic back to the legacy)
What do you think?
(maybe we can move the maxWeight to the level of the strategy.canary.trafficRouting.istio.virtualService.routes. Or remove maxWeight and configure ArgoRollouts to manage only the first 2 destinations in founds in a route...)
There was a problem hiding this comment.
There is no straightforward way to do it at the per-route level. Currently, routes is nothing but an array of strings which take the HTTP route names. For us to do it at the route-level, we need a struct/object similar to what we have for tlsRoutes where we can add an optional maxWeight prop.
One proposal is to add an optional httpRoutes which will be an array of objects similar to the tlsRoutes and gradually deprecate the use of routes over time. In the beginning, we can enforce the use of only one i.e either routes or httpRoutes. I proposed this to @jessesuen as well.
@jessesuen @huikang Any suggestions on what would be the best way to move forward here?
There was a problem hiding this comment.
Hi @agrawroh, I am interested in coming up with a solution for this as well. I would like to reopen this PR or something similar to support this feature.
@jessesuen @huikang are there any suggestions for moving forward ?
There was a problem hiding this comment.
@dnguy078 Sure, that'd be great. Please feel free to take it over or let me know how I can help.
There was a problem hiding this comment.
@dnguy078 any update? This is a pretty big blocker for argo-rollouts adoption on our end.
Also note that even though this PR was not addressing this exact issue, it would have been incredibly helpful for single route virtual service.
Change Description
This PR adds the support for defining an optional max weight for the destinations in Istio VirtualService. If the
maxWeightparameter is defined and has a valid value i.e.0 < maxWeight <= 100then it'll be used as an upper bound for the traffic sent to stable service + canary service.The use cases for defining
maxWeightincludes:Signed-off-by: Rohit Agrawal [email protected]
Checklist:
"fix(controller): Updates such and such. Fixes #1234".