Re-add -enable-nodefeature-api cmdline flag

marquiz · marquiz · commit 560bd11d8558 · 2024-05-16T10:53:49.000+03:00
Bring back the -enable-nodefeature-api command line flag and the corresponding enableNodeFeatureApi helm config value that were removed without deprecation when the NodeFeatureAPI feature gate was introduced. The thinking behind this change is to not break existing users (without warning) unless totally unavoidable. Now the -enable-nodefeature-api flag is marked as deprecated and slated for removal in NFD v0.17. The NodeFeatureAPI feature gate and the -enable-nodefeature-api flag work together so that the NodeFeature API is disabled (gRPC is enabled, instead) if either of them is set to false. This patch selectively reverts parts of 06c4733.
diff --git a/cmd/nfd-master/main.go b/cmd/nfd-master/main.go
@@ -81,6 +81,8 @@ func main() {
 			args.Overrides.ResyncPeriod = overrides.ResyncPeriod
 		case "nfd-api-parallelism":
 			args.Overrides.NfdApiParallelism = overrides.NfdApiParallelism
+		case "enable-nodefeature-api":
+			klog.InfoS("-enable-nodefeature-api is deprecated and will be removed in the next release, use -feature-gate NodeFeatureAPI instead")
 		case "ca-file":
 			klog.InfoS("-ca-file is deprecated, will be removed in a future release along with the deprecated gRPC API")
 		case "cert-file":
@@ -139,6 +141,9 @@ func initFlags(flagset *flag.FlagSet) (*master.Args, *master.ConfigOverrideArgs)
 		"Config file to use.")
 	flagset.StringVar(&args.Kubeconfig, "kubeconfig", "",
 		"Kubeconfig to use")
+	flagset.BoolVar(&args.EnableNodeFeatureApi, "enable-nodefeature-api", true,
+		"Enable the NodeFeature CRD API for receiving node features. This will automatically disable the gRPC communication."+
+			" DEPRECATED: will be removed in NFD v0.17. Use -feature-gate NodeFeatureAPI instead.")
 	flagset.BoolVar(&args.CrdController, "featurerules-controller", true,
 		"Enable NFD CRD API controller. DEPRECATED: use -crd-controller instead")
 	flagset.BoolVar(&args.CrdController, "crd-controller", true,
diff --git a/cmd/nfd-worker/main.go b/cmd/nfd-worker/main.go
@@ -69,6 +69,8 @@ func main() {
 			klog.InfoS("-cert-file is deprecated, will be removed in a future release along with the deprecated gRPC API")
 		case "key-file":
 			klog.InfoS("-key-file is deprecated, will be removed in a future release along with the deprecated gRPC API")
+		case "enable-nodefeature-api":
+			klog.InfoS("-enable-nodefeature-api is deprecated and will be removed in the next release, use -feature-gate NodeFeatureAPI instead")
 		case "server":
 			klog.InfoS("-server is deprecated, will be removed in a future release along with the deprecated gRPC API")
 		case "server-name-override":
@@ -132,6 +134,9 @@ func initFlags(flagset *flag.FlagSet) (*worker.Args, *worker.ConfigOverrideArgs)
 	flagset.StringVar(&args.KeyFile, "key-file", "",
 		"Private key matching -cert-file."+
 			" DEPRECATED: will be removed in a future release along with the deprecated gRPC API.")
+	flagset.BoolVar(&args.EnableNodeFeatureApi, "enable-nodefeature-api", true,
+		"Enable the NodeFeature CRD API for communicating with nfd-master. This will automatically disable the gRPC communication."+
+			" DEPRECATED: will be removed in NFD v0.17. Use -feature-gate NodeFeatureAPI instead.")
 	flagset.StringVar(&args.Kubeconfig, "kubeconfig", "",
 		"Kubeconfig to use")
 	flagset.BoolVar(&args.Oneshot, "oneshot", false,
diff --git a/deployment/helm/node-feature-discovery/templates/clusterrole.yaml b/deployment/helm/node-feature-discovery/templates/clusterrole.yaml
@@ -86,7 +86,7 @@ rules:
   - update
 {{- end }}
 
-{{- if and .Values.gc.enable .Values.gc.rbac.create (or .Values.featureGates.NodeFeatureAPI .Values.topologyUpdater.enable) }}
+{{- if and .Values.gc.enable .Values.gc.rbac.create (or (and .Values.featureGates.NodeFeatureAPI .Values.enableNodeFeatureApi) .Values.topologyUpdater.enable) }}
 ---
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole
diff --git a/deployment/helm/node-feature-discovery/templates/clusterrolebinding.yaml b/deployment/helm/node-feature-discovery/templates/clusterrolebinding.yaml
@@ -33,7 +33,7 @@ subjects:
   namespace: {{ include "node-feature-discovery.namespace" .  }}
 {{- end }}
 
-{{- if and .Values.gc.enable .Values.gc.rbac.create (or .Values.featureGates.NodeFeatureAPI  .Values.topologyUpdater.enable) }}
+{{- if and .Values.gc.enable .Values.gc.rbac.create (or (and .Values.featureGates.NodeFeatureAPI .Values.enableNodeFeatureApi) .Values.topologyUpdater.enable) }}
 ---
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRoleBinding
diff --git a/deployment/helm/node-feature-discovery/templates/master.yaml b/deployment/helm/node-feature-discovery/templates/master.yaml
@@ -73,7 +73,7 @@ spec:
             {{- if .Values.master.instance | empty | not }}
             - "-instance={{ .Values.master.instance }}"
             {{- end }}
-            {{- if not .Values.featureGates.NodeFeatureAPI }}
+            {{- if not (and .Values.featureGates.NodeFeatureAPI .Values.enableNodeFeatureApi) }}
             - "-port={{ .Values.master.port | default "8080" }}"
             {{- else if gt (int .Values.master.replicaCount) 1 }}
             - "-enable-leader-election"
diff --git a/deployment/helm/node-feature-discovery/templates/nfd-gc.yaml b/deployment/helm/node-feature-discovery/templates/nfd-gc.yaml
@@ -1,4 +1,4 @@
-{{- if and .Values.gc.enable (or .Values.featureGates.NodeFeatureAPI .Values.topologyUpdater.enable) -}}
+{{- if and .Values.gc.enable (or (and .Values.featureGates.NodeFeatureAPI .Values.enableNodeFeatureApi) .Values.topologyUpdater.enable) -}}
 apiVersion: apps/v1
 kind: Deployment
 metadata:
diff --git a/deployment/helm/node-feature-discovery/templates/service.yaml b/deployment/helm/node-feature-discovery/templates/service.yaml
@@ -1,4 +1,4 @@
-{{- if and (not .Values.featureGates.NodeFeatureAPI) .Values.master.enable }}
+{{- if and (not (and .Values.featureGates.NodeFeatureAPI .Values.enableNodeFeatureApi)) .Values.master.enable }}
 apiVersion: v1
 kind: Service
 metadata:
diff --git a/deployment/helm/node-feature-discovery/templates/serviceaccount.yaml b/deployment/helm/node-feature-discovery/templates/serviceaccount.yaml
@@ -27,7 +27,7 @@ metadata:
   {{- end }}
 {{- end }}
 
-{{- if and .Values.gc.enable .Values.gc.serviceAccount.create (or .Values.featureGates.NodeFeatureAPI .Values.topologyUpdater.enable) }}
+{{- if and .Values.gc.enable .Values.gc.serviceAccount.create (or (and .Values.featureGates.NodeFeatureAPI .Values.enableNodeFeatureApi) .Values.topologyUpdater.enable) }}
 ---
 apiVersion: v1
 kind: ServiceAccount
diff --git a/deployment/helm/node-feature-discovery/templates/worker.yaml b/deployment/helm/node-feature-discovery/templates/worker.yaml
@@ -72,7 +72,7 @@ spec:
         command:
         - "nfd-worker"
         args:
-{{- if not .Values.featureGates.NodeFeatureAPI }}
+{{- if not (and .Values.featureGates.NodeFeatureAPI .Values.enableNodeFeatureApi)   }}
         - "-server={{ include "node-feature-discovery.fullname" . }}-master:{{ .Values.master.service.port }}"
 {{- end }}
 {{- if .Values.tls.enable }}
diff --git a/deployment/helm/node-feature-discovery/values.yaml b/deployment/helm/node-feature-discovery/values.yaml
@@ -10,6 +10,8 @@ nameOverride: ""
 fullnameOverride: ""
 namespaceOverride: ""
 
+enableNodeFeatureApi: true
+
 featureGates:
   NodeFeatureAPI: true
 
diff --git a/docs/deployment/helm.md b/docs/deployment/helm.md
@@ -99,6 +99,7 @@ Chart parameters are available.
 | `tls.certManager.certManagerCertificate.issuerName` | string |           | If specified, it will use a pre-existing issuer instead for the required TLS certificates. **NOTE**: this parameter is related to the deprecated gRPC API and will be removed with it in a future release |
 | `tls.certManager.certManagerCertificate.issuerKind` | string |           | Specifies on what kind of issuer is used, can be either ClusterIssuer or Issuer (default).  Requires `tls.certManager.certManagerCertificate.issuerName` to be set.  **NOTE**: this parameter is related to the deprecated gRPC API and will be removed with it in a future release |
 | `featureGates.NodeFeatureAPI`| bool   | true                                    | Enable the [NodeFeature](../usage/custom-resources.md#nodefeature) CRD API for communicating node features. This will automatically disable the gRPC communication. **NOTE**: this parameter is related to the deprecated gRPC API and will be removed with it in a future release    |
+| `enableNodeFeatureApi`| bool   | true                                    | Enable the [NodeFeature](../usage/custom-resources.md#nodefeature) CRD API for communicating node features. This will automatically disable the gRPC communication. **DEPRECATED**: will be removed in NFD v0.17, use `featureGates.NodeFeatureAPI` instead.    |
 | `prometheus.enable`   | bool   | false                                   | Specifies whether to expose metrics using prometheus operator                                                                                |
 | `prometheus.labels`   | dict   | {}                                      | Specifies labels for use with the prometheus operator to control how it is selected                                                        |
 | `prometheus.scrapeInterval`    | string   | 10s                                      | Specifies the interval by which metrics are scraped                                                        |
diff --git a/docs/reference/master-commandline-reference.md b/docs/reference/master-commandline-reference.md
@@ -175,6 +175,27 @@ nfd-master -verify-node-name -ca-file=/opt/nfd/ca.crt \
     -cert-file=/opt/nfd/master.crt -key-file=/opt/nfd/master.key
 ```
 
+### -enable-nodefeature-api
+
+**DEPRECATED**: will be removed in NFD v0.17. Use'
+[`-feature-gates`](#-feature-gates)
+[NodeFeatureAPI](feature-gates.md#nodefeatureapi) instead.
+
+> **NOTE** the gRPC API is deprecated and will be removed in a future release.
+
+The `-enable-nodefeature-api` flag enables/disables the
+[NodeFeature](../usage/custom-resources.md#nodefeature) CRD API for receiving
+feature requests. This will also automatically disable/enable the gRPC
+interface.
+
+Default: true
+
+Example:
+
+```bash
+nfd-master -enable-nodefeature-api=false
+```
+
 ### -enable-leader-election
 
 The `-enable-leader-election` flag enables leader election for NFD-Master.
diff --git a/pkg/nfd-master/nfd-master.go b/pkg/nfd-master/nfd-master.go
@@ -108,15 +108,16 @@ type ConfigOverrideArgs struct {
 
 // Args holds command line arguments
 type Args struct {
-	CaFile        string
-	CertFile      string
-	ConfigFile    string
-	Instance      string
-	KeyFile       string
-	Klog          map[string]*utils.KlogFlagVal
-	Kubeconfig    string
-	CrdController bool
-	Port          int
+	CaFile               string
+	CertFile             string
+	ConfigFile           string
+	Instance             string
+	KeyFile              string
+	Klog                 map[string]*utils.KlogFlagVal
+	Kubeconfig           string
+	CrdController        bool
+	EnableNodeFeatureApi bool
+	Port                 int
 	// GrpcHealthPort is only needed to avoid races between tests (by skipping the health server).
 	// Could be removed when gRPC labler service is dropped (when nfd-worker tests stop running nfd-master).
 	GrpcHealthPort       int
@@ -318,7 +319,7 @@ func (m *nfdMaster) Run() error {
 	grpcErr := make(chan error)
 	// If the NodeFeature API is enabled, don'tregister the labeler API
 	// server. Otherwise, register the labeler server.
-	if !nfdfeatures.NFDFeatureGate.Enabled(nfdfeatures.NodeFeatureAPI) {
+	if !nfdfeatures.NFDFeatureGate.Enabled(nfdfeatures.NodeFeatureAPI) || !m.args.EnableNodeFeatureApi {
 		go m.runGrpcServer(grpcErr)
 	}
 
@@ -373,7 +374,7 @@ func (m *nfdMaster) Run() error {
 			m.nodeUpdaterPool.start(m.config.NfdApiParallelism)
 
 			// Update all nodes when the configuration changes
-			if m.nfdController != nil && nfdfeatures.NFDFeatureGate.Enabled(nfdfeatures.NodeFeatureAPI) {
+			if m.nfdController != nil && nfdfeatures.NFDFeatureGate.Enabled(nfdfeatures.NodeFeatureAPI) && m.args.EnableNodeFeatureApi {
 				m.nfdController.updateAllNodesChan <- struct{}{}
 			}
 
@@ -471,7 +472,7 @@ func (m *nfdMaster) runGrpcServer(errChan chan<- error) {
 func (m *nfdMaster) nfdAPIUpdateHandler() {
 	// We want to unconditionally update all nodes at startup if gRPC is
 	// disabled (i.e. NodeFeature API is enabled)
-	updateAll := nfdfeatures.NFDFeatureGate.Enabled(nfdfeatures.NodeFeatureAPI)
+	updateAll := nfdfeatures.NFDFeatureGate.Enabled(nfdfeatures.NodeFeatureAPI) && m.args.EnableNodeFeatureApi
 	updateNodes := make(map[string]struct{})
 	rateLimit := time.After(time.Second)
 	for {
@@ -1363,7 +1364,7 @@ func (m *nfdMaster) startNfdApiController() error {
 	}
 	klog.InfoS("starting the nfd api controller")
 	m.nfdController, err = newNfdController(kubeconfig, nfdApiControllerOptions{
-		DisableNodeFeature: !nfdfeatures.NFDFeatureGate.Enabled(nfdfeatures.NodeFeatureAPI),
+		DisableNodeFeature: !nfdfeatures.NFDFeatureGate.Enabled(nfdfeatures.NodeFeatureAPI) || !m.args.EnableNodeFeatureApi,
 		ResyncPeriod:       m.config.ResyncPeriod.Duration,
 	})
 	if err != nil {
diff --git a/pkg/nfd-worker/nfd-worker.go b/pkg/nfd-worker/nfd-worker.go
@@ -96,18 +96,19 @@ type Labels map[string]string
 
 // Args are the command line arguments of NfdWorker.
 type Args struct {
-	CaFile             string
-	CertFile           string
-	ConfigFile         string
-	KeyFile            string
-	Klog               map[string]*utils.KlogFlagVal
-	Kubeconfig         string
-	Oneshot            bool
-	Options            string
-	Server             string
-	ServerNameOverride string
-	MetricsPort        int
-	GrpcHealthPort     int
+	CaFile               string
+	CertFile             string
+	ConfigFile           string
+	EnableNodeFeatureApi bool
+	KeyFile              string
+	Klog                 map[string]*utils.KlogFlagVal
+	Kubeconfig           string
+	Oneshot              bool
+	Options              string
+	Server               string
+	ServerNameOverride   string
+	MetricsPort          int
+	GrpcHealthPort       int
 
 	Overrides ConfigOverrideArgs
 }
@@ -316,7 +317,7 @@ func (w *nfdWorker) Run() error {
 				return err
 			}
 			// Manage connection to master
-			if w.config.Core.NoPublish || !features.NFDFeatureGate.Enabled(features.NodeFeatureAPI) {
+			if w.config.Core.NoPublish || !features.NFDFeatureGate.Enabled(features.NodeFeatureAPI) || !w.args.EnableNodeFeatureApi {
 				w.grpcDisconnect()
 			}
 
@@ -660,7 +661,7 @@ func getFeatureLabels(source source.LabelSource, labelWhiteList regexp.Regexp) (
 
 // advertiseFeatures advertises the features of a Kubernetes node
 func (w *nfdWorker) advertiseFeatures(labels Labels) error {
-	if features.NFDFeatureGate.Enabled(features.NodeFeatureAPI) {
+	if features.NFDFeatureGate.Enabled(features.NodeFeatureAPI) && w.args.EnableNodeFeatureApi {
 		// Create/update NodeFeature CR object
 		if err := w.updateNodeFeatureObject(labels); err != nil {
 			return fmt.Errorf("failed to advertise features (via CRD API): %w", err)

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-{{- if and .Values.gc.enable (or .Values.featureGates.NodeFeatureAPI .Values.topologyUpdater.enable) -}}`
	`1`	`+{{- if and .Values.gc.enable (or (and .Values.featureGates.NodeFeatureAPI .Values.enableNodeFeatureApi) .Values.topologyUpdater.enable) -}}`
`2`	`2`	`apiVersion: apps/v1`
`3`	`3`	`kind: Deployment`
`4`	`4`	`metadata:`
Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-{{- if and (not .Values.featureGates.NodeFeatureAPI) .Values.master.enable }}`
	`1`	`+{{- if and (not (and .Values.featureGates.NodeFeatureAPI .Values.enableNodeFeatureApi)) .Values.master.enable }}`
`2`	`2`	`apiVersion: v1`
`3`	`3`	`kind: Service`
`4`	`4`	`metadata:`