Abstract logic to check and load missing KArgs

Logic to check missing kernel arguments is placed in a method
to be used by both OnNodeStateChange and CheckStatusChanges.

pkg/daemon/daemon.go

@@ -692,7 +692,12 @@ func (dn *Daemon) shouldSkipReconciliation(latestState *sriovnetworkv1.SriovNetw
 		genericPlugin, ok := dn.enabledPlugins[GenericPluginName]
 		if ok {
 			// Verify changes in the status of the SriovNetworkNodeState CR.
-			if genericPlugin.CheckStatusChanges(latestState) {
+			changed := false
+			changed, err = genericPlugin.CheckStatusChanges(latestState)
+			if err != nil {
+				return false, err
+			}
+			if changed {
 				return false, nil
 			}
 		}

pkg/plugins/fake/fake_plugin.go

@@ -19,8 +19,8 @@ func (f *FakePlugin) OnNodeStateChange(new *sriovnetworkv1.SriovNetworkNodeState
 	return false, false, nil
 }
 
-func (f *FakePlugin) CheckStatusChanges(new *sriovnetworkv1.SriovNetworkNodeState) bool {
-	return false
+func (f *FakePlugin) CheckStatusChanges(new *sriovnetworkv1.SriovNetworkNodeState) (bool, error) {
+	return false, nil
 }
 
 func (f *FakePlugin) Apply() error {

pkg/plugins/generic/generic_plugin.go

@@ -123,7 +123,7 @@ func (p *GenericPlugin) OnNodeStateChange(new *sriovnetworkv1.SriovNetworkNodeSt
 }
 
 // CheckStatusChanges verify whether SriovNetworkNodeState CR status present changes on configured VFs.
-func (p *GenericPlugin) CheckStatusChanges(new *sriovnetworkv1.SriovNetworkNodeState) bool {
+func (p *GenericPlugin) CheckStatusChanges(new *sriovnetworkv1.SriovNetworkNodeState) (bool, error) {
 	log.Log.Info("generic-plugin CheckStatusChanges()")
 
 	changed := false
@@ -144,7 +144,18 @@ func (p *GenericPlugin) CheckStatusChanges(new *sriovnetworkv1.SriovNetworkNodeS
 			log.Log.Info("CheckStatusChanges(): no status found for interface", "address", iface.PciAddress)
 		}
 	}
-	return changed
+
+	missingKernelArgs, err := p.getMissingKernelArgs()
+	if err != nil {
+		log.Log.Error(err, "generic-plugin CheckStatusChanges(): failed to verify missing kernel arguments")
+		return false, err
+	}
+
+	if len(missingKernelArgs) != 0 {
+		changed = true
+	}
+
+	return changed, nil
 }
 
 func (p *GenericPlugin) syncDriverState() error {
@@ -248,38 +259,48 @@ func (p *GenericPlugin) addToDesiredKernelArgs(karg string) {
 	}
 }
 
-// syncDesiredKernelArgs Should be called to set all the kernel arguments. Returns bool if node update is needed.
-func (p *GenericPlugin) syncDesiredKernelArgs() (bool, error) {
-	needReboot := false
+// getMissingKernelArgs gets Kernel arguments that have not been set.
+func (p *GenericPlugin) getMissingKernelArgs() ([]string, error) {
+	missingArgs := make([]string, 0, len(p.DesiredKernelArgs))
 	if len(p.DesiredKernelArgs) == 0 {
-		return false, nil
+		return nil, nil
 	}
+
 	kargs, err := p.helpers.GetCurrentKernelArgs()
 	if err != nil {
-		return false, err
+		return nil, err
 	}
-	for desiredKarg, attempted := range p.DesiredKernelArgs {
-		set := p.helpers.IsKernelArgsSet(kargs, desiredKarg)
-		if !set {
-			if attempted {
-				log.Log.V(2).Info("generic-plugin syncDesiredKernelArgs(): previously attempted to set kernel arg",
-					"karg", desiredKarg)
-			}
-			// There is a case when we try to set the kernel argument here, the daemon could decide to not reboot because
-			// the daemon encountered a potentially one-time error. However we always want to make sure that the kernel
-			// argument is set once the daemon goes through node state sync again.
-			update, err := setKernelArg(desiredKarg)
-			if err != nil {
-				log.Log.Error(err, "generic-plugin syncDesiredKernelArgs(): fail to set kernel arg", "karg", desiredKarg)
-				return false, err
-			}
-			if update {
-				needReboot = true
-				log.Log.V(2).Info("generic-plugin syncDesiredKernelArgs(): need reboot for setting kernel arg", "karg", desiredKarg)
-			}
-			p.DesiredKernelArgs[desiredKarg] = true
+	for desiredKarg := range p.DesiredKernelArgs {
+		if !p.helpers.IsKernelArgsSet(kargs, desiredKarg) {
+			missingArgs = append(missingArgs, desiredKarg)
 		}
 	}
+	return missingArgs, nil
+}
+
+// syncDesiredKernelArgs should be called to set all the kernel arguments. Returns bool if node update is needed.
+func (p *GenericPlugin) syncDesiredKernelArgs(kargs []string) (bool, error) {
+	needReboot := false
+
+	for _, karg := range kargs {
+		if p.DesiredKernelArgs[karg] {
+			log.Log.V(2).Info("generic-plugin syncDesiredKernelArgs(): previously attempted to set kernel arg",
+				"karg", karg)
+		}
+		// There is a case when we try to set the kernel argument here, the daemon could decide to not reboot because
+		// the daemon encountered a potentially one-time error. However we always want to make sure that the kernel
+		// argument is set once the daemon goes through node state sync again.
+		update, err := setKernelArg(karg)
+		if err != nil {
+			log.Log.Error(err, "generic-plugin syncDesiredKernelArgs(): fail to set kernel arg", "karg", karg)
+			return false, err
+		}
+		if update {
+			needReboot = true
+			log.Log.V(2).Info("generic-plugin syncDesiredKernelArgs(): need reboot for setting kernel arg", "karg", karg)
+		}
+		p.DesiredKernelArgs[karg] = true
+	}
 	return needReboot, nil
 }
 
@@ -347,18 +368,30 @@ func (p *GenericPlugin) addVfioDesiredKernelArg(state *sriovnetworkv1.SriovNetwo
 	}
 }
 
-func (p *GenericPlugin) needRebootNode(state *sriovnetworkv1.SriovNetworkNodeState) (needReboot bool, err error) {
-	needReboot = false
+func (p *GenericPlugin) needRebootNode(state *sriovnetworkv1.SriovNetworkNodeState) (bool, error) {
+	var err error
+	needReboot := false
+	updateNode := false
+
 	p.addVfioDesiredKernelArg(state)
 
-	updateNode, err := p.syncDesiredKernelArgs()
+	var missingKernelArgs []string
+	missingKernelArgs, err = p.getMissingKernelArgs()
 	if err != nil {
-		log.Log.Error(err, "generic-plugin needRebootNode(): failed to set the desired kernel arguments")
+		log.Log.Error(err, "generic-plugin needRebootNode(): failed to verify missing kernel arguments")
 		return false, err
 	}
-	if updateNode {
-		log.Log.V(2).Info("generic-plugin needRebootNode(): need reboot for updating kernel arguments")
-		needReboot = true
+
+	if len(missingKernelArgs) != 0 {
+		updateNode, err = p.syncDesiredKernelArgs(missingKernelArgs)
+		if err != nil {
+			log.Log.Error(err, "generic-plugin needRebootNode(): failed to set the desired kernel arguments")
+			return false, err
+		}
+		if updateNode {
+			log.Log.V(2).Info("generic-plugin needRebootNode(): need reboot for updating kernel arguments")
+			needReboot = true
+		}
 	}
 
 	// Create a map with all the PFs we will need to configure

pkg/plugins/generic/generic_plugin_test.go

@@ -182,7 +182,7 @@ var _ = Describe("Generic plugin", func() {
 				},
 			}
 
-			changed := genericPlugin.CheckStatusChanges(networkNodeState)
+			changed, err := genericPlugin.CheckStatusChanges(networkNodeState)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(changed).To(BeFalse())
 		})
@@ -236,7 +236,7 @@ var _ = Describe("Generic plugin", func() {
 				},
 			}
 
-			changed := genericPlugin.CheckStatusChanges(networkNodeState)
+			changed, err := genericPlugin.CheckStatusChanges(networkNodeState)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(changed).To(BeTrue())
 		})

pkg/plugins/intel/intel_plugin.go

@@ -41,9 +41,9 @@ func (p *IntelPlugin) OnNodeStateChange(*sriovnetworkv1.SriovNetworkNodeState) (
 }
 
 // OnNodeStatusChange verify whether SriovNetworkNodeState CR status present changes on configured VFs.
-func (p *IntelPlugin) CheckStatusChanges(*sriovnetworkv1.SriovNetworkNodeState) bool {
+func (p *IntelPlugin) CheckStatusChanges(*sriovnetworkv1.SriovNetworkNodeState) (bool, error) {
 	log.Log.Info("intel-plugin CheckStatusChanges()")
-	return false
+	return false, nil
 }
 
 // Apply config change

pkg/plugins/k8s/k8s_plugin.go

@@ -161,9 +161,9 @@ func (p *K8sPlugin) OnNodeStateChange(new *sriovnetworkv1.SriovNetworkNodeState)
 }
 
 // OnNodeStatusChange verify whether SriovNetworkNodeState CR status present changes on configured VFs.
-func (p *K8sPlugin) CheckStatusChanges(*sriovnetworkv1.SriovNetworkNodeState) bool {
+func (p *K8sPlugin) CheckStatusChanges(*sriovnetworkv1.SriovNetworkNodeState) (bool, error) {
 	log.Log.Info("k8s-plugin CheckStatusChanges()")
-	return false
+	return false, nil
 }
 
 // Apply config change

pkg/plugins/mellanox/mellanox_plugin.go

@@ -172,9 +172,9 @@ func (p *MellanoxPlugin) OnNodeStateChange(new *sriovnetworkv1.SriovNetworkNodeS
 }
 
 // OnNodeStatusChange verify whether SriovNetworkNodeState CR status present changes on configured VFs.
-func (p *MellanoxPlugin) CheckStatusChanges(*sriovnetworkv1.SriovNetworkNodeState) bool {
+func (p *MellanoxPlugin) CheckStatusChanges(*sriovnetworkv1.SriovNetworkNodeState) (bool, error) {
 	log.Log.Info("mellanox-Plugin CheckStatusChanges()")
-	return false
+	return false, nil
 }
 
 // Apply config change

pkg/plugins/plugin.go

@@ -15,5 +15,5 @@ type VendorPlugin interface {
 	// Apply config change
 	Apply() error
 	// CheckStatusChanges checks status changes on the SriovNetworkNodeState CR for configured VFs.
-	CheckStatusChanges(*sriovnetworkv1.SriovNetworkNodeState) bool
+	CheckStatusChanges(*sriovnetworkv1.SriovNetworkNodeState) (bool, error)
 }

pkg/plugins/virtual/virtual_plugin.go

@@ -68,9 +68,9 @@ func (p *VirtualPlugin) OnNodeStateChange(new *sriovnetworkv1.SriovNetworkNodeSt
 }
 
 // OnNodeStatusChange verify whether SriovNetworkNodeState CR status present changes on configured VFs.
-func (p *VirtualPlugin) CheckStatusChanges(*sriovnetworkv1.SriovNetworkNodeState) bool {
+func (p *VirtualPlugin) CheckStatusChanges(*sriovnetworkv1.SriovNetworkNodeState) (bool, error) {
 	log.Log.Info("virtual-plugin CheckStatusChanges()")
-	return false
+	return false, nil
 }
 
 // Apply config change