fix(kubernetes): taint nodes on cluster upgrade

[maxime1907]

What type of PR is this?
/kind bug

What this PR does / why we need it:
Currently we are setting node taints through kubelet with the option --register-with-taints.
This works fine when first deploying a cluster but when we need to upgrade, it just does not work because they are only added at node registration.
This PR attempts to fix this issue by simply using kubectl to taint nodes like its done with labels:

• set node label via kubectl label command #5257

Does this PR introduce a user-facing change?:

fix(kubernetes): taint nodes on cluster upgrade

[k8s-ci-robot]

Hi @maxime1907. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[yankay]

/ok-to-test

[VannTen]

I don't think we need to add so much set_fact here, couldn't you use vars instead ?

Regarding using kubectl taint vs --register... Hum, is there no way to re-register on upgrade instead ?

Each new tasks means more playbooks run time.

[maxime1907]

I don't think we need to add so much set_fact here, couldn't you use vars instead ?

Regarding using kubectl taint vs --register... Hum, is there no way to re-register on upgrade instead ?

Each new tasks means more playbooks run time.

Oh right we can just do this:

1. remove the node from the cluster with kubectl delete node
2. restart kubelet

I tried this on a vanilla kubernetes 1.27.7 and it works!
It also fixes another problem that i was facing when updating the cloud_provider value on an already running node, it then re register correctly with the ccm
The only problem i see with this behavior is that it changes the node age since its re added to the cluster (with creationTimestamp)
Also it removes the taints/roles on master nodes

But i dont think its a good idea to delete the node because in case of failure it will be bad...
What are your thoughts on this ?

[VannTen]

I'm not sold on deleting the node either, I was more thinking along the lines of "Maybe there is a way to re-register without deleting". It might be wishful thinking on my end though ^

[maxime1907]

I'm not sold on deleting the node either, I was more thinking along the lines of "Maybe there is a way to re-register without deleting". It might be wishful thinking on my end though ^

Yeah i don't think that it's possible to re-register a node without deleting it:

• --register-with-taints not tainting my controller nodes kubernetes/kubernetes#45361 (comment)

So i guess we can go with this solution as for now

[maxime1907]

@VannTen Any updates on this?

[VannTen]

I'm on vacation, I'll take a look when I get back (end of next week).

[VannTen]

What bugs me is that I don't think this will erase old taints if you happen to remove them from your inventory... not very declarative.
OTOH it's better than the current situation where nodes just keep their labels forever.
/lgtm
/assign @floryut
wdyt ?
Forcing to delete the node to re-register it with new tains is probably a bit extreme... 🤔

[floryut]

What bugs me is that I don't think this will erase old taints if you happen to remove them from your inventory... not very declarative. OTOH it's better than the current situation where nodes just keep their labels forever. /lgtm /assign @floryut wdyt ? Forcing to delete the node to re-register it with new tains is probably a bit extreme... 🤔

Agree with you, this PR is better than the actual process, so let's go with it as is even if the limitation you are mentioning is indeed not perfect

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floryut, maxime1907

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 [floryut]

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