feat(controller): Permit using newer revision when retrying failed sync (#11494)

[olivergondza]

When sync fails due to incorrect manifest declarations, this permits fixes to be deployed via future commits.

This is controlled by a new boolean Application CRD field syncPolicy.retry.refresh or via the --sync-retry-refresh flag.

Closes #11494
Related to #6055
Discussed at https://www.youtube.com/watch?v=baIX9Bk6f5w&t=1173s

Kudos to @aslafy-z and @Sayrus for doing most of the heavy lifting here (#15603).

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.
• 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.
• Optional. My organization is added to USERS.md.
• [n/a] Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

[bunnyshell]

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

• 🚀 /bns:deploy to deploy the environment

[codecov]

Codecov Report

❌ Patch coverage is 92.30769% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.36%. Comparing base (8b8d04e) to head (e22dfc2).
⚠️ Report is 786 commits behind head on master.

Files with missing lines	Patch %	Lines
cmd/argocd/commands/app.go	70.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #23038      +/-   ##
==========================================
+ Coverage   60.32%   60.36%   +0.03%     
==========================================
  Files         350      350              
  Lines       60032    60061      +29     
==========================================
+ Hits        36217    36254      +37     
+ Misses      20901    20895       -6     
+ Partials     2914     2912       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[olivergondza]

@jannfis, I understand you had a PoC doing something similar, I would appreciate your review. Thanks!

[agaudreault]

As discussed in contributor meeting, only auto-sync should have this behaviour. #11494 (comment)

[anandf]

If a sync fails, is it possible to differentiate between a retryable error (eg: nodes, resources, api server not available) and a non retryable error (eg: a manifest is not according to the schema, image does not exist etc). And the sync operation will retry only for a retryable error and for the other scenario, sync would just fail as it does not make sense to keep retrying if the error is say manifest not matching the schema.

[olivergondza]

If a sync fails, is it possible to differentiate between a retryable error (eg: nodes, resources, api server not available) and a non retryable error (eg: a manifest is not according to the schema, image does not exist etc).

Interesting point. I have few observations:

• image does not exist can be recoverable in case the tag is being pushed in parallel.
• Is there a way to tell what exactly have caused the sync error?
  • For manifest is not according to the schema, I agree it sounds not recoverable, provided we can tell it apart from CRD not created/updated yet.

And the sync operation will retry only for a retryable error and for the other scenario, sync would just fail as it does not make sense to keep retrying if the error is say manifest not matching the schema.

Agreed that manifests not matching the schema make no sense retrying with same commit. BUT it is exactly the thing I would like Argo CD to retry with new commit ASAP.

[olivergondza]

On the contributors meeting it was agreed that updating the commit sha (as originally implemented here) can be confusing for users as it does not indicate the fact a sync operation with HEAD~1 has failed. There was a consensus it is better to find out a way to get the current retry fail clearly, and let/force the new one to kick in.

[olivergondza]

Thanks for the pointers, @agaudreault.

I have changed the approach to let the current attempt fail, and let the new one to kick in, but I am struggling to get things to work. The phase never gets to Succeeded in the very last step.

Also, the UI seems to be in an odd state. The "SYNC STATUS" reports success on last (fixed) commit, but "LAST SYNC" reports fail on an earlier commit.

Hitting [Sync] in E2E UI gets the app go completely green, but for some reason, it does not happen on its own.

[olivergondza]

@agaudreault, I have updated the tests. Curious to hear your thoughts regarding the previous comment...

[agaudreault]

What is the motivation here? Do you suggest the explicit UI sync to behave the same?

These are the use cases / business requirements to test with this change. The UI sync behaviour should also populate the refresh option and correct sources. The rollback experience is different and should never try to refresh. The behaviour of the UI vs CLI for the same operation should be consistent with how retries are configured for each.

To make sure the operation can be refreshed correctly, you must make sure that the sources are properly set in the operation object.

[olivergondza]

@agaudreault, I have added the tests (and prod code fix) for explicit CLI sync.

Though, the (CLI) rollback seems to be a no-brainer: FailedPrecondition desc = rollback cannot be initiated when auto-sync is enabled.

[agaudreault]

LGTM

[olivergondza]

Thanks, @agaudreault. Your help was priceless!