argoproj / argo-cd

Declarative Continuous Deployment for Kubernetes
https://argo-cd.readthedocs.io
Apache License 2.0
17.55k stars 5.35k forks source link

Delete namespaces created by "CreateNamespace=true" sync option when application is deleted #7875

Open nnahaliel opened 2 years ago

nnahaliel commented 2 years ago

Summary

As far as i've seen, when using the sync option "CreateNamespace=true" in an Application resource, the namespace is created if it does not exist, but when/if the application is deleted, the (now empty) namespace gets left behind, unmanaged.

If that's not true or already solved, please let me know what i'm doing wrong (and no need to continue reading, i guess...)

I am aware that deleting the namespace could be a problem (if for example you have multiple applications deploying to the same namespace), but I think there's a place for a configuration option that will delete the namespace on application deletion, event if it won't be the default.

p.s. - The "CreateNamespace=true" sync option exists in The application CRD, but is not documented in the Sync options page of the docs.

Motivation

When using ArgoCD to create dynamic environments for development and testing (using namespace for environment), Using "CreateNamespace=true" sync option is very useful, but it will also leave an ever increasing number of empty namespaces in the cluster.

When an infra component is replaced by another, or just no longer required, deleting the application managing that component will leave behind an empty unmanaged namespace (again, assuming you use "CreateNamespace=true").

Proposal

I see a few options -

In general i have no real preference for how this is implemented, I just want the namespace to be deleted when the application is deleted.

lukpep commented 2 years ago

upvote from my side - exactly my case also - crating those on-demand, temporary environments and dealing with hanging namespaces after env deletion.

bkk-bcd commented 2 years ago

upvote here on both counts

The situation is worse when you use statefulsets with on-demand environments; because argocd doesn't delete the namespace the PV's are left around $$

+1 on documentation too...the Application CRD is not documented well at all and arguably one of the most import things to document.

liammurray commented 2 years ago

It would also be nice when using the ApplicationSet Pull Request generator.

FrittenToni commented 2 years ago

It would also be nice when using the ApplicationSet Pull Request generator.

It's a workaround but for our PR deployments we've just added a "Namespace" object to our helm charts that will be applied for pull requests only. The pull request generator injects the correct PR number and Argo creates and deletes our PR preview namespaces accordingly, e.g:

filename: pr-namespace.yaml

{{- if .Values.pr.enabled}}
apiVersion: v1
kind: Namespace
metadata:
  name: "appName-pr-{{.Values.pr.number}}"
{{- end }}
james-callahan commented 2 years ago

This is a duplicate of https://github.com/argoproj/argo-cd/issues/4435

lvijnck commented 1 year ago

+1

Would be very relevant for current setup.

stb1337 commented 1 year ago

+1

Really need this :)

kkamaldinov commented 1 year ago

+1 need this feature..

DanielCastronovo commented 1 year ago

+1 need this feature

lewandowskim1988 commented 1 year ago

+1 would be great to have this feature

sambonbonne commented 1 year ago

Just for your information (I'm not a maintainer or anything but just interested in this feature), there is the pull request https://github.com/argoproj/argo-cd/pull/11821 but it's still a draft (base on the issue https://github.com/argoproj/argo-cd/issues/4435 instead of the current one, which is better because the DeleteNamespace option would allow people to chose if the namespace should be removed or not).

chamseddinechrifa commented 1 year ago

+1 need this feature

crenshaw-dev commented 1 year ago

I appreciate the +1s, but for now let's keep those as thumbs-ups on the original post, just to avoid making the thread too long.

Unless you want to add details of your use case. Those are always helpful when designing a feature. :-)

MalteMagnussen commented 1 year ago

Renovate Bot + ArgoCD Pull Request Generator = Lots of empty namespaces

ci-poc                                     Active   7d4h
ci-poc-ci-showcase-3                       Active   15d
ci-poc-mhm-jest-17                         Active   10d
ci-poc-renovate-configure-7                Active   12d
ci-poc-renovate-eslint-8-x-13              Active   10d
ci-poc-renovate-lock-file-maintenance-15   Active   10d
ci-poc-renovate-lock-file-maintenance-18   Active   8d
ci-poc-renovate-lock-file-maintenance-22   Active   43h
ci-poc-renovate-nextjs-monorepo-12         Active   11d
ci-poc-renovate-nextjs-monorepo-21         Active   4d19h
ci-poc-renovate-nextjs-monorepo-23         Active   35h
ci-poc-renovate-nextjs-monorepo-9          Active   12d
ci-poc-renovate-node-18-x-11               Active   11d
ci-poc-renovate-node-18-x-14               Active   10d
ci-poc-renovate-node-18-x-19               Active   7d21h
ci-poc-renovate-node-18-x-20               Active   6d9h
ci-poc-renovate-node-18-x-25               Active   17h
ci-poc-renovate-node-18-x-8                Active   12d
ci-poc-renovate-react-monorepo-10          Active   12d
ci-poc-renovate-react-monorepo-16          Active   10d

Please add automatic cleanup.

omBratteng commented 1 year ago

We solved it by creating the namespace in the manifests, so when the generator started deleting the application, it also deleted the namespace along with it.

crenshaw-dev commented 1 year ago

Another workaround: now that it's possible to attach labels to auto-created namespaces, it's easier to write a cleanup job targeting that label.

Still not perfect, but maybe an improvement.

talayalonwsc commented 1 year ago

+1 for this feature request

semihural-tomtom commented 1 year ago

+1

leoluz commented 1 year ago

As we discussed in the Argo Contributor's meeting today, one edge case that we would need to address with this feature is:

  1. The namespace is created by argo-cd while syncing Application1
  2. The Application1's resources are synced in that namespace
  3. Another application (Application2) is created syncing resources in the same namespace
  4. Application1 is deleted with delete namespace enabled

In this case it is not safe to delete the namespace as it contains more resources that weren't owned by the Application1. In this case Argo CD would possibly need to validate if the namespace is empty before proceeding with the deletion.

yuezk commented 1 year ago

As we discussed in the Argo Contributor's meeting today, one edge case that we would need to address with this feature is:

  1. The namespace is created by argo-cd while syncing Application1
  2. The Application1's resources are synced in that namespace
  3. Another application (Application2) is created syncing resources in the same namespace
  4. Application1 is deleted with delete namespace enabled

In this case it is not safe to delete the namespace as it contains more resources that weren't owned by the Application1. In this case Argo CD would possibly need to validate if the namespace is empty before proceeding with the deletion.

Makes sense. It's the users' job to ensure they won't create other resources in the same namespace if they want to delete the namespace automatically. For me, the scenario is that I will provision a temp environment using Argo CD for each PR, and won't create other resources in the same namespace. So, I'm really excited about this feature.

james-callahan commented 1 year ago

In this case it is not safe to delete the namespace as it contains more resources that weren't owned by the Application1. In this case Argo CD would possibly need to validate if the namespace is empty before proceeding with the deletion.

While this sounds like a good idea on the surface, in practice many operators create resources that argocd shows as orphans. e.g. cert-manager will create secrets in response to an ingress (you can turn an ownerReferences on, but it's off by default, see https://github.com/cert-manager/cert-manager/issues/4517#issuecomment-942576725 )

I think ArgoCD should just go ahead and delete the namespace if it created it; even if there are orphans.

omBratteng commented 1 year ago

What if, both? One option for deleting namespaces, that'll fail if there are orphans, and another that enables it to delete orphans too when deleting namespaces.

whitelancer commented 1 year ago

I would also really like to see an option for this. Argo applies some annotations to its own namespaces (argocd.argoproj.io/sync-options: ServerSideApply=true) so we can identify those fairly simply. When using a review environment (such as with a PR that we want to create a temporary deployment) it's important to clean up that namespace after the fact. Currently there doesn't seem to be any way to do this, even if I include the namespace manifest definition as part of the application! We can't have orphaned/old namespaces, so this functionality is greatly desired and requested! Thanks!

crenshaw-dev commented 1 year ago

@omBratteng something like this I guess?

DeleteNamespace=always
DeleteNamespace=ifNoOrphans
omBratteng commented 1 year ago

@crenshaw-dev yeah, something like that could work.

But I also think using to separate options together would be a little bit safer. So you must use both to always delete namespaces, so something does not get accidentally deleted if using the wrong option. Alternatively just a big 'ol disclaimer

crenshaw-dev commented 1 year ago

So

DeleteNamespace=true
DeleteNamespaceIgnoreOrphans=true

I think it's the same problem, different syntax. Specifying the wrong thing is dangerous either way.

omBratteng commented 1 year ago

That is true, just personally I feel that the latter version is "safer" because you need to combine both to be able to ignore orphans, while the other one just requires one with the wrong option. Would be great to hear what other people think too

phoenix-bjoern commented 11 months ago

As a workaround we've created a Helm template for sync-wave:-10:

{{ if .Values.namespace.enabled }}
apiVersion: v1
kind: Namespace
metadata:
  annotations:
    argocd.argoproj.io/sync-wave: "-10"
    {{ if .Values.namespace.annotations }}
      {{- toYaml $.Values.namespace.annotations | nindent 4 }}
    {{ end }}
  {{ if .Values.namespace.labels }}
  labels:
    {{- toYaml $.Values.namespace.labels | nindent 4 }}
  {{ end }}
  name: {{ .Release.Namespace }}
{{ end }}

ArgoCD creates the namespace at the very beginning of the Sync and removes it during Application removal if syncPolicy.automated.prune: true is set.

aclark-pitchup commented 9 months ago

Unfortunately we've hit an issue trying to set up a feature-branch deployment process and including the namespace as part of the chart has some unexpected side-effects.

When including the namespace in our chart, we see the following events when deleting the Application object (with prune and selfHeal turned on):

  1. All resources in the chart (including the namespace) are simultaneously marked for deletion (my suspicion is that the NS is deleted first, which then triggers all other resources to be deleted)
  2. The Application considers itself out of sync because resources have been removed, so it tries to recreate them, but then complains that it can't do so because those resources are "marked for deletion"- so the application waits in a sync-pending state, with all resources marked as "missing" (i.e. with the "missing" icon, not the "for deletion" icon), when it waits for the namespace to be purged
  3. Eventually all resources are deleted and the namespace as well
  4. At this point, the ArgoCD application resurrects everything in the chart, recreating the namespace, all resources, running hooks, etc.
  5. Finally, once everything has come back, it all gets deleted one more time, this time successfully

I can't really explain the above behaviour other than, as mentioned, that maybe the namespace gets deleted first, before the Application is ready to delete all other resources, meaning that it first thinks it has to "heal" the chart, before finally realising that it's all pending deletion anyway and so correctly prunes it.

If the namespace isn't included in the chart then none of this behaviour occurs and pruning works as expected (but the namespace is left in place of course), which supports the above theory.

I've tested the above with a few different options:

There are a few other options that I'm going to try, to mitigate the above. They're not ideal but they might unblock our use-case, so mentioning here:

Given the above it would be amazing if we could get that option for ArgoCD to clean the namespace up itself as part of Application deletion. Alternatively, if there's an option that I've missed to tell the Application to delete the namespace last then that might avoid the odd behaviour that I'm seeing.

I'm happy to debug further if the above looks like a genuine bug. Thanks!

flabatut commented 7 months ago

FWIW, I could create and delete applications' namespace using conjointly argocd namespace metadata feature and resource tracking.

Here is the related syncPolicy:

  syncPolicy:
    automated:
      prune: true
      selfHeal: true
    managedNamespaceMetadata:
      labels:
        argocd.argoproj.io/instance: my-argocd-application-name
      annotations:
        argocd.argoproj.io/tracking-id: >-
          my-argocd-application-name:/Namespace:my-application-namespace/my-application-namespace
    syncOptions:
    - CreateNamespace=true

Once the app is created, the namespace resource appears in the UI:

Screenshot

and there is NO manifest for namespace resource within my git repository. ArgoCD configured to use both label and annotation (not sure if one or both change the behavior) Deleting the application deletes the namespace as well.

Not sure if this behavior is expected though.

phoenix-bjoern commented 7 months ago

@flabatut Thanks for the fabulous hint! I can confirm adding the tracking-id in the managedNamespaceMetadata section will remove the namespace initially created by ArgoCD on App deletion; no need to add a namespace resource in the manifest anymore.

So, the implementation of the feature request should be as simple as adding the tracking-id automatically on NS creation. The resource finalizer will do the rest on application removal. If that change in behavior shouldn't be enabled by default another syncOption like "CreateNamespaceTracking: true" could be added.

cpboyd commented 7 months ago

Can argocd.argoproj.io/tracking-id handle if multiple apps get deployed to that namespace?

amorozkin commented 7 months ago

FWIW, I could create and delete applications' namespace using conjointly argocd namespace metadata feature and resource tracking.

Here is the related syncPolicy:

  syncPolicy:
    automated:
      prune: true
      selfHeal: true
    managedNamespaceMetadata:
      labels:
        argocd.argoproj.io/instance: my-argocd-application-name
      annotations:
        argocd.argoproj.io/tracking-id: >-
          my-argocd-application-name:/Namespace:my-application-namespace/my-application-namespace
    syncOptions:
    - CreateNamespace=true

Once the app is created, the namespace resource appears in the UI:

Screenshot

and there is NO manifest for namespace resource within my git repository. ArgoCD configured to use both label and annotation (not sure if one or both change the behavior) Deleting the application deletes the namespace as well.

Not sure if this behavior is expected though.

In my case (argocd 2.6) adding argocd.argoproj.io/tracking-id annotation to managedNamespaceMetadata leads to OutOfSync - Sync loop (with actual namespace's removal/creation in destination cluster). Switching to sync policy "prune: false" stops that loop with namespace resource remaining in status "OutOfSync(requires pruning)" I wonder how in your case with NO manifest for namespace resource within your git repository namespace resource in the UI is still being displayed in Synced status?

phoenix-bjoern commented 7 months ago

@amorozkin change the ArgoCD App first, wait for the repo sync, and then remove the namespace manifest. I've changed multiple Apps and it worked perfectly in that order. Compiling the tracking-id manually is a bit tricky. Make sure you don't have a typo.

I've tested it with ArgoCD 2.10.1. Maybe there are some open issues in v2.6...

CyTechNomad commented 6 months ago

I too would like to know if there has been any updates with this. I like many other people would use and almost require this for PR sandbox generation. We know that these name spaces are specific to a PR, and we need to make sure all resources are cleansed after the PR is in a Terminal state.

evertonspader-tomtom commented 5 months ago

I am also trying to use this suggestion of using managedNamespaceMetadata but it doesn't seem to remove the namespace after the application manifest is removed from git source. I am on Argocd 2.9.6. Has anyone succeeded with this solution?

phoenix-bjoern commented 5 months ago

@evertonspader-tomtom yes, we are using it with >2.10.x and it removes all resources including the namespace. Are there maybe other resources in the namespace? Have you configured

  syncPolicy:
    automated:
      prune: true

in the ArgoCD application?

O5ten commented 3 months ago

FWIW, I could create and delete applications' namespace using conjointly argocd namespace metadata feature and resource tracking.

Here is the related syncPolicy:


  syncPolicy:
    automated:
      prune: true
      selfHeal: true
    managedNamespaceMetadata:
      labels:
        argocd.argoproj.io/instance: my-argocd-application-name
      annotations:
        argocd.argoproj.io/tracking-id: >-
          my-argocd-application-name:/Namespace:my-application-namespace/my-application-namespace
    syncOptions:
    - CreateNamespace=true

This solved it perfecly for us!

jasonwbarnett commented 3 months ago

As annoying as this is (from a notification perspective), I'm double doing this comment because there is so much noise in this issue and it was challenging and costly to weigh through it all.

As @O5ten mentioned, the solution that @flabatut mentioned works well. Just use managedNamespaceMetadata and define the the appropriate labels and annotations.

In my case though, I do not have a defined override for application.instanceLabelKey, so I used app.kubernetes.io/instance (the default) instead. Also, it wasn't super clear what the pattern for argocd.argoproj.io/tracking-id was, from what I gather it is: <argocd-app-name>:<api-group>/<kind>:<namespace>/<resource-name> (I may be wrong, but I think the pattern is right)

With that said, this is what worked for me:

  syncPolicy:
    automated:
      prune: true
      selfHeal: true
    managedNamespaceMetadata:
      labels:
        app.kubernetes.io/instance: my-argocd-application-name
      annotations:
        argocd.argoproj.io/tracking-id: >-
          <argocd-application-name>:/Namespace:<namespace>/<namespace>
    syncOptions:
      - CreateNamespace=true