Ctrl manager labels for webhooks and logging #191
Conversation
| control-plane: cinder-controller-manager | ||
| control-plane: controller-manager | ||
| app.kubernetes.io/name: cinder-operator | ||
| app.kubernetes.io/component: cinder |
There was a problem hiding this comment.
Where is this 'app.kubernetes.io/component' label used?
While it may not be a big deal, we already use a generic 'component' label on several resources that designates the specific cinder service (API, Backup, Scheduler, Volume). I don't know if others will be confused when we use the term "component" in a different way that depends on the context. In the app.kubernetes context, "component" means cinder, but in the cinder context, "component" means [API|Backup|Scheduler|Volume].
I'm not saying this is a problem, but am curious how others thing about this.
There was a problem hiding this comment.
To be honest, the app.kubernetes.io/component: <openstack service name> label doesn't seem to be used anywhere within the operator itself. It would be the same case for the app.kubernetes.io/name: <openstack service name>-operator label if we were not now using it as a unique label selector. The operator-sdk added these sorts of labels to other operators (Keystone and Placement, for example) and for some reason they were missing here and in other operators. My guess is that these labels are added via newer versions of operator-sdk. Since this Cinder operator was actually scaffolded back in 2020, the labels are simply missing. I suppose we could exclude adding app.kubernetes.io/component: cinder since it's not referenced elsewhere though. It just initially seemed prudent to repeat the newer scaffolded pattern and include it. I wonder if these new labels are added by the scaffolding because newer versions of OLM will try to look for them and thus use them for certain operations?
All that being said, I see what you mean regarding the potential "component" confusion (as an example: [1]). I'm not immediately sure what the right approach is, whether that's leaving it as-is, explicitly excluding/removing it, adding a comment to help dispel ambiguity, etc.
[1]
|
@abays: The following test 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abays, fmount 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 |
Force syncing the version in go.mod files
For initial webhook work, we found that we needed to remove the
control-plane: controller-managerlabel and label-selector from thecontroller-managerpod and its associated peripherals in order to properly direct webhook traffic to the appropriate webhook server for our various operators' CRDs. This had the unfortunate side effect of removing a label that could be used for bulk-selecting all ourcontroller-managerpods. This was noted here [1]. We therefore have opted to reintroduce thecontrol-plane: controller-managerdefault label (originally added byoperator-sdkscaffolding) and instead use another unique label (also added byoperator-sdkscaffolding, at least in newer version ofoperator-sdk) for routing webhook traffic. This unique label takes the form ofapp.kubernetes.io/name: <operator name>and can be used with label-selectors to ensure webhooks and other peripheral k8s services work correctly.[1] openshift/release#37084 (comment)