fix: Process webhook refresh in background to not block the request (#14269) #18173
Conversation
dhruvang1
force-pushed
the
process-webhook-in-background
branch
2 times, most recently
from
81d564b
to
409a7c2
Compare
dhruvang1
force-pushed
the
process-webhook-in-background
branch
from
409a7c2
to
9533cd5
Compare
dhruvang1
force-pushed
the
process-webhook-in-background
branch
2 times, most recently
from
a61c546
to
f720109
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18173 +/- ##
==========================================
+ Coverage 50.25% 50.28% +0.03%
==========================================
Files 315 315
Lines 43125 43162 +37
==========================================
+ Hits 21671 21705 +34
- Misses 18967 18976 +9
+ Partials 2487 2481 -6 ☔ View full report in Codecov by Sentry. |
dhruvang1
force-pushed
the
process-webhook-in-background
branch
2 times, most recently
from
92a549b
to
498b5ee
Compare
|
@gdsoumya Could you take a re-look when you are available? It would be nice to merge the PR as a lot of folks, including us, are impacted by this issue. |
| @@ -241,6 +242,7 @@ func NewCommand() *cobra.Command { | |||
| command.Flags().StringVar(&scmRootCAPath, "scm-root-ca-path", env.StringFromEnv("ARGOCD_APPLICATIONSET_CONTROLLER_SCM_ROOT_CA_PATH", ""), "Provide Root CA Path for self-signed TLS Certificates") | |||
| command.Flags().StringSliceVar(&globalPreservedAnnotations, "preserved-annotations", env.StringsFromEnv("ARGOCD_APPLICATIONSET_CONTROLLER_GLOBAL_PRESERVED_ANNOTATIONS", []string{}, ","), "Sets global preserved field values for annotations") | |||
| command.Flags().StringSliceVar(&globalPreservedLabels, "preserved-labels", env.StringsFromEnv("ARGOCD_APPLICATIONSET_CONTROLLER_GLOBAL_PRESERVED_LABELS", []string{}, ","), "Sets global preserved field values for labels") | |||
| command.Flags().IntVar(&webhookParallelism, "webhook-parallelism-limit", env.ParseNumFromEnv("ARGOCD_APPLICATIONSET_CONTROLLER_WEBHOOK_PARALLELISM_LIMIT", 25, 1, 1000), "Number of webhook requests processed concurrently") | |||
There was a problem hiding this comment.
Might want to increase the default value and the max limit. 1000 limit might be small for some users. Also we should document this so that others are aware of this setting.
There was a problem hiding this comment.
1000 is set as max limit to avoid footguns. If a large setup has say 5K Applications, and each application is 4KB (from what I see), 1000 concurrent processing will use 5K * 4KB * 1K = 20GB of memory. I think that already is a very large number. If you still think its less, I can set it to more, maybe 5K or 10K?
Do you have any guidance on default values? I can set it to 50 or 100.
Also we should document this so that others are aware of this setting.
I have noted this flag in argocd-server.md. Could you point out where else would you like this.
There was a problem hiding this comment.
@gdsoumya I have updated the default to 50. I also see you are talking about AppSet flag documentation, but I don't see any obvious place to document this. Maybe in the appset webhook section in the git generator?
There was a problem hiding this comment.
Adding to argocd-cmd-params-cm.yaml is good enough. Thanks!
…rgoproj#14269) Signed-off-by: dhruvang1 <dhruvang1@users.noreply.github.com>
dhruvang1
force-pushed
the
process-webhook-in-background
branch
from
498b5ee
to
5d4e308
Compare
This PR moves the webhook processing for both Application and ApplicationSet to a goroutine. This would allow the server to quickly send HTTP 200 to the webhook request adhering to the quick response guidelines set by GitHub, GitLab, etc.
Checklist:
Fixes #14269