Skip to content
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

Looks like the burst configuration for the Workflow Controller doesn't actually do anything #8576

Closed
Radolumbo opened this issue May 2, 2022 · 5 comments
Labels
area/controller Controller issues, panics problem/stale This has not had a response in some time type/bug

Comments

@Radolumbo
Copy link
Contributor

Was hunting down what burst actually does for https://cloud-native.slack.com/archives/C01QW9QSSSK/p1651410938829379

Looking for burst in https://github.com/argoproj/argo-workflows takes us to https://github.com/argoproj/argo-workflows/blob/master/workflow/controller/config.go#L85

Looks like it's using a built in golang Library, time/rate .. so we can take a look at https://pkg.go.dev/golang.org/x/time/rate#Limiter.Burst

This tells us

Burst returns the maximum burst size. Burst is the maximum number of tokens that can be consumed in a single call to Allow, Reserve, or Wait, so higher Burst values allow more events to happen at once. A zero Burst allows no events, unless limit == Inf.

Interesting. Where are we using any of those calls in the Workflow controller on this rateLimiter object from the first link? Looks like .. here

https://github.com/argoproj/argo-workflows/blob/master/workflow/controller/workflowpod.go#L429

So, we're using Allow. Based on

https://pkg.go.dev/golang.org/x/time/rate#Limiter.Allow

Allow is shorthand for AllowN(time.Now(), 1).

So wait -- burst tells us how many tokens for rate limiting can be requested in a single call to Allow. However, the controller only ever requests a single token at time.

Thus -- I actually think that the configured burst for the Workflow Controller doesn't do anything at all.

Let me know if it seems like I missed anything here, but I'm pretty sure burst does nothing for the Workflow controller.

@alexec
Copy link
Contributor

alexec commented May 2, 2022

This bug is correct. Yet, I don't think we can change the code to use AllowN, createWorkflowPod does not know how many pods will be created.

@alexec alexec added area/controller Controller issues, panics and removed triage labels May 2, 2022
@alexec alexec removed their assignment May 2, 2022
@Radolumbo
Copy link
Contributor Author

Sure, I totally agree -- not suggesting we try to add that functionality, but rather we should probably clear out code/guidance that is confusing to users. Maybe I can take a crack at some point soon.

@stale
Copy link

stale bot commented Jun 18, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is a mentoring request, please provide an update here. Thank you for your contributions.

@stale stale bot added the problem/stale This has not had a response in some time label Jun 18, 2022
@stale
Copy link

stale bot commented Jul 10, 2022

This issue has been closed due to inactivity. Feel free to re-open if you still encounter this issue.

@thor
Copy link

thor commented Mar 8, 2023

Coming across this by chance, I suggest re-opening this issue and marking it as frozen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/controller Controller issues, panics problem/stale This has not had a response in some time type/bug
Projects
None yet
Development

No branches or pull requests

3 participants