-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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(webhook): make app refresh operation concurrent (#14269) #15326
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #15326 +/- ##
==========================================
+ Coverage 49.67% 49.76% +0.09%
==========================================
Files 267 267
Lines 46383 46442 +59
==========================================
+ Hits 23039 23112 +73
+ Misses 21084 21068 -16
- Partials 2260 2262 +2
☔ View full report in Codecov by Sentry. |
Signed-off-by: phanama <[email protected]>
bfae177
to
5935fac
Compare
Signed-off-by: phanama <[email protected]>
Signed-off-by: phanama <[email protected]>
58d24f7
to
507530c
Compare
…tAppRefresh Signed-off-by: phanama <[email protected]>
b01e6a7
to
84ecbb3
Compare
… log placement logic Signed-off-by: phanama <[email protected]>
Signed-off-by: phanama <[email protected]>
Signed-off-by: Yudi A Phanama <[email protected]>
Signed-off-by: phanama <[email protected]>
Signed-off-by: phanama <[email protected]>
Signed-off-by: phanama <[email protected]>
cb6b011
to
277c83f
Compare
… guard clauses Signed-off-by: phanama <[email protected]>
if !appFilesHaveChanged(&app, changedFiles) { | ||
// we cannot retrieve previous and next commit SHA for bitbucket. | ||
// don't attempt to load/create cache for empty strings instead of SHA | ||
if change.shaBefore == "" && change.shaAfter == "" { |
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.
added the comment based on ref:
https://github.com/argoproj/argo-cd/pull/4699/files#r514417202
The current webhook handler processes all applications sequentially, with each iteration potentially calling the network-bound argo.RefreshApp() which calls k8s API to patch a refresh annotation. This makes webhook slow in scenarios involving hundreds of apps.
This PR introduces concurrent application processing to webhook HandleEvent(). We also expose a new configurable setting in ArgoCD CM
webhook.maxConcurrentAppRefresh
to be able to control the degree of concurrency.Fixes #14269
Checklist:
* [ ] I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.