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

Allow optionally setting node taints defined on the NodeFeatureRule CR #910

Merged
merged 3 commits into from
Dec 6, 2022

Conversation

fmuyassarov
Copy link
Member

@fmuyassarov fmuyassarov commented Oct 6, 2022

This PR enables setting node taints defined on NodeFeatureRule CR. User can
define a list of taints under spec.rule of the CR as in this example:

apiVersion: nfd.k8s-sigs.io/v1alpha1
kind: NodeFeatureRule
metadata:
  name: rule
spec:
  rules:
    - name: "rule"
      labels:
        "my-sample-feature": "true"
      taints:
        - effect: PreferNoSchedule
          key: "feature.node.kubernetes.io/special-node"
          value: "true"
        - effect: NoExecute
          key: "feature.node.kubernetes.io/dedicated-node"
      matchFeatures:
        - feature: kernel.loadedmodule
          matchExpressions:
            dummy: {op: Exists}
$ kubectl get no kind-worker -ojson | jq .spec.taints
[
  {
    "effect": "PreferNoSchedule",
    "key": "feature.node.kubernetes.io/special-node",
    "value": "true"
  },
  {
    "effect": "NoExecute",
    "key": "feature.node.kubernetes.io/dedicated-node",
  }
]
$ kubectl get no kind-worker -ojson | jq .metadata.annotations | grep taint
  "nfd.node.kubernetes.io/taints": "feature.node.kubernetes.io/special-node=true:PreferNoSchedule,feature.node.kubernetes.io/dedicated-node:NoExecute",

The only allowed values for the effect are NoExecute, NoSchedule, PrefereNoSchedule
(following k8s). By default this feature is disabled and enabling it requires explicitly setting
--enable-taints flag true. Even if there are taints defined in the NodeFeatureRule CR and the
rule matches, taints will not be set on the node until flag is enabled.

NFD master updates the node object based on the addition/removal of taints in the CR.
Deleting the CR results in deletion of the taints too. nfd.node.kubernetes.io/taints annotation
is added to keep track of taints. The value of the new annotation is a list of taints in the format
of nfd.node.kubernetes.io/taints: "<key>=<value>:<effect>,<key>=<value>:<effect>".

Fixes: #540

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 6, 2022
@netlify
Copy link

netlify bot commented Oct 6, 2022

Deploy Preview for kubernetes-sigs-nfd ready!

Name Link
🔨 Latest commit 984a3de
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-nfd/deploys/638a19df59623f0008b6cd96
😎 Deploy Preview https://deploy-preview-910--kubernetes-sigs-nfd.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@fmuyassarov
Copy link
Member Author

Ops, seems I'm behind the master. I'll rebase. Actually I missed to the part where taints are deleted based on the deletion of the NodeFeatureRule CR or the specific taints on the CR (will add it shortly).

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 6, 2022
@fmuyassarov
Copy link
Member Author

/hold

@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 Oct 6, 2022
@fmuyassarov fmuyassarov force-pushed the taint/feruz branch 2 times, most recently from 2c6ed87 to f100b61 Compare October 8, 2022 22:56
@fmuyassarov
Copy link
Member Author

/cc @marquiz
I think it is ready for the review and meanwhile I will add a commit with unit tests.

@k8s-ci-robot k8s-ci-robot requested a review from marquiz October 8, 2022 23:08
@fmuyassarov
Copy link
Member Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 8, 2022
@fmuyassarov
Copy link
Member Author

/test pull-node-feature-discovery-build-image-cross-generic

@fmuyassarov
Copy link
Member Author

/cc @ArangoGutierrez @zvonkok

@k8s-ci-robot k8s-ci-robot requested a review from zvonkok October 10, 2022 14:15
Copy link
Contributor

@marquiz marquiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @fmuyassarov! I took a first quick pass (incomplete, not even going through all the changes, yet) and this looks really good already.

One suggestion/ask: could you submit the package renames

- whatever "k8s.io/api/core/v1"
+ corev1 "k8s.io/api/core/v1"

and other possible "beautifying" as separate PR(s). This will greatly reduce the "noise" in this PR and make review easier

pkg/apihelper/apihelpers.go Outdated Show resolved Hide resolved
docs/advanced/customization-guide.md Outdated Show resolved Hide resolved
pkg/apis/nfd/v1alpha1/annotations_labels.go Outdated Show resolved Hide resolved
docs/advanced/customization-guide.md Outdated Show resolved Hide resolved
docs/advanced/customization-guide.md Outdated Show resolved Hide resolved
pkg/apis/nfd/v1alpha1/expression.go Outdated Show resolved Hide resolved
pkg/nfd-master/nfd-master-internal_test.go Outdated Show resolved Hide resolved
@marquiz
Copy link
Contributor

marquiz commented Oct 13, 2022

BTW, now that #848 is in, we should also add some e2e coverage. But that can be done in a separate PR (and tracked in a separate issue), imo

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2022
@fmuyassarov
Copy link
Member Author

BTW, now that #848 is in, we should also add some e2e coverage. But that can be done in a separate PR (and tracked in a separate issue), imo

Yep, I'm on it now 😊

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 14, 2022
fmuyassarov added a commit to fmuyassarov/node-feature-discovery that referenced this pull request Dec 2, 2022
Extend current E2E tests to check tainting feature of nfd implemented
in kubernetes-sigs#910

Signed-off-by: Feruzjon Muyassarov <[email protected]>
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2022
Extend NodeFeatureRule Spec with taints field to allow users to
specify the list of the taints they want to be set on the node if
rule matches.

Signed-off-by: Feruzjon Muyassarov <[email protected]>
This commits extends NFD master code to support adding node taints
from NodeFeatureRule CR. We also introduce a new annotation for
taints which helps to identify if the taint set on node is owned
by NFD or not. When user deletes the taint entry from
NodeFeatureRule CR, NFD will remove the taint from the node. But
to avoid accidental deletion of taints not owned by the NFD, it
needs to know the owner. Keeping track of NFD set taints in the
annotation can be used during the filtering of the owner. Also
enable-taints flag is added to allow users opt in/out for node
tainting feature. The flag takes precedence over taints defined
in NodeFeatureRule CR. In other words, if enbale-taints is set to
false(disabled) and user still defines taints on the CR, NFD will
ignore those taints and skip them from setting on the node.

Signed-off-by: Feruzjon Muyassarov <[email protected]>
@marquiz
Copy link
Contributor

marquiz commented Dec 2, 2022

@fmuyassarov please rebase
ping @ArangoGutierrez

Signed-off-by: Feruzjon Muyassarov <[email protected]>
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2022
@fmuyassarov
Copy link
Member Author

@fmuyassarov please rebase ping @ArangoGutierrez

done

Copy link
Contributor

@marquiz marquiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@ArangoGutierrez ArangoGutierrez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 6, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ArangoGutierrez, fmuyassarov, marquiz

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 merged commit 9f68f6c into kubernetes-sigs:master Dec 6, 2022
@fmuyassarov fmuyassarov deleted the taint/feruz branch December 7, 2022 12:45
fmuyassarov added a commit to fmuyassarov/node-feature-discovery that referenced this pull request Dec 7, 2022
Extend current E2E tests to check tainting feature of nfd implemented
in kubernetes-sigs#910

Signed-off-by: Feruzjon Muyassarov <[email protected]>
fmuyassarov added a commit to fmuyassarov/node-feature-discovery that referenced this pull request Dec 12, 2022
Extend current E2E tests to check tainting feature of nfd implemented
in kubernetes-sigs#910

Signed-off-by: Feruzjon Muyassarov <[email protected]>
fmuyassarov added a commit to fmuyassarov/node-feature-discovery that referenced this pull request Dec 15, 2022
Extend current E2E tests to check tainting feature of nfd implemented
in kubernetes-sigs#910

Signed-off-by: Feruzjon Muyassarov <[email protected]>
fmuyassarov added a commit to fmuyassarov/node-feature-discovery that referenced this pull request Dec 15, 2022
Extend current E2E tests to check tainting feature of nfd implemented
in kubernetes-sigs#910

Signed-off-by: Feruzjon Muyassarov <[email protected]>
fmuyassarov added a commit to fmuyassarov/node-feature-discovery that referenced this pull request Dec 16, 2022
Extend current E2E tests to check tainting feature of nfd implemented
in kubernetes-sigs#910

Signed-off-by: Feruzjon Muyassarov <[email protected]>
fmuyassarov added a commit to fmuyassarov/node-feature-discovery that referenced this pull request Dec 17, 2022
Extend current E2E tests to check tainting feature of nfd implemented
in kubernetes-sigs#910

Signed-off-by: Feruzjon Muyassarov <[email protected]>
fmuyassarov added a commit to fmuyassarov/node-feature-discovery that referenced this pull request Dec 17, 2022
Extend current E2E tests to check tainting feature of nfd implemented
in kubernetes-sigs#910

Signed-off-by: Feruzjon Muyassarov <[email protected]>
fmuyassarov added a commit to fmuyassarov/node-feature-discovery that referenced this pull request Dec 19, 2022
Extend current E2E tests to check tainting feature of nfd implemented
in kubernetes-sigs#910

Signed-off-by: Feruzjon Muyassarov <[email protected]>
fmuyassarov added a commit to fmuyassarov/node-feature-discovery that referenced this pull request Dec 19, 2022
Extend current E2E tests to check tainting feature of nfd implemented
in kubernetes-sigs#910

Signed-off-by: Feruzjon Muyassarov <[email protected]>
fmuyassarov added a commit to fmuyassarov/node-feature-discovery that referenced this pull request Dec 19, 2022
Extend current E2E tests to check tainting feature of nfd implemented
in kubernetes-sigs#910

Signed-off-by: Feruzjon Muyassarov <[email protected]>
@marquiz marquiz mentioned this pull request Dec 20, 2022
22 tasks
yevgeny-shnaidman pushed a commit to yevgeny-shnaidman/node-feature-discovery that referenced this pull request Feb 1, 2023
This commit adds an argument to updateNodeFeatures method for receiving
client argument, which currently gets initialized within the method
itself. This is a minor improvement for kubernetes-sigs/node-feature-discovery#910.

Ref:kubernetes-sigs/node-feature-discovery#910 (comment)

Signed-off-by: Feruzjon Muyassarov <[email protected]>
yevgeny-shnaidman pushed a commit to yevgeny-shnaidman/node-feature-discovery that referenced this pull request Feb 1, 2023
Extend current E2E tests to check tainting feature of nfd implemented
in kubernetes-sigs/node-feature-discovery#910

Signed-off-by: Feruzjon Muyassarov <[email protected]>
cruizen pushed a commit to platform9/node-feature-discovery that referenced this pull request Mar 12, 2024
Extend current E2E tests to check tainting feature of nfd implemented
in kubernetes-sigs#910

Signed-off-by: Feruzjon Muyassarov <[email protected]>
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature to allow optionally setting taints based on node properties
4 participants