Merge pull request #1950 from marquiz/devel/resource-labels

nfd-master: drop resourceLabels
kubernetes-sigs · Nov 7, 2024 · 7d81c85 · 7d81c85
2 parents 5ee8b0f + 45f49d5
commit 7d81c85
Show file tree

Hide file tree

Showing 10 changed files with 22 additions and 105 deletions.
diff --git a/cmd/nfd-master/main.go b/cmd/nfd-master/main.go
@@ -65,9 +65,6 @@ func main() {
 			args.Overrides.DenyLabelNs = overrides.DenyLabelNs
 		case "label-whitelist":
 			args.Overrides.LabelWhiteList = overrides.LabelWhiteList
-		case "resource-labels":
-			klog.InfoS("-resource-labels is deprecated, extended resources should be managed with NodeFeatureRule objects")
-			args.Overrides.ResourceLabels = overrides.ResourceLabels
 		case "enable-taints":
 			args.Overrides.EnableTaints = overrides.EnableTaints
 		case "no-publish":
@@ -132,7 +129,6 @@ func initFlags(flagset *flag.FlagSet) (*master.Args, *master.ConfigOverrideArgs)
 		LabelWhiteList: &utils.RegexpVal{},
 		DenyLabelNs:    &utils.StringSetVal{},
 		ExtraLabelNs:   &utils.StringSetVal{},
-		ResourceLabels: &utils.StringSetVal{},
 		ResyncPeriod:   &utils.DurationVal{Duration: time.Duration(1) * time.Hour},
 	}
 	flagset.Var(overrides.ExtraLabelNs, "extra-label-ns",
@@ -146,8 +142,6 @@ func initFlags(flagset *flag.FlagSet) (*master.Args, *master.ConfigOverrideArgs)
 		"Do not publish feature labels")
 	flagset.Var(overrides.DenyLabelNs, "deny-label-ns",
 		"Comma separated list of denied label namespaces")
-	flagset.Var(overrides.ResourceLabels, "resource-labels",
-		"Comma separated list of labels to be exposed as extended resources. DEPRECATED: use NodeFeatureRule objects instead")
 	flagset.Var(overrides.ResyncPeriod, "resync-period",
 		"Specify the NFD API controller resync period."+
 			"It does not have effect when the NodeFeature API has been disabled (with -feature-gates NodeFeatureAPI=false).")

diff --git a/deployment/components/master-config/nfd-master.conf.example b/deployment/components/master-config/nfd-master.conf.example
@@ -2,7 +2,6 @@
 # autoDefaultNs: true
 # extraLabelNs: ["added.ns.io","added.kubernets.io"]
 # denyLabelNs: ["denied.ns.io","denied.kubernetes.io"]
-# resourceLabels: ["vendor-1.com/feature-1","vendor-2.io/feature-2"]
 # enableTaints: false
 # labelWhiteList: "foo"
 # resyncPeriod: "2h"

diff --git a/deployment/helm/node-feature-discovery/templates/master.yaml b/deployment/helm/node-feature-discovery/templates/master.yaml
@@ -110,9 +110,6 @@ spec:
             {{- if .Values.master.denyLabelNs | empty | not }}
             - "-deny-label-ns={{- join "," .Values.master.denyLabelNs }}"
             {{- end }}
-            {{- if .Values.master.resourceLabels | empty | not }}
-            - "-resource-labels={{- join "," .Values.master.resourceLabels }}"
-            {{- end }}
             {{- if .Values.master.enableTaints }}
             - "-enable-taints"
             {{- end }}

diff --git a/deployment/helm/node-feature-discovery/values.yaml b/deployment/helm/node-feature-discovery/values.yaml
@@ -26,7 +26,6 @@ master:
     # autoDefaultNs: true
     # extraLabelNs: ["added.ns.io","added.kubernets.io"]
     # denyLabelNs: ["denied.ns.io","denied.kubernetes.io"]
-    # resourceLabels: ["vendor-1.com/feature-1","vendor-2.io/feature-2"]
     # enableTaints: false
     # labelWhiteList: "foo"
     # resyncPeriod: "2h"
@@ -75,7 +74,6 @@ master:
   resyncPeriod:
   denyLabelNs: []
   extraLabelNs: []
-  resourceLabels: []
   enableTaints: false
   featureRulesController: null
   nfdApiParallelism: null

diff --git a/docs/deployment/helm.md b/docs/deployment/helm.md
@@ -182,7 +182,6 @@ API's you need to install the prometheus operator in your cluster.
 | `master.instance`                           | string  |                                  | Instance name. Used to separate annotation namespaces for multiple parallel deployments                                                                                                               |
 | `master.resyncPeriod`                       | string  |                                  | NFD API controller resync period.                                                                                                                                                                     |
 | `master.extraLabelNs`                       | array   | []                               | List of allowed extra label namespaces                                                                                                                                                                |
-| `master.resourceLabels`                     | array   | []                               | List of labels to be registered as extended resources                                                                                                                                                 |
 | `master.enableTaints`                       | bool    | false                            | Specifies whether to enable or disable node tainting                                                                                                                                                  |
 | `master.replicaCount`                       | integer | 1                                | Number of desired pods. This is a pointer to distinguish between explicit zero and not specified                                                                                                      |
 | `master.podSecurityContext`                 | dict    | {}                               | [PodSecurityContext](https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#set-the-security-context-for-a-pod) holds pod-level security attributes and common container settings |

diff --git a/docs/reference/master-commandline-reference.md b/docs/reference/master-commandline-reference.md
@@ -143,9 +143,6 @@ other vendor or application specific namespaces for custom labels from the
 local and custom feature sources, even though these labels were denied using
 the `deny-label-ns` flag.
 
-The same namespace control and this flag applies Extended Resources (created
-with `-resource-labels`), too.
-
 Default: *empty*
 
 Example:
@@ -176,23 +173,6 @@ Example:
 nfd-master -deny-label-ns=*.vendor.com,vendor-2.io
 ```
 
-### -resource-labels
-
-**DEPRECATED**: [NodeFeatureRule](../usage/custom-resources.md#nodefeaturerule)
-should be used for managing extended resources in NFD.
-
-The `-resource-labels` flag specifies a comma-separated list of features to be
-advertised as extended resources instead of labels. Features that have integer
-values can be published as Extended Resources by listing them in this flag.
-
-Default: *empty*
-
-Example:
-
-```bash
-nfd-master -resource-labels=vendor-1.com/feature-1,vendor-2.io/feature-2
-```
-
 ### -config
 
 The `-config` flag specifies the path of the nfd-master configuration file to

diff --git a/docs/reference/master-configuration-reference.md b/docs/reference/master-configuration-reference.md
@@ -40,9 +40,6 @@ other vendor or application specific namespaces for custom labels from the
 local and custom feature sources, even though these labels were denied using
 the `denyLabelNs` parameter.
 
-The same namespace control and this option applies to Extended Resources (created
-with `resourceLabels`), too.
-
 Default: *empty*
 
 Example:
@@ -104,23 +101,6 @@ Example:
 autoDefaultNs: false
 ```
 
-## resourceLabels
-
-**DEPRECATED**: [NodeFeatureRule](../usage/custom-resources.md#nodefeaturerule)
-should be used for managing extended resources in NFD.
-
-The `resourceLabels` option specifies a list of features to be
-advertised as extended resources instead of labels. Features that have integer
-values can be published as Extended Resources by listing them in this option.
-
-Default: *empty*
-
-Example:
-
-```yaml
-resourceLabels: ["vendor-1.com/feature-1","vendor-2.io/feature-2"]
-```
-
 ## enableTaints
 `enableTaints` enables/disables node tainting feature of NFD.
 

diff --git a/pkg/nfd-master/nfd-master-internal_test.go b/pkg/nfd-master/nfd-master-internal_test.go
@@ -272,39 +272,39 @@ func TestAddingExtResources(t *testing.T) {
 		fakeMaster := newFakeMaster()
 		Convey("When there are no matching labels", func() {
 			testNode := newTestNode()
-			resourceLabels := ExtendedResources{}
-			patches := fakeMaster.createExtendedResourcePatches(testNode, resourceLabels)
+			extendedResources := ExtendedResources{}
+			patches := fakeMaster.createExtendedResourcePatches(testNode, extendedResources)
 			So(len(patches), ShouldEqual, 0)
 		})
 
 		Convey("When there are matching labels", func() {
 			testNode := newTestNode()
-			resourceLabels := ExtendedResources{"feature-1": "1", "feature-2": "2"}
+			extendedResources := ExtendedResources{"feature-1": "1", "feature-2": "2"}
 			expectedPatches := []utils.JsonPatch{
 				utils.NewJsonPatch("add", "/status/capacity", "feature-1", "1"),
 				utils.NewJsonPatch("add", "/status/capacity", "feature-2", "2"),
 			}
-			patches := fakeMaster.createExtendedResourcePatches(testNode, resourceLabels)
+			patches := fakeMaster.createExtendedResourcePatches(testNode, extendedResources)
 			So(sortJsonPatches(patches), ShouldResemble, sortJsonPatches(expectedPatches))
 		})
 
 		Convey("When the resource already exists", func() {
 			testNode := newTestNode()
 			testNode.Status.Capacity[corev1.ResourceName(nfdv1alpha1.FeatureLabelNs+"/feature-1")] = *resource.NewQuantity(1, resource.BinarySI)
-			resourceLabels := ExtendedResources{nfdv1alpha1.FeatureLabelNs + "/feature-1": "1"}
-			patches := fakeMaster.createExtendedResourcePatches(testNode, resourceLabels)
+			extendedResources := ExtendedResources{nfdv1alpha1.FeatureLabelNs + "/feature-1": "1"}
+			patches := fakeMaster.createExtendedResourcePatches(testNode, extendedResources)
 			So(len(patches), ShouldEqual, 0)
 		})
 
 		Convey("When the resource already exists but its capacity has changed", func() {
 			testNode := newTestNode()
 			testNode.Status.Capacity[corev1.ResourceName("feature-1")] = *resource.NewQuantity(2, resource.BinarySI)
-			resourceLabels := ExtendedResources{"feature-1": "1"}
+			extendedResources := ExtendedResources{"feature-1": "1"}
 			expectedPatches := []utils.JsonPatch{
 				utils.NewJsonPatch("replace", "/status/capacity", "feature-1", "1"),
 				utils.NewJsonPatch("replace", "/status/allocatable", "feature-1", "1"),
 			}
-			patches := fakeMaster.createExtendedResourcePatches(testNode, resourceLabels)
+			patches := fakeMaster.createExtendedResourcePatches(testNode, extendedResources)
 			So(sortJsonPatches(patches), ShouldResemble, sortJsonPatches(expectedPatches))
 		})
 	})
@@ -315,29 +315,29 @@ func TestRemovingExtResources(t *testing.T) {
 		fakeMaster := newFakeMaster()
 		Convey("When none are removed", func() {
 			testNode := newTestNode()
-			resourceLabels := ExtendedResources{nfdv1alpha1.FeatureLabelNs + "/feature-1": "1", nfdv1alpha1.FeatureLabelNs + "/feature-2": "2"}
+			extendedResources := ExtendedResources{nfdv1alpha1.FeatureLabelNs + "/feature-1": "1", nfdv1alpha1.FeatureLabelNs + "/feature-2": "2"}
 			testNode.Annotations[nfdv1alpha1.AnnotationNs+"/extended-resources"] = "feature-1,feature-2"
 			testNode.Status.Capacity[corev1.ResourceName(nfdv1alpha1.FeatureLabelNs+"/feature-1")] = *resource.NewQuantity(1, resource.BinarySI)
 			testNode.Status.Capacity[corev1.ResourceName(nfdv1alpha1.FeatureLabelNs+"/feature-2")] = *resource.NewQuantity(2, resource.BinarySI)
-			patches := fakeMaster.createExtendedResourcePatches(testNode, resourceLabels)
+			patches := fakeMaster.createExtendedResourcePatches(testNode, extendedResources)
 			So(len(patches), ShouldEqual, 0)
 		})
 		Convey("When the related label is gone", func() {
 			testNode := newTestNode()
-			resourceLabels := ExtendedResources{nfdv1alpha1.FeatureLabelNs + "/feature-4": "", nfdv1alpha1.FeatureLabelNs + "/feature-2": "2"}
+			extendedResources := ExtendedResources{nfdv1alpha1.FeatureLabelNs + "/feature-4": "", nfdv1alpha1.FeatureLabelNs + "/feature-2": "2"}
 			testNode.Annotations[nfdv1alpha1.AnnotationNs+"/extended-resources"] = "feature-4,feature-2"
 			testNode.Status.Capacity[corev1.ResourceName(nfdv1alpha1.FeatureLabelNs+"/feature-4")] = *resource.NewQuantity(4, resource.BinarySI)
 			testNode.Status.Capacity[corev1.ResourceName(nfdv1alpha1.FeatureLabelNs+"/feature-2")] = *resource.NewQuantity(2, resource.BinarySI)
-			patches := fakeMaster.createExtendedResourcePatches(testNode, resourceLabels)
+			patches := fakeMaster.createExtendedResourcePatches(testNode, extendedResources)
 			So(len(patches), ShouldBeGreaterThan, 0)
 		})
 		Convey("When the extended resource is no longer wanted", func() {
 			testNode := newTestNode()
 			testNode.Status.Capacity[corev1.ResourceName(nfdv1alpha1.FeatureLabelNs+"/feature-1")] = *resource.NewQuantity(1, resource.BinarySI)
 			testNode.Status.Capacity[corev1.ResourceName(nfdv1alpha1.FeatureLabelNs+"/feature-2")] = *resource.NewQuantity(2, resource.BinarySI)
-			resourceLabels := ExtendedResources{nfdv1alpha1.FeatureLabelNs + "/feature-2": "2"}
+			extendedResources := ExtendedResources{nfdv1alpha1.FeatureLabelNs + "/feature-2": "2"}
 			testNode.Annotations[nfdv1alpha1.AnnotationNs+"/extended-resources"] = "feature-1,feature-2"
-			patches := fakeMaster.createExtendedResourcePatches(testNode, resourceLabels)
+			patches := fakeMaster.createExtendedResourcePatches(testNode, extendedResources)
 			So(len(patches), ShouldBeGreaterThan, 0)
 		})
 	})
@@ -528,7 +528,7 @@ func TestRemoveLabelsWithPrefix(t *testing.T) {
 func TestConfigParse(t *testing.T) {
 	Convey("When parsing configuration", t, func() {
 		master := newFakeMaster()
-		overrides := `{"noPublish": true, "enableTaints": true, "extraLabelNs": ["added.ns.io","added.kubernetes.io"], "denyLabelNs": ["denied.ns.io","denied.kubernetes.io"], "resourceLabels": ["vendor-1.com/feature-1","vendor-2.io/feature-2"], "labelWhiteList": "foo"}`
+		overrides := `{"noPublish": true, "enableTaints": true, "extraLabelNs": ["added.ns.io","added.kubernetes.io"], "denyLabelNs": ["denied.ns.io","denied.kubernetes.io"], "labelWhiteList": "foo"}`
 
 		Convey("and no core cmdline flags have been specified", func() {
 			So(master.configure("non-existing-file", overrides), ShouldBeNil)
@@ -537,7 +537,6 @@ func TestConfigParse(t *testing.T) {
 				So(master.config.EnableTaints, ShouldResemble, true)
 				So(master.config.ExtraLabelNs, ShouldResemble, utils.StringSetVal{"added.ns.io": struct{}{}, "added.kubernetes.io": struct{}{}})
 				So(master.config.DenyLabelNs, ShouldResemble, utils.StringSetVal{"denied.ns.io": struct{}{}, "denied.kubernetes.io": struct{}{}})
-				So(master.config.ResourceLabels, ShouldResemble, utils.StringSetVal{"vendor-1.com/feature-1": struct{}{}, "vendor-2.io/feature-2": struct{}{}})
 				So(master.config.LabelWhiteList.String(), ShouldEqual, "foo")
 			})
 		})
@@ -563,7 +562,6 @@ func TestConfigParse(t *testing.T) {
 		_, err = f.WriteString(`
 noPublish: true
 denyLabelNs: ["denied.ns.io","denied.kubernetes.io"]
-resourceLabels: ["vendor-1.com/feature-1","vendor-2.io/feature-2"]
 enableTaints: false
 labelWhiteList: "foo"
 leaderElection:
@@ -582,7 +580,6 @@ leaderElection:
 				So(master.config.NoPublish, ShouldBeTrue)
 				So(master.config.EnableTaints, ShouldBeFalse)
 				So(master.config.ExtraLabelNs, ShouldResemble, utils.StringSetVal{"override.added.ns.io": struct{}{}})
-				So(master.config.ResourceLabels, ShouldResemble, utils.StringSetVal{"vendor-1.com/feature-1": struct{}{}, "vendor-2.io/feature-2": struct{}{}}) // from cmdline
 				So(master.config.DenyLabelNs, ShouldResemble, utils.StringSetVal{"denied.ns.io": struct{}{}, "denied.kubernetes.io": struct{}{}})
 				So(master.config.LabelWhiteList.String(), ShouldEqual, "foo")
 				So(master.config.LeaderElection.LeaseDuration.Seconds(), ShouldEqual, float64(20))

diff --git a/pkg/nfd-master/nfd-master.go b/pkg/nfd-master/nfd-master.go
@@ -87,7 +87,6 @@ type NFDConfig struct {
 	ExtraLabelNs      utils.StringSetVal
 	LabelWhiteList    *regexp.Regexp
 	NoPublish         bool
-	ResourceLabels    utils.StringSetVal
 	EnableTaints      bool
 	ResyncPeriod      utils.DurationVal
 	LeaderElection    LeaderElectionConfig
@@ -108,7 +107,6 @@ type ConfigOverrideArgs struct {
 	DenyLabelNs       *utils.StringSetVal
 	ExtraLabelNs      *utils.StringSetVal
 	LabelWhiteList    *utils.RegexpVal
-	ResourceLabels    *utils.StringSetVal
 	EnableTaints      *bool
 	NoPublish         *bool
 	ResyncPeriod      *utils.DurationVal
@@ -252,7 +250,6 @@ func newDefaultConfig() *NFDConfig {
 		NoPublish:         false,
 		AutoDefaultNs:     true,
 		NfdApiParallelism: 10,
-		ResourceLabels:    utils.StringSetVal{},
 		EnableTaints:      false,
 		ResyncPeriod:      utils.DurationVal{Duration: time.Duration(1) * time.Hour},
 		LeaderElection: LeaderElectionConfig{
@@ -526,7 +523,7 @@ func (m *nfdMaster) updateMasterNode() error {
 // into extended resources. This function also handles proper namespacing of
 // labels and ERs, i.e. adds the possibly missing default namespace for labels
 // arriving through the gRPC API.
-func (m *nfdMaster) filterFeatureLabels(labels Labels, features *nfdv1alpha1.Features) (Labels, ExtendedResources) {
+func (m *nfdMaster) filterFeatureLabels(labels Labels, features *nfdv1alpha1.Features) Labels {
 	outLabels := Labels{}
 	for name, value := range labels {
 		if value, err := m.filterFeatureLabel(name, value, features); err != nil {
@@ -537,28 +534,12 @@ func (m *nfdMaster) filterFeatureLabels(labels Labels, features *nfdv1alpha1.Fea
 		}
 	}
 
-	// Remove labels which are intended to be extended resources
-	extendedResources := ExtendedResources{}
-	for extendedResourceName := range m.config.ResourceLabels {
-		extendedResourceName := addNs(extendedResourceName, nfdv1alpha1.FeatureLabelNs)
-		if value, ok := outLabels[extendedResourceName]; ok {
-			if _, err := strconv.Atoi(value); err != nil {
-				klog.ErrorS(err, "bad label value encountered for extended resource", "labelKey", extendedResourceName, "labelValue", value)
-				nodeERsRejected.Inc()
-				continue // non-numeric label can't be used
-			}
-
-			extendedResources[extendedResourceName] = value
-			delete(outLabels, extendedResourceName)
-		}
-	}
-
 	if len(outLabels) > 0 && m.config.Restrictions.DisableLabels {
 		klog.V(2).InfoS("node labels are disabled in configuration (restrictions.disableLabels=true)")
 		outLabels = Labels{}
 	}
 
-	return outLabels, extendedResources
+	return outLabels
 }
 
 func (m *nfdMaster) filterFeatureLabel(name, value string, features *nfdv1alpha1.Features) (string, error) {
@@ -899,16 +880,12 @@ func (m *nfdMaster) refreshNodeFeatures(cli k8sclient.Interface, node *corev1.No
 
 	crLabels, crAnnotations, crExtendedResources, crTaints := m.processNodeFeatureRule(node.Name, features)
 
-	// Mix in CR-originated labels
+	// Labels
 	maps.Copy(labels, crLabels)
+	labels = m.filterFeatureLabels(labels, features)
 
-	// Remove labels which are intended to be extended resources via
-	// -resource-labels or their NS is not whitelisted
-	labels, extendedResources := m.filterFeatureLabels(labels, features)
-
-	// Mix in CR-originated extended resources with -resource-labels
-	maps.Copy(extendedResources, crExtendedResources)
-	extendedResources = m.filterExtendedResources(features, extendedResources)
+	// Extended resources
+	extendedResources := m.filterExtendedResources(features, crExtendedResources)
 
 	if len(extendedResources) > 0 && m.config.Restrictions.DisableExtendedResources {
 		klog.V(2).InfoS("extended resources are disabled in configuration (restrictions.disableExtendedResources=true)")
@@ -1258,9 +1235,6 @@ func (m *nfdMaster) configure(filepath string, overrides string) error {
 	if m.args.Overrides.ExtraLabelNs != nil {
 		c.ExtraLabelNs = *m.args.Overrides.ExtraLabelNs
 	}
-	if m.args.Overrides.ResourceLabels != nil {
-		c.ResourceLabels = *m.args.Overrides.ResourceLabels
-	}
 	if m.args.Overrides.EnableTaints != nil {
 		c.EnableTaints = *m.args.Overrides.EnableTaints
 	}

diff --git a/test/e2e/node_feature_discovery_test.go b/test/e2e/node_feature_discovery_test.go
@@ -112,9 +112,8 @@ func cleanupNode(ctx context.Context, cs clientset.Interface) {
 
 		// Remove extended resources
 		for key := range node.Status.Capacity {
-			// We check for FeatureLabelNs as -resource-labels can create ERs there
 			_, ok := nfdERs[string(key)]
-			if ok || strings.HasPrefix(string(key), nfdv1alpha1.FeatureLabelNs) {
+			if ok || strings.HasPrefix(string(key), nfdv1alpha1.ExtendedResourceNs) {
 				delete(node.Status.Capacity, key)
 				delete(node.Status.Allocatable, key)
 				updateStatus = true