Repository: kubernetes-sigs/node-feature-discovery
Pull request #601 (2021-09-20 04:48:07)

source: introduce FeatureSource interface
Specify a new interface for managing "raw" feature data. This is the
first step to separate raw feature data from node labels. None of the
feature sources implement this interface, yet.

This patch unifies the data format of "raw" features by dividing them
into three different basic types.
- `keys`, a set of names without any associated values, e.g. CPUID flags
  or loaded kernel modules
- `values`, a map of key-value pairs, for features with a single value,
  e.g. kernel config flags or os version
- `instances`, a list of instances each of which has multiple attributes
  (key-value pairs of their own), e.g. PCI or USB devices

The new feature data types are defined in a new "pkg/api/feature"
package, catering decoupling and re-usability of code e.g. within future
extentions of the NFD gRPC API.

Rename the `Discover()` method of LabelSource interface to `GetLabels()`.

This is part of #464 - a separate smaller PR hopefully making review easier.

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/601#" 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":[]} -->

Patch:
diff --git a/pkg/api/feature/feature.go b/pkg/api/feature/feature.go
new file mode 100644
index 000000000..08dafb8cf
--- /dev/null
+++ b/pkg/api/feature/feature.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 feature
+
+// NewDomainFeatures creates a new instance of Features, initializing specified
+// features to empty values
+func NewDomainFeatures() *DomainFeatures {
+	return &DomainFeatures{
+		Keys:      make(map[string]KeyFeatureSet),
+		Values:    make(map[string]ValueFeatureSet),
+		Instances: make(map[string]InstanceFeatureSet)}
+}
+
+func NewKeyFeatures() *KeyFeatureSet { return &KeyFeatureSet{Elements: make(map[string]Nil)} }
+
+func NewValueFeatures() *ValueFeatureSet { return &ValueFeatureSet{Elements: make(map[string]string)} }
+
+func NewInstanceFeatures() *InstanceFeatureSet { return &InstanceFeatureSet{} }
+
+func NewInstanceFeature() *InstanceFeature {
+	return &InstanceFeature{Attributes: make(map[string]string)}
+}
diff --git a/pkg/api/feature/types.go b/pkg/api/feature/types.go
new file mode 100644
index 000000000..dbcf6655a
--- /dev/null
+++ b/pkg/api/feature/types.go
@@ -0,0 +1,51 @@
+/*
+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 feature
+
+// Features is a collection of all features of the system, arranged by domain.
+// +protobuf
+type Features map[string]*DomainFeatures
+
+// DomainFeatures is the collection of all discovered features of one domain.
+type DomainFeatures struct {
+	Keys      map[string]KeyFeatureSet      `protobuf:"bytes,1,rep,name=keys"`
+	Values    map[string]ValueFeatureSet    `protobuf:"bytes,2,rep,name=values"`
+	Instances map[string]InstanceFeatureSet `protobuf:"bytes,3,rep,name=instances"`
+}
+
+// KeyFeatureSet is a set of simple features only containing names without values.
+type KeyFeatureSet struct {
+	Elements map[string]Nil `protobuf:"bytes,1,rep,name=elements"`
+}
+
+// ValueFeatureSet is a set of features having string value.
+type ValueFeatureSet struct {
+	Elements map[string]string `protobuf:"bytes,1,rep,name=elements"`
+}
+
+// InstanceFeatureSet is a set of features each of which is an instance having multiple attributes.
+type InstanceFeatureSet struct {
+	Elements []InstanceFeature `protobuf:"bytes,1,rep,name=elements"`
+}
+
+// InstanceFeature represents one instance of a complex features, e.g. a device.
+type InstanceFeature struct {
+	Attributes map[string]string `protobuf:"bytes,1,rep,name=attributes"`
+}
+
+// Nil is a dummy empty struct for protobuf compatibility
+type Nil struct{}
diff --git a/pkg/nfd-client/worker/nfd-worker.go b/pkg/nfd-client/worker/nfd-worker.go
index 3d8c47770..1db56bc3a 100644
--- a/pkg/nfd-client/worker/nfd-worker.go
+++ b/pkg/nfd-client/worker/nfd-worker.go
@@ -175,6 +175,14 @@ func (w *nfdWorker) Run() error {
 	for {
 		select {
 		case <-labelTrigger:
+			// Run feature discovery
+			for n, s := range source.GetAllFeatureSources() {
+				klog.V(2).Infof("running discovery for %q source", n)
+				if err := s.Discover(); err != nil {
+					klog.Errorf("feature discovery of %q source failed: %v", n, err)
+				}
+			}
+
 			// Get the set of feature labels.
 			labels := createFeatureLabels(w.enabledSources, w.config.Core.LabelWhiteList.Regexp)
 
@@ -394,7 +402,7 @@ func (w *nfdWorker) configure(filepath string, overrides string) error {
 func createFeatureLabels(sources []source.LabelSource, labelWhiteList regexp.Regexp) (labels Labels) {
 	labels = Labels{}
 
-	// Do feature discovery from all configured sources.
+	// Get labels from all enabled label sources
 	klog.Info("starting feature discovery...")
 	for _, source := range sources {
 		labelsFromSource, err := getFeatureLabels(source, labelWhiteList)
@@ -416,7 +424,7 @@ func createFeatureLabels(sources []source.LabelSource, labelWhiteList regexp.Reg
 // supplied source.
 func getFeatureLabels(source source.LabelSource, labelWhiteList regexp.Regexp) (labels Labels, err error) {
 	labels = Labels{}
-	features, err := source.Discover()
+	features, err := source.GetLabels()
 	if err != nil {
 		return nil, err
 	}
diff --git a/source/cpu/cpu.go b/source/cpu/cpu.go
index c23fff4f3..8fa65f373 100644
--- a/source/cpu/cpu.go
+++ b/source/cpu/cpu.go
@@ -113,7 +113,8 @@ func (s *cpuSource) SetConfig(conf source.Config) {
 // Priority method of the LabelSource interface
 func (s *cpuSource) Priority() int { return 0 }
 
-func (s *cpuSource) Discover() (source.FeatureLabels, error) {
+// GetLabels method of the LabelSource interface
+func (s *cpuSource) GetLabels() (source.FeatureLabels, error) {
 	features := source.FeatureLabels{}
 
 	// Check if hyper-threading seems to be enabled
diff --git a/source/custom/custom.go b/source/custom/custom.go
index 6bfaf582a..fdaf76269 100644
--- a/source/custom/custom.go
+++ b/source/custom/custom.go
@@ -85,8 +85,8 @@ func (s *customSource) SetConfig(conf source.Config) {
 // Priority method of the LabelSource interface
 func (s *customSource) Priority() int { return 10 }
 
-// Discover features
-func (s *customSource) Discover() (source.FeatureLabels, error) {
+// GetLabels method of the LabelSource interface
+func (s *customSource) GetLabels() (source.FeatureLabels, error) {
 	features := source.FeatureLabels{}
 	allFeatureConfig := append(getStaticFeatureConfig(), *s.config...)
 	allFeatureConfig = append(allFeatureConfig, getDirectoryFeatureConfig()...)
diff --git a/source/fake/fake.go b/source/fake/fake.go
index 9d9f8d2aa..edab2f549 100644
--- a/source/fake/fake.go
+++ b/source/fake/fake.go
@@ -77,8 +77,8 @@ func (s *fakeSource) Configure([]byte) error { return nil }
 // Priority method of the LabelSource interface
 func (s *fakeSource) Priority() int { return 0 }
 
-// Discover returns feature names for some fake features.
-func (s *fakeSource) Discover() (source.FeatureLabels, error) {
+// GetLabels method of the LabelSource interface
+func (s *fakeSource) GetLabels() (source.FeatureLabels, error) {
 	// Adding three fake features.
 	features := make(source.FeatureLabels, len(s.config.Labels))
 	for k, v := range s.config.Labels {
diff --git a/source/iommu/iommu.go b/source/iommu/iommu.go
index f731bf934..d22b8ef0a 100644
--- a/source/iommu/iommu.go
+++ b/source/iommu/iommu.go
@@ -39,7 +39,8 @@ var (
 // Priority method of the LabelSource interface
 func (s *iommuSource) Priority() int { return 0 }
 
-func (s *iommuSource) Discover() (source.FeatureLabels, error) {
+// GetLabels method of the LabelSource interface
+func (s *iommuSource) GetLabels() (source.FeatureLabels, error) {
 	features := source.FeatureLabels{}
 
 	// Check if any iommu devices are available
diff --git a/source/kernel/kernel.go b/source/kernel/kernel.go
index 045809135..5e0de7a4a 100644
--- a/source/kernel/kernel.go
+++ b/source/kernel/kernel.go
@@ -80,7 +80,8 @@ func (s *kernelSource) SetConfig(conf source.Config) {
 // Priority method of the LabelSource interface
 func (s *kernelSource) Priority() int { return 0 }
 
-func (s *kernelSource) Discover() (source.FeatureLabels, error) {
+// GetLabels method of the LabelSource interface
+func (s *kernelSource) GetLabels() (source.FeatureLabels, error) {
 	features := source.FeatureLabels{}
 
 	// Read kernel version
diff --git a/source/local/local.go b/source/local/local.go
index dc3427fe1..7c77db26a 100644
--- a/source/local/local.go
+++ b/source/local/local.go
@@ -53,8 +53,8 @@ func (s *localSource) Name() string { return Name }
 // Priority method of the LabelSource interface
 func (s *localSource) Priority() int { return 20 }
 
-// Discover method of the LabelSource interface
-func (s *localSource) Discover() (source.FeatureLabels, error) {
+// GetLabels method of the LabelSource interface
+func (s *localSource) GetLabels() (source.FeatureLabels, error) {
 	featuresFromHooks, err := getFeaturesFromHooks()
 	if err != nil {
 		klog.Error(err)
diff --git a/source/memory/memory.go b/source/memory/memory.go
index 9fde92010..244503887 100644
--- a/source/memory/memory.go
+++ b/source/memory/memory.go
@@ -43,8 +43,8 @@ func (s *memorySource) Name() string { return Name }
 // Priority method of the LabelSource interface
 func (s *memorySource) Priority() int { return 0 }
 
-// Discover returns feature names for memory: numa if more than one memory node is present.
-func (s *memorySource) Discover() (source.FeatureLabels, error) {
+// GetLabels method of the LabelSource interface
+func (s *memorySource) GetLabels() (source.FeatureLabels, error) {
 	features := source.FeatureLabels{}
 
 	// Detect NUMA
diff --git a/source/mock_LabelSource.go b/source/mock_LabelSource.go
index 96865c6fb..173f40796 100644
--- a/source/mock_LabelSource.go
+++ b/source/mock_LabelSource.go
@@ -11,8 +11,8 @@ type MockLabelSource struct {
 	mock.Mock
 }
 
-// Discover provides a mock function with given fields:
-func (_m *MockLabelSource) Discover() (FeatureLabels, error) {
+// GetLabels provides a mock function with given fields:
+func (_m *MockLabelSource) GetLabels() (FeatureLabels, error) {
 	ret := _m.Called()
 
 	var r0 FeatureLabels
diff --git a/source/network/network.go b/source/network/network.go
index 2ebf26778..ebda09409 100644
--- a/source/network/network.go
+++ b/source/network/network.go
@@ -56,8 +56,8 @@ func (s *networkSource) Name() string { return Name }
 // Priority method of the LabelSource interface
 func (s *networkSource) Priority() int { return 0 }
 
-// Discover returns feature names sriov-configured and sriov if SR-IOV capable NICs are present and/or SR-IOV virtual functions are configured on the node
-func (s *networkSource) Discover() (source.FeatureLabels, error) {
+// GetLabels method of the LabelSource interface
+func (s *networkSource) GetLabels() (source.FeatureLabels, error) {
 	features := source.FeatureLabels{}
 
 	netInterfaces, err := ioutil.ReadDir(source.SysfsDir.Path(sysfsBaseDir))
diff --git a/source/pci/pci.go b/source/pci/pci.go
index b100f0fbe..de7a9f71a 100644
--- a/source/pci/pci.go
+++ b/source/pci/pci.go
@@ -75,8 +75,8 @@ func (s *pciSource) SetConfig(conf source.Config) {
 // Priority method of the LabelSource interface
 func (s *pciSource) Priority() int { return 0 }
 
-// Discover features
-func (s *pciSource) Discover() (source.FeatureLabels, error) {
+// GetLabels method of the LabelSource interface
+func (s *pciSource) GetLabels() (source.FeatureLabels, error) {
 	features := source.FeatureLabels{}
 
 	// Construct a device label format, a sorted list of valid attributes
diff --git a/source/source.go b/source/source.go
index e490857f5..967c24f92 100644
--- a/source/source.go
+++ b/source/source.go
@@ -18,6 +18,8 @@ package source
 
 import (
 	"fmt"
+
+	"sigs.k8s.io/node-feature-discovery/pkg/api/feature"
 )
 
 // Source is the base interface for all other source interfaces
@@ -26,12 +28,23 @@ type Source interface {
 	Name() string
 }
 
+// FeatureSource is an interface for discovering node features
+type FeatureSource interface {
+	Source
+
+	// Discover does feature discovery
+	Discover() error
+
+	// GetFeatures returns discovered features in raw form
+	GetFeatures() *feature.DomainFeatures
+}
+
 // LabelSource represents a source of node feature labels
 type LabelSource interface {
 	Source
 
-	// Discover returns discovered feature labels
-	Discover() (FeatureLabels, error)
+	// GetLabels returns discovered feature labels
+	GetLabels() (FeatureLabels, error)
 
 	// Priority returns the priority of the source
 	Priority() int
@@ -80,6 +93,25 @@ func Register(s Source) {
 	sources[s.Name()] = s
 }
 
+// GetFeatureSource returns a registered FeatureSource interface
+func GetFeatureSource(name string) FeatureSource {
+	if s, ok := sources[name].(FeatureSource); ok {
+		return s
+	}
+	return nil
+}
+
+// GetAllFeatureSources returns all registered label sources
+func GetAllFeatureSources() map[string]FeatureSource {
+	all := make(map[string]FeatureSource)
+	for k, v := range sources {
+		if s, ok := v.(FeatureSource); ok {
+			all[k] = s
+		}
+	}
+	return all
+}
+
 // GetLabelSource a registered label source
 func GetLabelSource(name string) LabelSource {
 	if s, ok := sources[name].(LabelSource); ok {
diff --git a/source/storage/storage.go b/source/storage/storage.go
index 34383340a..3770e8657 100644
--- a/source/storage/storage.go
+++ b/source/storage/storage.go
@@ -40,8 +40,8 @@ func (s *storageSource) Name() string { return Name }
 // Priority method of the LabelSource interface
 func (s *storageSource) Priority() int { return 0 }
 
-// Discover returns feature names for storage: nonrotationaldisk if any SSD drive present.
-func (s *storageSource) Discover() (source.FeatureLabels, error) {
+// 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
diff --git a/source/system/system.go b/source/system/system.go
index 2ee4ac029..701948ce6 100644
--- a/source/system/system.go
+++ b/source/system/system.go
@@ -48,7 +48,8 @@ func (s *systemSource) Name() string { return Name }
 // Priority method of the LabelSource interface
 func (s *systemSource) Priority() int { return 0 }
 
-func (s *systemSource) Discover() (source.FeatureLabels, error) {
+// GetLabels method of the LabelSource interface
+func (s *systemSource) GetLabels() (source.FeatureLabels, error) {
 	features := source.FeatureLabels{}
 
 	release, err := parseOSRelease()
diff --git a/source/usb/usb.go b/source/usb/usb.go
index 80774fbec..a86cf15d4 100644
--- a/source/usb/usb.go
+++ b/source/usb/usb.go
@@ -78,8 +78,8 @@ func (s *usbSource) SetConfig(conf source.Config) {
 // Priority method of the LabelSource interface
 func (s *usbSource) Priority() int { return 0 }
 
-// Discover features
-func (s *usbSource) Discover() (source.FeatureLabels, error) {
+// GetLabels method of the LabelSource interface
+func (s *usbSource) GetLabels() (source.FeatureLabels, error) {
 	features := source.FeatureLabels{}
 
 	// Construct a device label format, a sorted list of valid attributes


Test patch:
diff --git a/pkg/nfd-client/worker/nfd-worker-internal_test.go b/pkg/nfd-client/worker/nfd-worker-internal_test.go
index 9070c0969..e81c36a66 100644
--- a/pkg/nfd-client/worker/nfd-worker-internal_test.go
+++ b/pkg/nfd-client/worker/nfd-worker-internal_test.go
@@ -39,7 +39,7 @@ import (
 
 const fakeLabelSourceName string = "testSource"
 
-func TestDiscoveryWithMockSources(t *testing.T) {
+func TestGetLabelsWithMockSources(t *testing.T) {
 	Convey("When I discover features from fake source and update the node using fake client", t, func() {
 		mockLabelSource := new(source.MockLabelSource)
 		allFeatureNames := []string{"testfeature1", "testfeature2", "test.ns/test", "test.ns/foo", "/no-ns-label", "invalid/test/feature"}
@@ -54,7 +54,7 @@ func TestDiscoveryWithMockSources(t *testing.T) {
 
 		Convey("When I successfully get the labels from the mock source", func() {
 			mockLabelSource.On("Name").Return(fakeLabelSourceName)
-			mockLabelSource.On("Discover").Return(fakeFeatures, nil)
+			mockLabelSource.On("GetLabels").Return(fakeFeatures, nil)
 
 			returnedLabels, err := getFeatureLabels(fakeLabelSource, labelWhiteList.Regexp)
 			Convey("Proper label is returned", func() {
@@ -67,7 +67,7 @@ func TestDiscoveryWithMockSources(t *testing.T) {
 
 		Convey("When I fail to get the labels from the mock source", func() {
 			expectedError := errors.New("fake error")
-			mockLabelSource.On("Discover").Return(nil, expectedError)
+			mockLabelSource.On("GetLabels").Return(nil, expectedError)
 
 			returnedLabels, err := getFeatureLabels(fakeLabelSource, labelWhiteList.Regexp)
 			Convey("No label is returned", func() {
diff --git a/source/source_test.go b/source/source_test.go
index c8a6980ae..fb1da297e 100644
--- a/source/source_test.go
+++ b/source/source_test.go
@@ -17,6 +17,7 @@ limitations under the License.
 package source_test
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -61,3 +62,19 @@ func TestConfigurableSources(t *testing.T) {
 		assert.Equalf(t, c, rc, "testing ConfigurableSource %q failed", n)
 	}
 }
+
+func TestFeatureSources(t *testing.T) {
+	sources := source.GetAllFeatureSources()
+
+	for n, s := range sources {
+		msg := fmt.Sprintf("testing FeatureSource %q failed", n)
+
+		assert.Equal(t, n, s.Name(), msg)
+
+		f := s.GetFeatures()
+		assert.NotNil(t, f, msg)
+		assert.Empty(t, (*f).Keys, msg)
+		assert.Empty(t, (*f).Values, msg)
+		assert.Empty(t, (*f).Instances, msg)
+	}
+}