Parameterize DSPO max concurrency count #394
Conversation
|
A new image has been built to help with testing out this PR: To use this image run the following: cd $(mktemp -d)
git clone git@github.com:opendatahub-io/data-science-pipelines-operator.git
cd data-science-pipelines-operator/
git fetch origin pull/394/head
git checkout -b pullrequest 7672d8c2f19d680dd96b6fb5b0b11dcad3aee308
make deploy IMG="quay.io/opendatahub/data-science-pipelines-operator:pr-394"More instructions here on how to deploy and test a Data Science Pipelines Application. |
config/manager/manager.yaml
Outdated
| @@ -29,6 +29,7 @@ spec: | |||
| args: | |||
| - --leader-elect | |||
| - --zap-log-level=$(ZAP_LOG_LEVEL) | |||
| - --MaxConcurrentReconciles=20 | |||
There was a problem hiding this comment.
can we extract this out to an env var (like $(ZAP_LOG_LEVEL) above) so that we can more easily configure this without needing to make manifest changes?
If that's outside the scope of this PR, maybe even just for now set the env with
- name: MAX_CONCURRENT_RECONCILES
value: 20
config/manager/manager.yaml
Outdated
| @@ -29,6 +29,7 @@ spec: | |||
| args: | |||
| - --leader-elect | |||
| - --zap-log-level=$(ZAP_LOG_LEVEL) | |||
| - --MaxConcurrentReconciles=20 | |||
There was a problem hiding this comment.
+1 to what Giulio mentioned, the env var could be defined like $(ZAP_LOG_LEVEL), and the value could be initialized in the params.env. You will also need to add a reference in the base kustomization file.
|
Change to PR detected. A new PR build was completed. |
| flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.") | ||
| flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.") | ||
| flag.StringVar(&configPath, "config", "", "Path to JSON file containing config") | ||
| flag.BoolVar(&enableLeaderElection, "leader-elect", false, | ||
| "Enable leader election for controller manager. "+ | ||
| "Enabling this will ensure there is only one active controller manager.") | ||
| flag.IntVar(&maxConcurrentReconciles, "MaxConcurrentReconciles", config.DefaultMaxConcurrentReconciles, "Maximum concurrent reconciles") |
There was a problem hiding this comment.
@HumairAK flag package is taking care of assigning the default value when there is no param. We are passing maxConcurrentreconciles (which will either have param value or default value to SetupWithManager function in the controller.
|
Change to PR detected. A new PR build was completed. |
1 similar comment
|
Change to PR detected. A new PR build was completed. |
VaniHaripriya
force-pushed
the
issue-368
branch
from
b3f51a4
to
d57153f
Compare
|
Change to PR detected. A new PR build was completed. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: HumairAK 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 |
The issue resolved by this Pull Request:
Resolves #368
Description of your changes:
Added a new argument "MaxConcurrentReconciles" in the manager.yaml file and read the value from the argument to use it in the controller.
Testing instructions
Pull my branch and once you deploy it, we should be able to see the worker count in the logs as below:
Starting workers {"controller": "datasciencepipelinesapplication", "controllerGroup": "datasciencepipelinesapplications.opendatahub.io", "controllerKind": "DataSciencePipelinesApplication", "worker count": 10}
If you want to update the count, update "MaxConcurrentReconciles" arg in manager.yaml file and redeploy to see the updated count in logs.
If you want to see the usage of "DefaultMaxConcurrentReconciles" which is set to 10, remove the "MaxConcurrentReconciles" arg in manager.yaml file and redeploy to see the worker count automatically updates to 10.
Checklist