Repository: kubernetes-sigs/node-feature-discovery
Pull request #673 (2021-11-25 12:21:07)

nfd-worker: rename 'sources' config option
Rename `core.sources` config file option to `core.labelSources` and `-sources` command line flag to `-label-sources`. Also provide backwards compatibility via deprecated options. Multiple smaller pieces as separate commits:

- rename config option `sources` to `labelSources`
  The goal is to make the name more descriptive. Also keeping in mind a
possible future addition a 'featureSources' option (or similar) for
controlling the feature discovery.
- provide deprecated core.sources config option 
  Provide backwards compatibility via a deprecated 'core.sources' config
file option. This will override 'core.labelSources'. A warning is
printed in the log if this option is detected.
- introduce -label-sources cmdline flag 
  Useful for development, testing and debugging.
- update docs on label-sources option 
  Update documentation on core.labelSources/core.sources config file
options and -label-sources/-sources command line flags.


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/673#" 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: Let's see what I think about this after sleeping over it 🧐
/hold
k8s-ci-robot: @marquiz: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-node-feature-discovery-build-image | 6b4de00cdfb4abf60acb112616ae3667647a7d8b | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_node-feature-discovery/673/pull-node-feature-discovery-build-image/1463845204082561024) | true | `/test pull-node-feature-discovery-build-image`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=node-feature-discovery&pr=673). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Amarquiz). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/node-feature-discovery/issues?q=is:issue+is:open) when you hit one in your PR.

<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. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
marquiz: Depends on #671 
ArangoGutierrez: /test all

Patch:
diff --git a/cmd/nfd-worker/main.go b/cmd/nfd-worker/main.go
index 2867646c1..836505a1c 100644
--- a/cmd/nfd-worker/main.go
+++ b/cmd/nfd-worker/main.go
@@ -80,6 +80,8 @@ func parseArgs(flags *flag.FlagSet, osArgs ...string) *worker.Args {
 		switch f.Name {
 		case "no-publish":
 			args.Overrides.NoPublish = overrides.NoPublish
+		case "label-sources":
+			args.Overrides.LabelSources = overrides.LabelSources
 		case "label-whitelist":
 			klog.Warningf("-label-whitelist is deprecated, use 'core.labelWhiteList' option in the config file, instead")
 			args.Overrides.LabelWhiteList = overrides.LabelWhiteList
@@ -87,8 +89,8 @@ func parseArgs(flags *flag.FlagSet, osArgs ...string) *worker.Args {
 			klog.Warningf("-sleep-interval is deprecated, use 'core.sleepInterval' option in the config file, instead")
 			args.Overrides.SleepInterval = overrides.SleepInterval
 		case "sources":
-			klog.Warningf("-sources is deprecated, use 'core.sources' option in the config file, instead")
-			args.Overrides.Sources = overrides.Sources
+			klog.Warningf("-sources is deprecated, use '-label-sources' flag, instead")
+			args.Overrides.LabelSources = overrides.LabelSources
 		}
 	})
 
@@ -121,10 +123,12 @@ func initFlags(flagset *flag.FlagSet) (*worker.Args, *worker.ConfigOverrideArgs)
 	// Flags overlapping with config file options
 	overrides := &worker.ConfigOverrideArgs{
 		LabelWhiteList: &utils.RegexpVal{},
-		Sources:        &utils.StringSliceVal{},
+		LabelSources:   &utils.StringSliceVal{},
 	}
 	overrides.NoPublish = flagset.Bool("no-publish", false,
 		"Do not publish discovered features, disable connection to nfd-master.")
+	flagset.Var(overrides.LabelSources, "label-sources",
+		"Comma separated list of label sources. Special value 'all' enables all feature sources.")
 	flagset.Var(overrides.LabelWhiteList, "label-whitelist",
 		"Regular expression to filter label names to publish to the Kubernetes API server. "+
 			"NB: the label namespace is omitted i.e. the filter is only applied to the name part after '/'. "+
@@ -132,9 +136,9 @@ func initFlags(flagset *flag.FlagSet) (*worker.Args, *worker.ConfigOverrideArgs)
 	overrides.SleepInterval = flagset.Duration("sleep-interval", 0,
 		"Time to sleep between re-labeling. Non-positive value implies no re-labeling (i.e. infinite sleep). "+
 			"DEPRECATED: This parameter should be set via the config file")
-	flagset.Var(overrides.Sources, "sources",
-		"Comma separated list of feature sources. Special value 'all' enables all feature sources. "+
-			"DEPRECATED: This parameter should be set via the config file")
+	flagset.Var(overrides.LabelSources, "sources",
+		"Comma separated list of label sources. Special value 'all' enables all feature sources. "+
+			"DEPRECATED: use -label-sources instead")
 
 	return args, overrides
 }
diff --git a/deployment/components/worker-config/nfd-worker.conf.example b/deployment/components/worker-config/nfd-worker.conf.example
index 2a5d2961b..11a63d725 100644
--- a/deployment/components/worker-config/nfd-worker.conf.example
+++ b/deployment/components/worker-config/nfd-worker.conf.example
@@ -2,7 +2,7 @@
 #  labelWhiteList:
 #  noPublish: false
 #  sleepInterval: 60s
-#  sources: [all]
+#  labelSources: [all]
 #  klog:
 #    addDirHeader: false
 #    alsologtostderr: false
diff --git a/deployment/helm/node-feature-discovery/values.yaml b/deployment/helm/node-feature-discovery/values.yaml
index b7b50142b..a76f7bce1 100644
--- a/deployment/helm/node-feature-discovery/values.yaml
+++ b/deployment/helm/node-feature-discovery/values.yaml
@@ -91,7 +91,7 @@ worker:
     #  labelWhiteList:
     #  noPublish: false
     #  sleepInterval: 60s
-    #  sources: [all]
+    #  labelSources: [all]
     #  klog:
     #    addDirHeader: false
     #    alsologtostderr: false
diff --git a/docs/advanced/worker-commandline-reference.md b/docs/advanced/worker-commandline-reference.md
index 1d60d4293..2328c7ab4 100644
--- a/docs/advanced/worker-commandline-reference.md
+++ b/docs/advanced/worker-commandline-reference.md
@@ -136,12 +136,14 @@ Example:
 nfd-worker -server-name-override=localhost
 ```
 
-### -sources
+### -label-sources
 
-The `-sources` flag specifies a comma-separated list of enabled feature
-sources. A special value `all` enables all feature sources.
+The `-label-sources` flag specifies a comma-separated list of enabled label
+sources. A special value `all` enables all sources. Consider using the
+`core.labelSources` config file option, instead, allowing dynamic
+configurability.
 
-Note: This flag takes precedence over the `core.sources` configuration
+Note: This flag takes precedence over the `core.labelSources` configuration
 file option.
 
 Default: all
@@ -149,11 +151,12 @@ Default: all
 Example:
 
 ```bash
-nfd-worker -sources=kernel,system,local
+nfd-worker -label-sources=kernel,system,local
 ```
 
-**DEPRECATED**: you should use the `core.sources` option in the
-configuration file, instead.
+### -sources
+
+**DEPRECATED**: use [`-label-sources`](#-label-sources) instead.
 
 ### -no-publish
 
diff --git a/docs/advanced/worker-configuration-reference.md b/docs/advanced/worker-configuration-reference.md
index bdd400d6a..215740e1a 100644
--- a/docs/advanced/worker-configuration-reference.md
+++ b/docs/advanced/worker-configuration-reference.md
@@ -43,13 +43,12 @@ core:
   sleepInterval: 60s
 ```
 
-### core.sources
+### core.labelSources
 
-`core.sources` specifies the list of enabled feature sources. A special value
-`all` enables all feature sources.
+`core.labelSources` specifies the list of enabled label sources. A special
+value `all` enables all sources.
 
-Note: Overridden by the deprecated `-sources` command line flag (if
-specified).
+Note: Overridden by the `-label-sources` command line flag (if specified).
 
 Default: `[all]`
 
@@ -62,6 +61,13 @@ core:
     - custom
 ```
 
+### core.sources
+
+**DEPRECATED**: use [`core.labelSources`](#core.labelSources) instead.
+
+Note: `core.sources` takes precedence over the `core.labelSources`
+configuration file option.
+
 ### core.labelWhiteList
 
 `core.labelWhiteList` specifies a regular expression for filtering feature
diff --git a/docs/get-started/features.md b/docs/get-started/features.md
index 7fa16a8fb..e1f84ac8e 100644
--- a/docs/get-started/features.md
+++ b/docs/get-started/features.md
@@ -50,7 +50,8 @@ The last component (i.e. `attribute-name`) is optional, and only used if a
 feature logically has sub-hierarchy, e.g. `sriov.capable` and
 `sriov.configure` from the `network` source.
 
-The `-sources` flag controls which sources to use for discovery.
+The `-label-sources` flag controls which sources to enable for label
+generation.
 
 *Note: Consecutive runs of nfd-worker will update the labels on a
 given node. If features are not discovered on a consecutive run, the corresponding
diff --git a/pkg/nfd-client/worker/nfd-worker.go b/pkg/nfd-client/worker/nfd-worker.go
index 5d2395844..a37add85b 100644
--- a/pkg/nfd-client/worker/nfd-worker.go
+++ b/pkg/nfd-client/worker/nfd-worker.go
@@ -64,7 +64,8 @@ type coreConfig struct {
 	Klog           map[string]string
 	LabelWhiteList utils.RegexpVal
 	NoPublish      bool
-	Sources        []string
+	Sources        *[]string
+	LabelSources   []string
 	SleepInterval  duration
 }
 
@@ -92,7 +93,7 @@ type ConfigOverrideArgs struct {
 	// Deprecated
 	LabelWhiteList *utils.RegexpVal
 	SleepInterval  *time.Duration
-	Sources        *utils.StringSliceVal
+	LabelSources   *utils.StringSliceVal
 }
 
 type nfdWorker struct {
@@ -138,7 +139,7 @@ func newDefaultConfig() *NFDConfig {
 		Core: coreConfig{
 			LabelWhiteList: utils.RegexpVal{Regexp: *regexp.MustCompile("")},
 			SleepInterval:  duration{60 * time.Second},
-			Sources:        []string{"all"},
+			LabelSources:   []string{"all"},
 			Klog:           make(map[string]string),
 		},
 	}
@@ -294,7 +295,7 @@ func (w *nfdWorker) configureCore(c coreConfig) error {
 
 	// Determine enabled feature sources
 	enabled := make(map[string]source.LabelSource)
-	for _, name := range c.Sources {
+	for _, name := range c.LabelSources {
 		if name == "all" {
 			for n, s := range source.GetAllLabelSources() {
 				if ts, ok := s.(source.TestSource); !ok || !ts.IsTestSource() {
@@ -358,6 +359,12 @@ func (w *nfdWorker) configure(filepath string, overrides string) error {
 			if err != nil {
 				return fmt.Errorf("failed to parse config file: %s", err)
 			}
+
+			if c.Core.Sources != nil {
+				klog.Warningf("found deprecated 'core.sources' config file option, please use 'core.labelSources' instead")
+				c.Core.LabelSources = *c.Core.Sources
+			}
+
 			klog.Infof("configuration file %q parsed", filepath)
 		}
 	}
@@ -376,8 +383,8 @@ func (w *nfdWorker) configure(filepath string, overrides string) error {
 	if w.args.Overrides.SleepInterval != nil {
 		c.Core.SleepInterval = duration{*w.args.Overrides.SleepInterval}
 	}
-	if w.args.Overrides.Sources != nil {
-		c.Core.Sources = *w.args.Overrides.Sources
+	if w.args.Overrides.LabelSources != nil {
+		c.Core.LabelSources = *w.args.Overrides.LabelSources
 	}
 
 	c.Core.sanitize()


Test patch:
diff --git a/cmd/nfd-worker/main_test.go b/cmd/nfd-worker/main_test.go
index 1ff22f5e1..043eedb00 100644
--- a/cmd/nfd-worker/main_test.go
+++ b/cmd/nfd-worker/main_test.go
@@ -38,7 +38,7 @@ func TestParseArgs(t *testing.T) {
 				So(args.Overrides.NoPublish, ShouldBeNil)
 				So(args.Overrides.LabelWhiteList, ShouldBeNil)
 				So(args.Overrides.SleepInterval, ShouldBeNil)
-				So(args.Overrides.Sources, ShouldBeNil)
+				So(args.Overrides.LabelSources, ShouldBeNil)
 			})
 		})
 
@@ -46,14 +46,14 @@ func TestParseArgs(t *testing.T) {
 			args := parseArgs(flags,
 				"-no-publish",
 				"-label-whitelist=.*rdt.*",
-				"-sources=fake1,fake2,fake3",
+				"-label-sources=fake1,fake2,fake3",
 				"-sleep-interval=30s")
 
 			Convey("args.sources is set to appropriate values", func() {
 				So(args.Oneshot, ShouldBeFalse)
 				So(*args.Overrides.NoPublish, ShouldBeTrue)
 				So(*args.Overrides.SleepInterval, ShouldEqual, 30*time.Second)
-				So(*args.Overrides.Sources, ShouldResemble, utils.StringSliceVal{"fake1", "fake2", "fake3"})
+				So(*args.Overrides.LabelSources, ShouldResemble, utils.StringSliceVal{"fake1", "fake2", "fake3"})
 				So(args.Overrides.LabelWhiteList.Regexp.String(), ShouldResemble, ".*rdt.*")
 			})
 		})
diff --git a/pkg/nfd-client/worker/nfd-worker-internal_test.go b/pkg/nfd-client/worker/nfd-worker-internal_test.go
index e81c36a66..e1804bdb0 100644
--- a/pkg/nfd-client/worker/nfd-worker-internal_test.go
+++ b/pkg/nfd-client/worker/nfd-worker-internal_test.go
@@ -100,22 +100,22 @@ func TestConfigParse(t *testing.T) {
 		w, err := NewNfdWorker(&Args{})
 		So(err, ShouldBeNil)
 		worker := w.(*nfdWorker)
-		overrides := `{"core": {"sources": ["fake"],"noPublish": true},"sources": {"cpu": {"cpuid": {"attributeBlacklist": ["foo","bar"]}}}}`
+		overrides := `{"core": {"labelSources": ["fake"],"noPublish": true},"sources": {"cpu": {"cpuid": {"attributeBlacklist": ["foo","bar"]}}}}`
 
 		Convey("and no core cmdline flags have been specified", func() {
 			So(worker.configure("non-existing-file", overrides), ShouldBeNil)
 
 			Convey("core overrides should be in effect", func() {
-				So(worker.config.Core.Sources, ShouldResemble, []string{"fake"})
+				So(worker.config.Core.LabelSources, ShouldResemble, []string{"fake"})
 				So(worker.config.Core.NoPublish, ShouldBeTrue)
 			})
 		})
 		Convey("and a non-accessible file, but core cmdline flags and some overrides are specified", func() {
-			worker.args = Args{Overrides: ConfigOverrideArgs{Sources: &utils.StringSliceVal{"cpu", "kernel", "pci"}}}
+			worker.args = Args{Overrides: ConfigOverrideArgs{LabelSources: &utils.StringSliceVal{"cpu", "kernel", "pci"}}}
 			So(worker.configure("non-existing-file", overrides), ShouldBeNil)
 
 			Convey("core cmdline flags should be in effect instead overrides", func() {
-				So(worker.config.Core.Sources, ShouldResemble, []string{"cpu", "kernel", "pci"})
+				So(worker.config.Core.LabelSources, ShouldResemble, []string{"cpu", "kernel", "pci"})
 			})
 			Convey("overrides should take effect", func() {
 				So(worker.config.Core.NoPublish, ShouldBeTrue)
@@ -145,13 +145,13 @@ sources:
 		So(err, ShouldBeNil)
 
 		Convey("and a proper config file is specified", func() {
-			worker.args = Args{Overrides: ConfigOverrideArgs{Sources: &utils.StringSliceVal{"cpu", "kernel", "pci"}}}
+			worker.args = Args{Overrides: ConfigOverrideArgs{LabelSources: &utils.StringSliceVal{"cpu", "kernel", "pci"}}}
 			So(worker.configure(f.Name(), ""), ShouldBeNil)
 
 			Convey("specified configuration should take effect", func() {
 				// Verify core config
 				So(worker.config.Core.NoPublish, ShouldBeFalse)
-				So(worker.config.Core.Sources, ShouldResemble, []string{"cpu", "kernel", "pci"}) // from cmdline
+				So(worker.config.Core.LabelSources, ShouldResemble, []string{"cpu", "kernel", "pci"}) // from cmdline
 				So(worker.config.Core.LabelWhiteList.String(), ShouldEqual, "foo")
 				So(worker.config.Core.SleepInterval.Duration, ShouldEqual, 10*time.Second)
 
@@ -167,13 +167,13 @@ sources:
 		Convey("and a proper config file and overrides are given", func() {
 			sleepIntervalArg := 15 * time.Second
 			worker.args = Args{Overrides: ConfigOverrideArgs{SleepInterval: &sleepIntervalArg}}
-			overrides := `{"core": {"sources": ["fake"],"noPublish": true},"sources": {"pci": {"deviceClassWhitelist": ["03"]}}}`
+			overrides := `{"core": {"labelSources": ["fake"],"noPublish": true},"sources": {"pci": {"deviceClassWhitelist": ["03"]}}}`
 			So(worker.configure(f.Name(), overrides), ShouldBeNil)
 
 			Convey("overrides should take precedence over the config file", func() {
 				// Verify core config
 				So(worker.config.Core.NoPublish, ShouldBeTrue)
-				So(worker.config.Core.Sources, ShouldResemble, []string{"fake"}) // from overrides
+				So(worker.config.Core.LabelSources, ShouldResemble, []string{"fake"}) // from overrides
 				So(worker.config.Core.LabelWhiteList.String(), ShouldEqual, "foo")
 				So(worker.config.Core.SleepInterval.Duration, ShouldEqual, 15*time.Second) // from cmdline
 
@@ -220,8 +220,8 @@ core:
 		w, err := NewNfdWorker(&Args{
 			ConfigFile: configFile,
 			Overrides: ConfigOverrideArgs{
-				Sources:   &utils.StringSliceVal{"fake"},
-				NoPublish: &noPublish},
+				LabelSources: &utils.StringSliceVal{"fake"},
+				NoPublish:    &noPublish},
 		})
 		So(err, ShouldBeNil)
 		worker := w.(*nfdWorker)
@@ -314,7 +314,7 @@ func TestNewNfdWorker(t *testing.T) {
 		})
 
 		Convey("with non-empty Sources arg specified", func() {
-			args := &Args{Overrides: ConfigOverrideArgs{Sources: &utils.StringSliceVal{"fake"}}}
+			args := &Args{Overrides: ConfigOverrideArgs{LabelSources: &utils.StringSliceVal{"fake"}}}
 			w, err := NewNfdWorker(args)
 			Convey("no error should be returned", func() {
 				So(err, ShouldBeNil)
diff --git a/pkg/nfd-client/worker/nfd-worker_test.go b/pkg/nfd-client/worker/nfd-worker_test.go
index 9f7a7e456..33892ea8a 100644
--- a/pkg/nfd-client/worker/nfd-worker_test.go
+++ b/pkg/nfd-client/worker/nfd-worker_test.go
@@ -97,7 +97,7 @@ func TestRun(t *testing.T) {
 				Args: nfdclient.Args{
 					Server: "localhost:8192"},
 				Oneshot:   true,
-				Overrides: worker.ConfigOverrideArgs{Sources: &utils.StringSliceVal{"fake"}},
+				Overrides: worker.ConfigOverrideArgs{LabelSources: &utils.StringSliceVal{"fake"}},
 			}
 			fooasdf, _ := worker.NewNfdWorker(args)
 			err := fooasdf.Run()
@@ -128,7 +128,7 @@ func TestRunTls(t *testing.T) {
 					ServerNameOverride: "nfd-test-master",
 				},
 				Oneshot:   true,
-				Overrides: worker.ConfigOverrideArgs{Sources: &utils.StringSliceVal{"fake"}},
+				Overrides: worker.ConfigOverrideArgs{LabelSources: &utils.StringSliceVal{"fake"}},
 			}
 			w, _ := worker.NewNfdWorker(&workerArgs)
 			err := w.Run()