fix(controller): delay background analysis until the canary receives traffic

[n1koo]

For a traffic-routed canary, the background AnalysisRun is created as soon as the new ReplicaSet exists — reconcileBackgroundAnalysisRun checks only that the rollout is not fully promoted, has a stable RS, and is not before startingStep. Nothing checks that the controller has actually programmed the traffic router for that ReplicaSet.

A canary-scoped metric is then evaluated against a ReplicaSet receiving no traffic. Either outcome is wrong: it passes on no evidence, or — the common case, since most providers return no data for a pod with no requests — it fails or errors and aborts a healthy rollout.

The window is real and not small. The canary has to scale up before reconcileTrafficRouting will give it any weight (newRS.Status.AvailableReplicas == 0 forces desiredWeight to 0), and on ALB the target group has to converge on top of that.

The fix

Gate creation on canaryWeightReached():

1. Status.Canary.Weights != nil — the router has been programmed for some canary.
2. Weights.Canary.PodTemplateHash matches the current canary ReplicaSet — not a previous revision that still holds the traffic split.
3. Weights.Canary.Weight >= GetCurrentSetWeight(rollout).
4. If the router populates Weights.Verified, it must be true.

Weights are read from newStatus, which reconcileTrafficRouting populates earlier in the same reconcile (rolloutCanary runs it at canary.go:67, analysis at :76), so the gate does not lag a reconcile behind the router.

Only applies when TrafficRouting != nil; a basic canary shifts traffic by moving service selectors, so there is nothing to wait for.

Where a router does verify weights, the timing works out. Background creation is retried every reconcile while the rollout sits at its setWeight step, and at a setWeight step ShouldVerifyWeight returns true, so Weights.Verified is meaningful exactly when the gate consults it. Note this only applies to ALB with --aws-verify-target-group enabled (defaults.defaultVerifyTargetGroup is false) or a plugin that populates Verified; every other in-tree router returns nil, and for those the gate reduces to "the controller has programmed the step's weight".

Why not startingStep

RolloutAnalysisBackground.StartingStep is the closest existing knob, but BeforeStartingStep compares step indices only. Reaching step 1 does not mean step 0's weight landed: with a slow-scaling canary, or an ALB target group that has not converged, it demonstrably has not. startingStep also cannot express "step 0's weight actually took effect", which is the condition that matters here.

Behaviour change

For a traffic-routed canary with background analysis, the AnalysisRun now starts once traffic has shifted rather than immediately. For analysis scoped to the canary — the documented purpose — that is the intended fix. For a template that measures overall service health as a kill switch, it means the analysis no longer covers the canary scale-up window; nothing is routed to the canary during that window, so a regression there is not attributable to it, but it is a change and worth calling out.

If you would rather this were opt-in behind a field rather than the default, I am happy to rework it that way. I have proposed it as the default because the current behaviour aborts healthy rollouts.

Step-based analysis is deliberately not gated

An earlier version of this PR gated reconcileStepBasedAnalysisRun too. I dropped it:

• It is redundant. completedCurrentCanaryStep (canary.go:319-327) already refuses to advance past a setWeight step until AtDesiredReplicaCountsForCanary and Weights.Verified != false. An analysis step that follows a setWeight step is therefore already reached only after the weight has landed and been verified.
• It could not deliver the verification it implied. At an Analysis step currentStep.SetWeight == nil, so ShouldVerifyWeight is false, ALB's VerifyWeight returns nil, nil, and Verified is overwritten to nil — the clause is inert precisely where a step gate would rely on it.
• It risked an unbounded stall. isIndefiniteStep (sync.go:563) covers Analysis steps, so the progressDeadlineExceeded branch at sync.go:708 is skipped for them. A step-based AnalysisRun that is never created would hang the rollout with no timeout and no explanation.

The background gate has none of these problems: when the canary never becomes ready the rollout stays parked on the setWeight step, which is not an indefinite step, so progressDeadlineExceeded still fires and the rollout goes Degraded with an accurate reason.

The one case dropping the step gate leaves uncovered: [setWeight 50, pause 10m, analysis] where canary pods churn during the pause and the weight regresses before the step's AnalysisRun is created. That is worth a separate change with its own evidence, not a hang risk carried here.

Related work

#4867 gates the abort scale-down delay on the canary traffic having shifted away. Same underlying idea — don't act until the router has converged — but a different predicate: that one asks "has the weight reached zero", this one asks "has it reached the step's target". They share only the one-line Weights.Verified check, so I have kept them as separate helpers in separate files rather than coupling the two PRs to share it. The PRs are independent and can merge in either order.

Testing

• Unit: TestDelayBackgroundAnalysisUntilCanaryWeightReached — no recorded weights, weight 0, weights belonging to a previous ReplicaSet, unverified router, verified router, non-verifying router, and a basic canary (which must be unaffected). Each of the "wait" cases fails if the gate is removed, and the basic-canary case fails if the traffic-routing guard is removed.
• E2E: TestIstioSuite/TestIstioCanaryBackgroundAnalysisWaitsForCanaryWeight — against a real Istio VirtualService, holds the canary pod un-ready so the canary stays at weight 0 and asserts no AnalysisRun is created; marking the pod ready takes the weight to 50 and the AnalysisRun appears.
• Docs: a short note in the existing "Delay Analysis Runs" section of docs/features/analysis.md, alongside initialDelay and startingStep.
• go build, go vet, golangci-lint run, gofmt, and the full ./rollout/... suite: clean. (gofmt flags test/e2e/functional_test.go, which is pre-existing on master and untouched here.)

No filed upstream issue matches this behaviour; the closest related report is #2519 (analysis running multiple times), which is a different failure.

Checklist:

• (b) this is a bug fix
• The title of the PR is conventional and states what changed. No related upstream issue is filed for this specific behaviour, so no Fixes # suffix.
• I've signed my commits with DCO
• My builds are green.
• I have written unit tests for my change and an Istio e2e test.
• I have run all tests locally and they pass on my workstation.
• I have used LLM/AI/Agent tools for this PR but I am responsible for all code of this PR.
• I understand what the code does and WHY/HOW it works in several scenarios.
• I know if my code is just adding new functionality or changing old functionality for existing users.
• My organization is added to USERS.md.

[github-actions]

Published E2E Test Results

  4 files    4 suites   3h 57m 27s ⏱️
124 tests 114 ✅  7 💤 3 ❌
504 runs  469 ✅ 28 💤 7 ❌

For more details on these failures, see this check.

Results for commit c30e93a.

♻️ This comment has been updated with latest results.

[github-actions]

Published Unit Test Results

2 580 tests   2 580 ✅  3m 28s ⏱️
  130 suites      0 💤
    1 files        0 ❌

Results for commit c30e93a.

♻️ This comment has been updated with latest results.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.08%. Comparing base (d1dd467) to head (c30e93a).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4864      +/-   ##
==========================================
- Coverage   85.12%   85.08%   -0.05%     
==========================================
  Files         166      166              
  Lines       19238    19251      +13     
==========================================
+ Hits        16376    16379       +3     
- Misses       2019     2024       +5     
- Partials      843      848       +5     

Flag	Coverage Δ
e2e	52.41% <69.23%> (-0.10%)	⬇️
unit-tests	81.56% <100.00%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[n1koo]

The startingStep is a field from the above custom resource. It is not a variable name from code?

Not sure if I followed you though, mind explaining bit more?

[Hariharasuthan99]

@kostis-codefresh @zachaller The reason behind this change looks valid and I can't think of a scenario where this change can impact negatively. Although if some users have come to expect the analysis to start from the beginning of the rollout irrespective of the traffic being routed to the canary pods or not, then it would be a noticeable change in behaviour. But still, this change seems valid.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud