fix(controller): honor scaleDownDelaySeconds before scaling down the intermediate RS

[andrewjamesbrown]

We are experiencing a problem with our use of Istio and Argo Rollouts. During an "interrupted canary", we see:

• Set weight to 0 for all Istio VirtualServices
• Update DestinationRule to point to new canary
• Scale down intermediate replicaset
• Set weight to 0 for all Istio VirtualServices (this is done twice in 1.9 to avoid setting the weight prior to the DR - see fix(trafficrouting): ensure early SetWeight runs after checkReplicasAvailable #4639 and fix(trafficrouting): set weight before hash for new canary #4564

The issue occurs because the Istio control plane is async - it takes time for the configuration to sync to all the proxies, and during this time, we've started to scale down the intermediate replicaset. In this case, it's desirable to delay the scale-down of the intermediate replicaset until the Istio data plane is in sync.

We see this on a 1.9-equivalent release that we build internally.

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
• The title of the PR is (a) conventional with a list of types and scopes found here, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
• I've signed my commits with DCO
• My builds are green. Try syncing with master if they are not.
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• I have run all tests locally (including the flaky ones) 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   4h 14m 2s ⏱️
149 tests 137 ✅  7 💤 5 ❌
604 runs  568 ✅ 28 💤 8 ❌

For more details on these failures, see this check.

Results for commit 1523afc.

♻️ This comment has been updated with latest results.

[github-actions]

Published Unit Test Results

2 629 tests   2 629 ✅  3m 31s ⏱️
  131 suites      0 💤
    1 files        0 ❌

Results for commit 1523afc.

♻️ This comment has been updated with latest results.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.20%. Comparing base (f2c5c2b) to head (1523afc).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4645      +/-   ##
==========================================
+ Coverage   85.17%   85.20%   +0.02%     
==========================================
  Files         166      166              
  Lines       19453    19456       +3     
==========================================
+ Hits        16570    16578       +8     
+ Misses       2030     2027       -3     
+ Partials      853      851       -2     

Flag	Coverage Δ
e2e	53.17% <60.00%> (+0.14%)	⬆️
unit-tests	81.65% <80.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.

[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

[Hariharasuthan99]

LGTM!

[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

[Hariharasuthan99]

Hello @andrewjamesbrown, I see after you merged master, one of the unit test has failed, could you check and see if you could fix it?

[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

[kostis-codefresh]

I have no major comments regarding the code just a question for backwards compatibiility.

The original purpose of dynamicStableScale was for cost reasons where users
don't want to keep too many pods active.

But if this PR is merged, then canary users that have enabled dynamicStableScale will see
out of the blue the intermediate RS still running for 30 seconds (default value for scaleDownDelaySeconds) in interrupted canaries even though they have explicitly defined that they don't want any extra pods. So their workflow will change simply by upgrading to a new version of Rollouts.

Not only that, but if they try to add scaleDownDelaySeconds to 0, it turns out that existing validation doesn't allow both scaleDownDelaySeconds and dynamicStableScale to be active.

Do I get that correctly?

Also it turns out that there is a similar PR at #4925 It has a quick fix that drops the validation and allows people to define both scaleDownDelaySeconds and dynamicStableScale. But I am not sure if this is the correct solution either. It will open a can of worms as it doesn't make sense to have both of them enabled at the same time (for the happy path scenario).

I think the bigger question to ask here is if scaleDownDelaySeconds needs to be overloaded with this responsibility. Maybe we need a brand new field here?

I see three ways forward with this PR

1. Merge it as is and note in the release notes the behavior change. Accept the risk of breaking existing users

2. Drop the validation of both fields together. Merge and then accept the future risk of opening a can of worms. Users will set scaleDownDelaySeconds expecting it to apply even in the happy path scenario and it won't.

3. Create a brand new variable (needs design and discussion) specifically about intermediate/interrupted canaries.

@zachaller any thoughts?