📖 Propagating taints from Cluster API to Nodes #12329
Conversation
Signed-off-by: Nolan Brubaker <[email protected]>
k8s-ci-robot
added
area/machine
k8s-ci-robot
added
the
size/L
|
@nrb should we tag some people to review the taints proposal? I am going to tag in the ones that were participating to the original discussion over at #11175 for a review /assign @fabriziopandini @sbueringer @chrischdi @JoelSpeed @richardcase |
k8s-ci-robot
assigned
chrischdi,
fabriziopandini,
JoelSpeed,
richardcase and
sbueringer
|
In yesterday's CAPI Community meeting we talked about the state of this Open Proposal and the fact that it is still very much alive for us and we are still willing to make active progress on that. I think @fabriziopandini mentioned he wants to have a look at it IIRC. |
There was a problem hiding this comment.
We're not yet at that point but we should explicitly consider cluster-api taints to not be added via this mechanism / have validation for it.
As of today these are (if I looked it up correctly):
node.cluster.x-k8s.io/outdated-revision:PreferNoSchedulenode.cluster.x-k8s.io/uninitialized:NoSchedule
So we may want to block the whole node.cluster.x-k8s.io/ prefix from being set to also consider future taints.
Should we add "Supporting MachinePools without Machines" as a non-goal?
Edit 2025-10-14: fixed the above taints (I did write outdated-revision as NoSchedule but it is PreferNoScheduler).
|
@nrb It would be nice if we can close comments already addressed |
|
FYI: I am taking over some work here and pushed an update to this proposal :-) |
|
/retitle 📖 Propagating taints from Cluster API to Nodes |
k8s-ci-robot
changed the title
| Note: With this change we should also add the information to the "Contract rules for BootstrapConfig" documentation, that `Initialize` taints are only added when a bootstrap provider supports the `node.cluster.x-k8s.io/uninitialized`. | ||
|
|
||
| To keep track of which taints have been added by the controller, the controller also adds the annotation `cluster.x-k8s.io/taints-from-machine` to the Node. | ||
| This follows the convention established by `cluster.x-k8s.io/labels-from-machine`. |
There was a problem hiding this comment.
trying to create a hypothetical scenario (which might also be a problem for labels and other propagated fields),
if a set of taints are propagated to the node by a CAPI controller, but then a third party controller changes one of the taints. now we have set of taints that a tracked as owned by CAPI according to the annotation, but would that be the accurate state of the world, given the third party controller made a change too?
There was a problem hiding this comment.
additionally, what happens if the CAPI users starts attempting to propagate common Taints like those that are managed by the core K8s Node controller?
There was a problem hiding this comment.
As we are continuously writing the always taints it would be accurate.
We should probably block some more taints (probably entire domains like we do for labels and annotations)
There was a problem hiding this comment.
I agree, we should block taints we know that only causes trouble.
I think for other cases we could document (in this proposal? in the book?) how to add a ValidatingAdmissionPolicy to deny setting certain taints.
(Alternative: adding a flag which configures what taints are allowed, but I think with support for VAPs in all supported management cluster versions we are already good?)
There was a problem hiding this comment.
In general it makes sense to me to have an additional flag for taint sync, just like we have for labels & annotations. I think we just need a bit of discussion about what should or shouldn't be synced per default.
I think for taints the default could be to sync everything apart from a few taints that we always want to block (and then also actually block them in the validating webhook of our objects). For labels/annotations the considerations were more complicated as labels/annotations are also for a high number of other objects and we didn't want to just per default propagate all labels/annotations onto Nodes.
There was a problem hiding this comment.
Yes. Let's update the CP provider contract. Good catch!
There was a problem hiding this comment.
Added a contract PR: #12893
And updated the proposal
There was a problem hiding this comment.
- block the taints
node-role.kubernetes.io/control-planeandnode-role.kubernetes.io/master(deprecated) for control-plane Machines (identified by existence of the labelcluster.x-k8s.io/control-plane) and for MachineDeployments and MachineSets.
I think we wanted to allow them for CP Machines.
Let's block node-role.kubernetes.io/master entirely. The new taint was set since Kubernetes 1.24. I don't want to enable anyone to continue to use this taint (xref: #3279 (comment))
There was a problem hiding this comment.
Thanks for verifying again! updated!
There was a problem hiding this comment.
Thx. All good from my side. I'll leave this conversation open in case someone else wants to comment further
| Note: With this change we should also add the information to the "Contract rules for BootstrapConfig" documentation, that `Initialize` taints are only added when a bootstrap provider supports the `node.cluster.x-k8s.io/uninitialized`. | ||
|
|
||
| To keep track of which taints have been added by the controller, the controller also adds the annotation `cluster.x-k8s.io/taints-from-machine` to the Node. | ||
| This follows the convention established by `cluster.x-k8s.io/labels-from-machine`. |
There was a problem hiding this comment.
If we introduce a sort of list of taint that the users should not set, I would also block them also in the webhooks (might be with "ratcheting" to prevent objects to become invalid when the list changes)
|
|
||
| ##### Type definition of a taint in Cluster API objects | ||
|
|
||
| Note: To reduce verbosity, this proposal does not include all kinds of validation markers. |
There was a problem hiding this comment.
Perhaps create a WIP API PR where the validation markers can be demonstrated in full and reviewed? Link that here?
There was a problem hiding this comment.
Adjusted the code examples here with validation markers for review :-)
There was a problem hiding this comment.
I think we're okay for this PR
Let's just do the full review on the PR after this proposal is merged and then sync it back here (or rather just reference from here to the implementation instead of duplicating the code)
I think the types are good enough for the purpose of the proposal
| This follows the convention established by `cluster.x-k8s.io/labels-from-machine`. | ||
|
|
||
| The taints will be concatenated with `,` and the serialization will look like this given the 4 ways a taint can be specified: | ||
| `cluster.x-k8s.io/taints-from-machine:<key1>=<value1>:<effect1>,<key2>=<value2>:,<key3>:<effect3>,<key4>` |
There was a problem hiding this comment.
Do we need to delineate in this whether a taint is OnInitialization or not?
There was a problem hiding this comment.
Updated the doc (pending push): We only track Always taints as detailed as this.
For OnInitialization knowing we are done is enough (existence of the annotation).
Also now only using key:effect as syntax because these are the map keys. No need to add the value.
Doc should now reflect that.
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: f007fabc73f2b0d850c970782b0ec0e836ae50cd
|
|
great work! I'm also +1 to discuss a lazy consensus deadline for this proposal in the next office hours |
k8s-ci-robot
removed
the
lgtm
k8s-ci-robot
requested review from
JoelSpeed,
chrischdi,
fabriziopandini and
sbueringer
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 124a3a8370bf6062acba5fd8c50735fa4be13e1a
|
|
/lgtm |
|
we are talking about this issue during the office hours today, @chrischdi is suggesting a lazy consensus period for this PR of 1 week. if there are no further suggestions or comments we will plan to merge this by 5 november. |
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
Couple of nits, but otherwise LGTM
|
|
||
| If a Node re-registers itself, the controller would add the `OnInitialization` taints again in addition to the `Always` taints because the tracking annotation does not exist. | ||
|
|
||
| **Note:** If a bootstrap provider supports the `node.cluster.x-k8s.io/uninitialized` taint as documented in [Contract rules for BootstrapConfig](https://cluster-api.sigs.k8s.io/developer/providers/contracts/bootstrap-config#taint-nodes-at-creation), the implementation ensures that all `OnInitialization` taints are applied in the same call through which the `node.cluster.x-k8s.io/uninitialized` taint is getting removed. This prevents that workload without proper tolerations could be scheduled to the node in the time between node creation and Cluster API adding the `OnInitialization` taints. |
There was a problem hiding this comment.
I don't think this needs to be in the same call, but it does need to be before the uninitialized taint is removed
There was a problem hiding this comment.
Yup. Doesn't strictly have to be. But from an implementation perspective it doesn't make sense to separate this into a separate call (we are already updating labels/annotations/taints in one single call)
There was a problem hiding this comment.
Agree that it doesn't need to be, but also it does make sense when taking a look at the code to make it at the same point in time to not require an additional patch/update call.
There was a problem hiding this comment.
Different bootstrap providers may structure their code differently, I would recommend talking about the strict requirements rather than what makes sense for a particular provider implementation, even if that is the "reference" implementation
There was a problem hiding this comment.
This is not referring to something a bootstrap provider has to do. This is referring to how we implement it in the Machine controller (and we don't have a contract for that one :)). Maybe should make that clearer though :)
There was a problem hiding this comment.
Ahh... yeah I misread who the actor was there, ack
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 7289ac91ee6b255e319ef413f22d3688dc598621
|
|
/lgtm |
|
As discussed last week, merging this PR before today's office hours kudos to @nrb and @chrischdi for keeping working on this proposal |
|
[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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
Note: Implementation will be tracked in: |
10 participants
What this PR does / why we need it:
This PR contains a proposal to allow Cluster API to sync taints from resources referencing a MachineTemplate to the managed Machines and ultimately Nodes.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):In support of #11175
/area machine
/area machineset
/area machinedeployment
/area machinepool