fix(controller): require missing stable RS before blue-green fast-track on empty selector - #4967
Open
pujitha24 wants to merge 1 commit into
Open
fix(controller): require missing stable RS before blue-green fast-track on empty selector#4967pujitha24 wants to merge 1 commit into
pujitha24 wants to merge 1 commit into
Conversation
…ck on empty selector Motivation: In blue-green rollouts, isBlueGreenFastTracked() treated a missing rollouts-pod-template-hash key on the active Service's selector as an unconditional signal to fast-track the update: skip the blue-green pause and (via reconcileActiveService) switch the active selector straight to the incoming ReplicaSet's hash, bypassing prePromotionAnalysis and the autoPromotionEnabled=false gate. This was only supposed to apply to a genuine first-ever deploy, but the code never checked whether a stable ReplicaSet already existed. If the active Service's selector was ever missing/cleared for any other reason while a stable revision was already recorded, the controller would misclassify the situation and promote an unverified new ReplicaSet straight to active without any of the configured safety gates. This matches the report: an active service selector observed as empty during a Progressing rollout with autoPromotionEnabled=false and prePromotionAnalysis configured, followed by a SwitchService event moving the selector directly to a bad revision's hash with no analysis or manual approval in between. Approach: Gate the "missing selector key" fast-track branch in isBlueGreenFastTracked() on c.stableRS == nil (the same signal shouldFullPromote already uses elsewhere for "no stable ReplicaSet recorded yet"). This is a narrowing change: it only removes fast-tracking in the case where a stable ReplicaSet already exists, which was never a correct case to fast-track through. Note on scope: this change stops the controller from promoting an unverified new ReplicaSet when the active selector is unexpectedly empty. It does not restore active-service traffic on its own — while a stable ReplicaSet exists and pause/prePromotionAnalysis have not yet completed, the active selector stays as-is (still empty in that scenario) rather than jumping to the new ReplicaSet. It also does not address what causes the active Service's selector to become empty in the first place; that is a separate, unconfirmed question raised in the report. Validation: - go build ./... (clean) - go vet ./rollout/... (clean) - go test ./rollout/... -count=1 (all packages pass) - Targeted regression test: with rollout/bluegreen.go reverted (git stash) to the pre-fix code, `go test ./rollout/... -run TestBlueGreenFastTrackRequiresNoStableRS -v` fails on the "ExistingStableRSIsNotFastTrackedDespiteEmptySelector" subtest exactly as expected (isBlueGreenFastTracked wrongly returns true). Restoring the fix makes the same test pass. No e2e/live-cluster reproduction was run; this is a pure controller-logic defect fully covered by the failing-then-passing unit test above. Report: argoproj#4750 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4967 +/- ##
==========================================
- Coverage 85.17% 85.13% -0.05%
==========================================
Files 166 166
Lines 19453 19453
==========================================
- Hits 16570 16562 -8
- Misses 2030 2034 +4
- Partials 853 857 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Published E2E Test Results 4 files 4 suites 4h 7m 49s ⏱️ For more details on these failures, see this check. Results for commit ceffa71. |
Contributor
Published Unit Test Results2 628 tests 2 628 ✅ 3m 30s ⏱️ Results for commit ceffa71. |
Author
|
This is rebased and green whenever someone has a moment to take a look — happy to make any changes that would help review go smoother. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist:
"fix(controller): Updates such and such. Fixes #1234".AI assistance: this change was drafted with Claude Code.
Fixes #4750