Adds --max-workers flag to the exec-entrypoint helm command #2607
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
size/S
| c, err := controller.New(controllerName, mgr, controller.Options{Reconciler: r}) | ||
| c, err := controller.New(controllerName, mgr, controller.Options{ | ||
| Reconciler: r, | ||
| MaxConcurrentReconciles: options.MaxWorkers, |
There was a problem hiding this comment.
The name is the same used in Ansible. So it shows great 👍
There was a problem hiding this comment.
Really thank you for your contribution. 👍
Just a few nits
- We need to add an entry in the CHANGELOG about it. Something such as:
Add the
--max-workersflag to the commandsoperator-sdk exec-entrypointandoperator-sdk run --localfor Helm based-operators with the purpose of X. (#2607)
- Also, could you add an example of its usage in the docs? Maybe in https://github.com/operator-framework/operator-sdk/blob/master/doc/helm/user-guide.md
openshift-ci-robot
added
size/M
| - Add event stats output to the operator logs for Ansible based-operators. ([2580](https://github.com/operator-framework/operator-sdk/pull/2580)) | ||
| - Improve Ansible logs by allowing output the full Ansible result for Ansible based-operators configurable by environment variable. ([2589](https://github.com/operator-framework/operator-sdk/pull/2589)) |
There was a problem hiding this comment.
I think my editor just stripped out some white spaces from these lines.
|
Hi @estroz,
Are you saying that it should be done for Ansible too? Am I right? So, this implementation already exist for Ansible. See: https://github.com/operator-framework/operator-sdk/blob/master/pkg/ansible/controller/controller.go#L92. And it is in the AnsibleFlags which means that is done for the entrypoint and run --local commands. See: https://github.com/operator-framework/operator-sdk/blob/master/pkg/ansible/flags/flag.go#L29 |
|
Happy Monday everyone. Thanks for looking at this @estroz . As @camilamacedo86 was mentioning, the feature already exists for Ansible. I made sure to use the same arg name to keep it consistent. Let me know if there is anything else I could improve on this PR. |
|
Yep, I added this initially to the Ansible side. @razvan-wework thanks great work. |
jmrodri
dismissed
estroz’s stale review
I'm dismissing Eric's review because as his concerns have already been address on the ansible side.
Description of the change:
Adds --max-workers flag to the exec-entrypoint helm command
Motivation for the change:
Currently the helm operator runs with just 1 worker. Reconciling helm releases can sometime take >1 second, using a single worker will not be able to keep up for more than 60 CRs of the same type.