fix: retain count and order of revisions for multi source apps (#14108)

[woehrl01]

Fixes #14108
Fixes #14109

Supersedes #14081

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• The title of the PR conforms to the Toolchain Guide
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• Optional. My organization is added to USERS.md.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).
• My new feature complies with the feature status guidelines.
• I have added a brief description of why this PR is necessary and/or what this PR solves.

Please see Contribution FAQs if you have questions about your pull-request.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (8f63ba3) 49.61% compared to head (189952f) 49.61%.

❗ Current head 189952f differs from pull request most recent head 0a7837a. Consider uploading reports for the commit 0a7837a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #14113      +/-   ##
==========================================
- Coverage   49.61%   49.61%   -0.01%     
==========================================
  Files         256      256              
  Lines       43934    43924      -10     
==========================================
- Hits        21800    21795       -5     
+ Misses      19975    19972       -3     
+ Partials     2159     2157       -2     

Impacted Files	Coverage Δ
controller/state.go	71.78% <100.00%> (+0.35%)	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[ishitasequeira]

One thing to note is that, for the repo used just as a reference to values files (e.g. In below example, ref-repo), we do not generate any manifests and thus do not end up printing revision for the same. This leads to not mentioning the revision of the referenced repository used for external values files.

See below example,

spec:
  sources:
  - helm:
      valueFiles:
      - $values_test/values.yaml
    path: helm-guestbook
    repoURL: https://github.com/argoproj/argocd-example-apps.git
    targetRevision: HEAD
  - ref: values_test                          <--------------------------------- ref-Repo
    repoURL: https://github.com/ishitasequeira/values-test.git
    targetRevision: HEAD
status:
    revisions:
    - 53e28ff20cc530b9ada2173fbbd64d48338583ba

Also, if there are more than one repositories, it is difficult to associate which revision belongs to which repository. (below example)

spec:
  sources:
  - helm:
      valueFiles:
      - $values_test/values.yaml
    path: helm-guestbook
    repoURL: https://github.com/argoproj/argocd-example-apps.git
    targetRevision: HEAD
  - ref: values_test
    repoURL: https://github.com/ishitasequeira/values-test.git
    targetRevision: HEAD
  - path: guestbook
    repoURL: https://github.com/ishitasequeira/argocd-example-apps.git
    targetRevision: HEAD
status:
      revisions:
      - 53e28ff20cc530b9ada2173fbbd64d48338583ba
      - 86f65c1eacddba892814445f88631101b55f4bcf

In my opinion, it was okay to show revision directly earlier when we had only one source. But with multiple sources, we should add source to the revision as well (only for status.revisions). Something like below:

status:
      revisions:
      - repoUrl: https://github.com/argoproj/argocd-example-apps.git
        revision: 53e28ff20cc530b9ada2173fbbd64d48338583ba
      - repoUrl: https://github.com/ishitasequeira/argocd-example-apps.git
        revision: 86f65c1eacddba892814445f88631101b55f4bcf

Let me know what do you think.

cc: @crenshaw-dev

[woehrl01]

@ishitasequeira there is a place in the code with skips the revision if there is no manifest, I suggest removing that case for the revision output. Then the revisions always align up by index.

[woehrl01]

I just pushed the changes. This will also directly fix a recurring cache invalidation bug, where the results would never be cached in the mentioned case of @ishitasequeira , because the length of the array will never be the same.

argo-cd/controller/state.go

Lines 532 to 536 in b392917

	revisionChanged := len(manifestInfoMap) != len(sources) || !reflect.DeepEqual(app.Status.Sync.Revisions, manifestRevisions)
	specChanged := !reflect.DeepEqual(app.Status.Sync.ComparedTo, v1alpha1.ComparedTo{Source: app.Spec.GetSource(), Destination: app.Spec.Destination, Sources: sources})
	_, refreshRequested := app.IsRefreshRequested()
	noCache = noCache || refreshRequested || app.Status.Expired(m.statusRefreshTimeout) || specChanged || revisionChanged

[crenshaw-dev]

I think listing all revisions and using the index as key is fine, as long as the change doesn't have any unfortunate side-effects (looking at the PR, I don't think it does).

[crenshaw-dev]

The second revision is appearing as an empty string for me.

[woehrl01]

@crenshaw-dev Looks like this is related to the following condition:

argo-cd/reposerver/repository/repository.go

Lines 341 to 346 in b392917

	if hasMultipleSources && source.Path == "" && source.Chart == "" {
	log.WithFields(map[string]interface{}{
	"source": source,
	}).Debugf("not generating manifests as path and chart fields are empty")
	return nil
	}

I currently don't see a reason for having this check in place?

[ishitasequeira]

@crenshaw-dev Looks like this is related to the following condition:

argo-cd/reposerver/repository/repository.go

Lines 341 to 346 in b392917

	if hasMultipleSources && source.Path == "" && source.Chart == "" {
	log.WithFields(map[string]interface{}{
	"source": source,
	}).Debugf("not generating manifests as path and chart fields are empty")
	return nil
	}

That is the expected condition and we would not want to remove that as we are not generating manifests for the specific scenarios when the source does not have a path / chart in the repo. The source can be used only for external values files.

[woehrl01]

@ishitasequeira I see. But we still have to process this method to return the resolving of the ref (e.g. HEAD) to commit hash, don't we?

[woehrl01]

@ishitasequeira after looking at the code, and getting a bit familiar with it, my suggestion would be to slightly modify this part:

argo-cd/reposerver/repository/repository.go

Lines 512 to 528 in b392917

	operation := func(repoRoot, commitSHA, cacheKey string, ctxSrc operationContextSrc) error {
	promise = s.runManifestGen(ctx, repoRoot, commitSHA, cacheKey, ctxSrc, q)
	// The fist channel to send the message will resume this operation.
	// The main purpose for using channels here is to be able to unlock
	// the repository as soon as the lock in not required anymore. In
	// case of CMP the repo is compressed (tgz) and sent to the cmp-server
	// for manifest generation.
	select {
	case err := <-promise.errCh:
	return err
	case resp := <-promise.responseCh:
	res = resp
	case tarDone := <-promise.tarDoneCh:
	tarConcluded = tarDone
	}
	return nil
	}

So that it has a new argument to not generate a manifest, but it returns the correct "ManifestResponse", instead of this:

argo-cd/reposerver/repository/repository.go

Lines 546 to 548 in b392917

	if q.HasMultipleSources && err == nil && res == nil {
	res = &apiclient.ManifestResponse{}
	}

Thoughts?

[woehrl01]

like this 93b0273 @ishitasequeira CC: @crenshaw-dev

Created that in my fork, to get your opinion.

[ishitasequeira]

I think that should work.. Thanks @woehrl01 !!

[crenshaw-dev]

@woehrl01 how do you feel about this slightly simplified version? woehrl01#1

[woehrl01]

Looks great, I'll pick that.

[crenshaw-dev]

One downside of my change is that we'll be running some unnecessary manifest-generation-related operations for ref sources (e.g. checking for out-of-bounds symlinks) which could be skipped. But for now I think the performance hit is worth it for a relatively simple fix.

[crenshaw-dev]

/cherry-pick release-2.6

[crenshaw-dev]

/cherry-pick release-2.7

[woehrl01]

@crenshaw-dev we also have to handle

argo-cd/reposerver/repository/repository.go

Line 1984 in 41dc06d

opContext, err := ctxSrc()

Do you have an idea what would be the best behaviour here? Should we just duplicate the check and return nil?

[ishitasequeira]

LGTM!!

[crenshaw-dev]

@woehrl01 I don't think the UI currently hits that endpoint for multi-source apps, precisely because it's not equipped to handle stuff like the ref-only sources. I think it's safe to ignore until someone starts working on multi-source UI support.

[gcp-cherry-pick-bot]

Cherry-pick failed with Merge error 59e6f2c3ac5029e018cae7bb8f08204597fae773 into temp-cherry-pick-60e3a3-release-2.6

[gcp-cherry-pick-bot]

Cherry-pick failed with Merge error 59e6f2c3ac5029e018cae7bb8f08204597fae773 into temp-cherry-pick-60e3a3-release-2.7

[crenshaw-dev]

Fixes will be in 2.6.11 and 2.7.6.