fix(controller): sync status when traffic routing errors to allow progress deadline checks#4826
Conversation
…ss deadline checks When `reconcileTrafficRouting()` returns an error (e.g. Istio delaying the DestinationRule switch until the canary ReplicaSet is fully available), the original code returned immediately, so `syncRolloutStatusCanary()` — and its `evaluateProgressDeadlineAbort()` call — was never reached on any reconciliation cycle. This meant a rollout with `progressDeadlineAbort: true` and canary pods stuck in CrashLoopBackOff would never be aborted, regardless of how much time had elapsed past `progressDeadlineSeconds`. Fix: when `reconcileTrafficRouting()` fails, call `syncRolloutStatusCanary()` before returning the error. Status is updated on every cycle, so the deadline check always fires. Fixes argoproj#4626 Signed-off-by: Goutham Annem <gouthemannem@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4826 +/- ##
==========================================
- Coverage 85.09% 85.04% -0.05%
==========================================
Files 166 166
Lines 19136 19139 +3
==========================================
- Hits 16283 16277 -6
- Misses 2005 2010 +5
- Partials 848 852 +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:
|
Published E2E Test Results 4 files 4 suites 3h 53m 25s ⏱️ For more details on these failures, see this check. Results for commit 29b4ddb. ♻️ This comment has been updated with latest results. |
Published Unit Test Results2 518 tests 2 518 ✅ 3m 23s ⏱️ Results for commit 29b4ddb. ♻️ This comment has been updated with latest results. |
…nd status sync fail Adds TestProgressDeadlineSyncStatusErrorWithTrafficRoutingError to exercise the branch where reconcileTrafficRouting() and syncRolloutStatusCanary() both return errors, verifying that the sync error (not the traffic routing error) is surfaced so the controller requeues. Signed-off-by: Goutham Annem <gouthemannem@gmail.com>
8cad80f to
ee830c2
Compare
TestProgressDeadlineAbortWithTrafficRoutingError and TestProgressDeadlineSyncStatusErrorWithTrafficRoutingError share identical fixture setup. Extract into newProgressDeadlineTrafficRoutingFixture to stay below SonarCloud's 40% new-code duplication threshold. Signed-off-by: Goutham Annem <gouthemannem@gmail.com>
|



What this PR does
When
reconcileTrafficRouting()returns an error — for example, when Istio delays the DestinationRule switch until the canary ReplicaSet is fully available — the controller previously returned immediately without callingsyncRolloutStatusCanary(). BecausesyncRolloutStatusCanary()is whereevaluateProgressDeadlineAbort()runs, the progress deadline could never fire while traffic routing kept failing on every reconciliation cycle.Root cause (from issue #4626):
Fix: When
reconcileTrafficRouting()fails, callsyncRolloutStatusCanary()before returning the error. Status is updated on every reconciliation cycle so the deadline check always gets a chance to run.Test coverage
TestProgressDeadlineAbortWithTrafficRoutingError: creates a canary rollout withprogressDeadlineAbort: true, injects an already-timed-out progressing condition, sets the mock traffic router to return an error, and verifies thatverifyPatchedRolloutAborted()fires even when the traffic routing error path is taken.TestReconcileTrafficRoutingSetWeightErr,TestReconcileTrafficRoutingVerifyWeightEndOfRollout,TestTrafficRoutingErrorsWhenNewCanaryHasNoReplicas) to expect the extra status patch that is now emitted on every traffic routing error.Which issue(s) this PR fixes
Fixes #4626