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: add webhook for manually approving traffic weight increase #849

Merged

Conversation

mayankshah1607
Copy link
Contributor

@mayankshah1607 mayankshah1607 commented Mar 11, 2021

What is this change?

This PR adds a new webhook for manually approving traffic weight increase. The logic is similar to confirm-promotion, but for approving traffic weight increase.

Why is this needed?

Organizations have use-cases where they would like to wait indefinitely before the traffic weight on the canary increases. For example, we may want to hold at 10% weight, run some checks, smoke/conformance tests or manually verify metrics to ensure that the canary is reliable. This is especially useful in orgs which are in the process of making metrics and SLOs mature.
(Similar to how argo rollouts works)

Although we could use the rollout webhook for this, it wouldn't wait indefinitely and eventually fail as a non-200 response from the webhook would count as an error.

Example

- name: "confirm traffic increase"
  type: confirm-traffic-increase
  url: http://flagger-loadtester.test/gate/check

Signed-off-by: Mayank Shah [email protected]

@mayankshah1607
Copy link
Contributor Author

Hey @stefanprodan! Could you take a look at this?
We at Grofers find this feature to be very useful, and would hence really appreciate if this could get merged. At present we're running our own fork that includes this patch.
Thanks :)

@stefanprodan
Copy link
Member

@mayankshah1607 can you please change the hook name to confirm-traffic-increase

@mayankshah1607 mayankshah1607 force-pushed the mayank/confirm-traffic-webhook branch 3 times, most recently from a1e84a7 to 5cea350 Compare March 16, 2021 14:46
@mayankshah1607 mayankshah1607 force-pushed the mayank/confirm-traffic-webhook branch from 5cea350 to 873141b Compare March 16, 2021 14:48
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.

@mayankshah1607 mayankshah1607 force-pushed the mayank/confirm-traffic-webhook branch from 7ce2c8e to 164bbb8 Compare March 16, 2021 15:02
@mayankshah1607
Copy link
Contributor Author

Thanks @stefanprodan , I have made the required changes.

@codecov-io
Copy link

codecov-io commented Mar 16, 2021

Codecov Report

Merging #849 (164bbb8) into main (f2d121a) will decrease coverage by 0.08%.
The diff coverage is 16.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #849      +/-   ##
==========================================
- Coverage   56.81%   56.72%   -0.09%     
==========================================
  Files          67       67              
  Lines        5418     5430      +12     
==========================================
+ Hits         3078     3080       +2     
- Misses       1881     1890       +9     
- Partials      459      460       +1     
Impacted Files Coverage Δ
pkg/controller/scheduler_hooks.go 9.67% <11.11%> (+0.24%) ⬆️
pkg/controller/scheduler.go 46.42% <33.33%> (-0.09%) ⬇️

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 f2d121a...164bbb8. Read the comment docs.

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 @mayankshah1607

@mayankshah1607
Copy link
Contributor Author

@stefanprodan Anything else to be done to get this one merged?

@stefanprodan
Copy link
Member

@mayankshah1607 all is Ok, I'll merge it before the next release.

@stefanprodan
Copy link
Member

@mayankshah1607 could you add Grofers to the user list here https://github.com/fluxcd/flagger#who-is-using-flagger?

@stefanprodan stefanprodan merged commit 16867db into fluxcd:main Mar 17, 2021
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.

3 participants