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

Deleting ApplicationSet causes Application deletion even if applicationset-controller policy is 'create-only' #12172

Closed
3 tasks done
mikutas opened this issue Jan 27, 2023 · 14 comments · Fixed by #15903
Closed
3 tasks done
Labels
bug Something isn't working component:application-sets Bulk application management related

Comments

@mikutas
Copy link
Contributor

mikutas commented Jan 27, 2023

Checklist:

  • I've searched in the docs and FAQ for my answer: https://bit.ly/argocd-faq.
  • I've included steps to reproduce the bug.
  • I've pasted the output of argocd version.

Describe the bug

To Reproduce

  • git clone https://github.com/mikutas/app-of-apps-with-helm-experiment
  • make argocd
  • kubectl edit -n argocd argocd-applicationset-controller
    • Add --policy create-only to spec.template.spec.containers[0].command
  • kustomize build appsets/overlays/development/ | k apply -f -
    • ApplicationSet test created
    • Application foo-apps created
  • kubectl delete -n argocd applicationset test

Expected behavior

Since I set 'create-only' policy to applicationset-controller, Application foo-apps are not deleted.

Screenshots

Version

argocd version --server localhost:8080 --insecure
argocd: v2.5.6+9db2c94
  BuildDate: 2023-01-10T19:51:43Z
  GitCommit: 9db2c9471f6ff599c3f630b446e940d3a065620b
  GitTreeState: clean
  GoVersion: go1.18.9
  Compiler: gc
  Platform: darwin/arm64
argocd-server: v2.5.8+bbe870f

Logs

application-controller (not applicationset-controller) outputs following logs.

time="2023-01-27T10:31:42Z" level=info msg="Deleting resources" application=argocd/foo-apps
time="2023-01-27T10:31:42Z" level=info msg="Deleting application's resources with Foreground propagation policy" application=argocd/foo-apps
time="2023-01-27T10:31:42Z" level=info msg="Successfully deleted 0 resources" application=argocd/foo-apps
@mikutas mikutas added the bug Something isn't working label Jan 27, 2023
@jaideepr97
Copy link
Contributor

Hi @mikutas
Thanks for raising this
This probably happens because each application created by the applicationset has their owner reference set to that applicationset, so they are garbage collected when the owner is deleted

However, I think I do agree that if the policy is set to create-only then the expectation would be that applications should not be affected by anything that is done by or to the appset controller
One way to do this could be to replace the owner reference with a tracking label for example (for reversibility of actions)

I can bring this up with the maintainers and see what they think

@ishitasequeira
Copy link
Member

I think this issue is currently addressed in the open PR #11462.

@crenshaw-dev
Copy link
Member

#11462 just makes the setting configurable on a per-ApplicationSet basis. It won't fix any bugs with the existing global configuration.

@jaideepr97
Copy link
Contributor

Hi @mikutas
I brought this issue up with the maintainers in last week's contributor call
There was a general agreement that it is a bug and that the correct thing to do would be to remove owner references from the child applications in that case

I believe the proposed idea was that we could set a finalizer on the applicationset resource, after which the applicationset controller can check if the --create-only flag is set, if so it can remove the owner references from child applications before the appset itself is deleted
if #11462 is merged then this logic could be adjusted to scoped to a per-applicationset basis

Does that make sense @jessesuen @crenshaw-dev?

@crenshaw-dev crenshaw-dev added the component:application-sets Bulk application management related label Feb 15, 2023
@crenshaw-dev
Copy link
Member

@jaideepr97 I think that does make sense!

@jaideepr97
Copy link
Contributor

@crenshaw-dev Thanks, I can work on a PR for this

@jdomag
Copy link

jdomag commented Mar 17, 2023

Hi @jaideepr97, is there any update about this one?

@zswanson
Copy link

zswanson commented May 4, 2023

Having just run into this myself I think after observing the behavior and re-reading the docs that this was probably the intended behavior, but the documentation doesn't make it clear that the setting only prevents the AppSetController itself from deleting/modifying Apps (ie, you change the git source somehow).

create-update should be included in any fix though.

@MattRipia
Copy link

👍

@crenshaw-dev
Copy link
Member

Looks like @jaideepr97 has not been able to work on this. If anyone else would like to pick up the work, I'd happily review.

@zenitraM
Copy link

For anyone else also looking at a workaround until this is fixed, the trick is to delete the ApplicationSet with --cascade=orphan which will orphan all the applications (which would be the default behavior on ordinary ApplicationSet deletions with #15903).

If this is on an ArgoCD app that creates/manages ApplicationSets inside, this can be done by setting

spec:
  syncPolicy:
    syncOptions:
    - PrunePropagationPolicy=orphan

on the parent app that creates the ApplicationSets (see https://argo-cd.readthedocs.io/en/stable/user-guide/sync-options/#resources-prune-deletion-propagation-policy).

ishitasequeira pushed a commit that referenced this issue Nov 10, 2023
…#15903)

* fix(applicationset): prevent app deletion according to appset policy

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* test: add unit test

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* fix: unit test

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* fix: remove TODO

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

---------

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>
jmilic1 pushed a commit to jmilic1/argo-cd that referenced this issue Nov 13, 2023
…j#12172) (argoproj#15903)

* fix(applicationset): prevent app deletion according to appset policy

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* test: add unit test

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* fix: unit test

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* fix: remove TODO

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

---------

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>
Signed-off-by: jmilic1 <70441727+jmilic1@users.noreply.github.com>
crenshaw-dev pushed a commit that referenced this issue Dec 8, 2023
… allow app's deletion (#12172) (#16506)

* fix(appset): remove unnecessary condition

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* docs: update explanation about policy

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

---------

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>
vladfr pushed a commit to vladfr/argo-cd that referenced this issue Dec 13, 2023
…j#12172) (argoproj#15903)

* fix(applicationset): prevent app deletion according to appset policy

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* test: add unit test

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* fix: unit test

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* fix: remove TODO

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

---------

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>
vladfr pushed a commit to vladfr/argo-cd that referenced this issue Dec 13, 2023
… allow app's deletion (argoproj#12172) (argoproj#16506)

* fix(appset): remove unnecessary condition

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* docs: update explanation about policy

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

---------

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>
tesla59 pushed a commit to tesla59/argo-cd that referenced this issue Dec 16, 2023
…j#12172) (argoproj#15903)

* fix(applicationset): prevent app deletion according to appset policy

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* test: add unit test

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* fix: unit test

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* fix: remove TODO

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

---------

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>
tesla59 pushed a commit to tesla59/argo-cd that referenced this issue Dec 16, 2023
… allow app's deletion (argoproj#12172) (argoproj#16506)

* fix(appset): remove unnecessary condition

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* docs: update explanation about policy

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

---------

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>
JulienFuix pushed a commit to JulienFuix/argo-cd that referenced this issue Feb 6, 2024
… allow app's deletion (argoproj#12172) (argoproj#16506)

* fix(appset): remove unnecessary condition

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* docs: update explanation about policy

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

---------

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>
@DimitarKapashikov
Copy link

Hi All, I tested the mentioned fix, but I am still able to delete an ApplicationSet and as a consequence the child Applications are deleted too. Unfortunately in Application Set controller there is not logging when the deletion timestamp is set. How can investigate why this happens?

@svghadi
Copy link
Contributor

svghadi commented Feb 13, 2024

I am trying to come up with a fix for a similar issue where spec.syncPolicy.PreserveResourcesOnDeletion doesn't preserve apps when appset is deleted. I suspect it is happening because of the owner references set on the application resources created by appset. Whenever appset is deleted, the k8s removes the dependent resources directly without waiting for the applicationset-controller's Reconcile function to handle the removal of ownerrefs which was added to fix this.

@svghadi
Copy link
Contributor

svghadi commented Feb 16, 2024

If i add a finalizer on applicationset, the applicationset-controller gets chance to remove the ownerrefs when applicationset is deleted. Applications are preserved when later finalizer is removed to complete applicationset cleanup.
I will give it a try and see if finalizers can be used to come up with a fix for this.

lyda pushed a commit to lyda/argo-cd that referenced this issue Mar 28, 2024
…j#12172) (argoproj#15903)

* fix(applicationset): prevent app deletion according to appset policy

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* test: add unit test

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* fix: unit test

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* fix: remove TODO

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

---------

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>
Signed-off-by: Kevin Lyda <kevin@lyda.ie>
lyda pushed a commit to lyda/argo-cd that referenced this issue Mar 28, 2024
… allow app's deletion (argoproj#12172) (argoproj#16506)

* fix(appset): remove unnecessary condition

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* docs: update explanation about policy

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

---------

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>
Signed-off-by: Kevin Lyda <kevin@lyda.ie>
Hariharasuthan99 pushed a commit to AmadeusITGroup/argo-cd that referenced this issue Jun 16, 2024
…j#12172) (argoproj#15903)

* fix(applicationset): prevent app deletion according to appset policy

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* test: add unit test

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* fix: unit test

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* fix: remove TODO

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

---------

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>
Hariharasuthan99 pushed a commit to AmadeusITGroup/argo-cd that referenced this issue Jun 16, 2024
… allow app's deletion (argoproj#12172) (argoproj#16506)

* fix(appset): remove unnecessary condition

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

* docs: update explanation about policy

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>

---------

Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working component:application-sets Bulk application management related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants