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

fix: canary replicas/weight could flap during abort with dynamic scaling #1794

Merged
merged 1 commit into from
Jan 25, 2022

Conversation

jessesuen
Copy link
Member

@jessesuen jessesuen commented Jan 20, 2022

Resolves #1665
Resolves #1701

When using dynamicStableScaling, and the rollout was aborted, we would incorrectly increase the weight of the canary if the available stable replicas was flapping. This fix ensures that when we are aborting, we are only ever decreasing the weight and never increasing this.

While testing this fix, I also discovered that a portion of the traffic routing unit tests was not functioning properly because the expected mock functions were not executed properly (Issue #1701)

Signed-off-by: Jesse Suen [email protected]

@jessesuen jessesuen force-pushed the fix-dynamicscale-canary-flap branch from b1df211 to b3a0913 Compare January 20, 2022 20:57
@codecov
Copy link

codecov bot commented Jan 20, 2022

Codecov Report

Merging #1794 (4f2f027) into master (a74ad8e) will increase coverage by 0.20%.
The diff coverage is 88.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1794      +/-   ##
==========================================
+ Coverage   82.07%   82.28%   +0.20%     
==========================================
  Files         116      116              
  Lines       16349    16366      +17     
==========================================
+ Hits        13419    13466      +47     
+ Misses       2243     2220      -23     
+ Partials      687      680       -7     
Impacted Files Coverage Δ
utils/conditions/conditions.go 80.76% <ø> (ø)
rollout/trafficrouting.go 80.45% <85.00%> (+9.37%) ⬆️
rollout/restart.go 98.69% <100.00%> (+0.04%) ⬆️
rollout/trafficrouting/smi/smi.go 94.82% <100.00%> (-0.09%) ⬇️
rollout/controller.go 75.38% <0.00%> (+0.57%) ⬆️
rollout/replicaset.go 69.44% <0.00%> (+1.85%) ⬆️
rollout/canary.go 76.90% <0.00%> (+1.97%) ⬆️

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 a74ad8e...4f2f027. Read the comment docs.

@harikrongali
Copy link
Contributor

LGTM,. @alexmt please approve & merge

@jessesuen
Copy link
Member Author

jessesuen commented Jan 21, 2022

LGTM,. @alexmt please approve & merge

Please approve, but I will merge after I rebase ontop of another PR from external contributor since I don't want them to deal with my conflicts.

EDIT: rebase complete and conflicts resolved

@jessesuen jessesuen force-pushed the fix-dynamicscale-canary-flap branch from b3a0913 to 9753009 Compare January 21, 2022 08:31
@jessesuen jessesuen force-pushed the fix-dynamicscale-canary-flap branch from 9753009 to 4f2f027 Compare January 21, 2022 08:33
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
8.5% 8.5% Duplication

@alexmt alexmt merged commit 9374486 into argoproj:master Jan 25, 2022
@jessesuen jessesuen deleted the fix-dynamicscale-canary-flap branch October 3, 2023 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants