argoproj / argo-cd

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

Multi-Source App that is OutOfSync without selfHeal and with manifest-generate-paths is synced when refresh is triggered #20809

Closed Ezzahhh closed 1 week ago

Ezzahhh commented 1 week ago

Checklist:

Describe the bug

A multi-source app with selfHeal disabled and with manifest-generate-paths annotation will be synced (when it is already OutOfSync) if a refresh on it is triggered (either manually or through automatic refreshes 3min by default) and the last sync is not the same as HEAD.

Perhaps these PRs could be related? https://github.com/argoproj/argo-cd/pull/19799 https://github.com/argoproj/argo-cd/pull/19828

To Reproduce

Create a Multi-Source AppSet argocd.argoproj.io/manifest-generate-paths annotation

  ignoreApplicationDifferences:
    - jsonPointers:
        - /spec/syncPolicy

Enable auto-sync default selfHeal to true Choose an App of the AppSet to disable the selfHeal. Make a change on the cluster to force the App OutOfSync. Push to the repo so that the last sync of the App and HEAD will be different. Wait for the automatic refresh or refresh manually on the App. Observe the OutOfSync App to be synced despite self heal is disabled.

Expected behavior

An OutOfSync multi-source app with selfHeal disabled should not be synced when unrelated commits to HEAD occur and the App is refreshed. In other words, the selfHeal and manifest-generate-paths should be respected regardless.

Version

{
    "Version": "v2.13.0+347f221",
    "BuildDate": "2024-11-04T12:09:06Z",
    "GitCommit": "347f221adba5599ef4d5f12ee572b2c17d01db4d",
    "GitTreeState": "clean",
    "GoVersion": "go1.23.1",
    "Compiler": "gc",
    "Platform": "linux/amd64",
    "KustomizeVersion": "v5.4.3 2024-07-19T16:40:33Z",
    "HelmVersion": "v3.15.4+gfa9efb0",
    "KubectlVersion": "v0.31.0",
    "JsonnetVersion": "v0.20.0"
}
andrii-korotkov-verkada commented 1 week ago

Is autosync enabled?

Ezzahhh commented 1 week ago

@andrii-korotkov-verkada Yes, auto-sync is enabled. I've been reviewing some of the code and it seems like this could potentially be intended behaviour? However, I think it's confusing to be able to disable self heal and then have subsequent non-related commits to cause a sync - I think it's expected if a related commit will cause a sync only if the App has actually been changed (e.g. paths to manifest-generate-paths has changed relevant to the App).

andrii-korotkov-verkada commented 1 week ago

Yes, autosync enabled would cause automatic updates after refresh if there are changes - that's intended. Self-heal serves a different purpose, to override manual changes. Do you think documentation update would help clarify things?

Ezzahhh commented 1 week ago

Yes, I think it might help clarify the self heal functionality as I made some assumptions about how it's meant to work (which was incorrect). The confusing thing for me is that when using non-multi-source apps there is no issue because when deploying helm I am usually not changing the helm target version (but the values) so only changes to that specific App values (like changing image tag) will cause a sync but other commits to the repo for other Apps do not cause unrelated syncing. However, since we point to HEAD in multi-source apps to obtain values, Argo only knows that the target revision has changed but not whether the multi-source App has any of its files materially changed (I think we do this check in GetRepoObjs but it's not passed into autoSync). As a result, we get this divergence of expected behaviour when making the switch from non-multi-source to multi-source - maybe this is worth pointing out in the docs?

Perhaps just an extra sentence in this section?

Thank you for the quick response and clarifying btw!

andrii-korotkov-verkada commented 1 week ago

when using non-multi-source apps there is no issue because when deploying helm I am usually not changing the helm target version (but the values)

since we point to HEAD in multi-source apps to obtain values

What's the reason for this discrepancy?

Ezzahhh commented 1 week ago

Originally, our Apps were not multi-source so they look like:

      source:
        chart: my-chart
        repoURL: my-chart.com
        targetRevision: 0.1.0
        helm:
          releaseName: my-release
          values: |
            # my values

Usually we would only make two kind of changes: 1) changing the values (like the image tag); or 2) changing the target revision on the chart. Doing either of these changes would understandably cause a sync, regardless if Apps have selfheal disabled or not.

As far as I understand, in this case, Argo is keeping track of the target revision from the helm repository (e.g. 0.1.0). I think this is why subsequent unrelated pushes to the argo repo does not cause a sync/refresh even if it is OutOfSync with self heal disabled.

Once we convert this into a multi-source app, it looks like this:

      sources:
        - repoURL: git@github.com:x/x-x-x.git
          targetRevision: HEAD
          ref: values
        - chart: my-chart
          repoURL: my-chart.com
          targetRevision: 0.1.0
          helm:
            releaseName: my-release
            ignoreMissingValueFiles: true
            valueFiles:
              - $values/chart/values.yaml
            values: |
              # my values

Argo now keeps track of two revisions. One for helm and the other being the HEAD commit of my argo repo. Now, whenever an unrelated commit is pushed to the argo repo, it will trigger a refresh of this OutOfSync app because the HEAD target revision will change on any commit.

In the non-multi-source app, any subsequent refreshes that are triggered will not cause a sync (as long as the helm chart version is not changed or any value overrides in the app). I think it just comes down to the fact that the git repo's commit is not directly relevant for non-multi-source helm apps. Perhaps it will have the same behaviour as multi-source if the helm chart points to the repo as opposed to a helm repository? I am not sure.

andrii-korotkov-verkada commented 1 week ago

I see, I think you just need to have autosync disabled to avoid this.