-
-
Notifications
You must be signed in to change notification settings - Fork 320
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
allow configuring controller's concurrency #1277
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1277 +/- ##
==========================================
+ Coverage 72.21% 72.43% +0.22%
==========================================
Files 75 75
Lines 6258 6337 +79
==========================================
+ Hits 4519 4590 +71
- Misses 1739 1747 +8
|
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.
hey again! i like the general approach here. only had time to put some quick comments on the public interface (which only has a few nits) as i'm not super online this week. hoping to leave the small internal extension there to natalie.
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.
Finally gotten around to really look at this, and even tested this out a little bit in a busy controller. Functionally, it looks great 👍. It seems to be behaving the desired way by flattening the curve (compressing the reconciliation rate graph down).
The only points i have to make here are about the 1. complexity of the implementation and 2. if backpressure will become a problem here.
- This is already a very complicated part of the codebase, and would like to have the helpers as contained as possible. Have put some questions/comments in various places.
- If users set
concurrency: 1
(controller runtime's default btw) in a highly parallel controller you might end up in a bad failure mode; throttling the controller, but continuing to fill the scheduler'spending
set (afaikt). Maybe we need a configurable max limit on the amount of pending reconciliations and just return a new error type after this? Or maybe we let it grow for now and defer to backpressure mechanisms in the applier or issues at a later time (this is an opt-in thing after all, and we already have deduping to help us out). I am not sure, but I am curious to see what does controller-runtime does here - if they do anything.
The memory cost of the |
Thanks @nightkr, that is reassuring. I'll resolve the comments related to congestion or backpressure. |
Add `concurrency` to `controller::Config` which defines a limit on the number of concurrent reconciliations that the controller can execute at any given moment. Its default by 0, which lets the controller run with unbounded concurrency. Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
b6e13e4
to
90faa0f
Compare
Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>
90faa0f
to
9e3334b
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.
Thank you very much. This is looking good to go from my end 👍
Motivation
Its often desirable to control the max no of reconciliations that the controller can run at a given moment to see more predictable behavior or to better utilize the host machine's resources.
Fixes #1248
Solution
Add
concurrency
tocontroller::Config
which defines a limit on the number of concurrent reconciliations that the controller can execute at any given moment. Its default by 0, which lets the controller run with unbounded concurrency.