-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
feat: Annotation to support per app refresh timeouts (soft & hard) - #9499 #20422
base: master
Are you sure you want to change the base?
Conversation
✅ Preview Environment deployed on Bunnyshell
See: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #20422 +/- ##
=========================================
Coverage ? 56.02%
=========================================
Files ? 322
Lines ? 44820
Branches ? 0
=========================================
Hits ? 25109
Misses ? 17107
Partials ? 2604 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Kai Fink <[email protected]>
Signed-off-by: Kai Fink <[email protected]>
Signed-off-by: Kai Fink <[email protected]>
Signed-off-by: Kai Fink <[email protected]>
Signed-off-by: Kai Fink <[email protected]>
70b2cb7
to
be1571e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR.
I tend to think that giving users control over the refresh time would not be a good idea, as this will put unexpected strain on the application controller and repository server - especially in the case of a hard refresh.
I'm also not sure yet whether a setting such as this should be an annotation, or whether it is better housed in the spec.
IMHO, we should think this through, and come up with a proper way to allow/disallow users from actually leveraging this setting (e.g. by having a flag in the AppProject).
TL;DR: I think this needs further discussion.
Hey! I absolutely understand that having to low timeouts will harm performance, but at the moment, the user only has the option to set it on the global level(https://github.com/argoproj/argo-cd/blob/master/cmd/argocd-application-controller/commands/argocd_application_controller.go#L235) which can even more easily harm performance Regarding the way of implementation, i just used annotations cause there was already a annotation for forcing a refresh, but i can also adjust it and define it via spec :) |
I think there is a subtle difference: It's not the user who has control over the global setting, but the Argo CD administrator or operator. With an annotation on the application, that control is handed over to a regular, low-privileged user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think this needs further discussion. I agree with @jannfis on the different arguments above. Argo CD is a GitOps tool and react to git changes.
I think if there is a need to monitor an external non-git systems, like an artifact registry to detect non-gitops changes, this can be done by another controller. Then that controller can request a refresh of the application with the current annotation/api when it detects changes that would require a refresh. A naive implementation would be to do it periodically
Ahh yeah, our perspective as serviceprovider, where we use argocd to baseline and are admin & user, there was not a big difference. But a global setting to limit the options or disable it overall should be usefull.
For issues like the linked, this could definitely be a option. Regarding the second usecase with config management plugins(like vault secret plugin), where the same source can result in a different result based on external conditions, this would need a whole restructuring of the plugin design from my perspective. |
Hey together,
with this pr i added two new annotations to control the refresh timeouts(soft and hard) on a per app basis. This should solve the issue #9499 and also allow users to specify hard refresh intervals, when using plugins like argocd-vault-plugin, to refresh changed secrets(https://argocd-vault-plugin.readthedocs.io/en/stable/usage/#refreshing-values-from-secrets-managers)
Fixes #9499
Checklist: