Skip to content

fix(argo-cd): make ttlSecondsAfterFinished configurable for redisSecretInit - #4050

Open
duncan485 wants to merge 3 commits into
argoproj:mainfrom
duncan485:main
Open

fix(argo-cd): make ttlSecondsAfterFinished configurable for redisSecretInit#4050
duncan485 wants to merge 3 commits into
argoproj:mainfrom
duncan485:main

Conversation

@duncan485

Copy link
Copy Markdown

Description

This PR makes ttlSecondsAfterFinished configurable via redisSecretInit.ttlSecondsAfterFinished (default bumped from 60 to 600 seconds), giving ArgoCD's controller much more time to observe hook completion before the Job is cleaned up.

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:

  • I have bumped the chart version according to versioning
  • I have updated the documentation according to documentation
  • I have updated the chart changelog with all the changes that come with this pull request according to changelog.
  • Any new values are backwards compatible and/or have sensible default.
  • I have signed off all my commits as required by DCO.
  • I have created a separate pull request for each chart according to pull requests
  • My build is green (troubleshooting builds).

@duncan485 duncan485 changed the title fix(redis-secret-init): make ttlSecondsAfterFinished configurable for redisSecretInit fix(argo-cd): make ttlSecondsAfterFinished configurable for redisSecretInit Sep 1, 2026
@jmeridth

jmeridth commented Sep 2, 2026

Copy link
Copy Markdown
Member

Bumping from 60 to 600 is a serious change IMO. 1 minute to 10 minutes. Please change back to 60 as the default. People can bump it higher if needed after your change. Unless you can defend the 9 minute bump? Thank you for the contribution.

@jmeridth

jmeridth commented Sep 2, 2026

Copy link
Copy Markdown
Member

🤖 Following up on my earlier comment after digging through the history, because the 60/600 question turns out to be more interesting than a simple default change.

What I found:

So the key question: which Argo CD version are you running when you hit this? This chart's current appVersion (3.5.x) should contain the finalizer fix. If you're reproducing the race on 3.5.x, that's an upstream regression worth reporting on argoproj/argo-cd#6880 with your details, and a wider TTL default here is a reasonable stopgap. If you're on an older Argo CD, the default should stay 60 and the configurability this PR adds is the fix for your case.

Either way, two asks:

  1. If we do change the default, the values comment and changelog need to say why (the TTL-vs-hook-observation race, with links to ArgoCD stuck in waiting for completion of hook batch/Job/argocd-redis-secret-init #2887 and waiting for completion of hook and hook never succeds argo-cd#6880), because 600 looks arbitrary without that context and the next person will try to "clean it up" back to 60.
  2. Version collision heads-up: this targets chart 10.6.1, which the just-approved fix(argo-cd): support tpl in configs.cm.url #3888 also targets; whichever merges second needs a re-bump.

The configurability itself is welcome regardless of where the default lands. Thanks for tracing this into #2887; that issue has needed movement for a long time.

Comment thread charts/argo-cd/values.yaml Outdated
extraArgs: []

# -- Seconds after which the completed Redis secret-init Job is automatically deleted
ttlSecondsAfterFinished: 600

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bumps the default from 60 to 600 as well as making it configurable, so the render changes for everyone rather than only for people who set it. Shouldn't that be a minor rather than a patch? (versioning)

@duncan485

Copy link
Copy Markdown
Author

Hey @jmeridth I am actually on argo 3.3.9 (helm version 9.5.11). So I don't think that upsteam worked for me. I will edit my MR so it just makes it configurable, but still defaults to 60. Than at least we can try if this fixes the actual issue. If it does, we can change the default in a later MR?

@jmeridth

jmeridth commented Sep 2, 2026

Copy link
Copy Markdown
Member

@duncan485 thanks for confirming the version — argo-cd 3.3.9 matters more than you might expect: the upstream finalizer fix for exactly this race (argoproj/gitops-engine#646) shipped in the 3.0 line, so reproducing the TTL race on 3.3.9 is evidence of a gap in that fix. When you have a moment, please post your reproduction details on argoproj/argo-cd#6880 (version, self-managed setup, the stuck-hook state); the maintainers there have been asking whether post-3.0 reports still exist.

Your revised plan is exactly right for this PR: configurable ttlSecondsAfterFinished with the default staying 60. Two mechanical notes for the update: per @tico24's point and our versioning conventions, a new value means a chart minor bump (and with the recent merge activity, rebase onto current main — argo-cd chart is at 10.7.0, so target 10.8.0); and please include the rationale in the value's helm-docs comment (why one might raise it, with a pointer to #2887) so the number has context. Happy to review once pushed.

@duncan485
duncan485 force-pushed the main branch 2 times, most recently from f446161 to 0f7fd28 Compare September 3, 2026 10:04
…ance documentation for hook-completion race condition

Signed-off-by: duncan485 <bakkerduncan@gmail.com>
Signed-off-by: duncan485 <bakkerduncan@gmail.com>
@github-actions github-actions Bot added size/S and removed size/XS labels Sep 3, 2026
@duncan485

Copy link
Copy Markdown
Author

Hey @jmeridth Thanks, I updated the MR. As the default is now kept at 60, I think say a patch version should be the right bump.

I will definitely post a to the original story on how to reproduce. But I am also verry curious to see if this solves it in my case.

Comment thread charts/argo-cd/values.yaml
Comment thread charts/argo-cd/Chart.yaml
…etInit

Signed-off-by: duncan485 <bakkerduncan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants