Repository: kubernetes-sigs/node-feature-discovery
Pull request #291 (2020-02-19 13:34:11)

master: add extended resource support
@marquiz 
#229 

This adds support for making selected labels extended resources.

Signed-off-by: Ukri Niemimuukko <ukri.niemimuukko@intel.com>

k8s-ci-robot: Welcome @uniemimu! <br><br>It looks like this is your first PR to <a href='https://github.com/kubernetes-sigs/node-feature-discovery'>kubernetes-sigs/node-feature-discovery</a> 🎉. Please refer to our [pull request process documentation](https://git.k8s.io/community/contributors/guide/pull-requests.md) to help your PR have a smooth ride to approval. <br><br>You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. [Here is the bot commands documentation](https://go.k8s.io/bot-commands). <br><br>You can also check if kubernetes-sigs/node-feature-discovery has [its own contribution guidelines](https://github.com/kubernetes-sigs/node-feature-discovery/tree/master/CONTRIBUTING.md). <br><br>You may want to refer to our [testing guide](https://git.k8s.io/community/contributors/devel/sig-testing/testing.md) if you run into trouble with your tests not passing. <br><br>If you are having difficulty getting your pull request seen, please follow the [recommended escalation practices](https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#why-is-my-pull-request-not-getting-reviewed). Also, for tips and tricks in the contribution process you may want to read the [Kubernetes contributor cheat sheet](https://git.k8s.io/community/contributors/guide/contributor-cheatsheet/README.md). We want to make sure your contribution gets all the attention it needs! <br><br>Thank you, and welcome to Kubernetes. :smiley:
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/node-feature-discovery/pull/291#" title="Author self-approved">uniemimu</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **balajismaniam**
You can assign the PR to them by writing `/assign @balajismaniam` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fnode-feature-discovery).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-sigs/node-feature-discovery/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["balajismaniam"]} -->
uniemimu: > What I was wondering whether it makes sense to create the label at all if the feature is published as an extended resource. My first thought is that it doesn't as they are sort of overlapping mechanisms. WDYT?

The rationale for the current way of having the label at the same time was:
1) smaller change, no need to filter out anything while adding labels
2) it doesn't hurt, and somebody might come with a use-case where the label is wanted anyway. Suppose you have nodes with an extended resource Foo with capacity value 1024, and some other nodes with the extended resource Foo with capacity value 2048. Also suppose that for some reason, you want some special POD which consumes 1000 Foos to be only deployed to the nodes which have a capacity of 2048 Foos, because those nodes are so much better. Now you could easily use a node selector for that, if the label is there also. If you leave it only for the scheduler, it can pick the wrong nodes and you need to figure out something else like another label. A corner case, but maybe plausible.


marquiz: > 1. smaller change, no need to filter out anything while adding labels

Mm, I understand :)


> 2. it doesn't hurt, and somebody might come with a use-case where the label is wanted anyway.

My motive was limiting unnecessary labels.

> Suppose you have nodes with an extended resource Foo with capacity value 1024, and some other nodes with the extended resource Foo with capacity value 2048. Also suppose that for some reason, you want some special POD which consumes 1000 Foos to be only deployed to the nodes which have a capacity of 2048 Foos, because those nodes are so much better.

I understand the thinking, but I don't know how valid use case this is and what would be the right thing to do. In the case you presented it might be good to have some other label indicating the superiority of the other node(?) OTOH, this new feature (i.e. turning labels into extended resources) needs to be explicitly enabled and probably only used in specialised deployments so having few extra labels doesn't hurt.
marquiz: @zvonkok @fabiand @mmirecki any thoughts about this PR?
zvonkok: > The rationale for the current way of having the label at the same time was:
> 
> 1. smaller change, no need to filter out anything while adding labels
> 2. it doesn't hurt, and somebody might come with a use-case where the label is wanted anyway. Suppose you have nodes with an extended resource Foo with capacity value 1024, and some other nodes with the extended resource Foo with capacity value 2048. Also suppose that for some reason, you want some special POD which consumes 1000 Foos to be only deployed to the nodes which have a capacity of 2048 Foos, because those nodes are so much better. Now you could easily use a node selector for that, if the label is there also. If you leave it only for the scheduler, it can pick the wrong nodes and you need to figure out something else like another label. A corner case, but maybe plausible.

I am just wondering if a Foo:2048 is so much better than a Foo:1000, what features make these nodes "better". Is it the device that is exposed or the combination of the device + node features? 

If it is a different Foo device, this is maybe already exposed through PCI vendor,class,device.
e.g. older GPUs with lower CUDA core count have a class id 0x300, 
newer GPUs with higher CUDA core count have a class id 0x302 . 

If it is a feature of the node, what would this be that NFD is currently not exposing? 

Any specific use-case you have in mind? What was the rationale in using extended resources? 
An example would help to understand your point. 





uniemimu: > I am just wondering if a Foo:2048 is so much better than a Foo:1000, what features make these nodes "better". Is it the device that is exposed or the combination of the device + node features?
> 
> If it is a different Foo device, this is maybe already exposed through PCI vendor,class,device.
> e.g. older GPUs with lower CUDA core count have a class id 0x300,
> newer GPUs with higher CUDA core count have a class id 0x302 .
> 
> If it is a feature of the node, what would this be that NFD is currently not exposing?
> 
> Any specific use-case you have in mind? What was the rationale in using extended resources?
> An example would help to understand your point.

I'm not really claiming bigger to be always better, sometimes you may want just the opposite. That was just an example, one could want to prefer the nodes with less resources for some PODs and keep the nodes with a huge amount of resources for some other PODs that really need huge amount of resources. I'm just trying to describe plausible scenarios. The resource capacity can form a fingerprint for the hardware just like something encoded in the PCI class may do.

Sometimes you may have a difference in the PCI information, sometimes maybe not. Sometimes you might get that fingerprint you need, from e.g. an accelerator memory amount exposed through NFD-hooks and labels.

This mechanism in this PR then is just a scheduling aid for keeping book that the extended resource doesn't run out, but like said, I can imagine someone having a need for the label at the same time to further control where different PODs get deployed.

The rationale for this PR comes from the need of sharing accelerator hardware for multiple PODs in a node with known consumption of the accelerator memory resources per POD and a limited amount of said memory resources in the accelerator at each node.
zvonkok: /assign @zvonkok 
zvonkok: @marquiz I am still trying to wrap my head around about the extended resources. If a resource is countable we can use extended resources but isn't that a job for the DevicePlugin? If a resource is infinite we can use a label. 

A feature depending on the amount or count of the specific hardware is as specific asset of this hardware, for base detection, I am in favour of just having the extended resource, and if you need specific labels for this specific hardware to steer scheduling you could create namespaced labels that are created by using the hooks or feature capability of NFD. 

Taking the GPU as an example: 

1. NFD labels the node with the PCI class and vendor id: `pci-0300_10de.present = true`
2. Deploy the NV enablement stack on nodeSelector  `pci-0300_10de.present = true`
   (now we have the driver and can extract sophisticated features about the hw)
3. Deploy GPU feature discovery to expose scheduling features (sidecar container using the feature hook) 
```
   nvidia.com/memory = xxxx
   nvidia.com/driver-version = xxxx
   nvidia.com/compute-capability = xxxx
```

Now you can steer your workload on the best `Foo`. 

The DevicePlugin takes care of exposing the count to the cluster and marking unhealthy cards as not usable. 

I am still thinking about a real-world use-case to use extended resources ... 













uniemimu: >    nvidia.com/memory = xxxx
>    nvidia.com/driver-version = xxxx
>    nvidia.com/compute-capability = xxxx
> ```
> 
> Now you can steer your workload on the best `Foo`.
> 
> The DevicePlugin takes care of exposing the count to the cluster and marking unhealthy cards as not usable.
> 
> I am still thinking about a real-world use-case to use extended resources ...

Taking your own example here as a baseline, you could have a use-case where you know the maximum nvidia.com/memory consumption for your PODs, and you would like to have several PODs being run on a single GPU in order to get the GPU utilization up, limited only by their memory consumption. Now with the extended resource you could do that without risking running out of memory, provided that you have the POD spec written correctly. The standard approach of course is that the GPU is not shared between POD instances like that, but suppose it could be.

You are correct in that the plugin could probably do whatever kind of capability labeling needed, it doesn't have to be the same name as the extended resource. And those could then be used for scheduling purposes.
mythi: > I am still thinking about a real-world use-case to use extended resources ...

Another example from me is similar to what @uniemimu had: SGX

The workloads need the access to `/dev/sgx/enclave` (a shared device) and they can specify how much SGX EPC memory they need. The amount of memory depends on the workload. 

The existing SGX device plugins out there do the SGX EPC book keeping with 1Mb granularity. A workload needing, let's say 64Mb SGX EPC would result in 64 `Allocate()`  and still get that one shared file.

With this feature, extended resources, and a simple NFD source plugin, such as 

```
package main

import (
        "fmt"
        "github.com/klauspost/cpuid"
)

func main() {
        var total uint64 = 0
        if cpuid.CPU.SGX.Available {
                for _, s := range cpuid.CPU.SGX.EPCSections {
                        total += s.EPCSize
                }
                if total != 0 {
                        fmt.Printf("sgx.intel.com/epc=%d", total)
                }
        }
}
```

We could have

```
sgx.intel.com/device: <some big value>
sgx.intel.com/epc: <extended resources reported by NFD source plugin>
feature.node.kubernetes.io/cpu-cpuid.SGX: "true"
feature.node.kubernetes.io/cpu-cpuid.SGXLE: "true"
```

and EPC would be a simple counter...
marquiz: > If a resource is countable we can use extended resources but isn't that a job for the DevicePlugin?

@uniemimu please correct me if I'm wrong, but AFAIU, the desire to use NFD instead of device plugins is the problematic device plugin API. With my limited understanding of device plugins I assume that for every resource unit you will get one `allocate()` or similar calls which would be unbearable if you have like `8Gi` units of a resource.


> A feature depending on the amount or count of the specific hardware is as specific asset of this hardware, for base detection, I am in favour of just having the extended resource, and if you need specific labels for this specific hardware to steer scheduling you could create namespaced labels that are created by using the hooks or feature capability of NFD.

I'm inclined to think the same way. Having these numerical labels generally feels sort of non-features and user-unfriendly. Well, at least for the moment, unless proven otherwise :wink: 

> I am still thinking about a real-world use-case to use extended resources

I guess this `nvidia.com/memory` presented above could be one. What I'm still missing in this particular case is how you would handle the case where a node would have multiple GPUs(?) Sharing memory between them might be expensive(?)
uniemimu: > > If a resource is countable we can use extended resources but isn't that a job for the DevicePlugin?
> 
> @uniemimu please correct me if I'm wrong, but AFAIU, the desire to use NFD instead of device plugins is the problematic device plugin API. With my limited understanding of device plugins I assume that for every resource unit you will get one `allocate()` or similar calls which would be unbearable if you have like `8Gi` units of a resource.

Something like that. Each increase of the resource value done via the device plugin API requires a registration, and the increase is +1 for each registration. It is not feasible for a larger amount, and indeed each registration quite likely results in memory allocations of its own.

> 
> > A feature depending on the amount or count of the specific hardware is as specific asset of this hardware, for base detection, I am in favour of just having the extended resource, and if you need specific labels for this specific hardware to steer scheduling you could create namespaced labels that are created by using the hooks or feature capability of NFD.
> 
> I'm inclined to think the same way. Having these numerical labels generally feels sort of non-features and user-unfriendly. Well, at least for the moment, unless proven otherwise wink

Well, I will then filter out the labels. It just increases the patch size.

k8s-ci-robot: @uniemimu: PR needs rebase.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
uniemimu: > I guess this `nvidia.com/memory` presented above could be one. What I'm still missing in this particular case is how you would handle the case where a node would have multiple GPUs(?) Sharing memory between them might be expensive(?)

The multiple GPU case is interesting but in my opinion outside the scope of this PR. A k8s scheduler extension could maybe be used for handling cases where the total resource amount at a node does not match the largest possible single allocation of said resources, or potentially other more complex cases. In practice, it is not overly difficult for the scheduler extension to filter out a node based on whatever heuristics are needed, even if k8s scheduler would initially think that the node is a potential deployment target based on blind resource amount.

In short the answer is: I would handle that case outside NFD, but I would probably still need the resource amount exposed by NFD.
marquiz: @zvonkok any comments?

I see this mostly as an experimental feature. The default operation is not affected.
uniemimu: > I think we should add a (commented out) example of the required RBAC rule(s) in `nfd-master.yaml.template`

Indeed, that is a good idea. About the namespace adding and removing magic: I tend to be in the KISS-camp, but since such a mechanism exists for the labels already, I'll try to mimic that for the extended resources.
marquiz: > I tend to be in the KISS-camp, but since such a mechanism exists for the labels already, I'll try to mimic that for the extended resources.

👍 Yes, the namespacing is a unfortunately a bit convoluted, because of historical reasons. However, before this PR it has mostly been an internal implementation detail in worker-master communication
zvonkok: @marquiz I would also see it as an experimental feature. My only concern is that we here create a loop-hole for not using device-plugins where the community has agreed upon on how to handle devices with health checks and updating the count of resources being used of free to allocate. 

On the other hand, e.g. on-die features that "just" work, do not need a device-plugin one just wants to expose and use them. To not oversubscribe would be the responsibility of the admin or workload maintainer, pipeline architect or whoever is designing the application. 

Regarding how the slices are allocated by a device plugin, it is how the device plugin was developed. If you e.g. allocate 4 GPUs the device-plugin will not allocate one by one but rather return a list of GPUs [https://github.com/NVIDIA/k8s-device-plugin/blob/f737ebb9aab0d50b24b1ce513236410fcfd0dee1/server.go#L212](https://github.com/NVIDIA/k8s-device-plugin/blob/f737ebb9aab0d50b24b1ce513236410fcfd0dee1/server.go#L212)

Long story short I am good with it as an experimental feature, we might get more cases in the future where this functionality makes sense. 

We should explicitly state somewhere that this is experimental and not a replacement for device-plugins. 



mythi: >  If you e.g. allocate 4 GPUs the device-plugin will not allocate one by one but rather return a list of GPUs

Right, this was a miss from my side. The "list" is still very long if the resources are memory slices and the app is requesting a lot of them. Likewise, kubelet has internal structures to deal with all registered "devices".

> We should explicitly state somewhere that this is experimental and not a replacement for device-plugins.

You'd still need the device-plugin to be able to access the device so I see this as an optimization.
marquiz: @zvonkok thanks for your thoughts. Very valid points (of view) and concerns. I think we're on the same page. And, we can go forward with this.

@uniemimu please add the disclaimers (experimental and not to be used in place of device plugins) to the README.

Patch:
diff --git a/.gitignore b/.gitignore
index f2a14cff7..8f5bba89f 100644
--- a/.gitignore
+++ b/.gitignore
@@ -7,8 +7,3 @@ intel-cmt-cat/
 rdt-discovery/l2-alloc-discovery
 rdt-discovery/l3-alloc-discovery
 rdt-discovery/mon-discovery
-
-nfd-daemonset-combined.yaml
-nfd-master.yaml
-nfd-worker-daemonset.yaml
-nfd-worker-job.yaml
diff --git a/README.md b/README.md
index cd4789b13..eefc008e1 100644
--- a/README.md
+++ b/README.md
@@ -8,6 +8,7 @@
 - [Feature discovery](#feature-discovery)
   - [Feature sources](#feature-sources)
   - [Feature labels](#feature-labels)
+- [Extended resources (experimental)](#extended-resources-experimental)
 - [Getting started](#getting-started)
   - [System requirements](#system-requirements)
   - [Usage](#usage)
@@ -53,7 +54,7 @@ nfd-master.
   Usage:
   nfd-master [--no-publish] [--label-whitelist=<pattern>] [--port=<port>]
      [--ca-file=<path>] [--cert-file=<path>] [--key-file=<path>]
-     [--verify-node-name] [--extra-label-ns=<list>]
+     [--verify-node-name] [--extra-label-ns=<list>] [--resource-labels=<list>]
   nfd-master -h | --help
   nfd-master --version
 
@@ -76,6 +77,8 @@ nfd-master.
                                   publish to the Kubernetes API server. [Default: ]
   --extra-label-ns=<list>         Comma separated list of allowed extra label namespaces
                                   [Default: ]
+  --resource-labels=<list>        Comma separated list of labels to be exposed as extended resources.
+                                  [Default: ]
 ```
 
 ### NFD-Worker
@@ -441,6 +444,37 @@ temporary file (outside the `source.d` and `features.d` directories), and,
 atomically create/update the original file by doing a filesystem move
 operation.
 
+## Extended resources (experimental)
+
+This feature is experimental and by no means a replacement for the usage of
+device plugins.
+
+Labels which have integer values, can be promoted to Kubernetes extended
+resources by listing them to the master `--resource-labels` command line flag.
+These labels won't then show in the node label section, they will appear only
+as extended resources.
+
+An example use-case for the extended resources could be based on a hook which
+creates a label for the node SGX EPC memory section size. By giving the name of
+that label in the `--resource-labels` flag, that value will then turn into an
+extended resource of the node, allowing PODs to request that resource and the
+Kubernetes scheduler to schedule such PODs to only those nodes which have a
+sufficient capacity of said resource left.
+
+Similar to labels, the default namespace `feature.node.kubernetes.io` is
+automatically prefixed to the extended resource, if the promoted label doesn't
+have a namespace.
+
+Example usage of the command line arguments, using a new namespace:
+`nfd-master --resource-labels=my_source-my.feature,sgx.some.ns/epc --extra-label-ns=sgx.some.ns`
+
+The above would result in following extended resources provided that related
+labels exist:
+```
+  sgx.some.ns/epc: <label value>
+  feature.node.kubernetes.io/my_source-my.feature: <label value>
+```
+
 ## Getting started
 
 For a stable version with ready-built images see the
@@ -460,17 +494,8 @@ If you want to use the latest development version (master branch) you need to
 
 #### nfd-master
 
-Nfd-master runs as a deployment (with a replica count of 1), by default
-it prefers running on the cluster's master nodes but will run on worker
-nodes if no master nodes are found.
-
-For High Availability, you should simply increase the replica count of
-the deployment object. You should also look into adding [inter-pod](https://kubernetes.io/docs/concepts/configuration/assign-pod-node/#affinity-and-anti-affinity)
-affinity to prevent masters from running on the same node.
-However note that inter-pod affinity is costly and is not recommended
-in bigger clusters.
-
-You can use the template spec provided to deploy nfd-master, or
+Nfd-master runs as a DaemonSet, by default in the master node(s)
+only. You can use the template spec provided to deploy nfd-master, or
 use `nfd-master.yaml` generated by `Makefile`. The latter includes
 `image:` and `namespace:` definitions that match the latest built
 image. Example:
diff --git a/cmd/nfd-master/main.go b/cmd/nfd-master/main.go
index 161f3d83c..dbc5d6989 100644
--- a/cmd/nfd-master/main.go
+++ b/cmd/nfd-master/main.go
@@ -65,7 +65,7 @@ func argsParse(argv []string) (master.Args, error) {
   Usage:
   %s [--no-publish] [--label-whitelist=<pattern>] [--port=<port>]
      [--ca-file=<path>] [--cert-file=<path>] [--key-file=<path>]
-     [--verify-node-name] [--extra-label-ns=<list>]
+     [--verify-node-name] [--extra-label-ns=<list>] [--resource-labels=<list>]
   %s -h | --help
   %s --version
 
@@ -87,6 +87,8 @@ func argsParse(argv []string) (master.Args, error) {
   --label-whitelist=<pattern>     Regular expression to filter label names to
                                   publish to the Kubernetes API server. [Default: ]
   --extra-label-ns=<list>         Comma separated list of allowed extra label namespaces
+                                  [Default: ]
+  --resource-labels=<list>        Comma separated list of labels to be exposed as extended resources.
                                   [Default: ]`,
 		ProgramName,
 		ProgramName,
@@ -113,6 +115,7 @@ func argsParse(argv []string) (master.Args, error) {
 	}
 	args.VerifyNodeName = arguments["--verify-node-name"].(bool)
 	args.ExtraLabelNs = strings.Split(arguments["--extra-label-ns"].(string), ",")
+	args.ResourceLabels = strings.Split(arguments["--resource-labels"].(string), ",")
 
 	return args, nil
 }
diff --git a/nfd-master.yaml.template b/nfd-master.yaml.template
index 59ef65252..b3e438f22 100644
--- a/nfd-master.yaml.template
+++ b/nfd-master.yaml.template
@@ -18,6 +18,9 @@ rules:
   - ""
   resources:
   - nodes
+# when using command line flag --resource-labels to create extended resources
+# you will need to uncomment "- nodes/status"
+# - nodes/status
   verbs:
   - get
   - patch
@@ -37,14 +40,13 @@ subjects:
   namespace: node-feature-discovery
 ---
 apiVersion: apps/v1
-kind: Deployment
+kind: DaemonSet
 metadata:
   labels:
     app: nfd-master
   name: nfd-master
   namespace: node-feature-discovery
 spec:
-  replicas: 1
   selector:
     matchLabels:
       app: nfd-master
@@ -54,15 +56,8 @@ spec:
         app: nfd-master
     spec:
       serviceAccount: nfd-master
-      affinity:
-        nodeAffinity:
-          preferredDuringSchedulingIgnoredDuringExecution:
-            - weight: 1
-              preference:
-                matchExpressions:
-                  - key: "node-role.kubernetes.io/master"
-                    operator: In
-                    values: [""]
+      nodeSelector:
+        node-role.kubernetes.io/master: ""
       tolerations:
         - key: "node-role.kubernetes.io/master"
           operator: "Equal"
diff --git a/pkg/apihelper/apihelpers.go b/pkg/apihelper/apihelpers.go
index 50dbbabe4..81240b06a 100644
--- a/pkg/apihelper/apihelpers.go
+++ b/pkg/apihelper/apihelpers.go
@@ -31,4 +31,7 @@ type APIHelpers interface {
 
 	// UpdateNode updates the node via the API server using a client.
 	UpdateNode(*k8sclient.Clientset, *api.Node) error
+
+	// PatchStatus updates the node status via the API server using a client.
+	PatchStatus(*k8sclient.Clientset, string, interface{}) error
 }
diff --git a/pkg/apihelper/k8shelpers.go b/pkg/apihelper/k8shelpers.go
index 4c2ef3402..695c400f3 100644
--- a/pkg/apihelper/k8shelpers.go
+++ b/pkg/apihelper/k8shelpers.go
@@ -17,8 +17,11 @@ limitations under the License.
 package apihelper
 
 import (
+	"encoding/json"
+
 	api "k8s.io/api/core/v1"
 	meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 	k8sclient "k8s.io/client-go/kubernetes"
 	restclient "k8s.io/client-go/rest"
 )
@@ -59,3 +62,13 @@ func (h K8sHelpers) UpdateNode(c *k8sclient.Clientset, n *api.Node) error {
 
 	return nil
 }
+
+func (h K8sHelpers) PatchStatus(c *k8sclient.Clientset, nodeName string, marshalable interface{}) error {
+	// Send the updated node to the apiserver.
+	patch, err := json.Marshal(marshalable)
+	if err == nil {
+		_, err = c.CoreV1().Nodes().Patch(nodeName, types.JSONPatchType, patch, "status")
+	}
+
+	return err
+}
diff --git a/pkg/apihelper/mock_APIHelpers.go b/pkg/apihelper/mock_APIHelpers.go
index 0df06f6a9..4788406ca 100644
--- a/pkg/apihelper/mock_APIHelpers.go
+++ b/pkg/apihelper/mock_APIHelpers.go
@@ -59,6 +59,20 @@ func (_m *MockAPIHelpers) GetNode(_a0 *kubernetes.Clientset, _a1 string) (*v1.No
 	return r0, r1
 }
 
+// PatchStatus provides a mock function with given fields: _a0, _a1, _a2
+func (_m *MockAPIHelpers) PatchStatus(_a0 *kubernetes.Clientset, _a1 string, _a2 interface{}) error {
+	ret := _m.Called(_a0, _a1, _a2)
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(*kubernetes.Clientset, string, interface{}) error); ok {
+		r0 = rf(_a0, _a1, _a2)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
 // UpdateNode provides a mock function with given fields: _a0, _a1
 func (_m *MockAPIHelpers) UpdateNode(_a0 *kubernetes.Clientset, _a1 *v1.Node) error {
 	ret := _m.Called(_a0, _a1)
diff --git a/pkg/nfd-master/nfd-master.go b/pkg/nfd-master/nfd-master.go
index 37a5b13aa..a28dc92fb 100644
--- a/pkg/nfd-master/nfd-master.go
+++ b/pkg/nfd-master/nfd-master.go
@@ -26,6 +26,7 @@ import (
 	"os"
 	"regexp"
 	"sort"
+	"strconv"
 	"strings"
 	"time"
 
@@ -57,6 +58,9 @@ var (
 // Labels are a Kubernetes representation of discovered features.
 type Labels map[string]string
 
+// ExtendedResources are k8s extended resources which are created from discovered features.
+type ExtendedResources map[string]string
+
 // Annotations are used for NFD-related node metadata
 type Annotations map[string]string
 
@@ -70,6 +74,7 @@ type Args struct {
 	NoPublish      bool
 	Port           int
 	VerifyNodeName bool
+	ResourceLabels []string
 }
 
 type NfdMaster interface {
@@ -84,6 +89,21 @@ type nfdMaster struct {
 	ready  chan bool
 }
 
+// statusOp is a json marshaling helper used for patching node status
+type statusOp struct {
+	Op    string `json:"op"`
+	Path  string `json:"path"`
+	Value string `json:"value,omitempty"`
+}
+
+func createStatusOp(verb string, resource string, path string, value string) statusOp {
+	if !strings.Contains(resource, "/") {
+		resource = LabelNs + resource
+	}
+	res := strings.ReplaceAll(resource, "/", "~1")
+	return statusOp{verb, "/status/" + path + "/" + res, value}
+}
+
 // Create new NfdMaster server instance.
 func NewNfdMaster(args Args) (*nfdMaster, error) {
 	nfd := &nfdMaster{args: args, ready: make(chan bool, 1)}
@@ -204,7 +224,7 @@ func updateMasterNode(helper apihelper.APIHelpers) error {
 }
 
 // Filter labels by namespace and name whitelist
-func filterFeatureLabels(labels Labels, extraLabelNs []string, labelWhiteList *regexp.Regexp) Labels {
+func filterFeatureLabels(labels Labels, extraLabelNs []string, labelWhiteList *regexp.Regexp, extendedResourceNames []string) (Labels, ExtendedResources) {
 	for label := range labels {
 		split := strings.SplitN(label, "/", 2)
 		name := split[0]
@@ -229,7 +249,24 @@ func filterFeatureLabels(labels Labels, extraLabelNs []string, labelWhiteList *r
 			delete(labels, label)
 		}
 	}
-	return labels
+
+	// Remove labels which are intended to be extended resources
+	extendedResources := ExtendedResources{}
+	for _, extendedResourceName := range extendedResourceNames {
+		// remove possibly given default LabelNs to keep annotations shorter
+		extendedResourceName = strings.TrimPrefix(extendedResourceName, LabelNs)
+		if _, ok := labels[extendedResourceName]; ok {
+			if _, err := strconv.Atoi(labels[extendedResourceName]); err != nil {
+				stderrLogger.Printf("bad label value encountered for extended resource: %s", err.Error())
+				continue // non-numeric label can't be used
+			}
+
+			extendedResources[extendedResourceName] = labels[extendedResourceName]
+			delete(labels, extendedResourceName)
+		}
+	}
+
+	return labels, extendedResources
 }
 
 // Implement LabelerServer
@@ -265,19 +302,28 @@ func (s *labelerServer) SetLabels(c context.Context, r *pb.SetLabelsRequest) (*p
 	}
 	stdoutLogger.Printf("REQUEST Node: %s NFD-version: %s Labels: %s", r.NodeName, r.NfdVersion, r.Labels)
 
-	labels := filterFeatureLabels(r.Labels, s.args.ExtraLabelNs, s.args.LabelWhiteList)
+	labels, extendedResources := filterFeatureLabels(r.Labels, s.args.ExtraLabelNs, s.args.LabelWhiteList, s.args.ResourceLabels)
 
 	if !s.args.NoPublish {
-		// Advertise NFD worker version and label names as annotations
-		keys := make([]string, 0, len(labels))
-		for k, _ := range labels {
-			keys = append(keys, k)
+		// Advertise NFD worker version, label names and extended resources as annotations
+		labelKeys := make([]string, 0, len(labels))
+		for k := range labels {
+			labelKeys = append(labelKeys, k)
 		}
-		sort.Strings(keys)
+		sort.Strings(labelKeys)
+
+		extendedResourceKeys := make([]string, 0, len(extendedResources))
+		for key := range extendedResources {
+			extendedResourceKeys = append(extendedResourceKeys, key)
+		}
+		sort.Strings(extendedResourceKeys)
+
 		annotations := Annotations{"worker.version": r.NfdVersion,
-			"feature-labels": strings.Join(keys, ",")}
+			"feature-labels":     strings.Join(labelKeys, ","),
+			"extended-resources": strings.Join(extendedResourceKeys, ","),
+		}
 
-		err := updateNodeFeatures(s.apiHelper, r.NodeName, labels, annotations)
+		err := updateNodeFeatures(s.apiHelper, r.NodeName, labels, annotations, extendedResources)
 		if err != nil {
 			stderrLogger.Printf("failed to advertise labels: %s", err.Error())
 			return &pb.SetLabelsReply{}, err
@@ -288,7 +334,7 @@ func (s *labelerServer) SetLabels(c context.Context, r *pb.SetLabelsRequest) (*p
 
 // advertiseFeatureLabels advertises the feature labels to a Kubernetes node
 // via the API server.
-func updateNodeFeatures(helper apihelper.APIHelpers, nodeName string, labels Labels, annotations Annotations) error {
+func updateNodeFeatures(helper apihelper.APIHelpers, nodeName string, labels Labels, annotations Annotations, extendedResources ExtendedResources) error {
 	cli, err := helper.GetClient()
 	if err != nil {
 		return err
@@ -300,6 +346,9 @@ func updateNodeFeatures(helper apihelper.APIHelpers, nodeName string, labels Lab
 		return err
 	}
 
+	// Resolve publishable extended resources before node is modified
+	statusOps := getExtendedResourceOps(node, extendedResources)
+
 	// Remove old labels
 	if l, ok := node.Annotations[AnnotationNs+"feature-labels"]; ok {
 		oldLabels := strings.Split(l, ",")
@@ -323,7 +372,16 @@ func updateNodeFeatures(helper apihelper.APIHelpers, nodeName string, labels Lab
 		return err
 	}
 
-	return nil
+	// patch node status with extended resource changes
+	if len(statusOps) > 0 {
+		err = helper.PatchStatus(cli, node.Name, statusOps)
+		if err != nil {
+			stderrLogger.Printf("error while patching extended resources: %s", err.Error())
+			return err
+		}
+	}
+
+	return err
 }
 
 // Remove any labels having the given prefix
@@ -346,6 +404,42 @@ func removeLabels(n *api.Node, labelNames []string) {
 	}
 }
 
+// getExtendedResourceOps returns a slice of operations to perform on the node status
+func getExtendedResourceOps(n *api.Node, extendedResources ExtendedResources) []statusOp {
+	var statusOps []statusOp
+
+	oldResources := strings.Split(n.Annotations[AnnotationNs+"extended-resources"], ",")
+
+	// figure out which resources to remove
+	for _, resource := range oldResources {
+		if _, ok := n.Status.Capacity[api.ResourceName(addNs(resource, LabelNs))]; ok {
+			// check if the ext resource is still needed
+			_, extResNeeded := extendedResources[resource]
+			if !extResNeeded {
+				statusOps = append(statusOps, createStatusOp("remove", resource, "capacity", ""))
+				statusOps = append(statusOps, createStatusOp("remove", resource, "allocatable", ""))
+			}
+		}
+	}
+
+	// figure out which resources to replace and which to add
+	for resource, value := range extendedResources {
+		// check if the extended resource already exists with the same capacity in the node
+		if quantity, ok := n.Status.Capacity[api.ResourceName(addNs(resource, LabelNs))]; ok {
+			val, _ := quantity.AsInt64()
+			if strconv.FormatInt(val, 10) != value {
+				statusOps = append(statusOps, createStatusOp("replace", resource, "capacity", value))
+				statusOps = append(statusOps, createStatusOp("replace", resource, "allocatable", value))
+			}
+		} else {
+			statusOps = append(statusOps, createStatusOp("add", resource, "capacity", value))
+			// "allocatable" gets added implicitly after adding to capacity
+		}
+	}
+
+	return statusOps
+}
+
 // Add NFD labels to a Node object.
 func addLabels(n *api.Node, labels map[string]string) {
 	for k, v := range labels {
@@ -363,3 +457,11 @@ func addAnnotations(n *api.Node, annotations map[string]string) {
 		n.Annotations[AnnotationNs+k] = v
 	}
 }
+
+// addNs adds a namespace if one isn't already found from src string
+func addNs(src string, nsToAdd string) string {
+	if strings.Contains(src, "/") {
+		return src
+	}
+	return nsToAdd + src
+}


Test patch:
diff --git a/pkg/nfd-master/nfd-master-internal_test.go b/pkg/nfd-master/nfd-master-internal_test.go
index 182a69d78..f0615118c 100644
--- a/pkg/nfd-master/nfd-master-internal_test.go
+++ b/pkg/nfd-master/nfd-master-internal_test.go
@@ -23,9 +23,11 @@ import (
 	"testing"
 
 	. "github.com/smartystreets/goconvey/convey"
+	"github.com/stretchr/testify/mock"
 	"github.com/vektra/errors"
 	"golang.org/x/net/context"
 	api "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	k8sclient "k8s.io/client-go/kubernetes"
 	"sigs.k8s.io/node-feature-discovery/pkg/apihelper"
@@ -43,15 +45,18 @@ func init() {
 
 func newMockNode() *api.Node {
 	n := api.Node{}
+	n.Name = mockNodeName
 	n.Labels = map[string]string{}
 	n.Annotations = map[string]string{}
+	n.Status.Capacity = api.ResourceList{}
 	return &n
 }
 
 func TestUpdateNodeFeatures(t *testing.T) {
 	Convey("When I update the node using fake client", t, func() {
-		fakeFeatureLabels := map[string]string{"source-feature.1": "val1", "source-feature.2": "val2", "source-feature.3": "val3"}
+		fakeFeatureLabels := map[string]string{"source-feature.1": "1", "source-feature.2": "2", "source-feature.3": "val3"}
 		fakeAnnotations := map[string]string{"version": version.Get()}
+		fakeExtResources := ExtendedResources{"source-feature.1": "", "source-feature.2": ""}
 		fakeFeatureLabelNames := make([]string, 0, len(fakeFeatureLabels))
 		for k, _ := range fakeFeatureLabels {
 			fakeFeatureLabelNames = append(fakeFeatureLabelNames, k)
@@ -70,7 +75,8 @@ func TestUpdateNodeFeatures(t *testing.T) {
 			mockAPIHelper.On("GetClient").Return(mockClient, nil)
 			mockAPIHelper.On("GetNode", mockClient, mockNodeName).Return(mockNode, nil).Once()
 			mockAPIHelper.On("UpdateNode", mockClient, mockNode).Return(nil).Once()
-			err := updateNodeFeatures(mockAPIHelper, mockNodeName, fakeFeatureLabels, fakeAnnotations)
+			mockAPIHelper.On("PatchStatus", mockClient, mockNodeName, mock.Anything).Return(nil).Twice()
+			err := updateNodeFeatures(mockAPIHelper, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources)
 
 			Convey("Error is nil", func() {
 				So(err, ShouldBeNil)
@@ -90,7 +96,7 @@ func TestUpdateNodeFeatures(t *testing.T) {
 		Convey("When I fail to update the node with feature labels", func() {
 			expectedError := errors.New("fake error")
 			mockAPIHelper.On("GetClient").Return(nil, expectedError)
-			err := updateNodeFeatures(mockAPIHelper, mockNodeName, fakeFeatureLabels, fakeAnnotations)
+			err := updateNodeFeatures(mockAPIHelper, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources)
 
 			Convey("Error is produced", func() {
 				So(err, ShouldEqual, expectedError)
@@ -100,7 +106,7 @@ func TestUpdateNodeFeatures(t *testing.T) {
 		Convey("When I fail to get a mock client while updating feature labels", func() {
 			expectedError := errors.New("fake error")
 			mockAPIHelper.On("GetClient").Return(nil, expectedError)
-			err := updateNodeFeatures(mockAPIHelper, mockNodeName, fakeFeatureLabels, fakeAnnotations)
+			err := updateNodeFeatures(mockAPIHelper, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources)
 
 			Convey("Error is produced", func() {
 				So(err, ShouldEqual, expectedError)
@@ -111,7 +117,7 @@ func TestUpdateNodeFeatures(t *testing.T) {
 			expectedError := errors.New("fake error")
 			mockAPIHelper.On("GetClient").Return(mockClient, nil)
 			mockAPIHelper.On("GetNode", mockClient, mockNodeName).Return(nil, expectedError).Once()
-			err := updateNodeFeatures(mockAPIHelper, mockNodeName, fakeFeatureLabels, fakeAnnotations)
+			err := updateNodeFeatures(mockAPIHelper, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources)
 
 			Convey("Error is produced", func() {
 				So(err, ShouldEqual, expectedError)
@@ -123,7 +129,7 @@ func TestUpdateNodeFeatures(t *testing.T) {
 			mockAPIHelper.On("GetClient").Return(mockClient, nil)
 			mockAPIHelper.On("GetNode", mockClient, mockNodeName).Return(mockNode, nil).Once()
 			mockAPIHelper.On("UpdateNode", mockClient, mockNode).Return(expectedError).Once()
-			err := updateNodeFeatures(mockAPIHelper, mockNodeName, fakeFeatureLabels, fakeAnnotations)
+			err := updateNodeFeatures(mockAPIHelper, mockNodeName, fakeFeatureLabels, fakeAnnotations, fakeExtResources)
 
 			Convey("Error is produced", func() {
 				So(err, ShouldEqual, expectedError)
@@ -178,6 +184,72 @@ func TestUpdateMasterNode(t *testing.T) {
 	})
 }
 
+func TestAddingExtResources(t *testing.T) {
+	Convey("When adding extended resources", t, func() {
+		Convey("When there are no matching labels", func() {
+			mockNode := newMockNode()
+			mockResourceLabels := ExtendedResources{}
+			resourceOps := getExtendedResourceOps(mockNode, mockResourceLabels)
+			So(len(resourceOps), ShouldEqual, 0)
+		})
+
+		Convey("When there are matching labels", func() {
+			mockNode := newMockNode()
+			mockResourceLabels := ExtendedResources{"feature-1": "1", "feature-2": "2"}
+			resourceOps := getExtendedResourceOps(mockNode, mockResourceLabels)
+			So(len(resourceOps), ShouldBeGreaterThan, 0)
+		})
+
+		Convey("When the resource already exists", func() {
+			mockNode := newMockNode()
+			mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-1")] = *resource.NewQuantity(1, resource.BinarySI)
+			mockResourceLabels := ExtendedResources{"feature-1": "1"}
+			resourceOps := getExtendedResourceOps(mockNode, mockResourceLabels)
+			So(len(resourceOps), ShouldEqual, 0)
+		})
+
+		Convey("When the resource already exists but its capacity has changed", func() {
+			mockNode := newMockNode()
+			mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-1")] = *resource.NewQuantity(2, resource.BinarySI)
+			mockResourceLabels := ExtendedResources{"feature-1": "1"}
+			resourceOps := getExtendedResourceOps(mockNode, mockResourceLabels)
+			So(len(resourceOps), ShouldBeGreaterThan, 0)
+		})
+	})
+}
+
+func TestRemovingExtResources(t *testing.T) {
+	Convey("When removing extended resources", t, func() {
+		Convey("When none are removed", func() {
+			mockNode := newMockNode()
+			mockResourceLabels := ExtendedResources{"feature-1": "1", "feature-2": "2"}
+			mockNode.Annotations[AnnotationNs+"extended-resources"] = "feature-1,feature-2"
+			mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-1")] = *resource.NewQuantity(1, resource.BinarySI)
+			mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-2")] = *resource.NewQuantity(2, resource.BinarySI)
+			resourceOps := getExtendedResourceOps(mockNode, mockResourceLabels)
+			So(len(resourceOps), ShouldEqual, 0)
+		})
+		Convey("When the related label is gone", func() {
+			mockNode := newMockNode()
+			mockResourceLabels := ExtendedResources{"feature-4": "", "feature-2": "2"}
+			mockNode.Annotations[AnnotationNs+"extended-resources"] = "feature-4,feature-2"
+			mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-4")] = *resource.NewQuantity(4, resource.BinarySI)
+			mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-2")] = *resource.NewQuantity(2, resource.BinarySI)
+			resourceOps := getExtendedResourceOps(mockNode, mockResourceLabels)
+			So(len(resourceOps), ShouldBeGreaterThan, 0)
+		})
+		Convey("When the extended resource is no longer wanted", func() {
+			mockNode := newMockNode()
+			mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-1")] = *resource.NewQuantity(1, resource.BinarySI)
+			mockNode.Status.Capacity[api.ResourceName(LabelNs+"feature-2")] = *resource.NewQuantity(2, resource.BinarySI)
+			mockResourceLabels := ExtendedResources{"feature-2": "2"}
+			mockNode.Annotations[AnnotationNs+"extended-resources"] = "feature-1,feature-2"
+			resourceOps := getExtendedResourceOps(mockNode, mockResourceLabels)
+			So(len(resourceOps), ShouldBeGreaterThan, 0)
+		})
+	})
+}
+
 func TestSetLabels(t *testing.T) {
 	Convey("When servicing SetLabels request", t, func() {
 		const workerName = "mock-worker"
@@ -197,6 +269,7 @@ func TestSetLabels(t *testing.T) {
 		sort.Strings(mockLabelNames)
 		expectedAnnotations := map[string]string{"worker.version": workerVer}
 		expectedAnnotations["feature-labels"] = strings.Join(mockLabelNames, ",")
+		expectedAnnotations["extended-resources"] = ""
 
 		Convey("When node update succeeds", func() {
 			mockHelper.On("GetClient").Return(mockClient, nil)
@@ -231,7 +304,7 @@ func TestSetLabels(t *testing.T) {
 				So(len(mockNode.Labels), ShouldEqual, 1)
 				So(mockNode.Labels, ShouldResemble, map[string]string{LabelNs + "feature-2": "val-2"})
 
-				a := map[string]string{AnnotationNs + "worker.version": workerVer, AnnotationNs + "feature-labels": "feature-2"}
+				a := map[string]string{AnnotationNs + "worker.version": workerVer, AnnotationNs + "feature-labels": "feature-2", AnnotationNs + "extended-resources": ""}
 				So(len(mockNode.Annotations), ShouldEqual, len(a))
 				So(mockNode.Annotations, ShouldResemble, a)
 			})
@@ -254,7 +327,7 @@ func TestSetLabels(t *testing.T) {
 				So(len(mockNode.Labels), ShouldEqual, 2)
 				So(mockNode.Labels, ShouldResemble, map[string]string{LabelNs + "feature-1": "val-1", "valid.ns/feature-2": "val-2"})
 
-				a := map[string]string{AnnotationNs + "worker.version": workerVer, AnnotationNs + "feature-labels": "feature-1,valid.ns/feature-2"}
+				a := map[string]string{AnnotationNs + "worker.version": workerVer, AnnotationNs + "feature-labels": "feature-1,valid.ns/feature-2", AnnotationNs + "extended-resources": ""}
 				So(len(mockNode.Annotations), ShouldEqual, len(a))
 				So(mockNode.Annotations, ShouldResemble, a)
 			})