Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make kubespray authoritative on node taints #11697

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

VannTen
Copy link
Contributor

@VannTen VannTen commented Nov 8, 2024

What type of PR is this?
/kind feature

What this PR does / why we need it:
Currently, we only add/modify taints to nodes (not remove). This mean is
users remove taints from their kubespray inventories, they also have to
remove them manually from their clusters.

Switch to replacing the entire taints array by patching 'spec.taints';
we do preserve Kubernetes reserved taints
(https://kubernetes.io/docs/reference/labels-annotations-taints/).

The string from for providing the annotations is more complicated to
manipulate, in kubespray or in users inventory.

Deprecate the string form in favor of reusing the structure of the
Kubernetes API. We keep a compatibily layer which parse the string
on-the-fly, which we should remove in the N+1 release (N=next relase)

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
Kinda follow-up to #10705 @maxime1907 would love to hear your thoughts

Regarding the nvidia bits. Setting a boolean is marginally easier, but why not just ask users to have the nvidia taints in their group_vars for their nvidia nodes and just scrap those variables ? => and just add that to documentation ? Wdyt ?

Does this PR introduce a user-facing change?:

action required
Using `node_taints` as a string array is deprecated. See roles/kubernetes/node-taint/defaults/main.yml for the new format. 
Kubespray will now be authoritative on node taints, which means taints not defined in kubespray will be removed (except kubernetes.io/k8s.io prefixed taints)
The variable `nvidia_gpu_nodes` is not used anymore to set node taints. Instead, directly [set them in your inventory](docs/manage_your_inventory/dedicated_node_groups.md)

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 8, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: VannTen

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2024
@VannTen
Copy link
Contributor Author

VannTen commented Nov 8, 2024

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Nov 8, 2024
@maxime1907
Copy link
Contributor

nice finally a true config as code

for the nvidia part i would just remove it and add some documentation to avoid overengineering stuff

@VannTen VannTen marked this pull request as ready for review November 9, 2024 13:17
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 9, 2024
@VannTen
Copy link
Contributor Author

VannTen commented Nov 9, 2024

/label tide/merge-method-merge

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-merge Denotes a PR that should use a standard merge by tide when it merges. label Nov 9, 2024
Currently, we only add/modify taints to nodes (not remove). This mean is
users remove taints from their kubespray inventories, they also have to
remove them manually from their clusters.

Switch to replacing the entire taints array by patching 'spec.taints';
we do preserve Kubernetes reserved taints
(https://kubernetes.io/docs/reference/labels-annotations-taints/).

The string from for providing the annotations is more complicated to
manipulate, in kubespray or in users inventory.

Deprecate the string form in favor of reusing the structure of the
Kubernetes API. We keep a compatibily layer which parse the string
on-the-fly, which we should remove in the N+1 release (N=next relase)
@VannTen
Copy link
Contributor Author

VannTen commented Nov 10, 2024 via email

@VannTen
Copy link
Contributor Author

VannTen commented Nov 12, 2024

The answer to this last question is "not yet" kubernetes/kubernetes#117142

@VannTen
Copy link
Contributor Author

VannTen commented Nov 13, 2024 via email

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/feature Categorizes issue or PR as related to a new feature. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tide/merge-method-merge Denotes a PR that should use a standard merge by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants