pools: add default label for master/worker pool #1937
Conversation
openshift-ci-robot
added
the
approved
|
Hmm, the kubeletconfig can't target a name directly? It seems weird to add labels that just match our names. |
by design it searches the MCPs for a label that it wants to match to create a corresponding machineconfig 3 notes:
|
|
To be more specific on my previous point, I mean that we use labels to select the pool here: https://github.com/openshift/machine-config-operator/blob/master/pkg/controller/kubelet-config/kubelet_config_controller.go#L607 . It's not a direct kubeletconfig-pool matching. Would you say we should redesign that? |
|
/test e2e-aws-proxy |
3 similar comments
|
/test e2e-aws-proxy |
|
/test e2e-aws-proxy |
|
/test e2e-aws-proxy |
|
/test e2e-aws-proxy Just doing this for an environment, please ignore |
|
/test e2e-aws-proxy |
|
/retest |
|
/test e2e-aws-proxy |
uhm, I can't remember anything there 🤔 is it causing any specific issue?
I think we can target custom pools as well as we're growing further support as first-class citizen (stub ignition creation and the like) |
yuqi-zhang
force-pushed
the
add-default-mcp-label
branch
from
c4e1fc7
to
b2d57ed
Compare
|
Adding it as a template instead, so it should get applied to all pools. Tested with master/worker, will test with custom pools to make sure Also it seems that quotes or not, it doesn't make a difference so long as the string inside is the same |
| template: | ||
| metadata: | ||
| labels: | ||
| pools.operator.machineconfiguration.openshift.io/{{ status.configuration.name }}: "" |
There was a problem hiding this comment.
omg niceee
hmm..... What do you think this does? To my knowledge, this field doesn't appear in the CRD type, so it will be pruned out.
There was a problem hiding this comment.
Having a default label applied to each MCP object being created, do you know what the correct way to do this is?
yuqi-zhang
force-pushed
the
add-default-mcp-label
branch
from
b2d57ed
to
a275016
Compare
|
I can confirm that this works for installs/upgrades for master/worker pools, however the template doesn't seem to apply for custom pools by default. Maybe I'm mis-understanding the templating mechanism? |
|
alrighty, the master/worker MCP are still something managed by MCO and it makes sense to have default labels applied. Custom pools on the other hand are user-provided and it makes sense to follow the pattern in the default pool manifests (maybe we'll find a way to do that). This is an optimization for the default pools anyway, the old behavior is untouched as per acceptance criteria in the jira card |
yuqi-zhang
force-pushed
the
add-default-mcp-label
branch
from
a275016
to
8730656
Compare
|
@openshift/openshift-team-mco-maintainers ptal /approve |
|
I've changed it back to just adding a label for master/workers based on their manifests for now. In the long term we may want to rework the controller to manage the label |
Today the MCPs does not have any different labels by default. This means that if e.g. a user wants to add a kubeletconfig, they must first create a label for the pool and target the kubeletconfig against the label. This PR creates a default label for master/worker pools to skip that step. Signed-off-by: Yu Qi Zhang <jerzhang@redhat.com>
Update docs with the default pools, as well as minor updating of config names/spec version. Signed-off-by: Yu Qi Zhang <jerzhang@redhat.com>
yuqi-zhang
force-pushed
the
add-default-mcp-label
branch
from
8730656
to
e18c378
Compare
|
Added quotes to labels. Although I feel like we should remove them like other openshift repos, for now lets keep them consistent |
Wait, KubeletConfigs specify MachineConfigPools via label selectors to create MachineConfigs selected by that pool via label selectors? 😱 Isn't this backwards? Except that it sorta assumes this'll work based on the pool name. A lot of indirection going on. KubeletConfigs should not be aware of MachineConfigPools at all, nor should they rely on any sort of naming or labeling, and they definitely shouldn't be aware of templates or rendering logic. Gah, this deserves an issue at least |
|
We can add that as part of the KCC/CRCC rework items. What would you recommendation be in terms of hooking up KC/CRC to MCs? |
|
This PR just aims to add a default. We can always remove it if we rework the behaviour |
👍 sorry, yes this PR seems fine by me. Shouldn't have polluted the PR like that! Sorry again. /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ericavonb, runcom, yuqi-zhang 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 |
|
No worries at all! Thanks for the review :D |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
3 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@yuqi-zhang: 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. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
Today the master/worker MCP does not have any different labels.
This means that if e.g. a user wants to add a kubeletconfig, they
must first create a label for the pool and target the kubeletconfig
against the label. This PR creates a default label for master/worker
pools to skip that step.