feat: delete in reverse order of sync waves #3959

darshanime · 2020-07-19T17:40:06Z

Signed-off-by: darshanime [email protected]

closes #3211

Checklist:

Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
The title of the PR states what changed and the related issues number (used for the release note).
I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
Optional. My organization is added to USERS.md.
I've signed the CLA and my build is green (troubleshooting builds).

Signed-off-by: darshanime <[email protected]>

codecov-commenter · 2020-07-20T05:50:28Z

Codecov Report

Merging #3959 into master will decrease coverage by 0.01%.
The diff coverage is 82.60%.

@@            Coverage Diff             @@
##           master    #3959      +/-   ##
==========================================
- Coverage   42.09%   42.07%   -0.02%     
==========================================
  Files         122      123       +1     
  Lines       17944    17994      +50     
==========================================
+ Hits         7553     7571      +18     
- Misses       9425     9453      +28     
- Partials      966      970       +4

Impacted Files	Coverage Δ
controller/appcontroller.go	`43.65% <33.33%> (+0.22%)`	⬆️
controller/sort_delete.go	`100.00% <100.00%> (ø)`
util/rbac/rbac.go	`71.82% <0.00%> (-1.96%)`	⬇️
cmd/argocd/commands/app.go	`5.53% <0.00%> (-0.11%)`	⬇️
util/settings/filtered_resource.go	`100.00% <0.00%> (ø)`
util/argo/normalizers/diff_normalizer.go	`80.39% <0.00%> (+3.19%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e92e0fa...c55ca6d. Read the comment docs.

jannfis

Thanks @darshanime - LGTM so far. Only have some minor understanding question.

Just as a side-note: I know it's not widely spread in the ArgoCD codebase (yet!), but I'm a big fan of code comments explaining why something is done the way it is done - helps during reviews and also helps people understanding the code later on 🤓

jannfis · 2020-07-20T19:52:19Z

controller/appcontroller.go

-		if ctrl.shouldBeDeleted(app, objsMap[k]) && objsMap[k].GetDeletionTimestamp() == nil {
+		if objsMap[k].GetDeletionTimestamp() != nil {
+			logCtx.Infof("%d objects remaining for deletion", len(objsMap))
+			return objs, nil
+		}
+		if ctrl.shouldBeDeleted(app, objsMap[k]) {


What is the reason for the logic to return here if any of the objects has a deletion timestamp?

hi @jannfis,
if we have objects pending deletion we want to wait for them to complete before we proceed with the next wave of deletion. Since this function is registered as an update handler, it will be invoked when the deletes are completed, and we can issue kubectl.DeleteResource for the next wave then.

added a comment!

Signed-off-by: darshanime <[email protected]>

jannfis

LGTM!

jannfis · 2020-07-21T06:24:31Z

Just one further question: Does that mean if any of the objects in a higher sync wave can not be deleted, the whole delete operation will be stuck and no further objects will be deleted? Can this be manually circumvented somehow?

darshanime · 2020-07-21T06:59:12Z

Yes, that's correct @jannfis, sorry forgot to point it out.

I thought it's okay to have this behaviour since it's the same during sync as well - if lower sync wave fails to deploy syncing won't progress further. This can be circumvented by manually deleting the resources.

jannfis · 2020-07-21T07:51:30Z

Alright - I think we should document this behaviour in the sync wave docs. In a separate PR, before this feature is released :)

* feat: delete in reverse order of sync waves Signed-off-by: darshanime <[email protected]> * feat: add tests for deletion in order Signed-off-by: darshanime <[email protected]> * feat: fix lint for appcontroller.go Signed-off-by: darshanime <[email protected]> * feat: add comment to explain early return Signed-off-by: darshanime <[email protected]>

jgato · 2023-02-06T13:06:30Z

I have doubt about the reverse order. We have a situation when we deployed something like

ns (loweste wave)
cm, secrets
some other resource
one conflicting resource (highest wave)
This is created correctly.
When we delete everything the conflicting resource take long time to be deleted. But this dont avoids ArgoCD to try to delete all the resources, including the NS. The NS cannot be deleted because other resources are there. Now the conflicting resource cannot be deleted because the NS is marked for been deleted. So everything is blocked.
Reading some comments above (@darshanime) , the waves below the conflicting resource, cannot start deleting, until the conflicting resource is deleted, right? But when I try to delete all the objects in the app (we dont delete the app), the NS is inmediatly markes as finalizing. I dont see that waiting for the other waves..

darshanime added 2 commits July 19, 2020 23:07

feat: delete in reverse order of sync waves

87e0e7b

Signed-off-by: darshanime <[email protected]>

feat: add tests for deletion in order

f96ce1e

Signed-off-by: darshanime <[email protected]>

darshanime force-pushed the sync_delete branch 8 times, most recently from 502c684 to d6b2a47 Compare July 20, 2020 05:16

feat: fix lint for appcontroller.go

c2e5ccc

Signed-off-by: darshanime <[email protected]>

darshanime force-pushed the sync_delete branch from d6b2a47 to c2e5ccc Compare July 20, 2020 05:41

darshanime marked this pull request as ready for review July 20, 2020 06:04

jannfis reviewed Jul 20, 2020

View reviewed changes

feat: add comment to explain early return

c55ca6d

Signed-off-by: darshanime <[email protected]>

jannfis approved these changes Jul 21, 2020

View reviewed changes

jannfis merged commit 53a9222 into argoproj:master Jul 21, 2020

alexmt mentioned this pull request Jul 21, 2020

bug: deleting project and application at the same time using autosync&prune #3175

Closed

3 tasks

greglanthier mentioned this pull request Mar 19, 2022

Upgrade to argo-cd 2.3.x redhat-developer/gitops-operator#285

Closed

jgato mentioned this pull request Feb 9, 2023

Sync-waves not working as expected #12376

Closed

3 tasks

svghadi mentioned this pull request Aug 16, 2023

Prune resources in reverse order of syncwave during sync #15074

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: delete in reverse order of sync waves #3959

feat: delete in reverse order of sync waves #3959

darshanime commented Jul 19, 2020

codecov-commenter commented Jul 20, 2020 •

edited

Loading

jannfis left a comment

jannfis Jul 20, 2020

darshanime Jul 21, 2020

jannfis left a comment

jannfis commented Jul 21, 2020

darshanime commented Jul 21, 2020

jannfis commented Jul 21, 2020

jgato commented Feb 6, 2023

feat: delete in reverse order of sync waves #3959

feat: delete in reverse order of sync waves #3959

Conversation

darshanime commented Jul 19, 2020

codecov-commenter commented Jul 20, 2020 • edited Loading

Codecov Report

jannfis left a comment

Choose a reason for hiding this comment

jannfis Jul 20, 2020

Choose a reason for hiding this comment

darshanime Jul 21, 2020

Choose a reason for hiding this comment

jannfis left a comment

Choose a reason for hiding this comment

jannfis commented Jul 21, 2020

darshanime commented Jul 21, 2020

jannfis commented Jul 21, 2020

jgato commented Feb 6, 2023

codecov-commenter commented Jul 20, 2020 •

edited

Loading