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

Provide ability to ignore extra application objects (e.g. kustomize nameSuffixHash) #1629

Closed
jessesuen opened this issue May 21, 2019 · 15 comments · Fixed by #1680
Closed

Provide ability to ignore extra application objects (e.g. kustomize nameSuffixHash) #1629

jessesuen opened this issue May 21, 2019 · 15 comments · Fixed by #1680
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@jessesuen
Copy link
Member

jessesuen commented May 21, 2019

Kustomize has a feature to append a hash of the contents of configmap/secret, as part of the name. This feature allows kubectl apply to automatically trigger a rolling update of a deployment whenever the contents of the configmap/secret changes, while also remaining a no-op when the contents do not change.

This poses some challenges for Argo CD:

  1. The nameSuffixHash leaves a bunch of old, unused ConfigMaps/Secrets lying around in the namespace. Argo CD subsequently considers the application OutOfSync (which it in fact is), because there are extra resources in the system which no longer are defined in git.

  2. Although Argo CD can automatically delete the old ConfigMaps using the --prune flag to argocd app sync, doing so might be undesirable in some circumstances. For example, during a rolling update, Argo CD would delete the old ConfigMaps referenced by the previous deployment's pods, even when the deployment has not yet fully finished the rolling update.

Describe the solution you'd like

Some proposals:

  1. We need a way to perform argocd app sync but not error out in the CLI when it detects that there are resources that require pruning. To allow users to describe their intent, I propose a new annotation which can be set on resources, which if found on an "extra" resource object, we do not error in the CLI. Something like:
metadata:
  annotations:
    argocd.argoproj.io/ignore-sync-condition: Extra
  1. The annotation to ignore extra resource should also affect application sync status. In other words, if we find extra configmaps lying around the namespace, and those extra ConfigMaps have the argocd.argoproj.io/ignore-sync-condition: Extra annotation, then we do not contribute that configmap to the overall OutOfSync condition of the application.

  2. Today, pruning extra objects in Argo CD is all or nothing. As mentioned above, it is problematic to prune all of the extra objects early because of the issue of deleting the previous ConfigMap from underneath old deployments which are still referencing it. Even if we introduce the annotation to ignore extra configmaps, there will be an accumulation of configmaps indefinitely until argocd app sync --prune is invoked.

To mediate this, Argo CD could implement some sort of ConfigMap/Secret garbage collection. Note that there is proposals as part of kubernetes core, to GC configmaps/secrets: kubernetes/kubernetes#22368. So I'm hesitant to implement anything specific to Argo CD.

Furthermore, detecting if a ConfigMap or Secret is still "referenced" might be very difficult to detect. One heuristic might be to look at all pods in the namespace, and see if any pod spec is still referencing the configmap/secret it as a volume mount, env var, etc.... However this may be tricky to get right, subject to timing issues (deployment not fully rolled out), and might not work in all cases (e.g. if the configmap was retrieved via K8s API query)

@jessesuen jessesuen added the enhancement New feature or request label May 21, 2019
@AgarFu
Copy link

AgarFu commented May 21, 2019

The CG approach can not be purely based on the running pods in the namespace because it won't allow you to rollback your deployment to a previous version. A combination of time or number of non referenced ConfigMaps plus the verification that they are not in use seems about right to me.

@jessesuen
Copy link
Member Author

jessesuen commented May 21, 2019

it won't allow you to rollback your deployment to a previous version.

It will, because an argo-cd rollback is going to the manifests at a previous commit hash. At that hash, the old configmap would be re-deployed.

@AgarFu
Copy link

AgarFu commented May 21, 2019

I'm considering an emergency situation where because for whatever reason you do have access to the cluster but not to Argo

@jessesuen
Copy link
Member Author

jessesuen commented May 21, 2019

Even in that situation, you would do something like:

git checkout <OLDCOMMITSHA>
kustomize build . | kubectl apply -f -

There's nothing really magical about the way argocd deploys, and is why kubectl apply was chosen to perform deploys. Out-of-band deployments using kubectl is a scenario we intended to support, specifically for the disaster scenario that you are concerned with.

But what I'm trying to argue, is that it's unnecessary to implement more complicated logic involving N-revisions of unreferenced configmaps/secrets combined with some notion of time. This logic I don't believe can be easily be gotten right.

@jessesuen
Copy link
Member Author

jessesuen commented May 22, 2019

I think this is preferable syntax:

metadata:
  annotations:
    argocd.argoproj.io/compare-options: IgnoreExtra

@jessesuen
Copy link
Member Author

I split out the GC request as a separate feature request: #1636

Will leave this issue as the ability to have Argo CD ignore extra config which has the annotation

@jessesuen jessesuen changed the title Better support for kustomize's configmap/secrets nameSuffixHash Provide ability to ignore extra application objects (e.g. kustomize nameSuffixHash) May 22, 2019
@twz123
Copy link
Member

twz123 commented May 27, 2019

Actually, this sounds similar to #1373. In fact, this would already solve my problem described there (StatefulSets and their PVCs with kustomize).

@jessesuen
Copy link
Member Author

jessesuen commented May 29, 2019

In fact, this would already solve my problem described there (StatefulSets and their PVCs with kustomize).

I actually think #1373 is still useful as a separate feature. I feel it's desirable to have the ability to ignore OutOfSync conditions of a resource as a separate decision than skipping pruning of a resource.

To solve your issue where there are extra resources (caused by kustomize's commonLabels), I would propose something like:

metadata:
  annotations:
    argocd.argoproj.io/compare-options: IgnoreExtra
    argocd.argoproj.io/sync-options: SkipPrune
  • compare-options: IgnoreExtra would make sure the extra live resource with this annotation does not contribute to an OutOfSync condition of the application as a whole. Of note, it does not preclude the ability for Argo CD to perform pruning on the resource, should the user decide to perform argocd app sync --prune.

  • sync-options: SkipPrune would ensure that when encountering a live resource with the annotation, Argo CD would never delete it as part of pruning. It does not preclude the ability for the extra/OutOfSync resource to affect the sync status of the application as a whole. In fact, one use case might be to use the SkipPrune annotation as a safety measure in case someone accidentally renames a resource and performs a argocd app sync --prune, unintentionally deleting the old resource.

@twz123
Copy link
Member

twz123 commented May 29, 2019

Agree, both issues are in fact different. Very much like your proposal! 👍

@jessesuen jessesuen added this to the v1.1 milestone Jun 1, 2019
@alexec
Copy link
Contributor

alexec commented Jun 3, 2019

@jessesuen is this the Kustomize feature we're talking about please: https://github.com/kubernetes-sigs/kustomize/blob/master/examples/configGeneration.md

@jessesuen
Copy link
Member Author

Yes, but the problem applies in other use cases (unwanted label propagation). Config map generation with name suffix hash just highlights the problem in a concrete way.

@alexec alexec self-assigned this Jun 3, 2019
@alexec
Copy link
Contributor

alexec commented Jun 3, 2019

kind: Kustomization
generatorOptions:
  annotations:
    argcd.argoproj.io/compare-options: Ignore
configMapGenerator:
  - name: my-map
    literals:
      - foo=bar

@alexec
Copy link
Contributor

alexec commented Jun 4, 2019

Part of this work is in PR:

#1680

@jessesuen
Copy link
Member Author

I have agreement with @alexec on syntax. Resources can be annotated like the following:

metadata:
  annotations:
    compare-options: IgnoreExtraneous
    sync-options: Prune=false

In the future, these options might be expanded:

metadata:
  annotations:
    compare-options: IgnoreExtraneous,IgnoreDifference=/spec/some/field
    sync-options: Prune=false,Validate=false,Force=true,ConfigGC=enabled

@thesuperzapper
Copy link

For those coming to this issue, please note, the ability to IgnoreDifference with an annotation (suggested by @jessesuen) was never added, there is still an open issue:

#12686

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants