Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: rollouts getting stuck due to bad rs informer updates #3200

Merged
merged 10 commits into from
Dec 4, 2023

Conversation

zachaller
Copy link
Collaborator

@zachaller zachaller commented Nov 30, 2023

This fixes: #3080 and re-works the if logic to avoid the if err==nil constructs.

k8s also does not seem to have a way to test/mock informer update functions but I did add some unit tests for when rs updates fail.

@zachaller zachaller force-pushed the fix-rs-informer-update-logic branch from 63bddcc to 6e610a4 Compare November 30, 2023 21:53
Copy link
Contributor

github-actions bot commented Nov 30, 2023

Go Published Test Results

2 086 tests   2 086 ✔️  2m 47s ⏱️
   118 suites         0 💤
       1 files           0

Results for commit 938141a.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Nov 30, 2023

E2E Tests Published Test Results

    4 files      4 suites   3h 30m 9s ⏱️
103 tests   92 ✔️   6 💤   5
428 runs  389 ✔️ 24 💤 15

For more details on these failures, see this check.

Results for commit 938141a.

♻️ This comment has been updated with latest results.

Signed-off-by: Zach Aller <[email protected]>
Copy link

codecov bot commented Nov 30, 2023

Codecov Report

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

Comparison is base (2270a50) 81.84% compared to head (1996c84) 81.80%.
Report is 3 commits behind head on master.

❗ Current head 1996c84 differs from pull request most recent head 938141a. Consider uploading reports for the commit 938141a to get more accurate results

Files Patch % Lines
rollout/sync.go 23.07% 7 Missing and 3 partials ⚠️
rollout/replicaset.go 50.00% 4 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3200      +/-   ##
==========================================
- Coverage   81.84%   81.80%   -0.04%     
==========================================
  Files         134      134              
  Lines       20558    20576      +18     
==========================================
+ Hits        16825    16832       +7     
- Misses       2869     2875       +6     
- Partials      864      869       +5     

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

Signed-off-by: Zach Aller <[email protected]>
Signed-off-by: Zach Aller <[email protected]>
Signed-off-by: Zach Aller <[email protected]>
Signed-off-by: Zach Aller <[email protected]>
Signed-off-by: Zach Aller <[email protected]>
Signed-off-by: Zach Aller <[email protected]>
@zachaller zachaller requested a review from leoluz December 2, 2023 22:38
Copy link
Contributor

@leoluz leoluz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Signed-off-by: Zach Aller <[email protected]>
Copy link

sonarcloud bot commented Dec 4, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
11.1% 11.1% Duplication

@zachaller zachaller merged commit c067a69 into argoproj:master Dec 4, 2023
22 checks passed
zachaller added a commit that referenced this pull request Dec 4, 2023
* fix: for rollouts getting stuck due to bad rs informer updates

Signed-off-by: Zach Aller <[email protected]>

* fix error msg

Signed-off-by: Zach Aller <[email protected]>

* change logic

Signed-off-by: Zach Aller <[email protected]>

* error fmt

Signed-off-by: Zach Aller <[email protected]>

* change if logic

Signed-off-by: Zach Aller <[email protected]>

* add test

Signed-off-by: Zach Aller <[email protected]>

* cleanup test

Signed-off-by: Zach Aller <[email protected]>

* cleanup test

Signed-off-by: Zach Aller <[email protected]>

* do not double call

Signed-off-by: Zach Aller <[email protected]>

---------

Signed-off-by: Zach Aller <[email protected]>
@zachaller zachaller added the cherry-pick-completed Used once we have cherry picked the PR to all requested releases label Dec 4, 2023
ashutosh16 pushed a commit to ashutosh16/argo-rollouts that referenced this pull request Dec 8, 2023
…3200)

* fix: for rollouts getting stuck due to bad rs informer updates

Signed-off-by: Zach Aller <[email protected]>

* fix error msg

Signed-off-by: Zach Aller <[email protected]>

* change logic

Signed-off-by: Zach Aller <[email protected]>

* error fmt

Signed-off-by: Zach Aller <[email protected]>

* change if logic

Signed-off-by: Zach Aller <[email protected]>

* add test

Signed-off-by: Zach Aller <[email protected]>

* cleanup test

Signed-off-by: Zach Aller <[email protected]>

* cleanup test

Signed-off-by: Zach Aller <[email protected]>

* do not double call

Signed-off-by: Zach Aller <[email protected]>

---------

Signed-off-by: Zach Aller <[email protected]>
Signed-off-by: ashutosh16 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick/release-1.6 cherry-pick-completed Used once we have cherry picked the PR to all requested releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rollout failing with msg "the object has been modified; please apply your changes to the latest version"
2 participants