-
Notifications
You must be signed in to change notification settings - Fork 743
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
feat: custom weights in progression #726
feat: custom weights in progression #726
Conversation
canary: | ||
analysis: | ||
promotion: | ||
fullWeight: 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit confusing to use different units for custom weights. Did you consider using a normal 0-100 range where the promotion happens at the last defined value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works with Linkerd for now, but when Linkerd will switch to TrafficSplit v1alpha3 then it needs 0-100. @robq99 wouldn't this break all the other service meshes and ingress controllers? none accepts a weight bigger than 100.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit confusing to use different units for custom weights. Did you consider using a normal 0-100 range where the promotion happens at the last defined value?
I cannot use 0-100 range as I want to have more granular deployment, like 0.01% => 0.1% => 1%, etc. I would need to change weight to float, which would be more dramatic change (FYI: this is my first code in golang...), if possible at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could be a future improvement. Otherwise this creates feature drift where custom weight vs non custom weight strategies use different formats to represent the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works with Linkerd for now, but when Linkerd will switch to TrafficSplit v1alpha3 then it needs 0-100. @robq99 wouldn't this break all the other service meshes and ingress controllers? none accepts a weight bigger than 100.
SMI docs states in smi-example-implementation that weight can go above 100.
Of course, if SMI standard does not allow it then the whole idea is broken but as I was not able to find any constraints on weights on both, SMI and Linkerd2 side.
@stefanprodan I saw you are involved into v1alpha2
migration on Linkerd2 side, could it be updated in a way that any weights are supported? I.e. no 0-100 constraint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robq99 I forgot to update the SMI docs on this, the weight is of type int
and it should only accept 0-100
range like any other service mesh and ingress out there. Here is the actual code https://github.com/servicemeshinterface/smi-sdk-go/blob/master/pkg/apis/split/v1alpha3/traffic_split.go#L50
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any particular reason that this is int, not float? Can we potentially change it to float?
As with those two conditions (int, 0-100 range) it is not possible to have more granular rollouts, like 0.01%, 0.1%, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robq99 Envoy doesn't support floats nor does NGINX or any other proxy that I know of except for Linkerd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is about float vs int. So, float cannot be used, checked.
As I understand you want to keep compatibility with the current integrations. This requirement shall be met as fullWeight
is optional - 100
is used if it is not specified which is exactly what is used now. I could add additional validation if canary.provider
is not linkerd but having the range >100
is still open question.
Can we then use >100
total value for optional fullWeight
? Envoy and NGINX support total weight > 100, not sure about other proxies, though.
(I would also suggest using totalWeight
instead of fullWeight
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stefanprodan I removed fullWeight
from schemas. I extracted 100
in scheduler to totalWeight
method - this is not changing any weight related functionality but will make my life easier as I will play with total weight on my fork, I hope this is OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR but I can't accept something that would break all integrations. To get this into an acceptable state, there should be s single field added to the API stepWeights
of type []int
, if a value in the array exceeds 100
then the canary should be rejected.
@@ -0,0 +1,38 @@ | |||
# Linkerd Rollout Weights |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please move this to the Linkerd tutorial
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should be applicable to any rollout, not only Linkerd. What about removing Linkerd
from the title?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I also added test to cover new functionality, along with protection to weight overflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @robq99 🏅
This is an implementation for #164.
More info on the usage here: docs/gitbook/tutorials/rollout-weights.md.