From 45f49d574a9ca828b6a22d494f728acce27351af Mon Sep 17 00:00:00 2001 From: Markus Lehtonen Date: Thu, 7 Nov 2024 14:45:38 +0200 Subject: [PATCH] nfd-master: drop resourceLabels Drop the resourceLabels config file option and the corresponding -resource-labels command line flag. They were deprecated in NFD v0.13 so it's time to let them go. NodeFeatureRule(s) should be used to manage ERs, instead. --- cmd/nfd-master/main.go | 6 --- .../master-config/nfd-master.conf.example | 1 - .../templates/master.yaml | 3 -- .../helm/node-feature-discovery/values.yaml | 2 - docs/deployment/helm.md | 1 - .../reference/master-commandline-reference.md | 20 ---------- .../master-configuration-reference.md | 20 ---------- pkg/nfd-master/nfd-master-internal_test.go | 33 ++++++++-------- pkg/nfd-master/nfd-master.go | 38 +++---------------- test/e2e/node_feature_discovery_test.go | 3 +- 10 files changed, 22 insertions(+), 105 deletions(-) diff --git a/cmd/nfd-master/main.go b/cmd/nfd-master/main.go index 61078e385..bc702d0e7 100644 --- 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 index 96dd6d130..124848552 100644 --- 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 index 4795fc2cb..b27bab8a4 100644 --- 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 index 6c3c6f595..6ae8da521 100644 --- 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 index acf14479c..8f16d30b6 100644 --- 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 index 807e78088..e485c7841 100644 --- 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 index b7a06fe0a..5ade9a3ea 100644 --- 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 index b1d72916c..b881fa48f 100644 --- 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 index 822ed8ffd..085afb76e 100644 --- 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 index aea16051f..5e6777ac9 100644 --- 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