review comments

Author: ykulazhenkov

README.md

@@ -421,7 +421,7 @@ Feature gates are used to enable or disable specific features in the operator.
 
 6. **Block Device Plugin Until Configured** (`blockDevicePluginUntilConfigured`)
   - **Description:** Prevents the SR-IOV device plugin from starting until the sriov-config-daemon has applied the SR-IOV configuration for the node. When enabled, the device plugin daemonset runs an init container that sets a wait-for-config annotation on its pod and waits until the sriov-config-daemon removes this annotation after applying the configuration. This addresses the race condition where the device plugin starts and reports available resources before the configuration is actually applied, which can lead to pods being scheduled prematurely.
-  - **Default:** Disabled
+  - **Default:** Enabled
 
 ### Enabling Feature Gates
 

bindata/manifests/plugins/sriov-device-plugin.yaml

@@ -23,6 +23,14 @@ spec:
         component: network
         type: infra
         openshift.io/component: network
+        # NOTE: The controller uses equality.Semantic.DeepDerivative(in.Spec, ds.Spec)
+        # to detect changes in the DaemonSet's spec and decide if an update is needed.
+        # To ensure the init container is properly managed when toggling the
+        # BlockDevicePluginUntilConfigured feature gate, we define an explicit field
+        # (init-container-enabled) that is always present in the DaemonSet labels.
+        # Its value reflects the state of the feature gate and guarantees spec changes
+        # are propagated, ensuring the init container is added or removed as required.
+        init-container-enabled: "{{ .BlockDevicePluginUntilConfigured }}"
     spec:
       hostNetwork: true
       nodeSelector:
@@ -59,11 +67,7 @@ spec:
               fieldPath: metadata.namespace
       {{- end }}
       containers:
-      {{- if .BlockDevicePluginUntilConfigured }}
-      - name: sriov-device-plugin-main
-      {{- else }}
       - name: sriov-device-plugin
-      {{- end }}
         image: {{.SRIOVDevicePluginImage}}
         args:
         - --log-level=10

cmd/sriov-network-config-daemon/start.go

@@ -185,7 +185,7 @@ func initFeatureGates(defaultConfig *sriovnetworkv1.SriovOperatorConfig) (featur
 	featureGates := featuregate.New()
 	featureGates.Init(defaultConfig.Spec.FeatureGates)
 	fnLogger.Info("Enabled featureGates", "featureGates", featureGates.String())
-
+	vars.FeatureGate = featureGates
 	return featureGates, nil
 }
 

cmd/sriov-network-config-daemon/waitforconfig.go

@@ -31,6 +31,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/cache"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log"
+	metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server"
 
 	"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/consts"
 	snolog "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/log"
@@ -141,7 +142,7 @@ func startWaitForConfigManager(setupLog logr.Logger, config *rest.Config, podNam
 
 	// Set annotation on pod to signal that we are waiting for config
 	setupLog.Info("Setting annotation on pod", "annotation", consts.DevicePluginWaitConfigAnnotation)
-	err = setAnnotationOnPod(context.Background(), setupLog, tempClient, podName)
+	err = setAnnotationOnPod(ctx, setupLog, tempClient, podName)
 	if err != nil {
 		setupLog.Error(err, "failed to set annotation on pod")
 		return err
@@ -152,6 +153,7 @@ func startWaitForConfigManager(setupLog logr.Logger, config *rest.Config, podNam
 	// Watch only specific pod object
 	selector := fields.SelectorFromSet(fields.Set{"metadata.name": podName.Name})
 	mgr, err := ctrl.NewManager(config, ctrl.Options{
+		Metrics: metricsserver.Options{BindAddress: "0"},
 		Cache: cache.Options{
 			DefaultNamespaces: map[string]cache.Config{podName.Namespace: {}},
 			ByObject: map[client.Object]cache.ByObject{

controllers/helper.go

@@ -45,6 +45,7 @@ import (
 	sriovnetworkv1 "github.com/k8snetworkplumbingwg/sriov-network-operator/api/v1"
 	"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/apply"
 	constants "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/consts"
+	"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/featuregate"
 	"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/render"
 	"github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/vars"
 )
@@ -172,7 +173,8 @@ func GetNodeSelectorForDevicePlugin(dc *sriovnetworkv1.SriovOperatorConfig) map[
 func syncPluginDaemonObjs(ctx context.Context,
 	client k8sclient.Client,
 	scheme *runtime.Scheme,
-	dc *sriovnetworkv1.SriovOperatorConfig) error {
+	dc *sriovnetworkv1.SriovOperatorConfig,
+	featureGate featuregate.FeatureGate) error {
 	logger := log.Log.WithName("syncPluginDaemonObjs")
 	logger.V(1).Info("Start to sync sriov daemons objects")
 
@@ -186,7 +188,7 @@ func syncPluginDaemonObjs(ctx context.Context,
 	data.Data["ImagePullSecrets"] = GetImagePullSecrets()
 	data.Data["NodeSelectorField"] = GetNodeSelectorForDevicePlugin(dc)
 	data.Data["UseCDI"] = dc.Spec.UseCDI
-	data.Data["BlockDevicePluginUntilConfigured"] = dc.Spec.FeatureGates[constants.BlockDevicePluginUntilConfiguredFeatureGate]
+	data.Data["BlockDevicePluginUntilConfigured"] = featureGate.IsEnabled(constants.BlockDevicePluginUntilConfiguredFeatureGate)
 	objs, err := renderDsForCR(constants.PluginPath, &data)
 	if err != nil {
 		logger.Error(err, "Fail to render SR-IoV manifests")

controllers/sriovoperatorconfig_controller.go

@@ -138,7 +138,7 @@ func (r *SriovOperatorConfigReconciler) Reconcile(ctx context.Context, req ctrl.
 		return reconcile.Result{}, err
 	}
 
-	if err = syncPluginDaemonObjs(ctx, r.Client, r.Scheme, defaultConfig); err != nil {
+	if err = syncPluginDaemonObjs(ctx, r.Client, r.Scheme, defaultConfig, r.FeatureGate); err != nil {
 		return reconcile.Result{}, err
 	}
 

pkg/daemon/daemon.go

@@ -370,6 +370,20 @@ func (dn *NodeReconciler) checkSystemdStatus() (*hosttypes.SriovResult, bool, er
 // 7. Updating the lastAppliedGeneration to the current generation.
 func (dn *NodeReconciler) apply(ctx context.Context, desiredNodeState *sriovnetworkv1.SriovNetworkNodeState, reqReboot bool, sriovResult *hosttypes.SriovResult) (ctrl.Result, error) {
 	reqLogger := log.FromContext(ctx).WithName("Apply")
+
+	// Restart the device plugin *before* applying configuration if the
+	// BlockDevicePluginUntilConfiguredFeatureGate feature is enabled.
+	// With this gate enabled, the device plugin will remain blocked until it is
+	// explicitly unblocked after configuration (see waitForDevicePluginPodAndTryUnblock).
+	// If the feature gate is not enabled, preserve legacy behavior by
+	// restarting the device plugin *after* configuration is applied.
+	if vars.FeatureGate.IsEnabled(consts.BlockDevicePluginUntilConfiguredFeatureGate) {
+		if err := dn.restartDevicePluginPod(ctx); err != nil {
+			reqLogger.Error(err, "failed to restart device plugin on the node")
+			return ctrl.Result{}, err
+		}
+	}
+
 	// apply the additional plugins after we are done with drain if needed
 	for _, p := range dn.additionalPlugins {
 		err := p.Apply()
@@ -395,15 +409,18 @@ func (dn *NodeReconciler) apply(ctx context.Context, desiredNodeState *sriovnetw
 		return ctrl.Result{}, dn.rebootNode()
 	}
 
-	if err := dn.restartDevicePluginPod(ctx); err != nil {
-		reqLogger.Error(err, "failed to restart device plugin on the node")
-		return ctrl.Result{}, err
-	}
 	if vars.FeatureGate.IsEnabled(consts.BlockDevicePluginUntilConfiguredFeatureGate) {
 		if err := dn.waitForDevicePluginPodAndTryUnblock(ctx, desiredNodeState); err != nil {
 			reqLogger.Error(err, "failed to wait for device plugin pod to start and try to unblock it")
 			return ctrl.Result{}, err
 		}
+	} else {
+		// if the feature gate is not enabled we preserver the old behavior
+		// and restart device plugin after configuration is applied
+		if err := dn.restartDevicePluginPod(ctx); err != nil {
+			reqLogger.Error(err, "failed to restart device plugin on the node")
+			return ctrl.Result{}, err
+		}
 	}
 
 	err := dn.annotate(ctx, desiredNodeState, consts.DrainIdle)
@@ -452,7 +469,7 @@ func (dn *NodeReconciler) tryUnblockDevicePlugin(ctx context.Context,
 	for _, pod := range devicePluginPods {
 		if err := utils.RemoveAnnotationFromObject(ctx, &pod,
 			consts.DevicePluginWaitConfigAnnotation, dn.client); err != nil {
-			return fmt.Errorf("failed to remove wait-for-config annotation from pod: %w", err)
+			return fmt.Errorf("failed to remove %s annotation from pod: %w", consts.DevicePluginWaitConfigAnnotation, err)
 		}
 	}
 	return nil
@@ -577,18 +594,19 @@ func (dn *NodeReconciler) handleDrain(ctx context.Context, desiredNodeState *sri
 
 // getDevicePluginPods returns the device plugin pods running on this node
 func (dn *NodeReconciler) getDevicePluginPodsForNode(ctx context.Context) ([]corev1.Pod, error) {
+	funcLog := log.Log.WithName("getDevicePluginPodsForNode")
 	pods := &corev1.PodList{}
 	err := dn.client.List(ctx, pods, &client.ListOptions{
 		Namespace: vars.Namespace, Raw: &metav1.ListOptions{
 			LabelSelector: "app=sriov-device-plugin",
 			FieldSelector: "spec.nodeName=" + vars.NodeName,
 		}})
 	if err != nil {
-		log.Log.Error(err, "getDevicePluginPodsForNode(): failed to list device plugin pods")
+		funcLog.Error(err, "failed to list device plugin pods")
 		return []corev1.Pod{}, err
 	}
 	if len(pods.Items) == 0 {
-		log.Log.Info("getDevicePluginPodsForNode(): no device plugin pods found")
+		funcLog.Info("no device plugin pods found")
 		return []corev1.Pod{}, nil
 	}
 	return pods.Items, nil
@@ -649,18 +667,18 @@ func (dn *NodeReconciler) restartDevicePluginPod(ctx context.Context) error {
 // for the periodic check. We expect to have at least one device plugin pod for the node.
 func (dn *NodeReconciler) waitForDevicePluginPodAndTryUnblock(ctx context.Context, desiredNodeState *sriovnetworkv1.SriovNetworkNodeState) error {
 	funcLog := log.Log.WithName("waitForDevicePluginPodAndTryUnblock")
-	funcLog.Info("waiting for device plugin to set wait-for-config annotation")
+	funcLog.Info("waiting for device plugin to set wait-for-config annotation", "annotation", consts.DevicePluginWaitConfigAnnotation)
 	var devicePluginPods []corev1.Pod
 	err := wait.PollUntilContextTimeout(ctx, time.Second, 2*time.Minute, true,
 		func(ctx context.Context) (bool, error) {
 			var err error
 			devicePluginPods, err = dn.getDevicePluginPodsForNode(ctx)
 			if err != nil {
-				log.Log.Error(err, "waitForDevicePluginPodAndUnblock(): failed to get device plugin pod while waiting for a new pod to start")
+				funcLog.Error(err, "failed to get device plugin pod while waiting for a new pod to start")
 				return false, err
 			}
 			if len(devicePluginPods) == 0 {
-				log.Log.V(2).Info("waitForDevicePluginPodAndUnblock(): no device plugin pods found while waiting for a new pod to start")
+				funcLog.V(2).Info("no device plugin pods found while waiting for a new pod to start")
 				return false, nil
 			}
 			for _, pod := range devicePluginPods {
@@ -669,12 +687,12 @@ func (dn *NodeReconciler) waitForDevicePluginPodAndTryUnblock(ctx context.Contex
 				// may also match our selector. Since unmanaged pods won't have this annotation,
 				// we only require one pod (the managed one) to have it.
 				if utils.ObjectHasAnnotationKey(&pod, consts.DevicePluginWaitConfigAnnotation) {
-					log.Log.Info("waitForDevicePluginPodAndUnblock(): wait-for-config annotation found on pod",
+					funcLog.Info("wait-for-config annotation found on pod",
 						"pod", pod.Name)
 					return true, nil
 				}
 			}
-			log.Log.V(2).Info("waitForDevicePluginPodAndUnblock(): waiting for new device plugin pod to have wait-for-config annotation")
+			funcLog.V(2).Info("waiting for new device plugin pod to have wait-for-config annotation")
 			return false, nil
 		})
 	if err != nil {
@@ -683,8 +701,8 @@ func (dn *NodeReconciler) waitForDevicePluginPodAndTryUnblock(ctx context.Contex
 		}
 		// If annotation is not found within the timeout, log a warning and proceed.
 		// The device plugin pod will be unblocked by the periodic check logic in tryUnblockDevicePlugin.
-		log.Log.Info("waitForDevicePluginPodAndUnblock(): WARNING: device plugin pod with " +
-			"wait-for-config annotation not found within timeout")
+		funcLog.Info("WARNING: device plugin pod with wait-for-config annotation not found within timeout")
+		return nil
 	}
 	if len(devicePluginPods) > 0 {
 		// try to unblock all device plugin pods we retrieved with the latest loop iteration