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: continue update process in middle of update if spec.replicas is 0 #1764

Merged

Conversation

huikang
Copy link
Member

@huikang huikang commented Jan 13, 2022

  • if canary.spec.replicas==0 as in the canary steps, update continues

Signed-off-by: Hui Kang [email protected]

fix #1690

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed my commits with DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

@huikang huikang force-pushed the 1690-canary-not-trigger-weight-zero branch 3 times, most recently from 5d8eb22 to e91436e Compare January 14, 2022 15:06
@codecov
Copy link

codecov bot commented Jan 14, 2022

Codecov Report

Merging #1764 (4d1e68d) into master (f419820) will increase coverage by 0.01%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1764      +/-   ##
==========================================
+ Coverage   82.03%   82.05%   +0.01%     
==========================================
  Files         116      116              
  Lines       16175    16219      +44     
==========================================
+ Hits        13269    13308      +39     
- Misses       2227     2231       +4     
- Partials      679      680       +1     
Impacted Files Coverage Δ
rollout/canary.go 74.71% <92.85%> (+0.22%) ⬆️
pkg/apis/rollouts/validation/validation.go 93.96% <0.00%> (-0.11%) ⬇️
utils/replicaset/canary.go 89.49% <0.00%> (+0.07%) ⬆️

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 f419820...4d1e68d. Read the comment docs.

@huikang huikang requested a review from jessesuen January 14, 2022 16:43
- if canary.spec.replicas==0 as in the canary steps, update continues

Signed-off-by: Hui Kang <[email protected]>
@huikang huikang force-pushed the 1690-canary-not-trigger-weight-zero branch from e91436e to abc8029 Compare January 14, 2022 16:43
@harikrongali
Copy link
Contributor

thanks for fixing the issue. @huikang

@@ -630,5 +630,5 @@ func IsReplicaSetReady(rs *appsv1.ReplicaSet) bool {
}
replicas := rs.Spec.Replicas
readyReplicas := rs.Status.ReadyReplicas
return replicas != nil && *replicas != 0 && readyReplicas != 0 && *replicas <= readyReplicas
return (replicas != nil && *replicas != 0 && readyReplicas != 0 && *replicas <= readyReplicas) || (replicas != nil && *replicas == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you update description to the function with added condition

@@ -630,5 +630,5 @@ func IsReplicaSetReady(rs *appsv1.ReplicaSet) bool {
}
replicas := rs.Spec.Replicas
readyReplicas := rs.Status.ReadyReplicas
return replicas != nil && *replicas != 0 && readyReplicas != 0 && *replicas <= readyReplicas
return (replicas != nil && *replicas != 0 && readyReplicas != 0 && *replicas <= readyReplicas) || (replicas != nil && *replicas == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please add the given scenario as an e2e test?

@huikang huikang force-pushed the 1690-canary-not-trigger-weight-zero branch 7 times, most recently from 040f91f to d0c2d2a Compare January 15, 2022 23:18
@huikang huikang force-pushed the 1690-canary-not-trigger-weight-zero branch from d0c2d2a to 4d1e68d Compare January 16, 2022 03:08
@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
2.0% 2.0% Duplication

@jessesuen jessesuen merged commit c77f9b5 into argoproj:master Jan 18, 2022
@jessesuen
Copy link
Member

Great job!

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.

Manifest update in middle of canary is not deploying the latest
3 participants