feat: PostDelete hook support #16595

alexmt · 2023-12-12T19:11:54Z

PR introduces Post delete hooks support. Hooks are executed during application deletion after all resources as removed. Hooks respect deletion policies.

PostDelete hooks
Docs
Hooks delete policy
Tests

Screen.Recording.2023-12-15.at.3.04.37.PM.mov

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

codecov · 2023-12-16T21:48:20Z

Codecov Report

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

Comparison is base (a761a49) 49.50% compared to head (230db9b) 49.51%.
Report is 12 commits behind head on master.

Files	Patch %	Lines
controller/appcontroller.go	43.82%	43 Missing and 7 partials ⚠️
controller/hook.go	58.87%	29 Missing and 15 partials ⚠️
pkg/apis/application/v1alpha1/types.go	0.00%	6 Missing ⚠️
controller/state.go	70.00%	2 Missing and 1 partial ⚠️
controller/sync.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #16595    +/-   ##
========================================
  Coverage   49.50%   49.51%            
========================================
  Files         270      271     +1     
  Lines       47488    47659   +171     
========================================
+ Hits        23508    23597    +89     
- Misses      21670    21731    +61     
- Partials     2310     2331    +21

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

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

gdsoumya · 2023-12-17T15:22:09Z

controller/appcontroller.go

 		}
+		logCtx.Infof("Successfully deleted %d resources", len(objs))
+		logCtx.Infof("Resource entries removed from undefined cluster")


Is this log needed here? If it is needed not sure if it should be undefined cluster

Ops, thank you for catching it. I just forgot to delete this line - fixed!

gdsoumya · 2023-12-17T15:51:21Z

controller/hook.go

+			return false, err
+		}
+		for _, policy := range hook.DeletePolicies(obj) {
+			if policy == common.HookDeletePolicyHookFailed && hookHealth.Status == health.HealthStatusDegraded || policy == common.HookDeletePolicyHookSucceeded && hookHealth.Status == health.HealthStatusHealthy {


I think this behaviour is different from the delete policy of the other hook types, as far as I remember in gitops-engine the delete policy would be compared with the whole app status instead of just the hook status. We might want to keep the behaviour same or else it can be confusing. I understand at deletion we probably won't have the app status to compare maybe so might be better to have special deletion policy just for the delete hooks?

Another question here, what do we do if the deletion policy condition isn't met. Does the app itself remains or is it just the hook that sticks around?

You are right. App deletion does not have status, however, I think the use case is similar: the user just needs control to preserve some hook for debugging. To match with gitops engine behavior it makes sense to use aggregated hooks status. Implemented

gdsoumya · 2023-12-17T16:20:59Z

controller/hook.go

+			return false, err
+		}
+		for _, policy := range hook.DeletePolicies(obj) {
+			if policy == common.HookDeletePolicyHookFailed && hookHealth.Status == health.HealthStatusDegraded || policy == common.HookDeletePolicyHookSucceeded && hookHealth.Status == health.HealthStatusHealthy {


Another question here, what do we do if the deletion policy condition isn't met. Does the app itself remains or is it just the hook that sticks around?

alexmt · 2023-12-17T23:08:44Z

If the deletion policy does not match, then the hook stays. It matches Helm behavior . Updated documentation as well.

Thank you for review @gdsoumya . PTAL

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

* feat: PostDelete hooks support Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> --------- Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

* feat: PostDelete hooks support Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> --------- Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> Signed-off-by: penglongli <pelenli@tencent.com>

* feat: PostDelete hooks support Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> --------- Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> Signed-off-by: Kevin Lyda <kevin@lyda.ie>

* feat: PostDelete hooks support Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> --------- Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

* The finalizers: - post-delete-finalizer.argocd.argoproj.io - post-delete-finalizer.argoproj.io/cleanup are added by ArgoCD when the rendered manifests include a post-delete hook. In this case, the application will not be deleted since we could not satisfy the finalizer condition. Therefore, removed the finalizers. See: [1] for PostDeleteFinalizerName reference [2] for implementation of these finalizers adding --- [1] https://github.com/argoproj/argo-cd/blame/8a447d9ae01f0c4358bc0d7553c120c43a8b99e5/pkg/apis/application/v1alpha1/application_defaults.go#L12-L13 [2] argoproj/argo-cd#16595

@wingyplus

* Fixed applications removal stuck due to obstructive finalizers * The finalizers: - post-delete-finalizer.argocd.argoproj.io - post-delete-finalizer.argoproj.io/cleanup are added by ArgoCD when the rendered manifests include a post-delete hook. In this case, the application will not be deleted since we could not satisfy the finalizer condition. Therefore, removed the finalizers. See: [1] for PostDeleteFinalizerName reference [2] for implementation of these finalizers adding --- [1] https://github.com/argoproj/argo-cd/blame/8a447d9ae01f0c4358bc0d7553c120c43a8b99e5/pkg/apis/application/v1alpha1/application_defaults.go#L12-L13 [2] argoproj/argo-cd#16595 * refactor: adjust application finalizers filtering * Adjust application finalizers filtering, follow a suggestion and thanks to @wingyplus. Using .filter() for filtering as we still need the values of finalizers to show in the debug message. * Apply suggestions from code review * Clean up an extended comment with a clean and concise one. * Handle the errors from command output and YAML parsing separately for an easier to read code. * Inspect an error properly to align with the application output log. Co-authored-by: Dag Andersen <dagbjerreandersen@gmail.com> * Apply suggestions from code review * A proper json string argument passing, switch from `--patch-file` to `--patch` instead. Co-authored-by: Dag Andersen <dagbjerreandersen@gmail.com> * chore: drop unused module and apply code formatting * Drop unused module `std::env` * Apply code formatting --------- Co-authored-by: Dag Andersen <dagbjerreandersen@gmail.com>

alexmt force-pushed the post-delete-hooks branch 2 times, most recently from 688ac4c to 70c0a24 Compare December 16, 2023 21:40

feat: Pre/PostDelete hooks support

7a9bb3b

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

alexmt force-pushed the post-delete-hooks branch from 70c0a24 to 0e74e77 Compare December 16, 2023 22:10

tests

f3874a6

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

alexmt force-pushed the post-delete-hooks branch from 0e74e77 to f3874a6 Compare December 16, 2023 22:24

alexmt requested review from crenshaw-dev and gdsoumya December 16, 2023 22:25

alexmt marked this pull request as ready for review December 16, 2023 22:25

alexmt requested review from a team as code owners December 16, 2023 22:25

christianh814 approved these changes Dec 16, 2023

View reviewed changes

gdsoumya requested changes Dec 17, 2023

View reviewed changes

Address code review comments

230db9b

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

alexmt requested a review from gdsoumya December 17, 2023 23:17

gdsoumya approved these changes Dec 18, 2023

View reviewed changes

crenshaw-dev changed the title ~~feat: Pre/PostDelete hooks support~~ feat: PostDelete hook support Dec 18, 2023

alexmt merged commit dcc17f7 into argoproj:master Dec 18, 2023

alexmt deleted the post-delete-hooks branch December 18, 2023 16:40

aslafy-z mentioned this pull request Dec 18, 2023

[Bug] webhooks are not removed when using Argo CD with Kyverno helm chart kyverno/kyverno#8390

Closed

2 tasks

blakepettersson mentioned this pull request Jan 21, 2024

pre-deletion resource hooks #13975

Open

thesuperzapper mentioned this pull request Jan 29, 2024

[Bug] ensure webhooks are removed on helm uninstall (PROPOSAL) kyverno/kyverno#9551

Closed

2 tasks

ZF-fredericvanlinthoudt mentioned this pull request Feb 7, 2024

Sync loop for Helm Applications that are using post-delete hooks #17117

Open

3 tasks

This was referenced Feb 13, 2024

PostDelete Hook not work with Application generated from ApplicationSet #17181

Closed

PostDelete Hook with other resources than pod/job (like SA) #17191

Open

baurmatt mentioned this pull request Feb 13, 2024

PostDelete hook aren't applied on existing applications #17193

Closed

3 tasks

llavaud mentioned this pull request Mar 7, 2024

Post-Delete Hook support mutate Application object #17433

Open

joebowbeer mentioned this pull request May 7, 2024

PostDelete hook not usable when deleting files used by generators #18070

Open

3 tasks

ChristianCiach mentioned this pull request May 17, 2024

Application stuck in deletion because of a postDelete hook #18270

Closed

3 tasks

neutronth mentioned this pull request Feb 12, 2025

Fixed applications removal stuck due to obstructive finalizers dag-andersen/argocd-diff-preview#107

Merged

This was referenced Mar 7, 2025

feat: PreDelete hooks support (Issue #13975) #22253

Closed

feat: PreDelete hooks support (Issue #13975) #22288

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: PostDelete hook support #16595

feat: PostDelete hook support #16595

alexmt commented Dec 12, 2023 •

edited

Loading

codecov bot commented Dec 16, 2023 •

edited

Loading

gdsoumya Dec 17, 2023

alexmt Dec 17, 2023

gdsoumya Dec 17, 2023

gdsoumya Dec 17, 2023

alexmt Dec 17, 2023

gdsoumya Dec 17, 2023

alexmt commented Dec 17, 2023 •

edited

Loading

feat: PostDelete hook support #16595

feat: PostDelete hook support #16595

Conversation

alexmt commented Dec 12, 2023 • edited Loading

codecov bot commented Dec 16, 2023 • edited Loading

Codecov Report

gdsoumya Dec 17, 2023

Choose a reason for hiding this comment

alexmt Dec 17, 2023

Choose a reason for hiding this comment

gdsoumya Dec 17, 2023

Choose a reason for hiding this comment

gdsoumya Dec 17, 2023

Choose a reason for hiding this comment

alexmt Dec 17, 2023

Choose a reason for hiding this comment

gdsoumya Dec 17, 2023

Choose a reason for hiding this comment

alexmt commented Dec 17, 2023 • edited Loading

alexmt commented Dec 12, 2023 •

edited

Loading

codecov bot commented Dec 16, 2023 •

edited

Loading

alexmt commented Dec 17, 2023 •

edited

Loading