fix(controller): honor scaleUpPreviewCheckPoint reset on blueGreen pod template change. Fixes #4728#4729
Conversation
Published E2E Test Results 4 files 4 suites 3h 50m 12s ⏱️ For more details on these failures, see this check. Results for commit d573618. ♻️ This comment has been updated with latest results. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4729 +/- ##
==========================================
+ Coverage 84.99% 85.05% +0.06%
==========================================
Files 164 164
Lines 18998 18998
==========================================
+ Hits 16147 16159 +12
+ Misses 1998 1992 -6
+ Partials 853 847 -6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Published Unit Test Results2 474 tests 2 474 ✅ 3m 21s ⏱️ Results for commit d573618. ♻️ This comment has been updated with latest results. |
abizer
force-pushed
the
fix-bluegreen-preview-status-check
branch
from
b7f948b to
28c1d69
Compare
|
cc @argoproj/argo-rollouts-approvers |
…d template change When a new pod template is rolled out while the previous revision had already completed prePromotionAnalysis (and thus set status.blueGreen.scaleUpPreviewCheckPoint=true), the new revision inherits that value and Argo Rollouts skips creating an AnalysisRun for it entirely -- including the prePromotionAnalysis gate. syncRolloutStatusBlueGreen calls resetRolloutStatus(&newStatus) on pod template change, which clears newStatus.BlueGreen.ScaleUpPreviewCheckPoint. The subsequent call to calculateScaleUpPreviewCheckPoint read prevValue from c.rollout.Status (the pre-reset value) rather than the newStatus parameter that was just reset, immediately clobbering the reset. Read prevValue from newStatus so the reset applied earlier in the same reconcile is honored, matching the documented behavior: // ... gets reset to false when the pod template changes, or the // rollout fully promotes (stableRS == newRS) Add a regression test that fails on the old behavior and passes with the fix. Signed-off-by: Abizer Lokhandwala <abizer@abizer.me>
abizer
force-pushed
the
fix-bluegreen-preview-status-check
branch
from
28c1d69 to
d573618
Compare
|
|
@kostis-codefresh would you be able to take a look at this when you have a chance? it is a one line fix |
kostis-codefresh
left a comment
kostis-codefresh
left a comment
There was a problem hiding this comment.
I feel that this unit test doesn't really cover the scenario that was described in #4728
It seems to me that we don't have any e2e tests that use previewReplicaCount anyway, so I want to be very cautious about the effects of this PR.
So can we add some e2e test(s) that showcase the bug and verify the actual fix in a more realistic way please?
2 participants
What
Read
prevValuefromnewStatusinstead ofc.rollout.StatusincalculateScaleUpPreviewCheckPoint. WhensyncRolloutStatusBlueGreencallsresetRolloutStatuson pod template change, the reset mutatesnewStatus.BlueGreen.ScaleUpPreviewCheckPoint = false; reading fromc.rollout.Statusimmediately afterwards picks up the pre-reset value, clobbering the reset.Why
Matches the function's documented behavior:
Without this fix, a new revision started while the previous one was in the scale-up /
autoPromotionSecondswindow inheritsscaleUpPreviewCheckPoint: trueand skipsprePromotionAnalysisentirely — the new preview RS scales straight toreplicas(bypassingpreviewReplicaCount), no AnalysisRun is created, and cutover proceeds untested.Full repro + root cause + live-cluster evidence in #4728.
Tests
TestCalculateScaleUpPreviewCheckPointHonorsResetinrollout/bluegreen_test.go. Verified it fails on the unfixed code (Should be false) and passes with the fix.TestPreviewReplicaCountHandleScaleUpPreviewCheckPointsub-tests (TrueAfterMeetingMinAvailable,FalseAfterActiveServiceSwitch,TrueWhenScalingUpPreview) continue to pass../rollout/...package suite passes locally (make testequivalent).Checklist
fix(controller): ...), states what changed, and suffixes the related issue number (Fixes #4728).blueGreen+previewReplicaCount+prePromotionAnalysis: previously a back-to-back revision would skip the gate; now it correctly runsprePromotionAnalysisfor the new revision. No API or config change.