Allow enabling/disabling olm descriptors #1244
Conversation
Signed-off-by: Baiju Muthukadan <[email protected]>
Signed-off-by: Andy Sadler <[email protected]>
Signed-off-by: Pavel Macík <[email protected]>
Signed-off-by: Andy Sadler <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test 4.10-performance-reduced |
|
/test performance |
Signed-off-by: Andy Sadler <[email protected]>
sadlerap
force-pushed
the
disable-olm-descriptors
branch
from
c11d97d to
69b4798
Compare
|
/test 4.10-performance-reduced |
Signed-off-by: Andy Sadler <[email protected]>
sadlerap
force-pushed
the
disable-olm-descriptors
branch
from
60c1cab to
b32fec9
Compare
|
/test 4.10-performance-reduced |
|
@sadlerap: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
/lgtm @sadlerap The OpenShift acceptance tests fail because the OLM descriptors are not explicitelly enabled for them and only the OLM descriptor related tests fails. I have identified those scenarios and sent a PR (#1245) to mark them with the With the second PR I have temporarily disable them because I cannot use the code to patch SBO deployment to enable olm descriptors on SBO like you do until this PR is actually merged. (otherwise other PR's CI would fail complaining that the flag is invalid) Once the above PRs are merged to the respecive repos, you can rebase your PR and the OpenShift acceptance tests should pass again. |
| kubectl describe deployments.apps -n operators service-binding-operator | ||
| echo "##[endgroup]" | ||
| echo "##[group]Patch operator" | ||
| kubectl patch deployments.apps -n operators service-binding-operator --type=json -p='[{"op": "add", "path": "/spec/template/spec/containers/0/args/-", "value": "--olm-descriptor"}]' |
There was a problem hiding this comment.
@sadlerap are you sure this change to the deployment is not reverted back by the OLM immediatelly after it is patched?
| 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.BoolVar(&enableLeaderElection, "leader-elect", false, | ||
| "Enable leader election for controller manager. "+ | ||
| "Enabling this will ensure there is only one active controller manager.") | ||
| flag.BoolVar(&enableOLMDescriptor, "olm-descriptor", false, |
There was a problem hiding this comment.
I have doubts with the approach of using CLI parameters to set this flag.
What about using a ConfigMap to set/store such flag that SBO would look for at boot time and use the values from the CM. If the CM does not exist, SBO would create it automatically with the default values.
This allows setting this kind of flags at install time without the need to change or patch the actual Deployment objects (like in case of having a CLI parameter for --olm-descriptors). The installation would then be like... Create a ConfigMap in the operator's namespace of a particular name with enableOLMDescriptor: true data in it (if users want to enable it) or don't do it as before to keep it disabled by default.
There was a problem hiding this comment.
Yeah, I'm not sure I like the interface; a ConfigMap probably would work better. Are they the conventional way of providing configuration options for an operator?
There was a problem hiding this comment.
Well, it is a conventional (a "cloud-native") way in Kubernetes in general.
|
/hold |
|
Closing in favor of #1251. |
Changes
This introduces a new flag,
--olm-descriptors, that toggles whether olm descriptors are used as binding sources. The default value isfalse, i.e. olm descriptors are not checked by default. To re-enable, run the following:kubectl patch deployments.apps -n service-binding-operator service-binding-operator --type=json -p='[{"op": "add", "path": "/spec/template/spec/containers/0/args/-", "value": "--olm-descriptor"}]'There's probably some more test coverage I need to do for this, but I'd like to see how performance looks.
/kind enhancement
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
included if any changes are user facing
included if any functionality added or changed. For bugfixes please include tests that can catch regressions
@external-feedbacktag.