ensures support for cluster-wide monitoring - #22821
Conversation
Partially addresses keycloak#15888
98f39f1 to
1b32bef
Compare
|
quarkiverse/quarkus-operator-sdk#658 takes the place of the ControllerConfigSourceInterceptor from the other pr. And quarkiverse/quarkus-operator-sdk#653 / quarkiverse/quarkus-operator-sdk#654 mean that we can use the application.properties: quarkus.operator-sdk.namespaces=JOSDK_WATCH_CURRENT Instead of the individual controller workarounds. Otherwise the rest is basically the same - the addition of a ci test for olm, making the test scripts namespaces aware, and moving the examples to test resources. |
|
@vmuzikar the test failure was unrelated to these changes. |
| IFS=', ' read -r -a array <<< "$TARGET_NAMESPACES" | ||
| for element in "${array[@]}" | ||
| do | ||
| yq ea -i ".spec.targetNamespaces += [\"$element\"]" $OPERATOR_GROUP_FILE |
There was a problem hiding this comment.
spec.TargetNamespaces seems to be kinda deprecated? But maybe we don't even need it (specifying multiple namespaces here), see my comment above.
There was a problem hiding this comment.
I would be in favor of separating that off as a another issue around supporting the selector - in general that will be a problem as the sdk nor our logic is honoring that.
There was a problem hiding this comment.
olm does manage the watching of namespaces for the selector, and updates the operator deployment from there.
I see two issues with that (not related to keycloak):
- the docuemtation is wrong, you cannot directly specify the label under the selector, it needs to be under a matchLabels stanza - if you try it as documented it does end up watching all namespaces. I'll open something for that.
- if nothing matches the label olm won't update the deployment because it can't distinguish between watching nothing and watching everything. This seems like odd behavior as you'll end up still watching one or more namespaces that you don't intend to.
So on the script changes if you want I can either backout everything and we won't bother with those changes for now, or it could be updated to be based upon a label, or a label and target namespaces (such that the script adds the label to the targets).
There was a problem hiding this comment.
Thanks for double checking!
So on the script changes if you want I can either backout everything and we won't bother with those changes for now, or it could be updated to be based upon a label, or a label and target namespaces (such that the script adds the label to the targets).
I'd keep only what's strictly necessary for testing these two scenarios in the CI:
- Operator watching and being installed in a single namespace.
- Operator watching all namespaces, i.e. empty watched namespaces.
- And maybe one more scenario that we currently miss. That the Operator can watch a list of namespaces (MultiNamespace mode). I think we technically "support" this now. But we could add this test scenario later as a follow-up.
Which means we can keep everything in the scripts as we'd use targetNamespaces for the last scenario.
There was a problem hiding this comment.
Which means we can keep everything in the scripts as we'd use targetNamespaces for the last scenario.
If I understand you correctly you mean just leave it as is in the pr and don't try to switch to a label or namespace/label based mechanism.
There was a problem hiding this comment.
don't try to switch to a label or namespace/label based mechanism.
Yeah, I'd say it's OLM responsibility. No need to test it on our side.
Replaces #21231 - uses the new upstream support for quarkus.operator-sdk.generate-with-watched-namespaces
Partially addresses #15888