fix: stuck rollout when 2nd deployment happens before 1st finishes

[ashutosh16]

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
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My builds are green. Try syncing with master if they are not.
• My organization is added to USERS.md.

[github-actions]

Go Published Test Results

2 106 tests   2 106 ✅  2m 50s ⏱️
  118 suites      0 💤
    1 files        0 ❌

Results for commit 7e25564.

♻️ This comment has been updated with latest results.

[codecov]

Codecov Report

Attention: 69 lines in your changes are missing coverage. Please review.

Comparison is base (8405f2e) 81.83% compared to head (7e25564) 82.85%.
Report is 9 commits behind head on master.

Files	Patch %	Lines
controller/controller.go	21.68%	61 Missing and 4 partials ⚠️
utils/record/record.go	50.00%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3354      +/-   ##
==========================================
+ Coverage   81.83%   82.85%   +1.01%     
==========================================
  Files         135      135              
  Lines       20688    16841    -3847     
==========================================
- Hits        16931    13953    -2978     
+ Misses       2883     2013     -870     
- Partials      874      875       +1     

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

[github-actions]

E2E Tests Published Test Results

  4 files    4 suites   3h 46m 57s ⏱️
107 tests  94 ✅  6 💤  7 ❌
446 runs  404 ✅ 24 💤 18 ❌

For more details on these failures, see this check.

Results for commit 7e25564.

♻️ This comment has been updated with latest results.

[ashutosh16]

@meeech Would you be able to review the PR?

[meeech]

@ashutosh16 I'll have a look and test it out as well.

Q: Does this fix need to be done for the bluegreen as well. An identical change was made there.

[ashutosh16]

@ashutosh16 I'll have a look and test it out as well.

Q: Does this fix need to be done for the bluegreen as well. An identical change was made there.

I tested with the following scenario:

• basic canary. // worked with fix
• basic canary with analysis. // worked with fix
• basic canary with traffic router. // worked as expected
• blue green // worked as expected

the fix is not needed for b/g, since the svc is already update with correct rollouts-pod-template-hash when a new rs is created. Hence, isReplicaSetReferenced returns false, which means the svc is not referenced by any intermediary replication set

[meeech]

@ashutosh16 I ran the e2e tests with the fix removed, and this test passed. So I don't think this is a valid test? (at least for avoiding regressions)

[ashutosh16]

You're right the e2e does not simulate the scenario correctly. I looked into e2e function UpdateSpec() and it seems it resume the rollout and then update to new revision which doesn't cover this scenario. I'll look into it further.

[ashutosh16]

Fixed the testcase. It should fail without the fix.

[meeech]

So my only concern here is I removed the fix and the test still passed.

[sonarcloud]

Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
3.7% Duplication on New Code

See analysis details on SonarCloud

[meeech]

Linking back to the related issue: #3331