Repository: kubernetes-sigs/node-feature-discovery
Pull request #649 (2021-11-15 19:05:46)

source/storage: implement FeatureSource
Separate feature discovery and creation of feature labels. Generalize
the feature discovery so that block devices can be matched in custom
label rules in a similar fashion as pci and usb devices. This extends
the discovery to other block queue attributes than 'rotational': now we
also detect 'dax', 'nr_zones' and 'zoned'.

Labels created by the storage feature source are unchanged. The new
features being detected are available in custom rules only.

Example custom rules:

```
  - name: "my block rule 1"
    labels:
      my-block-feature-1: "true"
    matchFeatures:
      - feature: storage.block
          "rotational": {op: In, value: ["0"]}

  - name: "my block rule 2"
    labels:
      my-block-feature-2: "true"
    matchFeatures:
      - feature: storage.block
          "zoned": {op: In, value: [“host-aware”, “host-managed”]}
```

Also, add minimalist unit test.

Depends/builds on top of #464

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/649#" 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":[]} -->
marquiz: Waiting for #464 
/hold

Patch:
diff --git a/deployment/base/nfd-crds/cr-sample.yaml b/deployment/base/nfd-crds/cr-sample.yaml
index f9228140f..bc9c24829 100644
--- a/deployment/base/nfd-crds/cr-sample.yaml
+++ b/deployment/base/nfd-crds/cr-sample.yaml
@@ -46,6 +46,11 @@ spec:
             major: {op: In, value: ["5"]}
             minor: {op: Gt, value: ["10"]}
 
+        - feature: storage.block
+          matchExpressions:
+            rotational: {op: In, value: ["0"]}
+            dax: {op: In, value: ["0"]}
+
         - 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 c37860736..7ff515303 100644
--- a/deployment/components/worker-config/nfd-worker.conf.example
+++ b/deployment/components/worker-config/nfd-worker.conf.example
@@ -150,6 +150,10 @@
 #            major: {op: In, value: ["5"]}
 #            minor: {op: Gt, value: ["10"]}
 #
+#        - storage.block:
+#            rotational: {op: In, value: ["0"]}
+#            dax: {op: In, value: ["0"]}
+#
 #        - 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 1765d65e3..115a7d181 100644
--- a/deployment/helm/node-feature-discovery/values.yaml
+++ b/deployment/helm/node-feature-discovery/values.yaml
@@ -238,6 +238,10 @@ worker:
     #            major: {op: In, value: ["5"]}
     #            minor: {op: Gt, value: ["10"]}
     #
+    #        - storage.block:
+    #            rotational: {op: In, value: ["0"]}
+    #            dax: {op: In, value: ["0"]}
+    #
     #        - feature: system.osrelease
     #          matchExpressions:
     #            ID: {op: In, value: ["fedora", "centos"]}
diff --git a/source/storage/storage.go b/source/storage/storage.go
index 3770e8657..6f7555e54 100644
--- a/source/storage/storage.go
+++ b/source/storage/storage.go
@@ -19,21 +19,35 @@ package storage
 import (
 	"fmt"
 	"io/ioutil"
+	"path/filepath"
+	"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 = "storage"
 
-// storageSource implements the LabelSource interface.
-type storageSource struct{}
+const BlockFeature = "block"
+
+// storageSource implements the FeatureSource and LabelSource interfaces.
+type storageSource struct {
+	features *feature.DomainFeatures
+}
 
 // Singleton source instance
 var (
 	src storageSource
-	_   source.LabelSource = &src
+	_   source.FeatureSource = &src
+	_   source.LabelSource   = &src
 )
 
+// queueAttrs is the list of files under /sys/block/<dev>/queue that we're trying to read
+var queueAttrs = []string{"dax", "rotational", "nr_zones", "zoned"}
+
 // Name returns an identifier string for this feature source.
 func (s *storageSource) Name() string { return Name }
 
@@ -42,25 +56,70 @@ func (s *storageSource) Priority() int { return 0 }
 
 // GetLabels method of the LabelSource interface
 func (s *storageSource) GetLabels() (source.FeatureLabels, error) {
-	features := source.FeatureLabels{}
-
-	// Check if there is any non-rotational block devices attached to the node
-	blockdevices, err := ioutil.ReadDir(source.SysfsDir.Path("block"))
-	if err == nil {
-		for _, bdev := range blockdevices {
-			fname := source.SysfsDir.Path("block", bdev.Name(), "queue/rotational")
-			bytes, err := ioutil.ReadFile(fname)
-			if err != nil {
-				return nil, fmt.Errorf("can't read rotational status: %s", err.Error())
-			}
-			if bytes[0] == byte('0') {
-				// Non-rotational storage is present, add label.
-				features["nonrotationaldisk"] = true
-				break
-			}
+	labels := source.FeatureLabels{}
+	features := s.GetFeatures()
+
+	for _, dev := range features.Instances[BlockFeature].Elements {
+		if dev.Attributes["rotational"] == "0" {
+			labels["nonrotationaldisk"] = true
+			break
+		}
+	}
+
+	return labels, nil
+}
+
+// Discover method of the FeatureSource interface
+func (s *storageSource) Discover() error {
+	s.features = feature.NewDomainFeatures()
+
+	devs, err := detectBlock()
+	if err != nil {
+		return fmt.Errorf("failed to detect block devices: %w", err)
+	}
+	s.features.Instances[BlockFeature] = feature.InstanceFeatureSet{Elements: devs}
+
+	utils.KlogDump(3, "discovered storage features:", "  ", s.features)
+
+	return nil
+}
+
+// GetFeatures method of the FeatureSource Interface.
+func (s *storageSource) GetFeatures() *feature.DomainFeatures {
+	if s.features == nil {
+		s.features = feature.NewDomainFeatures()
+	}
+	return s.features
+}
+
+func detectBlock() ([]feature.InstanceFeature, error) {
+	sysfsBasePath := source.SysfsDir.Path("block")
+
+	blockdevices, err := ioutil.ReadDir(sysfsBasePath)
+	if err != nil {
+		return nil, fmt.Errorf("failed to list block devices: %w", err)
+	}
+
+	// Iterate over devices
+	info := make([]feature.InstanceFeature, 0, len(blockdevices))
+	for _, device := range blockdevices {
+		info = append(info, *readBlockDevQueueInfo(filepath.Join(sysfsBasePath, device.Name())))
+	}
+
+	return info, nil
+}
+
+func readBlockDevQueueInfo(path string) *feature.InstanceFeature {
+	attrs := map[string]string{"name": filepath.Base(path)}
+	for _, attrName := range queueAttrs {
+		data, err := ioutil.ReadFile(filepath.Join(path, "queue", attrName))
+		if err != nil {
+			klog.V(3).Infof("failed to read block device queue attribute %s: %w", attrName, err)
+			continue
 		}
+		attrs[attrName] = strings.TrimSpace(string(data))
 	}
-	return features, nil
+	return feature.NewInstanceFeature(attrs)
 }
 
 func init() {


Test patch:
diff --git a/source/storage/storage_test.go b/source/storage/storage_test.go
new file mode 100644
index 000000000..8f501a892
--- /dev/null
+++ b/source/storage/storage_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 storage
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"sigs.k8s.io/node-feature-discovery/pkg/api/feature"
+)
+
+func TestStorageSource(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)
+
+}