From 9162bf5a2f9daba0c86529bf27b7910d85c6a1bb Mon Sep 17 00:00:00 2001 From: adrianc Date: Wed, 27 Dec 2023 15:34:32 +0200 Subject: [PATCH 1/6] Rename plugin names - remove _plugin suffix from plugin names as it doesnt add any important information. - rename logs Signed-off-by: adrianc --- pkg/daemon/daemon.go | 8 ++-- pkg/plugins/generic/generic_plugin.go | 52 ++++++++++++------------- pkg/plugins/intel/intel_plugin.go | 6 +-- pkg/plugins/k8s/k8s_plugin.go | 20 +++++----- pkg/plugins/mellanox/mellanox_plugin.go | 10 ++--- pkg/plugins/virtual/virtual_plugin.go | 14 +++---- 6 files changed, 55 insertions(+), 55 deletions(-) diff --git a/pkg/daemon/daemon.go b/pkg/daemon/daemon.go index 9e25b7bef..461027643 100644 --- a/pkg/daemon/daemon.go +++ b/pkg/daemon/daemon.go @@ -619,10 +619,10 @@ func (dn *Daemon) nodeStateSyncHandler() error { // For BareMetal machines apply the generic plugin selectedPlugin, ok := dn.enabledPlugins[GenericPluginName] if ok { - // Apply generic_plugin last + // Apply generic plugin last err = selectedPlugin.Apply() if err != nil { - log.Log.Error(err, "nodeStateSyncHandler(): generic_plugin fail to apply") + log.Log.Error(err, "nodeStateSyncHandler(): generic plugin fail to apply") return err } } @@ -630,10 +630,10 @@ func (dn *Daemon) nodeStateSyncHandler() error { // For Virtual machines apply the virtual plugin selectedPlugin, ok = dn.enabledPlugins[VirtualPluginName] if ok { - // Apply virtual_plugin last + // Apply virtual plugin last err = selectedPlugin.Apply() if err != nil { - log.Log.Error(err, "nodeStateSyncHandler(): virtual_plugin failed to apply") + log.Log.Error(err, "nodeStateSyncHandler(): virtual plugin failed to apply") return err } } diff --git a/pkg/plugins/generic/generic_plugin.go b/pkg/plugins/generic/generic_plugin.go index 88aa38549..3cad7cfd6 100644 --- a/pkg/plugins/generic/generic_plugin.go +++ b/pkg/plugins/generic/generic_plugin.go @@ -21,7 +21,7 @@ import ( mlx "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/vendors/mellanox" ) -var PluginName = "generic_plugin" +var PluginName = "generic" // driver id const ( @@ -109,7 +109,7 @@ func (p *GenericPlugin) Spec() string { // OnNodeStateChange Invoked when SriovNetworkNodeState CR is created or updated, return if need drain and/or reboot node func (p *GenericPlugin) OnNodeStateChange(new *sriovnetworkv1.SriovNetworkNodeState) (needDrain bool, needReboot bool, err error) { - log.Log.Info("generic-plugin OnNodeStateChange()") + log.Log.Info("generic plugin OnNodeStateChange()") p.DesireState = new needDrain = p.needDrainNode(new.Spec.Interfaces, new.Status.Interfaces) @@ -129,7 +129,7 @@ func (p *GenericPlugin) syncDriverState() error { if !driverState.DriverLoaded && driverState.NeedDriverFunc(p.DesireState, driverState) { log.Log.V(2).Info("loading driver", "name", driverState.DriverName) if err := p.helpers.LoadKernelModule(driverState.DriverName); err != nil { - log.Log.Error(err, "generic-plugin syncDriverState(): fail to load kmod", "name", driverState.DriverName) + log.Log.Error(err, "generic plugin syncDriverState(): fail to load kmod", "name", driverState.DriverName) return err } driverState.DriverLoaded = true @@ -140,12 +140,12 @@ func (p *GenericPlugin) syncDriverState() error { // Apply config change func (p *GenericPlugin) Apply() error { - log.Log.Info("generic-plugin Apply()", "desiredState", p.DesireState.Spec) + log.Log.Info("generic plugin Apply()", "desiredState", p.DesireState.Spec) if p.LastState != nil { - log.Log.Info("generic-plugin Apply()", "lastState", p.LastState.Spec) + log.Log.Info("generic plugin Apply()", "lastState", p.LastState.Spec) if reflect.DeepEqual(p.LastState.Spec.Interfaces, p.DesireState.Spec.Interfaces) { - log.Log.Info("generic-plugin Apply(): desired and latest state are the same, nothing to apply") + log.Log.Info("generic plugin Apply(): desired and latest state are the same, nothing to apply") return nil } } @@ -199,7 +199,7 @@ func needDriverCheckVdpaType(state *sriovnetworkv1.SriovNetworkNodeState, driver // setKernelArg Tries to add the kernel args via ostree or grubby. func setKernelArg(karg string) (bool, error) { - log.Log.Info("generic-plugin setKernelArg()") + log.Log.Info("generic plugin setKernelArg()") var stdout, stderr bytes.Buffer cmd := exec.Command("/bin/sh", scriptsPath, karg) cmd.Stdout = &stdout @@ -208,18 +208,18 @@ func setKernelArg(karg string) (bool, error) { if err := cmd.Run(); err != nil { // if grubby is not there log and assume kernel args are set correctly. if utils.IsCommandNotFound(err) { - log.Log.Error(err, "generic-plugin setKernelArg(): grubby or ostree command not found. Please ensure that kernel arg are set", + log.Log.Error(err, "generic plugin setKernelArg(): grubby or ostree command not found. Please ensure that kernel arg are set", "kargs", karg) return false, nil } - log.Log.Error(err, "generic-plugin setKernelArg(): fail to enable kernel arg", "karg", karg) + log.Log.Error(err, "generic plugin setKernelArg(): fail to enable kernel arg", "karg", karg) return false, err } i, err := strconv.Atoi(strings.TrimSpace(stdout.String())) if err == nil { if i > 0 { - log.Log.Info("generic-plugin setKernelArg(): need to reboot node for kernel arg", "karg", karg) + log.Log.Info("generic plugin setKernelArg(): need to reboot node for kernel arg", "karg", karg) return true, nil } } @@ -229,7 +229,7 @@ func setKernelArg(karg string) (bool, error) { // 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 { - log.Log.Info("generic-plugin addToDesiredKernelArgs(): Adding to desired kernel arg", "karg", karg) + log.Log.Info("generic plugin addToDesiredKernelArgs(): Adding to desired kernel arg", "karg", karg) p.DesiredKernelArgs[karg] = false } } @@ -248,7 +248,7 @@ func (p *GenericPlugin) syncDesiredKernelArgs() (bool, error) { set := p.helpers.IsKernelArgsSet(kargs, desiredKarg) if !set { if attempted { - log.Log.V(2).Info("generic-plugin syncDesiredKernelArgs(): previously attempted to set kernel arg", + 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 @@ -256,12 +256,12 @@ func (p *GenericPlugin) syncDesiredKernelArgs() (bool, error) { // 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) + 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) + log.Log.V(2).Info("generic plugin syncDesiredKernelArgs(): need reboot for setting kernel arg", "karg", desiredKarg) } p.DesiredKernelArgs[desiredKarg] = true } @@ -270,7 +270,7 @@ func (p *GenericPlugin) syncDesiredKernelArgs() (bool, error) { } func (p *GenericPlugin) needDrainNode(desired sriovnetworkv1.Interfaces, current sriovnetworkv1.InterfaceExts) (needDrain bool) { - log.Log.V(2).Info("generic-plugin needDrainNode()", "current", current, "desired", desired) + log.Log.V(2).Info("generic plugin needDrainNode()", "current", current, "desired", desired) needDrain = false for _, ifaceStatus := range current { @@ -279,17 +279,17 @@ func (p *GenericPlugin) needDrainNode(desired sriovnetworkv1.Interfaces, current if iface.PciAddress == ifaceStatus.PciAddress { configured = true if ifaceStatus.NumVfs == 0 { - log.Log.V(2).Info("generic-plugin needDrainNode(): no need drain, for PCI address, current NumVfs is 0", + log.Log.V(2).Info("generic plugin needDrainNode(): no need drain, for PCI address, current NumVfs is 0", "address", iface.PciAddress) break } if sriovnetworkv1.NeedToUpdateSriov(&iface, &ifaceStatus) { - log.Log.V(2).Info("generic-plugin needDrainNode(): need drain, for PCI address request update", + log.Log.V(2).Info("generic plugin needDrainNode(): need drain, for PCI address request update", "address", iface.PciAddress) needDrain = true return } - log.Log.V(2).Info("generic-plugin needDrainNode(): no need drain,for PCI address", + log.Log.V(2).Info("generic plugin needDrainNode(): no need drain,for PCI address", "address", iface.PciAddress, "expected-vfs", iface.NumVfs, "current-vfs", ifaceStatus.NumVfs) } } @@ -297,26 +297,26 @@ func (p *GenericPlugin) needDrainNode(desired sriovnetworkv1.Interfaces, current // load the PF info pfStatus, exist, err := p.helpers.LoadPfsStatus(ifaceStatus.PciAddress) if err != nil { - log.Log.Error(err, "generic-plugin needDrainNode(): failed to load info about PF status for pci device", + log.Log.Error(err, "generic plugin needDrainNode(): failed to load info about PF status for pci device", "address", ifaceStatus.PciAddress) continue } if !exist { - log.Log.Info("generic-plugin needDrainNode(): PF name with pci address has VFs configured but they weren't created by the sriov operator. Skipping drain", + log.Log.Info("generic plugin needDrainNode(): PF name with pci address has VFs configured but they weren't created by the sriov operator. Skipping drain", "name", ifaceStatus.Name, "address", ifaceStatus.PciAddress) continue } if pfStatus.ExternallyManaged { - log.Log.Info("generic-plugin needDrainNode()(): PF name with pci address was externally created. Skipping drain", + log.Log.Info("generic plugin needDrainNode()(): PF name with pci address was externally created. Skipping drain", "name", ifaceStatus.Name, "address", ifaceStatus.PciAddress) continue } - log.Log.V(2).Info("generic-plugin needDrainNode(): need drain since interface needs to be reset", + log.Log.V(2).Info("generic plugin needDrainNode(): need drain since interface needs to be reset", "interface", ifaceStatus) needDrain = true return @@ -339,11 +339,11 @@ func (p *GenericPlugin) needRebootNode(state *sriovnetworkv1.SriovNetworkNodeSta updateNode, err := p.syncDesiredKernelArgs() 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 set the desired kernel arguments") return false, err } if updateNode { - log.Log.V(2).Info("generic-plugin needRebootNode(): need reboot for updating kernel arguments") + log.Log.V(2).Info("generic plugin needRebootNode(): need reboot for updating kernel arguments") needReboot = true } @@ -358,11 +358,11 @@ func (p *GenericPlugin) needRebootNode(state *sriovnetworkv1.SriovNetworkNodeSta updateNode, err = p.helpers.WriteSwitchdevConfFile(state, p.pfsToSkip) if err != nil { - log.Log.Error(err, "generic-plugin needRebootNode(): fail to write switchdev device config file") + log.Log.Error(err, "generic plugin needRebootNode(): fail to write switchdev device config file") return false, err } if updateNode { - log.Log.V(2).Info("generic-plugin needRebootNode(): need reboot for updating switchdev device configuration") + log.Log.V(2).Info("generic plugin needRebootNode(): need reboot for updating switchdev device configuration") needReboot = true } diff --git a/pkg/plugins/intel/intel_plugin.go b/pkg/plugins/intel/intel_plugin.go index 1c64a47fb..467116ddb 100644 --- a/pkg/plugins/intel/intel_plugin.go +++ b/pkg/plugins/intel/intel_plugin.go @@ -8,7 +8,7 @@ import ( plugin "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/plugins" ) -var PluginName = "intel_plugin" +var PluginName = "intel" type IntelPlugin struct { PluginName string @@ -36,12 +36,12 @@ func (p *IntelPlugin) Spec() string { // OnNodeStateChange Invoked when SriovNetworkNodeState CR is created or updated, return if need dain and/or reboot node func (p *IntelPlugin) OnNodeStateChange(new *sriovnetworkv1.SriovNetworkNodeState) (needDrain bool, needReboot bool, err error) { - log.Log.Info("intel-plugin OnNodeStateChange()") + log.Log.Info("intel plugin OnNodeStateChange()") return false, false, nil } // Apply config change func (p *IntelPlugin) Apply() error { - log.Log.Info("intel-plugin Apply()") + log.Log.Info("intel plugin Apply()") return nil } diff --git a/pkg/plugins/k8s/k8s_plugin.go b/pkg/plugins/k8s/k8s_plugin.go index 02b4e6336..faa862544 100644 --- a/pkg/plugins/k8s/k8s_plugin.go +++ b/pkg/plugins/k8s/k8s_plugin.go @@ -16,7 +16,7 @@ import ( "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/vars" ) -var PluginName = "k8s_plugin" +var PluginName = "k8s" type K8sPlugin struct { PluginName string @@ -118,7 +118,7 @@ func (p *K8sPlugin) Spec() string { // OnNodeStateChange Invoked when SriovNetworkNodeState CR is created or updated, return if need dain and/or reboot node func (p *K8sPlugin) OnNodeStateChange(new *sriovnetworkv1.SriovNetworkNodeState) (needDrain bool, needReboot bool, err error) { - log.Log.Info("k8s-plugin OnNodeStateChange()") + log.Log.Info("k8s plugin OnNodeStateChange()") needDrain = false needReboot = false @@ -133,7 +133,7 @@ func (p *K8sPlugin) OnNodeStateChange(new *sriovnetworkv1.SriovNetworkNodeState) // Check services err = p.switchDevServicesStateUpdate() if err != nil { - log.Log.Error(err, "k8s-plugin OnNodeStateChange(): failed") + log.Log.Error(err, "k8s plugin OnNodeStateChange(): failed") return } } @@ -142,7 +142,7 @@ func (p *K8sPlugin) OnNodeStateChange(new *sriovnetworkv1.SriovNetworkNodeState) // Check sriov service err = p.sriovServiceStateUpdate() if err != nil { - log.Log.Error(err, "k8s-plugin OnNodeStateChange(): failed") + log.Log.Error(err, "k8s plugin OnNodeStateChange(): failed") return } } @@ -151,9 +151,9 @@ func (p *K8sPlugin) OnNodeStateChange(new *sriovnetworkv1.SriovNetworkNodeState) needDrain = true if p.updateTarget.needReboot() { needReboot = true - log.Log.Info("k8s-plugin OnNodeStateChange(): needReboot to update", "target", p.updateTarget) + log.Log.Info("k8s plugin OnNodeStateChange(): needReboot to update", "target", p.updateTarget) } else { - log.Log.Info("k8s-plugin OnNodeStateChange(): needDrain to update", "target", p.updateTarget) + log.Log.Info("k8s plugin OnNodeStateChange(): needDrain to update", "target", p.updateTarget) } } @@ -162,7 +162,7 @@ func (p *K8sPlugin) OnNodeStateChange(new *sriovnetworkv1.SriovNetworkNodeState) // Apply config change func (p *K8sPlugin) Apply() error { - log.Log.Info("k8s-plugin Apply()") + log.Log.Info("k8s plugin Apply()") if err := p.updateSwitchdevService(); err != nil { return err } @@ -369,14 +369,14 @@ func (p *K8sPlugin) isSystemServiceNeedUpdate(serviceObj *hostTypes.Service) boo log.Log.Info("isSystemServiceNeedUpdate()") systemService, err := p.hostHelper.ReadService(serviceObj.Path) if err != nil { - log.Log.Error(err, "k8s-plugin isSystemServiceNeedUpdate(): failed to read sriov-config service file, ignoring", + log.Log.Error(err, "k8s plugin isSystemServiceNeedUpdate(): failed to read sriov-config service file, ignoring", "path", serviceObj.Path) return false } if systemService != nil { needChange, err := p.hostHelper.CompareServices(systemService, serviceObj) if err != nil { - log.Log.Error(err, "k8s-plugin isSystemServiceNeedUpdate(): failed to compare sriov-config service, ignoring") + log.Log.Error(err, "k8s plugin isSystemServiceNeedUpdate(): failed to compare sriov-config service, ignoring") return false } return needChange @@ -393,7 +393,7 @@ func (p *K8sPlugin) systemServicesStateUpdate() error { return err } if !exist { - return fmt.Errorf("k8s-plugin systemServicesStateUpdate(): %q not found", systemService.Name) + return fmt.Errorf("k8s plugin systemServicesStateUpdate(): %q not found", systemService.Name) } if p.isSystemServiceNeedUpdate(systemService) { services = append(services, systemService) diff --git a/pkg/plugins/mellanox/mellanox_plugin.go b/pkg/plugins/mellanox/mellanox_plugin.go index a95d24b9d..b9e05668b 100644 --- a/pkg/plugins/mellanox/mellanox_plugin.go +++ b/pkg/plugins/mellanox/mellanox_plugin.go @@ -11,7 +11,7 @@ import ( mlx "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/vendors/mellanox" ) -var PluginName = "mellanox_plugin" +var PluginName = "mellanox" type MellanoxPlugin struct { PluginName string @@ -46,7 +46,7 @@ func (p *MellanoxPlugin) Spec() string { // OnNodeStateChange Invoked when SriovNetworkNodeState CR is created or updated, return if need dain and/or reboot node func (p *MellanoxPlugin) OnNodeStateChange(new *sriovnetworkv1.SriovNetworkNodeState) (needDrain bool, needReboot bool, err error) { - log.Log.Info("mellanox-Plugin OnNodeStateChange()") + log.Log.Info("mellalnox plugin OnNodeStateChange()") needDrain = false needReboot = false @@ -167,17 +167,17 @@ func (p *MellanoxPlugin) OnNodeStateChange(new *sriovnetworkv1.SriovNetworkNodeS if needReboot { needDrain = true } - log.Log.V(2).Info("mellanox-plugin", "need-drain", needDrain, "need-reboot", needReboot) + log.Log.V(2).Info("mellalnox plugin", "need-drain", needDrain, "need-reboot", needReboot) return } // Apply config change func (p *MellanoxPlugin) Apply() error { if p.helpers.IsKernelLockdownMode() { - log.Log.Info("mellanox-plugin Apply() - skipping due to lockdown mode") + log.Log.Info("mellanox plugin Apply() - skipping due to lockdown mode") return nil } - log.Log.Info("mellanox-plugin Apply()") + log.Log.Info("mellanox plugin Apply()") return p.helpers.MlxConfigFW(attributesToChange) } diff --git a/pkg/plugins/virtual/virtual_plugin.go b/pkg/plugins/virtual/virtual_plugin.go index 06a57a80f..a942aaa86 100644 --- a/pkg/plugins/virtual/virtual_plugin.go +++ b/pkg/plugins/virtual/virtual_plugin.go @@ -12,7 +12,7 @@ import ( "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/vars" ) -var PluginName = "virtual_plugin" +var PluginName = "virtual" // VirtualPlugin Plugin type to use on a virtual platform type VirtualPlugin struct { @@ -52,7 +52,7 @@ func (p *VirtualPlugin) Spec() string { // OnNodeStateChange Invoked when SriovNetworkNodeState CR is created or updated, return if need dain and/or reboot node func (p *VirtualPlugin) OnNodeStateChange(new *sriovnetworkv1.SriovNetworkNodeState) (needDrain bool, needReboot bool, err error) { - log.Log.Info("virtual-plugin OnNodeStateChange()") + log.Log.Info("virtual plugin OnNodeStateChange()") needDrain = false needReboot = false err = nil @@ -69,7 +69,7 @@ func (p *VirtualPlugin) OnNodeStateChange(new *sriovnetworkv1.SriovNetworkNodeSt // Apply config change func (p *VirtualPlugin) Apply() error { - log.Log.Info("virtual-plugin Apply()", "desired-state", p.DesireState.Spec) + log.Log.Info("virtual plugin Apply()", "desired-state", p.DesireState.Spec) if p.LoadVfioDriver == loading { // In virtual deployments of Kubernetes where the underlying virtualization platform does not support a virtualized iommu @@ -78,21 +78,21 @@ func (p *VirtualPlugin) Apply() error { // NOTE: if VFIO was already loaded for some reason, we will not try to load it again with the new options. kernelArgs := "enable_unsafe_noiommu_mode=1" if err := p.helpers.LoadKernelModule("vfio", kernelArgs); err != nil { - log.Log.Error(err, "virtual-plugin Apply(): fail to load vfio kmod") + log.Log.Error(err, "virtual plugin Apply(): fail to load vfio kmod") return err } if err := p.helpers.LoadKernelModule("vfio_pci"); err != nil { - log.Log.Error(err, "virtual-plugin Apply(): fail to load vfio_pci kmod") + log.Log.Error(err, "virtual plugin Apply(): fail to load vfio_pci kmod") return err } p.LoadVfioDriver = loaded } if p.LastState != nil { - log.Log.Info("virtual-plugin Apply()", "last-state", p.LastState.Spec) + log.Log.Info("virtual plugin Apply()", "last-state", p.LastState.Spec) if reflect.DeepEqual(p.LastState.Spec.Interfaces, p.DesireState.Spec.Interfaces) { - log.Log.Info("virtual-plugin Apply(): nothing to apply") + log.Log.Info("virtual plugin Apply(): nothing to apply") return nil } } From c4e4daa446217b6b5578cb826cebd8180efbe902 Mon Sep 17 00:00:00 2001 From: adrianc Date: Wed, 27 Dec 2023 16:21:48 +0200 Subject: [PATCH 2/6] rename enabledPlugins to loadedPlugins in daemon This better reflects the meaning of whats being done. In addition it serves as prep work to support disable plugins as having both enabledPlugins and disabledPlugins attr in Daemon is confusing. Signed-off-by: adrianc --- pkg/daemon/daemon.go | 14 +++++++------- pkg/daemon/daemon_test.go | 2 +- pkg/daemon/plugin.go | 34 +++++++++++++++++----------------- 3 files changed, 25 insertions(+), 25 deletions(-) diff --git a/pkg/daemon/daemon.go b/pkg/daemon/daemon.go index 461027643..ab0cdb8df 100644 --- a/pkg/daemon/daemon.go +++ b/pkg/daemon/daemon.go @@ -69,7 +69,7 @@ type Daemon struct { nodeState *sriovnetworkv1.SriovNetworkNodeState - enabledPlugins map[string]plugin.VendorPlugin + loadedPlugins map[string]plugin.VendorPlugin HostHelpers helper.HostHelpersInterface @@ -508,8 +508,8 @@ func (dn *Daemon) nodeStateSyncHandler() error { latestState.Status = updatedState.Status // load plugins if it has not loaded - if len(dn.enabledPlugins) == 0 { - dn.enabledPlugins, err = enablePlugins(latestState, dn.HostHelpers) + if len(dn.loadedPlugins) == 0 { + dn.loadedPlugins, err = loadPlugins(latestState, dn.HostHelpers) if err != nil { log.Log.Error(err, "nodeStateSyncHandler(): failed to enable vendor plugins") return err @@ -520,7 +520,7 @@ func (dn *Daemon) nodeStateSyncHandler() error { reqDrain := false // check if any of the plugins required to drain or reboot the node - for k, p := range dn.enabledPlugins { + for k, p := range dn.loadedPlugins { d, r := false, false if dn.nodeState.GetName() == "" { log.Log.V(0).Info("nodeStateSyncHandler(): calling OnNodeStateChange for a new node state") @@ -570,7 +570,7 @@ func (dn *Daemon) nodeStateSyncHandler() error { log.Log.V(0).Info("nodeStateSyncHandler(): aggregated daemon", "drain-required", reqDrain, "reboot-required", reqReboot, "disable-drain", dn.disableDrain) - for k, p := range dn.enabledPlugins { + for k, p := range dn.loadedPlugins { // Skip both the general and virtual plugin apply them last if k != GenericPluginName && k != VirtualPluginName { err := p.Apply() @@ -617,7 +617,7 @@ func (dn *Daemon) nodeStateSyncHandler() error { if !reqReboot && !vars.UsingSystemdMode { // For BareMetal machines apply the generic plugin - selectedPlugin, ok := dn.enabledPlugins[GenericPluginName] + selectedPlugin, ok := dn.loadedPlugins[GenericPluginName] if ok { // Apply generic plugin last err = selectedPlugin.Apply() @@ -628,7 +628,7 @@ func (dn *Daemon) nodeStateSyncHandler() error { } // For Virtual machines apply the virtual plugin - selectedPlugin, ok = dn.enabledPlugins[VirtualPluginName] + selectedPlugin, ok = dn.loadedPlugins[VirtualPluginName] if ok { // Apply virtual plugin last err = selectedPlugin.Apply() diff --git a/pkg/daemon/daemon_test.go b/pkg/daemon/daemon_test.go index e81f7b677..f43a43493 100644 --- a/pkg/daemon/daemon_test.go +++ b/pkg/daemon/daemon_test.go @@ -140,7 +140,7 @@ var _ = Describe("Config Daemon", func() { er, ) - sut.enabledPlugins = map[string]plugin.VendorPlugin{generic.PluginName: &fake.FakePlugin{}} + sut.loadedPlugins = map[string]plugin.VendorPlugin{generic.PluginName: &fake.FakePlugin{}} go func() { defer GinkgoRecover() diff --git a/pkg/daemon/plugin.go b/pkg/daemon/plugin.go index 8cb344347..ebc04c7c5 100644 --- a/pkg/daemon/plugin.go +++ b/pkg/daemon/plugin.go @@ -30,57 +30,57 @@ var ( K8sPlugin = k8splugin.NewK8sPlugin ) -func enablePlugins(ns *sriovnetworkv1.SriovNetworkNodeState, helpers helper.HostHelpersInterface) (map[string]plugin.VendorPlugin, error) { +func loadPlugins(ns *sriovnetworkv1.SriovNetworkNodeState, helpers helper.HostHelpersInterface) (map[string]plugin.VendorPlugin, error) { log.Log.Info("enableVendorPlugins(): enabling plugins") - enabledPlugins := map[string]plugin.VendorPlugin{} + loadedPlugins := map[string]plugin.VendorPlugin{} if vars.PlatformType == consts.VirtualOpenStack { virtualPlugin, err := VirtualPlugin(helpers) if err != nil { - log.Log.Error(err, "enableVendorPlugins(): failed to load the virtual plugin") + log.Log.Error(err, "loadPlugins(): failed to load the virtual plugin") return nil, err } - enabledPlugins[virtualPlugin.Name()] = virtualPlugin + loadedPlugins[virtualPlugin.Name()] = virtualPlugin } else { - enabledVendorPlugins, err := registerVendorPlugins(ns, helpers) + loadedVendorPlugins, err := loadVendorPlugins(ns, helpers) if err != nil { return nil, err } - enabledPlugins = enabledVendorPlugins + loadedPlugins = loadedVendorPlugins if vars.ClusterType != consts.ClusterTypeOpenshift { k8sPlugin, err := K8sPlugin(helpers) if err != nil { - log.Log.Error(err, "enableVendorPlugins(): failed to load the k8s plugin") + log.Log.Error(err, "loadPlugins(): failed to load the k8s plugin") return nil, err } - enabledPlugins[k8sPlugin.Name()] = k8sPlugin + loadedPlugins[k8sPlugin.Name()] = k8sPlugin } genericPlugin, err := GenericPlugin(helpers) if err != nil { - log.Log.Error(err, "enableVendorPlugins(): failed to load the generic plugin") + log.Log.Error(err, "loadPlugins(): failed to load the generic plugin") return nil, err } - enabledPlugins[genericPlugin.Name()] = genericPlugin + loadedPlugins[genericPlugin.Name()] = genericPlugin } - pluginList := make([]string, 0, len(enabledPlugins)) - for pluginName := range enabledPlugins { + pluginList := make([]string, 0, len(loadedPlugins)) + for pluginName := range loadedPlugins { pluginList = append(pluginList, pluginName) } - log.Log.Info("enableVendorPlugins(): enabled plugins", "plugins", pluginList) - return enabledPlugins, nil + log.Log.Info("loadPlugins(): loaded plugins", "plugins", pluginList) + return loadedPlugins, nil } -func registerVendorPlugins(ns *sriovnetworkv1.SriovNetworkNodeState, helpers helper.HostHelpersInterface) (map[string]plugin.VendorPlugin, error) { +func loadVendorPlugins(ns *sriovnetworkv1.SriovNetworkNodeState, helpers helper.HostHelpersInterface) (map[string]plugin.VendorPlugin, error) { vendorPlugins := map[string]plugin.VendorPlugin{} for _, iface := range ns.Status.Interfaces { if val, ok := VendorPluginMap[iface.Vendor]; ok { plug, err := val(helpers) if err != nil { - log.Log.Error(err, "registerVendorPlugins(): failed to load plugin", "plugin-name", plug.Name()) - return vendorPlugins, fmt.Errorf("registerVendorPlugins(): failed to load the %s plugin error: %v", plug.Name(), err) + log.Log.Error(err, "loadVendorPlugins(): failed to load plugin", "plugin-name", plug.Name()) + return vendorPlugins, fmt.Errorf("loadVendorPlugins(): failed to load the %s plugin error: %v", plug.Name(), err) } if _, ok := vendorPlugins[plug.Name()]; !ok { vendorPlugins[plug.Name()] = plug From 45884f79b98895889bfbdc5adc76c5c7d4d34ba6 Mon Sep 17 00:00:00 2001 From: adrianc Date: Thu, 28 Dec 2023 12:13:09 +0200 Subject: [PATCH 3/6] Add disable plugins to config daemon - Add --disable-plugins CLI flag - parse and check flag values and pass to daemon - extend plugin load logic to skip disabled plugins Signed-off-by: adrianc --- cmd/sriov-network-config-daemon/start.go | 38 +++++++++++++++++++-- pkg/daemon/daemon.go | 9 +++-- pkg/daemon/daemon_test.go | 1 + pkg/daemon/plugin.go | 42 +++++++++++++++++++----- pkg/vars/vars.go | 3 ++ 5 files changed, 79 insertions(+), 14 deletions(-) diff --git a/cmd/sriov-network-config-daemon/start.go b/cmd/sriov-network-config-daemon/start.go index 1d7d4d00c..3ff895cae 100644 --- a/cmd/sriov-network-config-daemon/start.go +++ b/cmd/sriov-network-config-daemon/start.go @@ -46,6 +46,29 @@ import ( "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/vars" ) +// stringList is a list of strings, implements pflag.Value interface +type stringList []string + +func (sl *stringList) String() string { + return strings.Join(*sl, ",") +} + +func (sl *stringList) Set(arg string) error { + elems := strings.Split(arg, ",") + + for _, elem := range elems { + if len(elem) == 0 { + return fmt.Errorf("empty plugin name") + } + *sl = append(*sl, elem) + } + return nil +} + +func (sl *stringList) Type() string { + return "CommaSeparatedString" +} + var ( startCmd = &cobra.Command{ Use: "start", @@ -55,9 +78,10 @@ var ( } startOpts struct { - kubeconfig string - nodeName string - systemd bool + kubeconfig string + nodeName string + systemd bool + disabledPlugins stringList } ) @@ -66,6 +90,7 @@ func init() { startCmd.PersistentFlags().StringVar(&startOpts.kubeconfig, "kubeconfig", "", "Kubeconfig file to access a remote cluster (testing only)") startCmd.PersistentFlags().StringVar(&startOpts.nodeName, "node-name", "", "kubernetes node name daemon is managing") startCmd.PersistentFlags().BoolVar(&startOpts.systemd, "use-systemd-service", false, "use config daemon in systemd mode") + startCmd.PersistentFlags().VarP(&startOpts.disabledPlugins, "disable-plugins", "", "comma-separated list of plugins to disable") } func runStartCmd(cmd *cobra.Command, args []string) error { @@ -88,6 +113,12 @@ func runStartCmd(cmd *cobra.Command, args []string) error { } vars.NodeName = startOpts.nodeName + for _, p := range startOpts.disabledPlugins { + if _, ok := vars.DisableablePlugins[p]; !ok { + return fmt.Errorf("%s plugin cannot be disabled", p) + } + } + // This channel is used to ensure all spawned goroutines exit when we exit. stopCh := make(chan struct{}) defer close(stopCh) @@ -243,6 +274,7 @@ func runStartCmd(cmd *cobra.Command, args []string) error { syncCh, refreshCh, eventRecorder, + startOpts.disabledPlugins, ).Run(stopCh, exitCh) if err != nil { setupLog.Error(err, "failed to run daemon") diff --git a/pkg/daemon/daemon.go b/pkg/daemon/daemon.go index ab0cdb8df..121cb8160 100644 --- a/pkg/daemon/daemon.go +++ b/pkg/daemon/daemon.go @@ -69,6 +69,9 @@ type Daemon struct { nodeState *sriovnetworkv1.SriovNetworkNodeState + // list of disabled plugins + disabledPlugins []string + loadedPlugins map[string]plugin.VendorPlugin HostHelpers helper.HostHelpersInterface @@ -133,6 +136,7 @@ func New( syncCh <-chan struct{}, refreshCh chan<- Message, er *EventRecorder, + disabledPlugins []string, ) *Daemon { return &Daemon{ client: client, @@ -165,7 +169,8 @@ func New( workqueue: workqueue.NewNamedRateLimitingQueue(workqueue.NewMaxOfRateLimiter( &workqueue.BucketRateLimiter{Limiter: rate.NewLimiter(rate.Limit(updateDelay), 1)}, workqueue.NewItemExponentialFailureRateLimiter(1*time.Second, maxUpdateBackoff)), "SriovNetworkNodeState"), - eventRecorder: er, + eventRecorder: er, + disabledPlugins: disabledPlugins, } } @@ -509,7 +514,7 @@ func (dn *Daemon) nodeStateSyncHandler() error { // load plugins if it has not loaded if len(dn.loadedPlugins) == 0 { - dn.loadedPlugins, err = loadPlugins(latestState, dn.HostHelpers) + dn.loadedPlugins, err = loadPlugins(latestState, dn.HostHelpers, dn.disabledPlugins) if err != nil { log.Log.Error(err, "nodeStateSyncHandler(): failed to enable vendor plugins") return err diff --git a/pkg/daemon/daemon_test.go b/pkg/daemon/daemon_test.go index f43a43493..73c6f2a1b 100644 --- a/pkg/daemon/daemon_test.go +++ b/pkg/daemon/daemon_test.go @@ -138,6 +138,7 @@ var _ = Describe("Config Daemon", func() { syncCh, refreshCh, er, + nil, ) sut.loadedPlugins = map[string]plugin.VendorPlugin{generic.PluginName: &fake.FakePlugin{}} diff --git a/pkg/daemon/plugin.go b/pkg/daemon/plugin.go index ebc04c7c5..d0df619b6 100644 --- a/pkg/daemon/plugin.go +++ b/pkg/daemon/plugin.go @@ -30,8 +30,8 @@ var ( K8sPlugin = k8splugin.NewK8sPlugin ) -func loadPlugins(ns *sriovnetworkv1.SriovNetworkNodeState, helpers helper.HostHelpersInterface) (map[string]plugin.VendorPlugin, error) { - log.Log.Info("enableVendorPlugins(): enabling plugins") +func loadPlugins(ns *sriovnetworkv1.SriovNetworkNodeState, helpers helper.HostHelpersInterface, disabledPlugins []string) (map[string]plugin.VendorPlugin, error) { + log.Log.Info("loadPlugins(): loading plugins") loadedPlugins := map[string]plugin.VendorPlugin{} if vars.PlatformType == consts.VirtualOpenStack { @@ -40,9 +40,12 @@ func loadPlugins(ns *sriovnetworkv1.SriovNetworkNodeState, helpers helper.HostHe log.Log.Error(err, "loadPlugins(): failed to load the virtual plugin") return nil, err } - loadedPlugins[virtualPlugin.Name()] = virtualPlugin + pluginName := virtualPlugin.Name() + if !isPluginDisabled(pluginName, disabledPlugins) { + loadedPlugins[pluginName] = virtualPlugin + } } else { - loadedVendorPlugins, err := loadVendorPlugins(ns, helpers) + loadedVendorPlugins, err := loadVendorPlugins(ns, helpers, disabledPlugins) if err != nil { return nil, err } @@ -54,14 +57,21 @@ func loadPlugins(ns *sriovnetworkv1.SriovNetworkNodeState, helpers helper.HostHe log.Log.Error(err, "loadPlugins(): failed to load the k8s plugin") return nil, err } - loadedPlugins[k8sPlugin.Name()] = k8sPlugin + + pluginName := k8sPlugin.Name() + if !isPluginDisabled(pluginName, disabledPlugins) { + loadedPlugins[pluginName] = k8sPlugin + } } genericPlugin, err := GenericPlugin(helpers) if err != nil { log.Log.Error(err, "loadPlugins(): failed to load the generic plugin") return nil, err } - loadedPlugins[genericPlugin.Name()] = genericPlugin + pluginName := genericPlugin.Name() + if !isPluginDisabled(pluginName, disabledPlugins) { + loadedPlugins[pluginName] = genericPlugin + } } pluginList := make([]string, 0, len(loadedPlugins)) @@ -72,7 +82,7 @@ func loadPlugins(ns *sriovnetworkv1.SriovNetworkNodeState, helpers helper.HostHe return loadedPlugins, nil } -func loadVendorPlugins(ns *sriovnetworkv1.SriovNetworkNodeState, helpers helper.HostHelpersInterface) (map[string]plugin.VendorPlugin, error) { +func loadVendorPlugins(ns *sriovnetworkv1.SriovNetworkNodeState, helpers helper.HostHelpersInterface, disabledPlugins []string) (map[string]plugin.VendorPlugin, error) { vendorPlugins := map[string]plugin.VendorPlugin{} for _, iface := range ns.Status.Interfaces { @@ -82,11 +92,25 @@ func loadVendorPlugins(ns *sriovnetworkv1.SriovNetworkNodeState, helpers helper. log.Log.Error(err, "loadVendorPlugins(): failed to load plugin", "plugin-name", plug.Name()) return vendorPlugins, fmt.Errorf("loadVendorPlugins(): failed to load the %s plugin error: %v", plug.Name(), err) } - if _, ok := vendorPlugins[plug.Name()]; !ok { - vendorPlugins[plug.Name()] = plug + + pluginName := plug.Name() + if _, ok := vendorPlugins[pluginName]; !ok { + if !isPluginDisabled(pluginName, disabledPlugins) { + vendorPlugins[plug.Name()] = plug + } } } } return vendorPlugins, nil } + +func isPluginDisabled(pluginName string, disabledPlugins []string) bool { + for _, p := range disabledPlugins { + if p == pluginName { + log.Log.V(2).Info("plugin is disabled", "name", pluginName) + return true + } + } + return false +} diff --git a/pkg/vars/vars.go b/pkg/vars/vars.go index ebc49fa25..555d4389e 100644 --- a/pkg/vars/vars.go +++ b/pkg/vars/vars.go @@ -60,6 +60,9 @@ var ( // Namespace contains k8s namespace Namespace = "" + + // DisableablePlugins contains which plugins can be disabled in sriov config daemon + DisableablePlugins = map[string]struct{}{"mellanox": {}} ) func init() { From 65530fe0507424cc21e7468afb5063a7ed783905 Mon Sep 17 00:00:00 2001 From: adrianc Date: Thu, 28 Dec 2023 17:31:21 +0200 Subject: [PATCH 4/6] Add unit-tests for daemon plugin - Make fake plugin name configurable via attribute - adjust daemon test code - add daemon plugins tests to cover plugin loading logic Signed-off-by: adrianc --- pkg/daemon/daemon_test.go | 2 +- pkg/daemon/plugin_test.go | 136 ++++++++++++++++++++++++++++++++ pkg/plugins/fake/fake_plugin.go | 6 +- 3 files changed, 141 insertions(+), 3 deletions(-) create mode 100644 pkg/daemon/plugin_test.go diff --git a/pkg/daemon/daemon_test.go b/pkg/daemon/daemon_test.go index 73c6f2a1b..fecbae1b4 100644 --- a/pkg/daemon/daemon_test.go +++ b/pkg/daemon/daemon_test.go @@ -141,7 +141,7 @@ var _ = Describe("Config Daemon", func() { nil, ) - sut.loadedPlugins = map[string]plugin.VendorPlugin{generic.PluginName: &fake.FakePlugin{}} + sut.loadedPlugins = map[string]plugin.VendorPlugin{generic.PluginName: &fake.FakePlugin{PluginName: "fake"}} go func() { defer GinkgoRecover() diff --git a/pkg/daemon/plugin_test.go b/pkg/daemon/plugin_test.go new file mode 100644 index 000000000..a13fc1f8b --- /dev/null +++ b/pkg/daemon/plugin_test.go @@ -0,0 +1,136 @@ +package daemon + +import ( + gomock "github.com/golang/mock/gomock" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + v1 "github.com/k8snetworkplumbingwg/sriov-network-operator/api/v1" + "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/consts" + "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/helper" + helperMocks "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/helper/mock" + plugin "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/plugins" + fakePlugin "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/plugins/fake" + "github.com/k8snetworkplumbingwg/sriov-network-operator/pkg/vars" +) + +func validateVendorPlugins(loadedPlugins map[string]plugin.VendorPlugin, expectedPlugins []string) { + var loadedPluginsList []string + for k := range loadedPlugins { + loadedPluginsList = append(loadedPluginsList, k) + } + ExpectWithOffset(-1, loadedPluginsList).To(HaveLen(len(expectedPlugins))) + ExpectWithOffset(-1, loadedPluginsList).To(ContainElements(expectedPlugins)) +} + +var _ = Describe("config daemon plugin loading tests", func() { + Context("loadPlugins", func() { + var gmockController *gomock.Controller + var helperMock *helperMocks.MockHostHelpersInterface + + BeforeEach(func() { + prevK8sPluginFn := K8sPlugin + prevClusterType := vars.ClusterType + prevPlatformType := vars.PlatformType + DeferCleanup(func() { + vars.ClusterType = prevClusterType + vars.PlatformType = prevPlatformType + K8sPlugin = prevK8sPluginFn + }) + + vars.ClusterType = consts.ClusterTypeKubernetes + gmockController = gomock.NewController(GinkgoT()) + helperMock = helperMocks.NewMockHostHelpersInterface(gmockController) + // k8s plugin is ATM the only plugin which require mocking/faking, as its New method performs additional logic + // other than simple plugin struct initialization + K8sPlugin = func(_ helper.HostHelpersInterface) (plugin.VendorPlugin, error) { + return &fakePlugin.FakePlugin{PluginName: "k8s"}, nil + } + }) + + It("loads relevant plugins Baremetal platform, kubernetes cluster type", func() { + ns := &v1.SriovNetworkNodeState{ + Status: v1.SriovNetworkNodeStateStatus{ + Interfaces: v1.InterfaceExts{ + v1.InterfaceExt{Vendor: "15b3"}, + v1.InterfaceExt{Vendor: "8086"}}, + }, + } + vendorPlugins, err := loadPlugins(ns, helperMock, nil) + + Expect(err).ToNot(HaveOccurred()) + validateVendorPlugins(vendorPlugins, []string{"mellanox", "intel", "generic", "k8s"}) + }) + + It("loads relevant plugins Baremetal platform, openshift cluster type", func() { + vars.ClusterType = consts.ClusterTypeOpenshift + ns := &v1.SriovNetworkNodeState{ + Status: v1.SriovNetworkNodeStateStatus{ + Interfaces: v1.InterfaceExts{ + v1.InterfaceExt{Vendor: "15b3"}, + v1.InterfaceExt{Vendor: "8086"}}, + }, + } + vendorPlugins, err := loadPlugins(ns, helperMock, nil) + + Expect(err).ToNot(HaveOccurred()) + validateVendorPlugins(vendorPlugins, []string{"mellanox", "intel", "generic"}) + }) + + It("loads only virtual plugin in VirtualOpenstack platform", func() { + vars.PlatformType = consts.VirtualOpenStack + ns := &v1.SriovNetworkNodeState{ + Status: v1.SriovNetworkNodeStateStatus{ + Interfaces: v1.InterfaceExts{ + v1.InterfaceExt{Vendor: "15b3"}, + v1.InterfaceExt{Vendor: "8086"}}, + }, + } + vendorPlugins, err := loadPlugins(ns, helperMock, nil) + + Expect(err).ToNot(HaveOccurred()) + validateVendorPlugins(vendorPlugins, []string{"virtual"}) + }) + + It("loads vendor plugin according to vendors present", func() { + ns := &v1.SriovNetworkNodeState{ + Status: v1.SriovNetworkNodeStateStatus{ + Interfaces: v1.InterfaceExts{ + v1.InterfaceExt{Vendor: "8086"}}, + }, + } + vendorPlugins, err := loadPlugins(ns, helperMock, nil) + + Expect(err).ToNot(HaveOccurred()) + validateVendorPlugins(vendorPlugins, []string{"intel", "generic", "k8s"}) + }) + + It("does not load disabled vendor plugins", func() { + ns := &v1.SriovNetworkNodeState{ + Status: v1.SriovNetworkNodeStateStatus{ + Interfaces: v1.InterfaceExts{ + v1.InterfaceExt{Vendor: "15b3"}, + v1.InterfaceExt{Vendor: "8086"}}, + }, + } + vendorPlugins, err := loadPlugins(ns, helperMock, []string{"mellanox"}) + + Expect(err).ToNot(HaveOccurred()) + validateVendorPlugins(vendorPlugins, []string{"intel", "generic", "k8s"}) + }) + + It("does not load disabled non vendor plugins", func() { + ns := &v1.SriovNetworkNodeState{ + Status: v1.SriovNetworkNodeStateStatus{ + Interfaces: v1.InterfaceExts{ + v1.InterfaceExt{Vendor: "15b3"}, + v1.InterfaceExt{Vendor: "8086"}}, + }, + } + vendorPlugins, err := loadPlugins(ns, helperMock, []string{"generic"}) + + Expect(err).ToNot(HaveOccurred()) + validateVendorPlugins(vendorPlugins, []string{"intel", "k8s", "mellanox"}) + }) + }) +}) diff --git a/pkg/plugins/fake/fake_plugin.go b/pkg/plugins/fake/fake_plugin.go index ca85768c1..f7fe6e56f 100644 --- a/pkg/plugins/fake/fake_plugin.go +++ b/pkg/plugins/fake/fake_plugin.go @@ -5,10 +5,12 @@ import ( ) // This plugin is used in Daemon unit tests -type FakePlugin struct{} +type FakePlugin struct { + PluginName string +} func (f *FakePlugin) Name() string { - return "fake_plugin" + return f.PluginName } func (f *FakePlugin) Spec() string { From f543deaad88c1ca836288e0d6be427bc511d8baf Mon Sep 17 00:00:00 2001 From: adrianc Date: Thu, 28 Dec 2023 18:09:24 +0200 Subject: [PATCH 5/6] Set disable plugins in daemonset via controller - Extend sriovOperatorConfig CRD with disablePlugins list attribute which contains list of plugins to disable in sriov-network-config-daemon - sriovOperatorConfig controller: Update template data - Set disable-plugin flag in daemonset yaml - Add tests Signed-off-by: adrianc --- api/v1/sriovoperatorconfig_types.go | 18 +++++++++ api/v1/zz_generated.deepcopy.go | 24 ++++++++++++ bindata/manifests/daemon/daemonset.yaml | 3 ++ ...ork.openshift.io_sriovoperatorconfigs.yaml | 9 +++++ controllers/sriovoperatorconfig_controller.go | 6 +++ .../sriovoperatorconfig_controller_test.go | 38 ++++++++++++++++++- ...ork.openshift.io_sriovoperatorconfigs.yaml | 9 +++++ 7 files changed, 105 insertions(+), 2 deletions(-) diff --git a/api/v1/sriovoperatorconfig_types.go b/api/v1/sriovoperatorconfig_types.go index e1875140f..27417ce11 100644 --- a/api/v1/sriovoperatorconfig_types.go +++ b/api/v1/sriovoperatorconfig_types.go @@ -23,6 +23,22 @@ import ( // EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! // NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized. +// PluginNameValue defines the plugin name +// +kubebuilder:validation:Enum=mellanox +type PluginNameValue string + +// PluginNameSlice defines a slice of PluginNameValue +type PluginNameSlice []PluginNameValue + +// ToStringSlice converts PluginNameSlice to string slice +func (pns PluginNameSlice) ToStringSlice() []string { + ss := make([]string, 0, len(pns)) + for _, v := range pns { + ss = append(ss, string(v)) + } + return ss +} + // SriovOperatorConfigSpec defines the desired state of SriovOperatorConfig type SriovOperatorConfigSpec struct { // NodeSelector selects the nodes to be configured @@ -45,6 +61,8 @@ type SriovOperatorConfigSpec struct { ConfigurationMode ConfigurationModeType `json:"configurationMode,omitempty"` // Flag to enable Container Device Interface mode for SR-IOV Network Device Plugin UseCDI bool `json:"useCDI,omitempty"` + // DisablePlugins is a list of sriov-network-config-daemon plugins to disable + DisablePlugins PluginNameSlice `json:"disablePlugins,omitempty"` } // SriovOperatorConfigStatus defines the observed state of SriovOperatorConfig diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index f01d358dd..38b027179 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -143,6 +143,25 @@ func (in *OvsHardwareOffloadConfig) DeepCopy() *OvsHardwareOffloadConfig { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in PluginNameSlice) DeepCopyInto(out *PluginNameSlice) { + { + in := &in + *out = make(PluginNameSlice, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PluginNameSlice. +func (in PluginNameSlice) DeepCopy() PluginNameSlice { + if in == nil { + return nil + } + out := new(PluginNameSlice) + in.DeepCopyInto(out) + return *out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *SriovIBNetwork) DeepCopyInto(out *SriovIBNetwork) { *out = *in @@ -725,6 +744,11 @@ func (in *SriovOperatorConfigSpec) DeepCopyInto(out *SriovOperatorConfigSpec) { *out = new(bool) **out = **in } + if in.DisablePlugins != nil { + in, out := &in.DisablePlugins, &out.DisablePlugins + *out = make(PluginNameSlice, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new SriovOperatorConfigSpec. diff --git a/bindata/manifests/daemon/daemonset.yaml b/bindata/manifests/daemon/daemonset.yaml index 97504edc8..120d6069e 100644 --- a/bindata/manifests/daemon/daemonset.yaml +++ b/bindata/manifests/daemon/daemonset.yaml @@ -99,6 +99,9 @@ spec: {{- if .UsedSystemdMode}} - --use-systemd-service {{- end }} + {{- with index . "DisablePlugins" }} + - --disable-plugins={{.}} + {{- end }} env: - name: NODE_NAME valueFrom: diff --git a/config/crd/bases/sriovnetwork.openshift.io_sriovoperatorconfigs.yaml b/config/crd/bases/sriovnetwork.openshift.io_sriovoperatorconfigs.yaml index 1124d4e20..1f24bd8f5 100644 --- a/config/crd/bases/sriovnetwork.openshift.io_sriovoperatorconfigs.yaml +++ b/config/crd/bases/sriovnetwork.openshift.io_sriovoperatorconfigs.yaml @@ -52,6 +52,15 @@ spec: disableDrain: description: Flag to disable nodes drain during debugging type: boolean + disablePlugins: + description: DisablePlugins is a list of sriov-network-config-daemon + plugins to disable + items: + description: PluginNameValue defines the plugin name + enum: + - mellanox + type: string + type: array enableInjector: description: Flag to control whether the network resource injector webhook shall be deployed diff --git a/controllers/sriovoperatorconfig_controller.go b/controllers/sriovoperatorconfig_controller.go index d75371581..e845ba331 100644 --- a/controllers/sriovoperatorconfig_controller.go +++ b/controllers/sriovoperatorconfig_controller.go @@ -193,6 +193,12 @@ func (r *SriovOperatorConfigReconciler) syncConfigDaemonSet(ctx context.Context, logger.V(1).Info("New cni bin found", "CNIBinPath", envCniBinPath) data.Data["CNIBinPath"] = envCniBinPath } + + if len(dc.Spec.DisablePlugins) > 0 { + logger.V(1).Info("DisablePlugins provided", "DisablePlugins", dc.Spec.DisablePlugins) + data.Data["DisablePlugins"] = strings.Join(dc.Spec.DisablePlugins.ToStringSlice(), ",") + } + objs, err := render.RenderDir(consts.ConfigDaemonPath, &data) if err != nil { logger.Error(err, "Fail to render config daemon manifests") diff --git a/controllers/sriovoperatorconfig_controller_test.go b/controllers/sriovoperatorconfig_controller_test.go index a6aa5602a..05e6607b5 100644 --- a/controllers/sriovoperatorconfig_controller_test.go +++ b/controllers/sriovoperatorconfig_controller_test.go @@ -2,6 +2,7 @@ package controllers import ( goctx "context" + "strings" admv1 "k8s.io/api/admissionregistration/v1" appsv1 "k8s.io/api/apps/v1" @@ -32,11 +33,11 @@ var _ = Describe("Operator", func() { It("should have webhook enable", func() { mutateCfg := &admv1.MutatingWebhookConfiguration{} - err := util.WaitForNamespacedObject(mutateCfg, k8sClient, testNamespace, "sriov-operator-webhook-config", util.RetryInterval, util.APITimeout) + err := util.WaitForNamespacedObject(mutateCfg, k8sClient, testNamespace, "sriov-operator-webhook-config", util.RetryInterval, util.APITimeout*3) Expect(err).NotTo(HaveOccurred()) validateCfg := &admv1.ValidatingWebhookConfiguration{} - err = util.WaitForNamespacedObject(validateCfg, k8sClient, testNamespace, "sriov-operator-webhook-config", util.RetryInterval, util.APITimeout) + err = util.WaitForNamespacedObject(validateCfg, k8sClient, testNamespace, "sriov-operator-webhook-config", util.RetryInterval, util.APITimeout*3) Expect(err).NotTo(HaveOccurred()) }) @@ -173,5 +174,38 @@ var _ = Describe("Operator", func() { }, util.APITimeout, util.RetryInterval).Should(Equal(config.Spec.ConfigDaemonNodeSelector)) }) + It("should not render disable-plugins cmdline flag of sriov-network-config-daemon if disablePlugin not provided in spec", func() { + config := &sriovnetworkv1.SriovOperatorConfig{} + err := util.WaitForNamespacedObject(config, k8sClient, testNamespace, "default", util.RetryInterval, util.APITimeout) + Expect(err).NotTo(HaveOccurred()) + + Eventually(func() string { + daemonSet := &appsv1.DaemonSet{} + err := k8sClient.Get(goctx.TODO(), types.NamespacedName{Name: "sriov-network-config-daemon", Namespace: testNamespace}, daemonSet) + if err != nil { + return "" + } + return strings.Join(daemonSet.Spec.Template.Spec.Containers[0].Args, " ") + }, util.APITimeout*10, util.RetryInterval).Should(And(Not(BeEmpty()), Not(ContainSubstring("disable-plugins")))) + }) + + It("should render disable-plugins cmdline flag of sriov-network-config-daemon if disablePlugin provided in spec", func() { + config := &sriovnetworkv1.SriovOperatorConfig{} + err := util.WaitForNamespacedObject(config, k8sClient, testNamespace, "default", util.RetryInterval, util.APITimeout) + Expect(err).NotTo(HaveOccurred()) + + config.Spec.DisablePlugins = sriovnetworkv1.PluginNameSlice{"mellanox"} + err = k8sClient.Update(goctx.TODO(), config) + Expect(err).NotTo(HaveOccurred()) + + Eventually(func() string { + daemonSet := &appsv1.DaemonSet{} + err := k8sClient.Get(goctx.TODO(), types.NamespacedName{Name: "sriov-network-config-daemon", Namespace: testNamespace}, daemonSet) + if err != nil { + return "" + } + return strings.Join(daemonSet.Spec.Template.Spec.Containers[0].Args, " ") + }, util.APITimeout*10, util.RetryInterval).Should(ContainSubstring("disable-plugins=mellanox")) + }) }) }) diff --git a/deployment/sriov-network-operator/crds/sriovnetwork.openshift.io_sriovoperatorconfigs.yaml b/deployment/sriov-network-operator/crds/sriovnetwork.openshift.io_sriovoperatorconfigs.yaml index 1124d4e20..1f24bd8f5 100644 --- a/deployment/sriov-network-operator/crds/sriovnetwork.openshift.io_sriovoperatorconfigs.yaml +++ b/deployment/sriov-network-operator/crds/sriovnetwork.openshift.io_sriovoperatorconfigs.yaml @@ -52,6 +52,15 @@ spec: disableDrain: description: Flag to disable nodes drain during debugging type: boolean + disablePlugins: + description: DisablePlugins is a list of sriov-network-config-daemon + plugins to disable + items: + description: PluginNameValue defines the plugin name + enum: + - mellanox + type: string + type: array enableInjector: description: Flag to control whether the network resource injector webhook shall be deployed From baa18d017872ec9e42da8205429f2f2e19f57100 Mon Sep 17 00:00:00 2001 From: adrianc Date: Sun, 31 Dec 2023 16:30:52 +0200 Subject: [PATCH 6/6] doc updates update documentation for disable plugin feature Signed-off-by: adrianc --- README.md | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/README.md b/README.md index 3a588a87e..529b9f37c 100644 --- a/README.md +++ b/README.md @@ -228,6 +228,37 @@ This feature was created to support deployments where the user want to use some communication like storage network or out of band managment and the virtual functions must exist on boot and not only after the operator and config-daemon are running. +#### Disabling SR-IOV Config Daemon plugins + +It is possible to disable SR-IOV network operator config daemon plugins in case their operation +is not needed or un-desirable. + +As an example, some plugins perform vendor specific firmware configuration +to enable SR-IOV (e.g `mellanox` plugin). certain deployment environments may prefer to perform such configuration +once during node provisioning, while ensuring the configuration will be compatible with any sriov network node policy +defined for the particular environment. This will reduce or completely eliminate the need for reboot of nodes during SR-IOV +configurations by the operator. + +This can be done by setting SriovOperatorConfig `default` CR `spec.disablePlugins` with the list of desired plugins +to disable. + +**Example**: + +```yaml +apiVersion: sriovnetwork.openshift.io/v1 +kind: SriovOperatorConfig +metadata: + name: default + namespace: sriov-network-operator +spec: + ... + disablePlugins: + - mellanox + ... +``` + +> **NOTE**: Currently only `mellanox` plugin can be disabled. + ## Components and design This operator is split into 2 components: