node-feature-discovery

node-feature-discovery copied to clipboard

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: NoSchedule
          key: foo
          value: abc
        - effect: NoSchedule
          key: bar
          value: abc
      matchFeatures:
        - feature: kernel.loadedmodule
          matchExpressions:
            dummy: {op: Exists}

$ kubectl get no kind-worker -ojson | jq .spec.taints
[
  {
    "effect": "NoSchedule",
    "key": "foo",
    "value": "abc"
  },
  {
    "effect": "NoSchedule",
    "key": "bar",
    "value": "abc"
  }
]
$ kubectl get no kind-worker -ojson | jq .metadata.annotations | grep taint
  "nfd.node.kubernetes.io/taints": "foo=abc:NoSchedule,bar=abc:NoSchedule",

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 --no-taint flag false. 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: https://github.com/kubernetes-sigs/node-feature-discovery/issues/540

✅ Deploy Preview for kubernetes-sigs-nfd ready!

Name	Link
🔨 Latest commit	984a3de19894cc3fb2ed65d2339335724ea498eb
🔍 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... Use your smartphone camera to open QR code link.

---

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

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).

/hold

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

/hold cancel

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

/cc @ArangoGutierrez @zvonkok

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

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 :blush:

@marquiz thank you for the feedback. Updated commits as following

• took out the unrelated changes
• docs update
• deletion function call update PTAL.

One thing I didn't like is the naming of the flag I have chosen. Because the to enable it we need to set the false and true to disable, which seems confusing. Maybe I should rename it to something like --enable-taints, and false means disabled while true means enabled.

One thing I didn't like is the naming of the flag I have chosen. Because the to enable it we need to set the false and true to disable, which seems confusing. Maybe I should rename it to something like --enable-taints, and false means disabled while true means enabled.

I agree on this. Especially as this won't be enabled by default any time soon (I think). I'd rename this to --enable-taints (it is prolly better than just --taints)

Updated the flag name.

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: fmuyassarov Once this PR has been reviewed and has the lgtm label, please assign marquiz for approval by writing /assign @marquiz in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

Rebased

Rebased

@marquiz I've addressed the comments. PTAL

@marquiz I've addressed the comments. PTAL

I will as soon as I find a suitable time slot 😅 Thanks @fmuyassarov for the update

@marquiz I've addressed the comments. PTAL

I will as soon as I find a suitable time slot 😅 Thanks @fmuyassarov for the update

you can definitely ignore it this week 😀

Rebased and addressed comments. @marquiz PTAL and let me know please if there more things we need to fix before merging. Thanks.

Thanks for the feedback @marquiz . Please take a look on the updated version.

/assign

@marquiz @ArangoGutierrez I've addressed latest comments, thanks a lot for your time on the review. I've previously used patching for updating taints and annotations. For some mysterious reason, the logic didn't work, either only one taint gets updated or annotations are not patched properly, or annotations are updated but taints are not removed... 🤯. We even debugged it once with @marquiz and I thought we found the problem (with if) but then there were more :). So, at the end what we have now is we update taints, then annotations and then update the Node object once. I think it is better than sending patch requests to the API server and at least it works fine than with patching😂 Please have a look, if looks good to you, I will squash the last commit because that one is for addressing comments.

EDIT: thanks to @marquiz the bug with patching is fixed now.

ping @ArangoGutierrez

@fmuyassarov please rebase ping @ArangoGutierrez

@fmuyassarov please rebase ping @ArangoGutierrez

done

[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:

• ~~OWNERS~~ [marquiz]

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