node-feature-discovery

node-feature-discovery copied to clipboard

What would you like to be added: Discover node features as annotations instead of labels

Why is this needed: Some features don't feel like they belong to labels – some extra info about topology or internal info, which won't be used for scheduling or in selection, but is nice to have on Node. Also, there are limits on label contents in contrast to annotations.

Hmm, we just talked about this with @fmuyassarov yesterday (iirc) and I said that yeah, it should be fairly straightforward but nobody has requested it and so there's prolly no use/need for it 😅

So, I don't oppose and I think it could be useful in some scenarios as you described. I'd say the way to support annotations would be through NodeFeatureRule objects. Patches are welcome 😊

there's more to discuss before submitting any patches though :)

Should annotations be supported in nfd-worker config, or should they be exclusive for NodeFeatureRoles? I think supporting them in NFRs only is enough

Should there be additional flag, like extraLabelNs? or should same "namespaces" be allowed as for labels implicitly? I think there should be different flag for annotations

What is the process to introduce new features, which cause changes to CRDs and documentation? I see that NFR CRD has version v1alpha1, so new fields can be probably added without any versioning changes, but I'm still asking to be sure

there's more to discuss before submitting any patches though :)

For sure 😓 And give others time to chime in, too.

Should annotations be supported in nfd-worker config, or should they be exclusive for NodeFeatureRoles? I think supporting them in NFRs only is enough

Good question. I was pondering this yesterday when thinking about #540. Whatever we do I think taints and annotations should do the same. I'm inclined to agree with you that NodeFeatureRule could be enough. Even if it would mean "imparaity" between NFR and the worker config and might cause some confusion. Implementing it in worker-config wouldn't be very complex but more work and LOC in any case.

Should there be additional flag, like extraLabelNs? or should same "namespaces" be allowed as for labels implicitly? I think there should be different flag for annotations

The prefix (or annotation namespace) that we currently use in NFD is nfd.node.kubernetes.io/. I think allowing extra prefixes/namespaces should definitely be a separate option.

What is the process to introduce new features, which cause changes to CRDs and documentation? I see that NFR CRD has version v1alpha1, so new fields can be probably added without any versioning changes, but I'm still asking to be sure

We haven't gone through K8s API review (yet) so at this point we can do it inside NFD project. Especially backwards-compatible changes (like adding annotations: (and possibly annotationsTemplate) field into the CRD is simple.

@marquiz I'm also thinking taints and annotations are related and can be implemented in a similar way

I've posted some thoughts on it on slack, copying here too:

we’d also love to be able to add taints based on node features, so maybe both annotations and taints should be done together?

I’m thinking of:

• annotations, which would be map[string]string , with key being name and value being value

• annotationsTemplate, which would be a string , and template would be parsed as lines of name=value Ownership would be stored in annotation nfd.node.kubernetes.io/feature-annotations as list of annotation names extraAnnotationNs would be used to allow extra “namespaces” for annotations in addition to standard one

• taints which would be []Taint, with Taint having key, value and effect , or just []string, with each element being a taint in the format kubectl expects for taint command

• taintsTemplate which would be a string , and templated value would be parsed as lines of KEY=VALUE:EFFECT - in the format kubectl expects for taint command as of ownership - I believe specific keys should be owned by nfd as a whole, instead of specific combinations of key+value+effect extraTaintNs would be used to allow extra “namespaces” for taints in addition to standard one

The prefix (or annotation namespace) that we currently use in NFD is nfd.node.kubernetes.io/

What about using feature.node.kubernetes.io/ or some other value instead, and explicitly disallowing annotations with prefix nfd.node.kubernetes.io/ – those are used for bookkeeping and other internals, so allowing user-generated annotations in that namespace would make it impossible to safely add new annotations for internal usage or bookkeeping

Hi @stek29 . I'm about to start working on feature to allow optionally setting taints based on node properties. Can you please confirm that you are not working on adding taints and only annotations so that I don't interfere with your work in case you are planning to ?

@fmuyassarov I'm not working on taints since annotations haven't been looked at anyway, I'm waiting on review on annotations.

@fmuyassarov I'm not working on taints since annotations haven't been looked at anyway, I'm waiting on review on annotations.

thanks for confirming, I will then try to submit a patch for the taints soon.

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

/remove-lifecycle stale I shall come back to it very soon.

/remove-lifecycle stale

@VillePihlava have you been working on this?

@VillePihlava have you been working on this?

A more urgent task came up, so I'm currently not working on this.

Just wrote and outline of sub-tasks related to extended resources (in #1081). Annotations should follow mostly the same steps, copy-pasting them here:

1. Extend the NodeFeatureRule CRD API and helpers to support annotations
   1. Add new annotations field to NodeFeatureRuleSpec in pkg/apis/nfd/v1alpha1/types.go
   2. Run make generate to update yamls and auto-generated code
   3. update of the RuleOutput type (and related methods/funcs) in pkg/apis/nfd/v1alpha1/rule.go to return also snnotations (in addition to labels and taints)
2. Implement support in nfd-master (not super complex as the basic infrastructure for handling annotations is already in place)
   1. update processNodeFeatureRule() to return annotations
   2. update refreshNodeFeatures() to correspondingly update the node annotations
   3. add new special annotation like [nfd.node.kubernetes.io/feature-annotations](http://nfd.node.kubernetes.io/feature-annotations%60) to keep track on these feature annotations (originating from NodeFeatureRules) managed by NFD, again a new const in pkg/apis/nfd/v1alpha1/annotations_labels.go
3. Documentation: at least introduction.md and customization-guide.md
4. Extend e2e-tests to cover annotations

Few extra considerations regarding "namespacing" i.e. prefixing of names of annotations:

1. We probably want to specify a "default namespace" that gets applied if no namespace (i.e. <namespace>/ prefix is give). Basically add a new const like FeatureAnnotationNs in pkg/apis/nfd/v1alpha1/annotations_labels.go. Could be e.g. the same as with labels i.e. feature.node.kubernetes.io
2. It might be best to just allow any vendor specific namespace, but we want to deny kubernetes.io/ and its sub-namespaces (*.kubernetes.io)
3. In addition we need to make sure that the special nfd-annotations like FeatureLabelsAnnotation MasterVersionAnnotation cannot be overridden

/help

EDIT: fixed misspellings

@marquiz: This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed by commenting with the /remove-help command.

In response to this:

Just wrote and outline of sub-tasks related to extended resources (in #1081). Annotations should follow mostly the same steps, copy-pasting them here:

1. Extend the NodeFeatureRule CRD API and helpers to support annotations
   1. Add new annotations field to NodeFeatureSpec in pkg/apis/nfd/v1alpha1/types.go
   2. Run make generate to update yamls and auto-generated code
   3. update of the RuleOutput type (and related methods/funcs) in pkg/apis/nfd/v1alpha1/rule.go to return also snnotations (in addition to labels and taints)
2. Implement support in nfd-master (not super complex as the basic infrastructure for handling annotations is already in place)
   1. update processNodeFeatureRule() to return annotations
   2. update refreshNodeFeatures() to correspondingly update the node annotations
   3. add new special annotation like [nfd.node.kubernetes.io/feature-annotations](http://nfd.node.kubernetes.io/feature-annotations%60) to keep track on these feature annotations (originating from NodeFeatureRules) managed by NFD, again a new const in pkg/apis/nfd/v1alpha1/annotations_labels.go
3. Documentation: at least introduction.md and customization-guide.md
4. Extend e2e-tests to cover annotations

Few extra considerations regarding "namespacing" i.e. prefixing of names of annotations:

1. We probably want to specify a "default namespace" that gets applied if no namespace (i.e. <namespace>/ prefix is give). Basically add a new const like FeatureAnnotationNs in pkg/apis/nfd/v1alpha1/annotations_labels.go. Could be e.g. the same as with labels i.e. feature.node.kubernetes.io
2. It might be best to just allow any vendor specific namespace, but we want to deny kubernetes.io/ and its sub-namespaces (*.kubernetes.io)
3. In addition we need to make sure that the special nfd-annotations like FeatureLabelsAnnotation MasterVersionAnnotation cannot be overridden

/help

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.

If no one working on this issue, maybe I can do it.

If no one working on this issue, maybe I can do it.

That would be great! I think it's yours.

You can take a look at #910 (implemented support for taints) for some inspiration although the management of annotations in nfd-master will be much simpler than taints

/assign

Just wrote and outline of sub-tasks related to extended resources (in #1081). Annotations should follow mostly the same steps, copy-pasting them here:

1. Extend the NodeFeatureRule CRD API and helpers to support annotations

   1. Add new annotations field to NodeFeatureSpec in pkg/apis/nfd/v1alpha1/types.go
   2. Run make generate to update yamls and auto-generated code
   3. update of the RuleOutput type (and related methods/funcs) in pkg/apis/nfd/v1alpha1/rule.go to return also snnotations (in addition to labels and taints)

2. Implement support in nfd-master (not super complex as the basic infrastructure for handling annotations is already in place)

   1. update processNodeFeatureRule() to return annotations
   2. update refreshNodeFeatures() to correspondingly update the node annotations
   3. add new special annotation like [nfd.node.kubernetes.io/feature-annotations](http://nfd.node.kubernetes.io/feature-annotations%60) to keep track on these feature annotations (originating from NodeFeatureRules) managed by NFD, again a new const in pkg/apis/nfd/v1alpha1/annotations_labels.go

3. Documentation: at least introduction.md and customization-guide.md

4. Extend e2e-tests to cover annotations

Few extra considerations regarding "namespacing" i.e. prefixing of names of annotations:

1. We probably want to specify a "default namespace" that gets applied if no namespace (i.e. <namespace>/ prefix is give). Basically add a new const like FeatureAnnotationNs in pkg/apis/nfd/v1alpha1/annotations_labels.go. Could be e.g. the same as with labels i.e. feature.node.kubernetes.io
2. It might be best to just allow any vendor specific namespace, but we want to deny kubernetes.io/ and its sub-namespaces (*.kubernetes.io)
3. In addition we need to make sure that the special nfd-annotations like FeatureLabelsAnnotation MasterVersionAnnotation cannot be overridden

/help

@marquiz I think NodeFeatureSpec is NodeFeatureRuleSpec in "1. Add new annotations field to NodeFeatureSpec in pkg/apis/nfd/v1alpha1/types.go",if we want to extend the NodeFeatureRule CRD API.

The CR example maybe like this.

apiVersion: nfd.k8s-sigs.io/v1alpha1
kind: NodeFeatureRule
metadata:
  name: rule
spec:
  rules:
    - name: "rule"
      annotations:
        "nfd.node.kubernetes.io/feature-annotations": "my-sample-feature"
      matchFeatures:
        - feature: kernel.loadedmodule
          matchExpressions:
            dummy: {op: Exists}

And I don't understand the considerations of "namespacing",can you give me some examples?

I think NodeFeatureSpec is NodeFeatureRuleSpec in "1.

Yeah, correct, I misspelled it.

And I don't understand the considerations of "namespacing",can you give me some examples?

With "namespacing" here I mean the <prefix>/ of the name of the annotation. So if you specify just

      annotations:
        "foo": "bar"

(i.e. without any namespace/prefix) then NFD would add a default prefix and create something like nfd.node.kubernetes.io/foo=bar. And, we'd deny creating annotations prefixed with kubernetes.io/

Let's push this to v0.14

/milestone v0.14.0

@ArangoGutierrez let's drop this from v0.14, right?

Yup, I don't have the bandwidth for this after summer break

Let's push this to v0.15

/milestone v0.15.0

@ArangoGutierrez: The provided milestone is not valid for this repository. Milestones in this repository: [v0.14, v0.15]

Use /milestone clear to clear the milestone.

In response to this:

Let's push this to v0.15

/milestone v0.15.0

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.

/milestone v0.15

/assign