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

If HPA is set, it uses HPA minReplicas when scaling up the canary #1253

Merged

Conversation

andylibrian
Copy link
Contributor

Without this, the canary replicas are updated twice: to 1 replica then after a few seconds to the value of HPA minReplicas.

In some cases, when updated to 1 replica (before updated by HPA controller to the minReplicas), it's considered ready: 1 of 1 (readyThreshold 100%), and the canary weight is advanced to receive traffic with less capacity than expected.

Similar PR previously: #1110

Signed-off-by: Andy Librian andylibrian@gmail.com

@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2022

Codecov Report

Merging #1253 (1c33544) into main (e65dfbb) will decrease coverage by 0.05%.
The diff coverage is 31.25%.

@@            Coverage Diff             @@
##             main    #1253      +/-   ##
==========================================
- Coverage   54.74%   54.68%   -0.06%     
==========================================
  Files          81       81              
  Lines        6949     6965      +16     
==========================================
+ Hits         3804     3809       +5     
- Misses       2550     2559       +9     
- Partials      595      597       +2     
Impacted Files Coverage Δ
pkg/canary/deployment_controller.go 44.14% <31.25%> (-0.73%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@aryan9600 aryan9600 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 raising this PR @andylibrian 🙇

pkg/canary/deployment_controller.go Outdated Show resolved Hide resolved
@andylibrian
Copy link
Contributor Author

Thanks for the review, @aryan9600. I have added the v2 HPA support as you suggested. Do you have any other comment?

pkg/canary/deployment_controller.go Outdated Show resolved Hide resolved
@andylibrian andylibrian force-pushed the scale-from-zero-use-hpa-minreplicas branch from 72f7295 to 3df93b8 Compare August 17, 2022 02:34
Copy link
Member

@aryan9600 aryan9600 left a comment

Choose a reason for hiding this comment

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

could you please make this change and also modify the commit title to be something like "use min replicas set by autoscaler in ScaleFromZero, if autoscaler is specified"?

pkg/canary/deployment_controller.go Outdated Show resolved Hide resolved
@andylibrian andylibrian force-pushed the scale-from-zero-use-hpa-minreplicas branch from 066b314 to 1c33544 Compare August 18, 2022 06:13
…specified

Without this, the canary replicas are updated twice:
to 1 replica then after a few seconds to the value of HPA minReplicas.

In some cases, when updated to 1 replica (before updated by HPA
controller to the minReplicas), it's considered ready: 1 of 1 (readyThreshold 100%),
and the canary weight is advanced to receive traffic with less capacity
than expected.

Co-Authored-By: Joshua Gibeon <joshuagibeon7719@gmail.com>
Co-authored-by: Sanskar Jaiswal <hey@aryan.lol>

Signed-off-by: Andy Librian <andylibrian@gmail.com>
@andylibrian andylibrian force-pushed the scale-from-zero-use-hpa-minreplicas branch from 1c33544 to 8b11551 Compare August 18, 2022 06:42
Copy link
Member

@aryan9600 aryan9600 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 @andylibrian 🙇

@aryan9600 aryan9600 merged commit ae4613f into fluxcd:main Aug 24, 2022
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