✨ Add Node managed labels support #7173

enxebre · 2022-09-06T11:54:49Z

What this PR does / why we need it:
Implements part of #6255. Propagate managed labels from Machines to Nodes.

/hold

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

sbueringer · 2022-09-06T12:11:09Z

cc @fabriziopandini

fabriziopandini · 2022-09-07T14:40:11Z

I will take a look at this ASAP but I'm getting to the idea that one way to get this moving is to start acting on machine -> node propagation, which seems already to get consensus and then tackling md changes in a separated PR.
cc @vincepri

fabriziopandini

@enxebre thanks for keeping this effort moving, as said in the office hour meeting, I'm planning to invest more time helping on this topic

api/v1beta1/machine_types.go

internal/controllers/machine/machine_controller_noderef.go

internal/controllers/machinedeployment/machinedeployment_controller.go

kfox1111 · 2022-09-08T19:03:24Z

Does this cover taints as well?

enxebre · 2022-09-20T08:51:11Z

Updated to scope the PR to propagate from Machines to Nodes. cc @fabriziopandini

enxebre · 2022-09-22T08:03:16Z

/test pull-cluster-api-e2e-main

fabriziopandini

@enxebre thanks for reducing the scope of this PR.

I think the only point to be addressed is about the label prefixes to be reconciled once.

The proposal states:

One-time Apply of Standard Kubernetes Labels

The machine controller will apply the standard kubernetes labels, if specified, to the Node immediately after the Node enters the Ready state (ProviderID is set). We will enforce this one-time application by making the labels immutable via a validating webhook.

WRT to the first part, I think that we should revert the order (first set labels if not already there, then set the provider ID) so the operation is re-entrant.

WRT to making those labels immutable via webhooks, I'm personally leaning toward not implementing it, because I feel we should leave admin the freedom to manage what we do not reconcile, but this is up for discussion.

I was also thinking that we can eventually also change CABPK so the labels are applied by kubeadm on node creation (and give a recommendation to all the bootstrap provider to do the same), but this could be also a follow up

api/v1beta1/machine_types.go

internal/controllers/machine/machine_controller_noderef.go

enxebre · 2022-09-23T10:43:52Z

WRT to the first part, I think that we should revert the order (first set labels if not already there, then set the provider ID) so the operation is re-entrant.

So as the PR it's right now labels are applied along with annotations as soon as the matching Node is found. Does this sounds good? If so I'll update the proposal.

WRT to making those labels immutable via webhooks, I'm personally leaning toward not implementing it, because I feel we should leave admin the freedom to manage what we do not reconcile, but this is up for discussion.

I think I agree.

f I got it right with this approach we are continuously reconciling all the managed prefixes, but according to the proposal only one of them should be reconciled and the other two should be applied once.
Q: Should we add a note that those labels are reconciled only on node creation? (same for node-restriction)
@fabriziopandini this contradicts the previous point.

As it's right now all managed labels would be continuously reconciled from Machines to Nodes. I think that's ok particularly from a UX pov.
As it's right now when propagating this change via MachineDeployment this will trigger a rolling upgrade effectively making labels to only be applied on creation.

If the above makes sense I'll update the proposal to reflect it back.

sbueringer · 2022-09-23T10:51:36Z

WRT to making those labels immutable via webhooks, I'm personally leaning toward not implementing it, because I feel we should leave admin the freedom to manage what we do not reconcile, but this is up for discussion.

The reason that was introduced in the proposal is just that it's (after inline-propagation is implemented) a bit awkward if you change one of those labels and they are never rolled out at all (except if you trigger the rollout e.g. with rolloutAfter which is not implemented as of today, not sure if there any alternatives to trigger that going forward).

But making it immutable via a webhook is also not really a solution for that. So probably comes down to documenting this and getting rolloutAfter implemented in the fullness of time.

As it's right now all managed labels would be continuously reconciled from Machines to Nodes. I think that's ok particularly from a UX pov.

I'm not sure I follow. Are we talking about continuously reconciling node.cluster.x-k8s.io/* + node-role.kubernetes.io/* + node-restriction.kubernetes.io/*?

If yes, how do we resolve this issue surfaced in the proposal?

It's important to note that the these specific labels are not kept in sync, but rather applied only once after the Machine and its corresponding Node are first created. Taking authoritative ownership of these prefixes would preclude other entities from adding and removing such labels on the Node.

Or is the idea that for now it's okay as it's effectively only done on create because we don't have in-place propagation and when we implement inline-propagation we will adjust the Machine=>Node sync accordingly?

enxebre · 2022-09-23T13:42:01Z

Or is the idea that for now it's okay as it's effectively only done on create because we don't have in-place propagation and when we implement inline-propagation we will adjust the Machine=>Node sync accordingly?

I was suggesting this and deviate from current proposal a bit and reconcile them all anytime in the lifecycle of the machine, If you don't want CAPI to take authoritative ownership of these prefixes, then just don't set the labels.
But happy to update the PR to something different.

api/v1beta1/machine_types.go

sbueringer

Just took a quick look

internal/controllers/machine/machine_controller_noderef.go

api/v1beta1/machine_types.go

internal/controllers/machine/machine_controller_noderef.go

enxebre · 2023-01-11T14:21:32Z

updated, tests currently failing because of kubernetes/client-go#992

sbueringer · 2023-01-11T18:51:35Z

updated, tests currently failing because of kubernetes/client-go#992

That's fun. I would assume that the client-go issue isn't resolved quick enough that we wouldn't be significantly blocked for this feature.

Given that, I would suggest we look for the best way to "fixup" the tests in a pragmatic way.

Just a first suggestion, what if we add a flag to the reconciler "disableNodeLabelSync" that we only set for our tests (+ corresponding godoc with link to the client-go issue of course)

(Otherwise lgtm +/- golangci-lint)

enxebre · 2023-01-11T19:13:17Z

Just a first suggestion, what if we add a flag to the reconciler "disableNodeLabelSync" that we only set for our tests (+ corresponding godoc with link to the client-go issue of course)

Makes sense, updated.

I actually think of this as a feature where we give users more control over when to update labels in a node pool by explicitly signalling it.

sbueringer · 2023-01-11T21:24:16Z

@enxebre looks like the linter has some findings

enxebre · 2023-01-12T10:09:47Z

@enxebre looks like the linter has some findings

Thanks! fixed.

Given the current PR implementation via SSA, I think we might want to re-consider again extending the functionality to enable arbitrary labels as API input. I think it should just work with SSA and it would enable easier adoption for use cases where arbitrary labels are dictating workloads topology distribution/affinity via gitops or so. That could be an additive follow up if we wanted to thought. Thoughts?

sbueringer · 2023-01-12T10:19:40Z

Given the current PR implementation via SSA, I think we might want to re-consider again extending the functionality to enable arbitrary labels as API input. I think it should just work with SSA and it would enable easier adoption for use cases where arbitrary labels are dictating workloads topology distribution/affinity via gitops or so. That could be an additive follow up if we wanted to thought. Thoughts?

I'm not sure if we would want to take all Machine labels and then propagate them through to the Node. Right now it's relatively obvious which ones are also going to the Nodes. I think if we want to consider propagating aribtrary labels through I would prefer doing this via a separate "node labels" field of some sort.

I'm open to discussing this in a follow-up issue, but I would keep the scope of the current PR to what we decided in the proposal.
(as I think your intention was as well)

sbueringer · 2023-01-12T10:21:47Z

Thx!
/lgtm
from my side.

k8s-ci-robot · 2023-01-12T10:21:53Z

LGTM label has been added.

Git tree hash: f824a33f165ff6e31bae16ac2f7bc05da6587ec0

enxebre · 2023-01-12T10:23:04Z

labels through I would prefer doing this via a separate "node labels" field of some sort.

Right, I was referring to reconsider having a dedicated field for label sync in Machines given that with field input + SSA we cover current PR use case but also enable any other. Yeh let's leave that discussion aside from this PR 👍🏾.

sbueringer · 2023-01-16T09:57:08Z

/assign @fabriziopandini @ykakarap

PTAL. I think this is pretty close to merge-ready.

ykakarap · 2023-01-17T20:22:38Z

/lgtm

Looks good.

fabriziopandini

great works! thanks @enxebre and @ykakarap for pushing this after the finish line!
/lgtm
/approve

fabriziopandini · 2023-01-20T14:37:11Z

internal/controllers/machine/machine_controller.go

@@ -91,6 +91,7 @@ type Reconciler struct {
 	// nodeDeletionRetryTimeout determines how long the controller will retry deleting a node
 	// during a single reconciliation.
 	nodeDeletionRetryTimeout time.Duration
+	disableNodeLabelSync     bool


As a follow-up let's open an issue to track the migration of tests to envclient and the removal of this flag (another option might be to split up the func and then unit test the individual parts vs testing the entire controller).

In the meantime, we can use a follow-up PR to add a comment on this field and make it explicit that it is only for allowing partial testing of the controller logic using the fake client (because features based on SSA cannot be tested with it).

cc @ykakarap

Follow-up PR to add the comment: #7965

Issue to migrate tests to envtest and drop the flag: #7964 (also added to the tracking issue).

k8s-ci-robot · 2023-01-20T14:38:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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:

~~OWNERS~~ [fabriziopandini]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 6, 2022

k8s-ci-robot requested review from fabriziopandini and stmcginnis September 6, 2022 11:54

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 6, 2022

enxebre mentioned this pull request Sep 6, 2022

📖 Label Sync Between MachineDeployment and underlying Kubernetes Nodes #6255

Closed

fabriziopandini reviewed Sep 8, 2022

View reviewed changes

enxebre force-pushed the node-labels branch from ee9bb3e to b0fbb61 Compare September 20, 2022 08:46

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 20, 2022

enxebre force-pushed the node-labels branch from b0fbb61 to 081a0a9 Compare September 20, 2022 08:49

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 20, 2022

enxebre changed the title ~~Add .nodeLabels support~~ ✨ Add Node managed labels support Sep 20, 2022

enxebre force-pushed the node-labels branch 2 times, most recently from e291634 to b3b9c83 Compare September 20, 2022 09:32

fabriziopandini reviewed Sep 22, 2022

View reviewed changes

api/v1beta1/machine_types.go Outdated Show resolved Hide resolved

internal/controllers/machine/machine_controller_noderef.go Outdated Show resolved Hide resolved

vincepri reviewed Sep 26, 2022

View reviewed changes

api/v1beta1/machine_types.go Outdated Show resolved Hide resolved

enxebre force-pushed the node-labels branch 2 times, most recently from 5cc2e73 to 83633c3 Compare September 27, 2022 11:54

sbueringer reviewed Sep 28, 2022

View reviewed changes

internal/controllers/machine/machine_controller_noderef.go Outdated Show resolved Hide resolved

internal/controllers/machine/machine_controller_noderef.go Outdated Show resolved Hide resolved

internal/controllers/machine/machine_controller_noderef.go Outdated Show resolved Hide resolved

sbueringer reviewed Sep 28, 2022

View reviewed changes

internal/controllers/machine/machine_controller_noderef.go Outdated Show resolved Hide resolved

fabriziopandini reviewed Oct 11, 2022

View reviewed changes

api/v1beta1/machine_types.go Outdated Show resolved Hide resolved

internal/controllers/machine/machine_controller_noderef.go Outdated Show resolved Hide resolved

enxebre force-pushed the node-labels branch from 83633c3 to bd6a5e9 Compare October 11, 2022 09:59

enxebre force-pushed the node-labels branch from cc872ef to f04a05d Compare January 11, 2023 14:19

enxebre force-pushed the node-labels branch from f04a05d to df1de51 Compare January 11, 2023 19:12

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 11, 2023

enxebre force-pushed the node-labels branch 2 times, most recently from e48114e to 9a4ffef Compare January 11, 2023 20:44

Add support to reconcile labels from Machines to Nodes

4bc8972

enxebre force-pushed the node-labels branch from 9a4ffef to 4bc8972 Compare January 12, 2023 08:48

k8s-ci-robot assigned sbueringer Jan 12, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 12, 2023

k8s-ci-robot assigned fabriziopandini and ykakarap Jan 16, 2023

fabriziopandini reviewed Jan 20, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 20, 2023

k8s-ci-robot merged commit 56aa8be into kubernetes-sigs:main Jan 20, 2023

k8s-ci-robot added this to the v1.4 milestone Jan 20, 2023

ykakarap mentioned this pull request Jan 20, 2023

🌱 add comment to disableNodeLabelSync #7965

Merged

musaprg mentioned this pull request Oct 6, 2023

Provide ability to set annotations on worker nodes #7409

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add Node managed labels support #7173

✨ Add Node managed labels support #7173

enxebre commented Sep 6, 2022 •

edited

Loading

sbueringer commented Sep 6, 2022

fabriziopandini commented Sep 7, 2022

fabriziopandini left a comment

kfox1111 commented Sep 8, 2022

enxebre commented Sep 20, 2022

enxebre commented Sep 22, 2022

fabriziopandini left a comment

enxebre commented Sep 23, 2022 •

edited

Loading

sbueringer commented Sep 23, 2022 •

edited

Loading

enxebre commented Sep 23, 2022 •

edited

Loading

sbueringer left a comment

enxebre commented Jan 11, 2023

sbueringer commented Jan 11, 2023 •

edited

Loading

enxebre commented Jan 11, 2023

sbueringer commented Jan 11, 2023

enxebre commented Jan 12, 2023

sbueringer commented Jan 12, 2023

sbueringer commented Jan 12, 2023

k8s-ci-robot commented Jan 12, 2023

enxebre commented Jan 12, 2023 •

edited

Loading

sbueringer commented Jan 16, 2023

ykakarap commented Jan 17, 2023

fabriziopandini left a comment

fabriziopandini Jan 20, 2023

ykakarap Jan 20, 2023

k8s-ci-robot commented Jan 20, 2023

✨ Add Node managed labels support #7173

✨ Add Node managed labels support #7173

Conversation

enxebre commented Sep 6, 2022 • edited Loading

sbueringer commented Sep 6, 2022

fabriziopandini commented Sep 7, 2022

fabriziopandini left a comment

Choose a reason for hiding this comment

kfox1111 commented Sep 8, 2022

enxebre commented Sep 20, 2022

enxebre commented Sep 22, 2022

fabriziopandini left a comment

Choose a reason for hiding this comment

One-time Apply of Standard Kubernetes Labels

enxebre commented Sep 23, 2022 • edited Loading

sbueringer commented Sep 23, 2022 • edited Loading

enxebre commented Sep 23, 2022 • edited Loading

sbueringer left a comment

Choose a reason for hiding this comment

enxebre commented Jan 11, 2023

sbueringer commented Jan 11, 2023 • edited Loading

enxebre commented Jan 11, 2023

sbueringer commented Jan 11, 2023

enxebre commented Jan 12, 2023

sbueringer commented Jan 12, 2023

sbueringer commented Jan 12, 2023

k8s-ci-robot commented Jan 12, 2023

enxebre commented Jan 12, 2023 • edited Loading

sbueringer commented Jan 16, 2023

ykakarap commented Jan 17, 2023

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini Jan 20, 2023

Choose a reason for hiding this comment

ykakarap Jan 20, 2023

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 20, 2023

enxebre commented Sep 6, 2022 •

edited

Loading

enxebre commented Sep 23, 2022 •

edited

Loading

sbueringer commented Sep 23, 2022 •

edited

Loading

enxebre commented Sep 23, 2022 •

edited

Loading

sbueringer commented Jan 11, 2023 •

edited

Loading

enxebre commented Jan 12, 2023 •

edited

Loading