Kernel args set by the config daemon should be verified

In the case of vfio-pci, the config daemon might not notice it has to
reboot to add kernel arguments. This could happen if the vfio driver
state was updated but the config daemon was interrupted (e.g policy was
removed and readded quickly).

This could lead to a pending change to the kernel arguments via grubby
or os-tree but without a reboot, these changes wouldn't be applied.

This change introduces a map to hold the desired kernel arguments that
the config daemon wishes to apply. This map is then validated against
"/proc/cmdline" to see if the desired kernel arguments are actually
applied. If not then there will be a subsequent attempt to apply these
arguments.

We need to make sure that the kernel arguments are set, thus we should
pass the errors up to the config daemon instead of silently consuming
the errors when setting kernel arguments.

Signed-off-by: William Zhao <wizhao@redhat.com>

pkg/plugins/generic/generic_plugin.go

@@ -47,14 +47,15 @@ type DriverState struct {
 type DriverStateMapType map[uint]*DriverState
 
 type GenericPlugin struct {
-	PluginName     string
-	SpecVersion    string
-	DesireState    *sriovnetworkv1.SriovNetworkNodeState
-	LastState      *sriovnetworkv1.SriovNetworkNodeState
-	DriverStateMap DriverStateMapType
-	RunningOnHost  bool
-	HostManager    host.HostManagerInterface
-	StoreManager   utils.StoreManagerInterface
+	PluginName        string
+	SpecVersion       string
+	DesireState       *sriovnetworkv1.SriovNetworkNodeState
+	LastState         *sriovnetworkv1.SriovNetworkNodeState
+	DriverStateMap    DriverStateMapType
+	DesiredKernelArgs map[string]bool
+	RunningOnHost     bool
+	HostManager       host.HostManagerInterface
+	StoreManager      utils.StoreManagerInterface
 }
 
 const scriptsPath = "bindata/scripts/enable-kargs.sh"
@@ -84,12 +85,13 @@ func NewGenericPlugin(runningOnHost bool, hostManager host.HostManagerInterface,
 		DriverLoaded:   false,
 	}
 	return &GenericPlugin{
-		PluginName:     PluginName,
-		SpecVersion:    "1.0",
-		DriverStateMap: driverStateMap,
-		RunningOnHost:  runningOnHost,
-		HostManager:    hostManager,
-		StoreManager:   storeManager,
+		PluginName:        PluginName,
+		SpecVersion:       "1.0",
+		DriverStateMap:    driverStateMap,
+		DesiredKernelArgs: make(map[string]bool),
+		RunningOnHost:     runningOnHost,
+		HostManager:       hostManager,
+		StoreManager:      storeManager,
 	}, nil
 }
 
@@ -112,7 +114,10 @@ func (p *GenericPlugin) OnNodeStateChange(new *sriovnetworkv1.SriovNetworkNodeSt
 	p.DesireState = new
 
 	needDrain = p.needDrainNode(new.Spec.Interfaces, new.Status.Interfaces)
-	needReboot = needRebootNode(new, p.DriverStateMap)
+	needReboot, err = p.needRebootNode(new)
+	if err != nil {
+		return false, false, err
+	}
 
 	if needReboot {
 		needDrain = true
@@ -197,28 +202,28 @@ func needDriverCheckVdpaType(state *sriovnetworkv1.SriovNetworkNodeState, driver
 	return false
 }
 
-func tryEnableIommuInKernelArgs() (bool, error) {
-	glog.Info("generic-plugin tryEnableIommuInKernelArgs()")
-	args := [2]string{"intel_iommu=on", "iommu=pt"}
+// setKernelArg Tries to add the kernel args via ostree or grubby.
+func setKernelArg(karg string) (bool, error) {
+	glog.Info("generic-plugin setKernelArg()")
 	var stdout, stderr bytes.Buffer
-	cmd := exec.Command("/bin/sh", scriptsPath, args[0], args[1])
+	cmd := exec.Command("/bin/sh", scriptsPath, karg)
 	cmd.Stdout = &stdout
 	cmd.Stderr = &stderr
 
 	if err := cmd.Run(); err != nil {
 		// if grubby is not there log and assume kernel args are set correctly.
 		if isCommandNotFound(err) {
-			glog.Error("generic-plugin tryEnableIommuInKernelArgs(): grubby command not found. Please ensure that kernel args intel_iommu=on iommu=pt are set")
+			glog.Errorf("generic-plugin setKernelArg(): grubby or ostree command not found. Please ensure that kernel arg %s are set", karg)
 			return false, nil
 		}
-		glog.Errorf("generic-plugin tryEnableIommuInKernelArgs(): fail to enable iommu %s: %v", args, err)
+		glog.Errorf("generic-plugin setKernelArg(): fail to enable kernel arg %s: %v", karg, err)
 		return false, err
 	}
 
 	i, err := strconv.Atoi(strings.TrimSpace(stdout.String()))
 	if err == nil {
 		if i > 0 {
-			glog.Infof("generic-plugin tryEnableIommuInKernelArgs(): need to reboot node")
+			glog.Infof("generic-plugin setKernelArg(): need to reboot node for kernel arg %s", karg)
 			return true, nil
 		}
 	}
@@ -234,6 +239,48 @@ func isCommandNotFound(err error) bool {
 	return false
 }
 
+// addToDesiredKernelArgs Should be called to queue a kernel arg to be added to the node.
+func (p *GenericPlugin) addToDesiredKernelArgs(karg string) {
+	if _, ok := p.DesiredKernelArgs[karg]; !ok {
+		glog.Infof("generic-plugin addToDesiredKernelArgs(): Adding %s to desired kernel arg", karg)
+		p.DesiredKernelArgs[karg] = false
+	}
+}
+
+// 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
+	if len(p.DesiredKernelArgs) == 0 {
+		return false, nil
+	}
+	kargs, err := utils.GetCurrentKernelArgs(false)
+	if err != nil {
+		return false, err
+	}
+	for desiredKarg, attempted := range p.DesiredKernelArgs {
+		set := utils.IsKernelArgsSet(kargs, desiredKarg)
+		if !set {
+			if attempted {
+				glog.V(2).Infof("generic-plugin syncDesiredKernelArgs(): previously attempted to set kernel arg %s", 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 {
+				glog.Errorf("generic-plugin syncDesiredKernelArgs(): fail to set kernel arg %s: %v", desiredKarg, err)
+				return false, err
+			}
+			if update {
+				needReboot = true
+				glog.V(2).Infof("generic-plugin syncDesiredKernelArgs(): need reboot for setting kernel arg %s", desiredKarg)
+			}
+			p.DesiredKernelArgs[desiredKarg] = true
+		}
+	}
+	return needReboot, nil
+}
+
 func (p *GenericPlugin) needDrainNode(desired sriovnetworkv1.Interfaces, current sriovnetworkv1.InterfaceExts) (needDrain bool) {
 	glog.V(2).Infof("generic-plugin needDrainNode(): current state '%+v', desired state '%+v'", current, desired)
 
@@ -285,33 +332,39 @@ func (p *GenericPlugin) needDrainNode(desired sriovnetworkv1.Interfaces, current
 	return
 }
 
-func needRebootIfVfio(state *sriovnetworkv1.SriovNetworkNodeState, driverMap DriverStateMapType) (needReboot bool) {
-	driverState := driverMap[Vfio]
+func (p *GenericPlugin) addVfioDesiredKernelArg(state *sriovnetworkv1.SriovNetworkNodeState) {
+	driverState := p.DriverStateMap[Vfio]
 	if !driverState.DriverLoaded && driverState.NeedDriverFunc(state, driverState) {
-		var err error
-		needReboot, err = tryEnableIommuInKernelArgs()
-		if err != nil {
-			glog.Errorf("generic-plugin needRebootNode():fail to enable iommu in kernel args: %v", err)
-		}
-		if needReboot {
-			glog.V(2).Infof("generic-plugin needRebootNode(): need reboot for enabling iommu kernel args")
-		}
+		p.addToDesiredKernelArgs(utils.KernelArgIntelIommu)
+		p.addToDesiredKernelArgs(utils.KernelArgIommuPt)
 	}
-	return needReboot
 }
 
-func needRebootNode(state *sriovnetworkv1.SriovNetworkNodeState, driverMap DriverStateMapType) (needReboot bool) {
-	needReboot = needRebootIfVfio(state, driverMap)
-	updateNode, err := utils.WriteSwitchdevConfFile(state)
+func (p *GenericPlugin) needRebootNode(state *sriovnetworkv1.SriovNetworkNodeState) (needReboot bool, err error) {
+	needReboot = false
+	p.addVfioDesiredKernelArg(state)
 
+	updateNode, err := p.syncDesiredKernelArgs()
+	if err != nil {
+		glog.Errorf("generic-plugin needRebootNode(): failed to set the desired kernel arguments")
+		return false, err
+	}
+	if updateNode {
+		glog.V(2).Infof("generic-plugin needRebootNode(): need reboot for updating kernel arguments")
+		needReboot = true
+	}
+
+	updateNode, err = utils.WriteSwitchdevConfFile(state)
 	if err != nil {
 		glog.Errorf("generic-plugin needRebootNode(): fail to write switchdev device config file")
+		return false, err
 	}
 	if updateNode {
 		glog.V(2).Infof("generic-plugin needRebootNode(): need reboot for updating switchdev device configuration")
+		needReboot = true
 	}
-	needReboot = needReboot || updateNode
-	return
+
+	return needReboot, nil
 }
 
 // ////////////// for testing purposes only ///////////////////////

pkg/utils/utils.go

@@ -33,6 +33,7 @@ const (
 	sysBusPciDrivers      = "/sys/bus/pci/drivers"
 	sysBusPciDriversProbe = "/sys/bus/pci/drivers_probe"
 	sysClassNet           = "/sys/class/net"
+	procKernelCmdLine     = "/proc/cmdline"
 	netClass              = 0x02
 	numVfsFile            = "sriov_numvfs"
 
@@ -46,6 +47,9 @@ const (
 	udevRulesFolder = udevFolder + "/rules.d"
 	udevDisableNM   = "/bindata/scripts/udev-find-sriov-pf.sh"
 	nmUdevRule      = "SUBSYSTEM==\"net\", ACTION==\"add|change|move\", ATTRS{device}==\"%s\", IMPORT{program}=\"/etc/udev/disable-nm-sriov.sh $env{INTERFACE} %s\""
+
+	KernelArgIntelIommu = "intel_iommu=on"
+	KernelArgIommuPt    = "iommu=pt"
 )
 
 var InitialState sriovnetworkv1.SriovNetworkNodeState
@@ -62,6 +66,31 @@ func init() {
 	ClusterType = os.Getenv("CLUSTER_TYPE")
 }
 
+// GetCurrentKernelArgs This retrieves the kernel cmd line arguments
+func GetCurrentKernelArgs(chroot bool) (string, error) {
+	path := procKernelCmdLine
+	if !chroot {
+		path = "/host" + path
+	}
+	cmdLine, err := os.ReadFile(path)
+	if err != nil {
+		return "", fmt.Errorf("GetCurrentKernelArgs(): Error reading %s: %v", procKernelCmdLine, err)
+	}
+	return string(cmdLine), nil
+}
+
+// IsKernelArgsSet This checks if the kernel cmd line is set properly. Please note that the same key could be repeated
+// several times in the kernel cmd line. We can only ensure that the kernel cmd line has the key/val kernel arg that we set.
+func IsKernelArgsSet(cmdLine string, karg string) bool {
+	elements := strings.Fields(cmdLine)
+	for _, element := range elements {
+		if element == karg {
+			return true
+		}
+	}
+	return false
+}
+
 func DiscoverSriovDevices(withUnsupported bool, storeManager StoreManagerInterface) ([]sriovnetworkv1.InterfaceExt, error) {
 	glog.V(2).Info("DiscoverSriovDevices")
 	pfList := []sriovnetworkv1.InterfaceExt{}