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.

Sign up for GitHub

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

Jump to bottom

feat: Prune resources in reverse of sync wave order #538

Merged
merged 2 commits into from
Jan 24, 2024
Merged

feat: Prune resources in reverse of sync wave order #538

merged 2 commits into from
Jan 24, 2024

Conversation

svghadi
Copy link
Contributor

@svghadi svghadi commented Sep 3, 2023

This PR implements argoproj/argo-cd#15074 enhancement to prune resources in reverse order of sync waves.

Changes:

  • Adds reordering logic which swaps waves between resources marked for pruning such that resources with high syncwave value is deleted first before resources with lower syncwave value.
  • Adds wait till the pruned resource is actually cleaned up from the cluster before moving to next wave.
  • Updates prune last logic to also consider prune tasks to determine last wave.

Related: argoproj/argo-cd#12376

@codecov
Copy link

codecov bot commented Sep 3, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (aba3819) 54.47% compared to head (382d57b) 54.76%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #538      +/-   ##
==========================================
+ Coverage   54.47%   54.76%   +0.29%     
==========================================
  Files          41       41              
  Lines        4793     4824      +31     
==========================================
+ Hits         2611     2642      +31     
  Misses       1969     1969              
  Partials      213      213

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

@svghadi svghadi force-pushed the syncwave branch 2 times, most recently from e81753e to 3540566 Compare September 3, 2023 16:09
@drpaneas
Copy link

drpaneas commented Sep 4, 2023

What happens when:

  1. You have a deployment of 3 pods (1,2,3) with equiv waves (1,2,3)
  2. You set a finalizer on pod-2, so it cannot be deleted.
  3. You ask ArgoCD to delete and prune this deployment.

Can you run this experiment? If your code is correct, only pod-3 should have been deleted.

@sonarcloud
Copy link

sonarcloud bot commented Sep 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
0.0% 0.0% Duplication

@svghadi
Copy link
Contributor Author

svghadi commented Sep 4, 2023

Thanks @drpaneas . Your test scenario helped find a bug in my implementation. I updated the logic to use waveOverride var instead of directly patching live obj. It is now working as expected.

syncwave-prune-order-test.mov

@drpaneas
Copy link

drpaneas commented Sep 4, 2023

Thanks @drpaneas . Your test scenario helped find a bug in my implementation. I updated the logic to use waveOverride var instead of directly patching live obj. It is now working as expected.

Hehe, great ;)

/lgtm

@anandf
Copy link
Contributor

anandf commented Oct 19, 2023

/lgtm

@jgato
Copy link

jgato commented Nov 27, 2023

how is this feature progressing? do we have any plan on which version would be included?

@svghadi svghadi mentioned this pull request Nov 27, 2023
Closed
3 tasks
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Nov 30, 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
0.0% 0.0% Duplication

@xmj
Copy link

xmj commented Dec 6, 2023

@anandf @drpaneas maybe /lgtm wasn't enough.
Says now At least 1 approving review is required to merge this pull request.

@svghadi
Copy link
Contributor Author

svghadi commented Dec 6, 2023

@crenshaw-dev - Could you please take a look at this PR when you have some time? Thank you.

@vl-kp
Copy link

vl-kp commented Dec 20, 2023

Any timeline to merge this PR?

@svghadi
Copy link
Contributor Author

svghadi commented Dec 20, 2023

I have discussed the changes with maintainers on one of the call. Currently working on an e2e test to test the integration with Argo CD. Should be good to merge in next few weeks 🤞.

@svghadi
Prune resources in reverse of sync wave order
0ef379d 
Signed-off-by: Siddhesh Ghadi <sghadi1203@gmail.com>
@svghadi
Use waveOverride var instead of directly patching live obj
382d57b 
Directly patching live objs results into incorrect wave ordering
as the new wave value from live obj is used to perform reordering during next sync

Signed-off-by: Siddhesh Ghadi <sghadi1203@gmail.com>
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Jan 4, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
2.5% Duplication on New Code

See analysis details on SonarCloud

@svghadi svghadi mentioned this pull request Jan 4, 2024
Merged
14 tasks
@svghadi
Copy link
Contributor Author

svghadi commented Jan 4, 2024

Hi @crenshaw-dev, as discussed previously, I have added docs & a e2e test to test the integration with Argo CD in argoproj/argo-cd#16748. The CI is passing.

Is there anything else required to get this in?

ishitasequeira
ishitasequeira approved these changes Jan 9, 2024
View reviewed changes
Copy link
Member

@ishitasequeira ishitasequeira left a comment

Choose a reason for hiding this comment

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

/lgtm

@svghadi
Copy link
Contributor Author

svghadi commented Jan 17, 2024

Thanks @ishitasequeira. Anything else required for merge?

@ishitasequeira ishitasequeira merged commit 5fd9f44 into argoproj:master Jan 24, 2024
4 checks passed
@svghadi svghadi deleted the syncwave branch January 24, 2024 05:39
@svghadi
Copy link
Contributor Author

svghadi commented Jan 24, 2024

I have updated the argoproj/argo-cd#16748 PR to bring this change in ArgoCD. Please take a look. Thanks.

34fathombelow pushed a commit to 34fathombelow/gitops-engine that referenced this pull request Oct 26, 2024
@svghadi @34fathombelow
feat: Prune resources in reverse of sync wave order (argoproj#538)
e3170b1 
* Prune resources in reverse of sync wave order

Signed-off-by: Siddhesh Ghadi <sghadi1203@gmail.com>

* Use waveOverride var instead of directly patching live obj

Directly patching live objs results into incorrect wave ordering
as the new wave value from live obj is used to perform reordering during next sync

Signed-off-by: Siddhesh Ghadi <sghadi1203@gmail.com>

---------

Signed-off-by: Siddhesh Ghadi <sghadi1203@gmail.com>
Signed-off-by: Justin Marquis <34fathombelow@protonmail.com>
gdsoumya pushed a commit that referenced this pull request Oct 31, 2024
@34fathombelow @svghadi
feat: Prune resources in reverse of sync wave order (#538) (#632)
318ae8a 
* Prune resources in reverse of sync wave order



* Use waveOverride var instead of directly patching live obj

Directly patching live objs results into incorrect wave ordering
as the new wave value from live obj is used to perform reordering during next sync



---------

Signed-off-by: Siddhesh Ghadi <sghadi1203@gmail.com>
Signed-off-by: Justin Marquis <34fathombelow@protonmail.com>
Co-authored-by: Siddhesh Ghadi <61187612+svghadi@users.noreply.github.com>
@svghadi svghadi mentioned this pull request Nov 6, 2024
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ishitasequeira ishitasequeira ishitasequeira approved these changes

@anandf anandf anandf approved these changes

@drpaneas drpaneas drpaneas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@svghadi @drpaneas @anandf @jgato @xmj @vl-kp @ishitasequeira