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

Application CRD status.resources do not reflect argocd.argoproj.io/sync-options: Prune=false #17188

Open
3 tasks done
thesuperzapper opened this issue Feb 12, 2024 · 9 comments
Labels
bug Something isn't working component:application-controller component:sync more-information-needed Further information is requested

Comments

@thesuperzapper
Copy link

thesuperzapper commented Feb 12, 2024

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

The argocd.argoproj.io/sync-options: Prune=false annotations are not respected by the Application controller.

The application controller lists these resources as requiresPruning: true in the CRD status.resources[] field.
This is a problem in two significant places:

  1. The ArgoCD UI:
    • The UI will always show these resources in the diff, even if they would not be removed during an actual sync.
    • NOTE: this is also true if argocd.argoproj.io/compare-options: IgnoreExtraneous is set on the resource.
  2. The ArgoCD CLI:
    • When a user runs a argocd app get -o json command.

To Reproduce

  1. Create an ArgoCD application called my-app (it doesn't matter what is in it):
apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: guestbook
  namespace: argocd
spec:
  project: default
  source:
    repoURL: https://github.com/argoproj/argocd-example-apps.git
    targetRevision: HEAD
    path: guestbook
  destination:
    server: https://kubernetes.default.svc
    namespace: default
  1. Create a ConfigMap that shares the app.kubernetes.io/instance label with the ArgoCD app and argocd.argoproj.io/sync-options: Prune=false annotation:
apiVersion: v1
kind: ConfigMap
metadata:
  name: my-configmap
  namespace: default
  annotations:
    argocd.argoproj.io/sync-options: Prune=false
  labels:
    app.kubernetes.io/instance: guestbook
data:
  SOME_KEY: "SOME_VALUE"
  1. Observe that the argocd app get command shows the ConfigMap as requiresPruning: true:
ARGOCD_NAMESPACE="argocd"
ARGOCD_USERNAME="admin"
ARGOCD_PASSWORD=$(kubectl -n "argocd" get secret "argocd-initial-admin-secret" -o jsonpath="{.data.password}" | base64 -d)

# log in to argocd
export ARGOCD_OPTS="--port-forward --port-forward-namespace '${ARGOCD_NAMESPACE}'"
argocd login --username "${ARGOCD_USERNAME}" --password "${ARGOCD_PASSWORD}"

# get the application as JSON
_app_json=$(argocd app get "guestbook" -o json --refresh)

# extract the `status.resources` array from the JSON
_app_resources=$(echo "$_app_json" | jq -c '.status.resources[]?')

# print the resources
echo "$_app_resources"

# OUTPUT:
# {"version":"v1","kind":"ConfigMap","namespace":"default","name":"my-configmap","status":"OutOfSync","requiresPruning":true}
# {"version":"v1","kind":"Service","namespace":"default","name":"guestbook-ui","status":"Synced","health":{"status":"Healthy"}}
# {"group":"apps","version":"v1","kind":"Deployment","namespace":"default","name":"guestbook-ui","status":"Synced","health":{"status":"Healthy"}}
  1. Observe that the UI shows the resource as requiring pruning:

Screenshot 2024-02-12 at 12 34 18

Expected behavior

Both the CRD itself (and therefore ArgoCD CLI and UI) correctly reflect the presence of argocd.argoproj.io/sync-options: Prune=false.

For example, in the above situation, I would expect to see the status.resources like this:

apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  name: guestbook
spec:
  ...
status:
  ...
  resources:
    - version: v1
      kind: ConfigMap
      name: my-configmap
      namespace: default

      ## \/ this part should show as `false`, not `true`  \/ 
      requiresPruning: false

      ## NOTE: this will probably still need to be OutOfSync, until we can create a new "ignored" status:
      ##       https://github.com/argoproj/argo-cd/issues/14338
      status: OutOfSync

Version

argocd: v2.10.0+2175939.dirty
  BuildDate: 2024-02-06T15:31:09Z
  GitCommit: 2175939ed6156ddd743e60f427f7f48118c971bf
  GitTreeState: dirty
  GoVersion: go1.21.6
  Compiler: gc
  Platform: darwin/arm64

Other Notes

There is a similar issue with argocd.argoproj.io/compare-options: IgnoreExtraneous (see #14338), but that will be more complex to fix, because it probably requires a new "resource status" of "ignored" as it is not correct to say that an ignored resource is "Synced".

@thesuperzapper thesuperzapper added the bug Something isn't working label Feb 12, 2024
@thesuperzapper
Copy link
Author

@crenshaw-dev @gdsoumya @ishitasequeira @fengshunli I think this is an important bug to fix, and I am interested in your feedback.

@thesuperzapper
Copy link
Author

thesuperzapper commented Feb 12, 2024

Is it still the same if the configmap is present in the git source itself instead of being applied manually?

@gdsoumya I don't think that makes sense, because if the ConfigMap was in the git source, it would not require pruning.

And if it were previously in the git source (but was removed), then it would be functionally equivalent to creating the ConfigMap manually with the same app.kubernetes.io/instance label like we do in the example.

EDIT: looks like the comment I was replying to from @gdsoumya got deleted, but I will leave this comment up.

@thesuperzapper
Copy link
Author

@pasha-codefresh as this is probably related to argoproj/gitops-engine, I am interested to know your feedback.

Also, I found issue #8134, which shows that this issue has been impacting users since at least Jan 2022, because argocd app sync will incorrectly fail 100% of the time when there are resources with argocd.argoproj.io/sync-options: Prune=false.

@thesuperzapper thesuperzapper changed the title argocd.argoproj.io/sync-options: Prune=false not respected in Application CRD (and thus CLI and UI) Application CRD status.resources do not reflect argocd.argoproj.io/sync-options: Prune=false Apr 3, 2024
@thesuperzapper
Copy link
Author

@crenshaw-dev @ishitasequeira @pasha-codefresh @kostis-codefresh

This is a very important user issue with a relatively simple fix. We need to make it so that the status.resources correctly shows requiresPruning: false when a resource has argocd.argoproj.io/sync-options: Prune=false.

For most users, the biggest impact of the current issue is the ArgoCD UI will show resources with Prune=false as if they are going to be removed in the application diff, and it will make the entire app show as "out of sync".


There is a related issue #14338 for the argocd.argoproj.io/compare-options: IgnoreExtraneous annotation, but that is slightly more complex, as it will require a new "Ignored" resource status in addition to the "Synced" and "OutOfSync".

@crenshaw-dev
Copy link
Member

At a quick glance, what you're saying sounds reasonable. I'm inclined to accept that change.

@Talador12
Copy link

I agree with @crenshaw-dev and @thesuperzapper

@cezarsa
Copy link
Contributor

cezarsa commented Aug 29, 2024

@thesuperzapper Are you already working on a PR for that behavior change? If not, I should be able to give this a try in the coming weeks.

@thesuperzapper
Copy link
Author

@thesuperzapper Are you already working on a PR for that behavior change? If not, I should be able to give this a try in the coming weeks.

@cezarsa actually yes, I have an almost working prototype, I ended up approaching it slightly differently.

The key difference to what I proposed above is that I leave the requiresPruning as true, and added a new pruningDisabled field which is then used by the UI to display is differently.

This is because I still want to actually show these in the UI (with an optional filter on the left), because it's not correct to say that these resources don't "require pruning", and setting it to false just causes it to show as "in sync". It's better if we have a special icon to indicate that we aren't going to actually remove it, but that it is actually extraneous (not in the "desired" state).

I'm also planning to add an ignoreExtraneous field because that concept is separate from disabling pruning, and we can just gray out the icon to indicate that "out of sync"-ness is not propagating to the root app.

@andrii-korotkov-verkada
Copy link
Contributor

@thesuperzapper, are you still working on the PR?

@andrii-korotkov-verkada andrii-korotkov-verkada added the more-information-needed Further information is requested label Nov 11, 2024
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-controller component:sync more-information-needed Further information is requested
Projects
None yet
Development

No branches or pull requests

6 participants