Increase the default $BMO_CONCURRENCY for scale #906
Conversation
metal3-io-bot
added
the
size/S
|
/test-integration |
|
@flaper87: adding LGTM is restricted to approvers and reviewers in OWNERS files. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dtantsur The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
metal3-io-bot
added
the
approved
| @@ -1312,7 +1313,13 @@ func (r *BareMetalHostReconciler) updateEventHandler(e event.UpdateEvent) bool { | |||
| // SetupWithManager reigsters the reconciler to be run by the manager | |||
| func (r *BareMetalHostReconciler) SetupWithManager(mgr ctrl.Manager) error { | |||
|
|
|||
| maxConcurrentReconciles := 3 | |||
| maxConcurrentReconciles := runtime.NumCPU() | |||
| if maxConcurrentReconciles > 8 { | |||
There was a problem hiding this comment.
Instead of using an hard-coded value, I was thinking that it could be useful to set as the upper limit something like min(PROVISIONIG_LIMIT/2.5, 1), just to enforce the ratio between the two values in case the user will set a particularly low value for PROVISIONING_LIMIT.
There was a problem hiding this comment.
Is there a plan to keep this configurable as an option (via env)?
There was a problem hiding this comment.
Is there a plan to keep this configurable as an option (via env)?
It is still configurable.
There was a problem hiding this comment.
Instead of using an hard-coded value, I was thinking that it could be useful to set as the upper limit something like
min(PROVISIONIG_LIMIT/2.5, 1), just to enforce the ratio between the two values in case the user will set a particularly low value forPROVISIONING_LIMIT.
I can't imagine anyone setting a smaller limit except on very small hardware, in which case the CPU count should scale the default appropriately for us. I can imagine people setting a bigger limit, but I don't think we want to increase the default concurrency in that case. I don't think we should tie these two settings together because ultimately they limit different things.
There was a problem hiding this comment.
It's not about tying them together, it's just about the currently proposed value for the default upper limit of maxConcurrentReconciles - and I think there was an error in my previous comment, as I meant something like:
maxConcurrentReconciles = min(PROVISIONING_LIMIT/2.5, runtime.NumCPU())
which for a default PROVISIONING_LIMIT value of 20 will limit maxConcurrentReconciles to 8 on a system with more than 8 cores.
Given that the current proposal is to define the default value based on the number of available CPU, the basic idea is to try using all the available cpus as much possible - even though the benefits could be marginal (in general I'd expect that BMO will run within a container, which could have already resources and/or limits set), and at the same time trying to honor the fact that maxConcurrentReconciles << PROVISIONING_LIMIT
There was a problem hiding this comment.
This looks good, the only thing is we have to update the documentation whether we hard-code or set the upper limit to $BMO_CONCURRENCY accordingly.
Instead of defaulting the $BMO_CONCURRENCY value (the maximum number of Hosts to reconcile concurrently) to a hard-coded value of 3, set it instead to the number of CPU threads available, but constrained to a range between 2 and 8. We never default to 1 so that we don't inadvertantly rely on single-threadedness in tests. 8 seems to be a reasonable value for a large scale deployment, while still being substantially below the default $PROVISIONING_LIMIT of 20. There remains no restriction on the value that can be passed in the environment variable. Fixes metal3-io#905
We never call the provisioner in this state, so speed it up by never checking the current registration status.
|
/test-integration |
|
The current changes could be a good improvement in respect to the previous situation, and looks like there could be some room for improvements in the future /lgtm |
Instead of defaulting the $BMO_CONCURRENCY value (the maximum number of
Hosts to reconcile concurrently) to a hard-coded value of 3, set it
instead to the number of CPU threads available, but constrained to a
range between 2 and 8.
We never default to 1 so that we don't inadvertantly rely on
single-threadedness in tests. 8 seems to be a reasonable value for a
large scale deployment, while still being substantially below the
default $PROVISIONING_LIMIT of 20.
There remains no restriction on the value that can be passed in the
environment variable.
Fixes #905