fix(controller): use GetScaledValueFromIntOrPercent for maxSurge/maxUnavailable#4698
fix(controller): use GetScaledValueFromIntOrPercent for maxSurge/maxUnavailable#4698SAY-5 wants to merge 2 commits intoargoproj:masterfrom
Conversation
…navailable resolveFenceposts and MaxUnavailable used intstrutil.GetValueFromIntOrPercent to resolve maxSurge and maxUnavailable. That function has a long-standing bug documented in k8s.io/apimachinery: when the IntOrString carries a string whose content happens to be an integer (for example the user wrote maxUnavailable as a quoted string), it is interpreted as a percentage instead of an absolute count. k8s deprecated the call years ago and the Deployment controller already migrated to GetScaledValueFromIntOrPercent, which accepts the string-encoded integer as an absolute value. Swap the three call sites to GetScaledValueFromIntOrPercent. Argo Rollouts now matches Deployment semantics, so a rollout with an integer and a string integer for maxSurge / maxUnavailable behaves the same way. Fixes argoproj#4567. Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
Published E2E Test Results 4 files 4 suites 4h 3m 0s ⏱️ For more details on these failures, see this check. Results for commit 576fc86. ♻️ This comment has been updated with latest results. |
Published Unit Test Results2 453 tests 2 453 ✅ 3m 19s ⏱️ Results for commit 576fc86. ♻️ This comment has been updated with latest results. |
|
If you do use AI tools, please remember to leave the PR template intact https://github.com/argoproj/argo-rollouts/blob/master/.github/pull_request_template.md |
|
Restored the full PR template checklist, including the |
SAY-5
changed the title
…t percentages GetScaledValueFromIntOrPercent rejects an IntOrString whose string value has no '%' suffix. The old GetValueFromIntOrPercent silently coerced the bare string '25' into a percentage, which is the exact historical bug this PR is removing — but the defaults package was exploiting that coercion, so switching every call site to GetScaledValueFromIntOrPercent caused MaxUnavailable / resolveFenceposts to return the zero-value path for any rollout without an explicit maxSurge / maxUnavailable (e.g. TestRestartMaxUnavailable). Promote the string literal '25' to '25%' so the defaults parse correctly under the new function. No user-facing semantic change: '25' was always intended as a percentage. Signed-off-by: SAY-5 <say.apm35@gmail.com>
|
Follow-up commit addresses the unit-test failure in TestRestartMaxUnavailable: |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4698 +/- ##
==========================================
+ Coverage 84.92% 84.95% +0.02%
==========================================
Files 164 164
Lines 18966 18966
==========================================
+ Hits 16107 16112 +5
+ Misses 2002 1999 -3
+ Partials 857 855 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Checklist:
Description
resolveFencepostsandMaxUnavailableusedintstrutil.GetValueFromIntOrPercentto resolvemaxSurge/maxUnavailable. That function has a long-standing bug documented in k8s.io/apimachinery: when theIntOrStringcarries a string whose content happens to be an integer (for example the user wrotemaxUnavailable: "1"), it is interpreted as a percentage instead of an absolute count. Kubernetes deprecated it years ago, and the Deployment controller migrated toGetScaledValueFromIntOrPercentwhich parses the string as an absolute integer when it looks like one.Swap the three call sites in
utils/replicaset/replicaset.gotoGetScaledValueFromIntOrPercent. Argo Rollouts now matches Deployment semantics, so a rollout withmaxSurge: 1, maxUnavailable: "1"behaves the same way asmaxSurge: 1, maxUnavailable: 1.Fixes
Fixes #4567
Test Plan
go build ./...green.go test ./utils/replicaset/... -count=1passes (existing fencepost tests unchanged).gofmtclean.Signed-off-by: SAY-5 SAY-5@users.noreply.github.com
Checklist
"fix(controller): Updates such and such. Fixes #1234".