Repository: kubernetes-sigs/node-feature-discovery
Pull request #661 (2021-11-18 13:05:30)

source/memory: implement FeatureSource
Separate feature discovery and creation of feature labels.

Generalize the discovery of nvdimm devices so that they can be matched
in custom label rules in a similar fashion as pci and usb devices.
Available attributes for matching nvdimm devices are limited to:

- devtype
- mode

For numa we now detect the number of numa nodes which can be matched
agains in custom label rules.

Labels created by the memory feature source are unchanged. The new
features being detected are available in custom rules only.

Example custom rule:

```
  - name: "my memory rule"
    labels:
      my-memory-feature: "true"
    matchFeatures:
      - feature: memory.numa
        matchExpressions:
          "node_count": {op: Gt, value: ["3"]}
      - feature: memory.nv
        matchExpressions:
          "devtype" {op: In, value: ["nd_dax"]}
```

Also, add minimalist unit test.

k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/node-feature-discovery/pull/661#" title="Author self-approved">marquiz</a>*

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

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

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

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

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":[]} -->
k8s-ci-robot: @marquiz: 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>
ArangoGutierrez: @marquiz rebase :) 

k8s-ci-robot: @marquiz: 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>

Patch:
diff --git a/deployment/base/nfd-crds/cr-sample.yaml b/deployment/base/nfd-crds/cr-sample.yaml
index 876521315..ac33c8068 100644
--- a/deployment/base/nfd-crds/cr-sample.yaml
+++ b/deployment/base/nfd-crds/cr-sample.yaml
@@ -56,6 +56,14 @@ spec:
             operstate: {op: In, value: ["up"]}
             speed: {op: Gt, value: ["100"]}
 
+        - feature: memory.numa
+          matchExpressions:
+            node_count: {op: Gt, value: ["2"]}
+        - feature: memory.nv
+          matchExpressions:
+            devtype: {op: In, value: ["nd_dax"]}
+            mode: {op: In, value: ["memory"]}
+
         - feature: system.osrelease
           matchExpressions:
             ID: {op: In, value: ["fedora", "centos"]}
diff --git a/deployment/components/worker-config/nfd-worker.conf.example b/deployment/components/worker-config/nfd-worker.conf.example
index 5339c34f7..11d43450d 100644
--- a/deployment/components/worker-config/nfd-worker.conf.example
+++ b/deployment/components/worker-config/nfd-worker.conf.example
@@ -160,6 +160,14 @@
 #            operstate: {op: In, value: ["up"]}
 #            speed: {op: Gt, value: ["100"]}
 #
+#        - feature: memory.numa
+#          matchExpressions:
+#            node_count: {op: Gt, value: ["2"]}
+#        - feature: memory.nv
+#          matchExpressions:
+#            devtype: {op: In, value: ["nd_dax"]}
+#            mode: {op: In, value: ["memory"]}
+#
 #        - feature: system.osrelease
 #          matchExpressions:
 #            ID: {op: In, value: ["fedora", "centos"]}
diff --git a/deployment/helm/node-feature-discovery/values.yaml b/deployment/helm/node-feature-discovery/values.yaml
index 9265499e5..7474ebdd6 100644
--- a/deployment/helm/node-feature-discovery/values.yaml
+++ b/deployment/helm/node-feature-discovery/values.yaml
@@ -249,6 +249,14 @@ worker:
     #            operstate: {op: In, value: ["up"]}
     #            speed: {op: Gt, value: ["100"]}
     #
+    #        - feature: memory.numa
+    #          matchExpressions:
+    #            node_count: {op: Gt, value: ["2"]}
+    #        - feature: memory.nv
+    #          matchExpressions:
+    #            devtype: {op: In, value: ["nd_dax"]}
+    #            mode: {op: In, value: ["memory"]}
+    #
     #        - feature: system.osrelease
     #          matchExpressions:
     #            ID: {op: In, value: ["fedora", "centos"]}
diff --git a/source/memory/memory.go b/source/memory/memory.go
index 244503887..823ad41fb 100644
--- a/source/memory/memory.go
+++ b/source/memory/memory.go
@@ -17,24 +17,35 @@ limitations under the License.
 package memory
 
 import (
+	"fmt"
 	"io/ioutil"
 	"os"
+	"path/filepath"
+	"strconv"
 	"strings"
 
 	"k8s.io/klog/v2"
 
+	"sigs.k8s.io/node-feature-discovery/pkg/api/feature"
+	"sigs.k8s.io/node-feature-discovery/pkg/utils"
 	"sigs.k8s.io/node-feature-discovery/source"
 )
 
 const Name = "memory"
 
-// memorySource implements the LabelSource interface.
-type memorySource struct{}
+const NvFeature = "nv"
+const NumaFeature = "numa"
+
+// memorySource implements the FeatureSource and LabelSource interfaces.
+type memorySource struct {
+	features *feature.DomainFeatures
+}
 
 // Singleton source instance
 var (
 	src memorySource
-	_   source.LabelSource = &src
+	_   source.FeatureSource = &src
+	_   source.LabelSource   = &src
 )
 
 // Name returns an identifier string for this feature source.
@@ -45,78 +56,107 @@ func (s *memorySource) Priority() int { return 0 }
 
 // GetLabels method of the LabelSource interface
 func (s *memorySource) GetLabels() (source.FeatureLabels, error) {
-	features := source.FeatureLabels{}
+	labels := source.FeatureLabels{}
+	features := s.GetFeatures()
+
+	// NUMA
+	if len(features.Values[NumaFeature].Elements) > 0 {
+		labels["numa"] = true
+	}
+
+	// NVDIMM
+	if len(features.Instances[NvFeature].Elements) > 0 {
+		labels["nv.present"] = true
+	}
+	for _, dev := range features.Instances[NvFeature].Elements {
+		if dev.Attributes["devtype"] == "nd_dax" {
+			labels["nv.dax"] = true
+			break
+		}
+	}
+
+	return labels, nil
+}
+
+// Discover method of the FeatureSource interface
+func (s *memorySource) Discover() error {
+	s.features = feature.NewDomainFeatures()
 
 	// Detect NUMA
-	numa, err := isNuma()
-	if err != nil {
-		klog.Errorf("failed to detect NUMA topology: %s", err)
-	} else if numa {
-		features["numa"] = true
+	if numa, err := detectNuma(); err != nil {
+		klog.Errorf("failed to detect NUMA nodes: %v", err)
+	} else {
+		s.features.Values[NumaFeature] = feature.ValueFeatureSet{Elements: numa}
 	}
 
 	// Detect NVDIMM
-	nv, err := detectNvdimm()
-	if err != nil {
-		klog.Errorf("NVDIMM detection failed: %s", err)
+	if nv, err := detectNv(); err != nil {
+		klog.Errorf("failed to detect nvdimm devices: %v", err)
 	} else {
-		for k, v := range nv {
-			features["nv."+k] = v
-		}
+		s.features.Instances[NvFeature] = feature.InstanceFeatureSet{Elements: nv}
 	}
 
-	return features, nil
+	utils.KlogDump(3, "discovered memory features:", "  ", s.features)
+
+	return nil
 }
 
-// Detect if the platform has NUMA topology
-func isNuma() (bool, error) {
-	// Find out how many nodes are online
-	// Multiple nodes is a sign of NUMA
-	bytes, err := ioutil.ReadFile(source.SysfsDir.Path("devices/system/node/online"))
-	if err != nil {
-		return false, err
+// GetFeatures method of the FeatureSource Interface.
+func (s *memorySource) GetFeatures() *feature.DomainFeatures {
+	if s.features == nil {
+		s.features = feature.NewDomainFeatures()
 	}
+	return s.features
+}
+
+// detectNuma detects NUMA node information
+func detectNuma() (map[string]string, error) {
+	sysfsBasePath := source.SysfsDir.Path("bus/node/devices")
 
-	// File content is expected to be:
-	//   "0\n" in one-node case
-	//   "0-K\n" in N-node case where K=N-1
-	// presence of newline requires TrimSpace
-	if strings.TrimSpace(string(bytes)) != "0" {
-		// more than one node means NUMA
-		return true, nil
+	nodes, err := ioutil.ReadDir(sysfsBasePath)
+	if err != nil {
+		return nil, fmt.Errorf("failed to list numa nodes: %w", err)
 	}
-	return false, nil
+
+	return map[string]string{"node_count": strconv.Itoa(len(nodes))}, nil
 }
 
-// Detect NVDIMM devices and configuration
-func detectNvdimm() (map[string]bool, error) {
-	features := make(map[string]bool)
+// detectNv detects NVDIMM devices
+func detectNv() ([]feature.InstanceFeature, error) {
+	sysfsBasePath := source.SysfsDir.Path("bus/nd/devices")
+	info := make([]feature.InstanceFeature, 0)
+
+	devices, err := ioutil.ReadDir(sysfsBasePath)
+	if os.IsNotExist(err) {
+		klog.V(1).Info("No NVDIMM devices present")
+		return info, nil
+	} else if err != nil {
+		return nil, fmt.Errorf("failed to list nvdimm devices: %w", err)
+	}
 
-	// Check presence of physical devices
-	devices, err := ioutil.ReadDir(source.SysfsDir.Path("class/nd"))
-	if err == nil {
-		if len(devices) > 0 {
-			features["present"] = true
-		}
-	} else if os.IsNotExist(err) {
-		return nil, nil
-	} else {
-		return nil, err
+	// Iterate over devices
+	for _, device := range devices {
+		i := readNdDeviceInfo(filepath.Join(sysfsBasePath, device.Name()))
+		info = append(info, i)
 	}
 
-	// Check presence of DAX-configured regions
-	devices, err = ioutil.ReadDir(source.SysfsDir.Path("bus/nd/devices"))
-	if err == nil {
-		for _, d := range devices {
-			if strings.HasPrefix(d.Name(), "dax") {
-				features["dax"] = true
-			}
+	return info, nil
+}
+
+// ndDevAttrs is the list of sysfs files (under each nd device) that we're trying to read
+var ndDevAttrs = []string{"devtype", "mode"}
+
+func readNdDeviceInfo(path string) feature.InstanceFeature {
+	attrs := map[string]string{"name": filepath.Base(path)}
+	for _, attrName := range ndDevAttrs {
+		data, err := ioutil.ReadFile(filepath.Join(path, attrName))
+		if err != nil {
+			klog.V(3).Infof("failed to read nd device attribute %s: %w", attrName, err)
+			continue
 		}
-	} else {
-		klog.Warningf("failed to detect NVDIMM configuration: %s", err)
+		attrs[attrName] = strings.TrimSpace(string(data))
 	}
-
-	return features, nil
+	return *feature.NewInstanceFeature(attrs)
 }
 
 func init() {


Test patch:
diff --git a/source/memory/memory_test.go b/source/memory/memory_test.go
new file mode 100644
index 000000000..6b82ee750
--- /dev/null
+++ b/source/memory/memory_test.go
@@ -0,0 +1,36 @@
+/*
+Copyright 2021 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package memory
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"sigs.k8s.io/node-feature-discovery/pkg/api/feature"
+)
+
+func TestMemorySource(t *testing.T) {
+	assert.Equal(t, src.Name(), Name)
+
+	// Check that GetLabels works with empty features
+	src.features = feature.NewDomainFeatures()
+	l, err := src.GetLabels()
+
+	assert.Nil(t, err, err)
+	assert.Empty(t, l)
+
+}