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

feat: custom weights in progression #726

Merged
merged 12 commits into from
Nov 20, 2020

Conversation

robq99
Copy link
Contributor

@robq99 robq99 commented Nov 12, 2020

This is an implementation for #164.
More info on the usage here: docs/gitbook/tutorials/rollout-weights.md.

canary:
analysis:
promotion:
fullWeight: 1000
Copy link
Contributor

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?

Copy link
Member

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.

Copy link
Contributor Author

@robq99 robq99 Nov 16, 2020

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

@robq99 robq99 Nov 16, 2020

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).

Copy link
Contributor Author

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.

Copy link
Member

@stefanprodan stefanprodan left a 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
Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@robq99 robq99 Nov 18, 2020

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.

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @robq99 🏅

@stefanprodan stefanprodan merged commit 4c3bab7 into fluxcd:master Nov 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants