From b6074d4f5c878b6be00d88ea6a800259eaf177bb Mon Sep 17 00:00:00 2001 From: Danil Lifantev Date: Wed, 29 Jul 2026 14:39:54 +0200 Subject: [PATCH 1/3] mellanox: add skip gate for firmware reset on deselect Default preserves upstream behavior (deselected NICs get firmware VF settings staged to zero). Enabling mellanoxSkipFirmwareResetOnDeselect leaves firmware untouched, for substrates where NIC firmware is managed externally (DPUs). --- pkg/consts/constants.go | 4 ++ pkg/daemon/daemon.go | 1 + pkg/daemon/daemon_test.go | 38 ++++++++++++ pkg/plugins/mellanox/mellanox_plugin.go | 80 +++++++++++++------------ pkg/vars/vars.go | 3 + 5 files changed, 88 insertions(+), 38 deletions(-) diff --git a/pkg/consts/constants.go b/pkg/consts/constants.go index 6aadef648a..8f3ec55e59 100644 --- a/pkg/consts/constants.go +++ b/pkg/consts/constants.go @@ -160,6 +160,10 @@ const ( // MellanoxFirmwareResetFeatureGate: enables the firmware reset via mstfwreset before a reboot MellanoxFirmwareResetFeatureGate = "mellanoxFirmwareReset" + // MellanoxSkipFirmwareResetOnDeselectFeatureGate: when enabled, deselected NICs keep their firmware VF settings + // (for substrates where firmware is managed externally, e.g. DPUs) + MellanoxSkipFirmwareResetOnDeselectFeatureGate = "mellanoxSkipFirmwareResetOnDeselect" + // The path to the file on the host filesystem that contains the IB GUID distribution for IB VFs InfinibandGUIDConfigFilePath = SriovConfBasePath + "/infiniband/guids" ) diff --git a/pkg/daemon/daemon.go b/pkg/daemon/daemon.go index 53fe82b8ba..c8ca03978a 100644 --- a/pkg/daemon/daemon.go +++ b/pkg/daemon/daemon.go @@ -312,6 +312,7 @@ func (dn *Daemon) operatorConfigChangeHandler(old, new interface{}) { } vars.MlxPluginFwReset = dn.featureGate.IsEnabled(consts.MellanoxFirmwareResetFeatureGate) + vars.MlxPluginSkipFwResetOnDeselect = dn.featureGate.IsEnabled(consts.MellanoxSkipFirmwareResetOnDeselectFeatureGate) } func (dn *Daemon) nodeStateSyncHandler() error { diff --git a/pkg/daemon/daemon_test.go b/pkg/daemon/daemon_test.go index 67a56633ff..af53833e10 100644 --- a/pkg/daemon/daemon_test.go +++ b/pkg/daemon/daemon_test.go @@ -335,3 +335,41 @@ func updateSriovNetworkNodeState(c snclient.Interface, nodeState *sriovnetworkv1 Update(context.Background(), nodeState, metav1.UpdateOptions{}) return err } + +var _ = Describe("operatorConfigChangeHandler", func() { + It("updates Mellanox firmware feature gates", func() { + previousNamespace := vars.Namespace + previousReset := vars.MlxPluginFwReset + previousSkipResetOnDeselect := vars.MlxPluginSkipFwResetOnDeselect + DeferCleanup(func() { + vars.Namespace = previousNamespace + vars.MlxPluginFwReset = previousReset + vars.MlxPluginSkipFwResetOnDeselect = previousSkipResetOnDeselect + }) + vars.Namespace = "sriov-network-operator" + + d := &Daemon{featureGate: featuregate.New()} + + config := &sriovnetworkv1.SriovOperatorConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: consts.DefaultConfigName, + Namespace: vars.Namespace, + }, + Spec: sriovnetworkv1.SriovOperatorConfigSpec{ + FeatureGates: map[string]bool{ + consts.MellanoxFirmwareResetFeatureGate: true, + }, + }, + } + + d.operatorConfigChangeHandler(&sriovnetworkv1.SriovOperatorConfig{}, config) + + Expect(vars.MlxPluginFwReset).To(BeTrue()) + Expect(vars.MlxPluginSkipFwResetOnDeselect).To(BeFalse()) + + config.Spec.FeatureGates[consts.MellanoxSkipFirmwareResetOnDeselectFeatureGate] = true + d.operatorConfigChangeHandler(&sriovnetworkv1.SriovOperatorConfig{}, config) + + Expect(vars.MlxPluginSkipFwResetOnDeselect).To(BeTrue()) + }) +}) diff --git a/pkg/plugins/mellanox/mellanox_plugin.go b/pkg/plugins/mellanox/mellanox_plugin.go index 10b0152bb0..3464c56341 100644 --- a/pkg/plugins/mellanox/mellanox_plugin.go +++ b/pkg/plugins/mellanox/mellanox_plugin.go @@ -142,51 +142,55 @@ func (p *MellanoxPlugin) OnNodeStateChange(new *sriovnetworkv1.SriovNetworkNodeS } // Set total VFs to 0 for mellanox interfaces with no spec - for pciPrefix, portsMap := range mellanoxNicsStatus { - if _, ok := processedNics[pciPrefix]; ok { - continue - } + if !vars.MlxPluginSkipFwResetOnDeselect { + for pciPrefix, portsMap := range mellanoxNicsStatus { + if _, ok := processedNics[pciPrefix]; ok { + continue + } - // Add the nic to processed Nics to not repeat the process for dual nic ports - processedNics[pciPrefix] = true - pciAddress := pciPrefix + "0" + // Add the nic to processed Nics to not repeat the process for dual nic ports + processedNics[pciPrefix] = true + pciAddress := pciPrefix + "0" - // Skip devices not configured by the operator - isConfigured, err := p.nicConfiguredByOperator(portsMap) - if err != nil { - return false, false, err - } - if !isConfigured { - log.Log.V(2).Info("None of the ports are configured by the operator skipping firmware reset", - "portMap", portsMap) - continue - } + // Skip devices not configured by the operator + isConfigured, err := p.nicConfiguredByOperator(portsMap) + if err != nil { + return false, false, err + } + if !isConfigured { + log.Log.V(2).Info("None of the ports are configured by the operator skipping firmware reset", + "portMap", portsMap) + continue + } - // Skip externally managed NICs - hasExternally, err := p.nicHasExternallyManagedPFs(portsMap) - if err != nil { - return false, false, err - } - if hasExternally { - log.Log.V(2).Info("One of the ports is configured as externally managed skipping firmware reset", - "portMap", portsMap) - continue - } + // Skip externally managed NICs + hasExternally, err := p.nicHasExternallyManagedPFs(portsMap) + if err != nil { + return false, false, err + } + if hasExternally { + log.Log.V(2).Info("One of the ports is configured as externally managed skipping firmware reset", + "portMap", portsMap) + continue + } - // Skip unsupported devices - if id := sriovnetworkv1.GetVfDeviceID(portsMap[pciAddress].DeviceID); id == "" { - continue - } + // Skip unsupported devices + if id := sriovnetworkv1.GetVfDeviceID(portsMap[pciAddress].DeviceID); id == "" { + continue + } - _, fwNext, err := p.helpers.GetMlxNicFwData(pciAddress) - if err != nil { - return false, false, err - } + _, fwNext, err := p.helpers.GetMlxNicFwData(pciAddress) + if err != nil { + return false, false, err + } - if fwNext.TotalVfs > 0 || fwNext.EnableSriov { - attributesToChange[pciAddress] = mlx.MlxNic{TotalVfs: 0} - log.Log.V(2).Info("Changing TotalVfs to 0, doesn't require rebooting", "fwNext.totalVfs", fwNext.TotalVfs) + if fwNext.TotalVfs > 0 || fwNext.EnableSriov { + attributesToChange[pciAddress] = mlx.MlxNic{TotalVfs: 0} + log.Log.V(2).Info("Changing TotalVfs to 0, doesn't require rebooting", "fwNext.totalVfs", fwNext.TotalVfs) + } } + } else { + log.Log.Info("mellanox plugin OnNodeStateChange(): skipping TotalVfs reset for NICs with no spec") } if needReboot { diff --git a/pkg/vars/vars.go b/pkg/vars/vars.go index fc7108ed80..a378dd57dd 100644 --- a/pkg/vars/vars.go +++ b/pkg/vars/vars.go @@ -57,6 +57,9 @@ var ( // MlxPluginFwReset global variable enables mstfwreset before rebooting a node on VF changes MlxPluginFwReset = false + // MlxPluginSkipFwResetOnDeselect skips clearing firmware VF settings for deselected NICs + MlxPluginSkipFwResetOnDeselect = false + // FilesystemRoot used by test to mock interactions with filesystem FilesystemRoot = "" From 88350b355b8beb4305224d421175352be76df31d Mon Sep 17 00:00:00 2001 From: Danil Lifantev Date: Wed, 29 Jul 2026 14:45:28 +0200 Subject: [PATCH 2/3] mellanox: preserve firmware VF capacity Only increase TotalVfs to satisfy policy and cancel staged reductions. Explicit numVfs=0 policies still disable SR-IOV. --- pkg/vendors/mellanox/mellanox.go | 23 +++++-- pkg/vendors/mellanox/mellanox_test.go | 98 +++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 6 deletions(-) create mode 100644 pkg/vendors/mellanox/mellanox_test.go diff --git a/pkg/vendors/mellanox/mellanox.go b/pkg/vendors/mellanox/mellanox.go index 65106ab0fd..d13777eedc 100644 --- a/pkg/vendors/mellanox/mellanox.go +++ b/pkg/vendors/mellanox/mellanox.go @@ -268,7 +268,7 @@ func IsDualPort(pciAddress string, mellanoxNicsStatus map[string]map[string]srio return len(mellanoxNicsStatus[pciAddressPrefix]) > 1 } -// handleTotalVfs return required total VFs or max (required VFs for dual port NIC) and needReboot if totalVfs will change +// HandleTotalVfs returns the required VF capacity and whether a firmware change is needed. func HandleTotalVfs(fwCurrent, fwNext, attrs *MlxNic, ifaceSpec sriovnetworkv1.Interface, isDualPort bool, mellanoxNicsSpec map[string]sriovnetworkv1.Interface) ( totalVfs int, needReboot, changeWithoutReboot bool) { totalVfs = ifaceSpec.NumVfs @@ -295,17 +295,28 @@ func HandleTotalVfs(fwCurrent, fwNext, attrs *MlxNic, ifaceSpec sriovnetworkv1.I return } - if fwCurrent.TotalVfs != totalVfs { - log.Log.V(2).Info("Changing TotalVfs, needs reboot", "current", fwCurrent.TotalVfs, "requested", totalVfs) + // Preserve spare firmware capacity unless the policy explicitly disables SR-IOV. + if totalVfs == 0 && fwCurrent.TotalVfs != 0 { + log.Log.V(2).Info("Changing TotalVfs to 0 as explicitly requested by the policy, needs reboot", + "current", fwCurrent.TotalVfs) + attrs.TotalVfs = 0 + needReboot = true + } else if fwCurrent.TotalVfs < totalVfs { + log.Log.V(2).Info("Increasing TotalVfs, needs reboot", "current", fwCurrent.TotalVfs, "requested", totalVfs) attrs.TotalVfs = totalVfs needReboot = true } // Remove policy then re-apply it - if !needReboot && fwNext.TotalVfs != totalVfs { - log.Log.V(2).Info("Changing TotalVfs to same as Next Boot value, doesn't require rebooting", + if totalVfs == 0 && !needReboot && fwNext.TotalVfs != 0 { + log.Log.V(2).Info("Restoring staged Next Boot TotalVfs to 0, doesn't require rebooting", + "current", fwCurrent.TotalVfs, "next", fwNext.TotalVfs) + attrs.TotalVfs = 0 + changeWithoutReboot = true + } else if !needReboot && fwNext.TotalVfs < fwCurrent.TotalVfs { + log.Log.V(2).Info("Restoring staged Next Boot TotalVfs to the current firmware value, doesn't require rebooting", "current", fwCurrent.TotalVfs, "next", fwNext.TotalVfs, "requested", totalVfs) - attrs.TotalVfs = totalVfs + attrs.TotalVfs = fwCurrent.TotalVfs changeWithoutReboot = true } diff --git a/pkg/vendors/mellanox/mellanox_test.go b/pkg/vendors/mellanox/mellanox_test.go new file mode 100644 index 0000000000..070715975b --- /dev/null +++ b/pkg/vendors/mellanox/mellanox_test.go @@ -0,0 +1,98 @@ +package mlxutils + +import ( + "testing" + + sriovnetworkv1 "github.com/k8snetworkplumbingwg/sriov-network-operator/api/v1" +) + +func TestHandleTotalVfs(t *testing.T) { + tests := []struct { + name string + current int + next int + requested int + wantAttribute int + wantReboot bool + wantChangeWithoutReboot bool + }{ + { + name: "preserves spare capacity", + current: 8, + next: 8, + requested: 4, + wantAttribute: -1, + }, + { + name: "cancels staged reduction to policy value", + current: 8, + next: 4, + requested: 4, + wantAttribute: 8, + wantChangeWithoutReboot: true, + }, + { + name: "cancels staged reduction below policy value", + current: 8, + next: 0, + requested: 4, + wantAttribute: 8, + wantChangeWithoutReboot: true, + }, + { + name: "increases insufficient capacity", + current: 4, + next: 4, + requested: 8, + wantAttribute: 8, + wantReboot: true, + }, + { + name: "disables sriov explicitly", + current: 8, + next: 8, + requested: 0, + wantAttribute: 0, + wantReboot: true, + }, + { + name: "cancels staged increase when sriov is disabled", + current: 0, + next: 8, + requested: 0, + wantAttribute: 0, + wantChangeWithoutReboot: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + current := &MlxNic{TotalVfs: tt.current} + next := &MlxNic{TotalVfs: tt.next} + attributes := &MlxNic{TotalVfs: -1} + iface := sriovnetworkv1.Interface{NumVfs: tt.requested} + + totalVfs, needReboot, changeWithoutReboot := HandleTotalVfs( + current, + next, + attributes, + iface, + false, + nil, + ) + + if totalVfs != tt.requested { + t.Fatalf("totalVfs = %d, want %d", totalVfs, tt.requested) + } + if attributes.TotalVfs != tt.wantAttribute { + t.Errorf("attributes.TotalVfs = %d, want %d", attributes.TotalVfs, tt.wantAttribute) + } + if needReboot != tt.wantReboot { + t.Errorf("needReboot = %t, want %t", needReboot, tt.wantReboot) + } + if changeWithoutReboot != tt.wantChangeWithoutReboot { + t.Errorf("changeWithoutReboot = %t, want %t", changeWithoutReboot, tt.wantChangeWithoutReboot) + } + }) + } +} From ee7a0ebfb5835ca86413dc78ce24715b9a07cb69 Mon Sep 17 00:00:00 2001 From: Danil Lifantev Date: Wed, 29 Jul 2026 14:49:40 +0200 Subject: [PATCH 3/3] generic: reconcile VF drift on externally managed PFs Detect managed VF changes without treating externally owned PF settings as drift. --- pkg/plugins/generic/generic_plugin.go | 52 +++++++- pkg/plugins/generic/generic_plugin_test.go | 144 +++++++++++++++++++++ 2 files changed, 193 insertions(+), 3 deletions(-) diff --git a/pkg/plugins/generic/generic_plugin.go b/pkg/plugins/generic/generic_plugin.go index 948459a7f0..7ef7735ea3 100644 --- a/pkg/plugins/generic/generic_plugin.go +++ b/pkg/plugins/generic/generic_plugin.go @@ -172,10 +172,17 @@ func (p *GenericPlugin) CheckStatusChanges(current *sriovnetworkv1.SriovNetworkN for _, iface := range current.Spec.Interfaces { found := false for _, ifaceStatus := range current.Status.Interfaces { - // TODO: remove the check for ExternallyManaged - https://github.com/k8snetworkplumbingwg/sriov-network-operator/issues/632 - if iface.PciAddress == ifaceStatus.PciAddress && !iface.ExternallyManaged { + if iface.PciAddress == ifaceStatus.PciAddress { found = true - if sriovnetworkv1.NeedToUpdateSriov(&iface, &ifaceStatus) { + // Compare only VF state that the operator manages. + // TODO: remove the special handling for ExternallyManaged - https://github.com/k8snetworkplumbingwg/sriov-network-operator/issues/632 + if iface.ExternallyManaged { + if externallyManagedNeedToReconcile(&iface, &ifaceStatus) { + log.Log.Info("CheckStatusChanges(): out-of-band change detected for externally managed interface", + "address", iface.PciAddress) + return true, nil + } + } else if sriovnetworkv1.NeedToUpdateSriov(&iface, &ifaceStatus) { log.Log.Info("CheckStatusChanges(): status changed for interface", "address", iface.PciAddress) return true, nil } @@ -203,6 +210,45 @@ func (p *GenericPlugin) CheckStatusChanges(current *sriovnetworkv1.SriovNetworkN return shouldUpdate, nil } +// externallyManagedNeedToReconcile reports VF drift without comparing externally managed PF settings. +func externallyManagedNeedToReconcile(iface *sriovnetworkv1.Interface, ifaceStatus *sriovnetworkv1.InterfaceExt) bool { + // VF creation is handled externally, the daemon cannot repair missing VFs. + // Reconcile anyway: apply fails validation and surfaces a Failed sync status. + if ifaceStatus.NumVfs < iface.NumVfs { + log.Log.V(0).Info("externallyManagedNeedToReconcile(): externally managed VFs missing", + "address", iface.PciAddress, "current", ifaceStatus.NumVfs, "required", iface.NumVfs) + return true + } + + if iface.NumVfs > 0 { + for _, vfStatus := range ifaceStatus.VFs { + for _, groupSpec := range iface.VfGroups { + if !sriovnetworkv1.IndexInRange(vfStatus.VfID, groupSpec.VfRange) { + continue + } + if vfStatus.Driver == "" { + log.Log.V(0).Info("externallyManagedNeedToReconcile(): VF has no driver", + "address", iface.PciAddress, "vf", vfStatus.VfID, "desired", groupSpec.DeviceType) + return true + } + if groupSpec.DeviceType != "" && groupSpec.DeviceType != consts.DeviceTypeNetDevice { + if groupSpec.DeviceType != vfStatus.Driver { + log.Log.V(0).Info("externallyManagedNeedToReconcile(): VF driver mismatch", + "address", iface.PciAddress, "vf", vfStatus.VfID, + "desired", groupSpec.DeviceType, "current", vfStatus.Driver) + return true + } + } else if sriovnetworkv1.StringInArray(vfStatus.Driver, vars.DpdkDrivers) { + log.Log.V(0).Info("externallyManagedNeedToReconcile(): VF is bound to a DPDK driver but a netdevice is requested", + "address", iface.PciAddress, "vf", vfStatus.VfID, "current", vfStatus.Driver) + return true + } + } + } + } + return false +} + func (p *GenericPlugin) syncDriverState() error { for _, driverState := range p.DriverStateMap { if !driverState.DriverLoaded && driverState.NeedDriverFunc(p.DesireState, driverState) { diff --git a/pkg/plugins/generic/generic_plugin_test.go b/pkg/plugins/generic/generic_plugin_test.go index 2e2aed326c..e8faab8410 100644 --- a/pkg/plugins/generic/generic_plugin_test.go +++ b/pkg/plugins/generic/generic_plugin_test.go @@ -861,6 +861,150 @@ var _ = Describe("Generic plugin", func() { Expect(changed).To(BeTrue()) }) + It("should detect VF driver drift on externally managed interface", func() { + networkNodeState := &sriovnetworkv1.SriovNetworkNodeState{ + Spec: sriovnetworkv1.SriovNetworkNodeStateSpec{ + Interfaces: sriovnetworkv1.Interfaces{{ + PciAddress: "0000:00:00.0", + NumVfs: 1, + ExternallyManaged: true, + VfGroups: []sriovnetworkv1.VfGroup{{ + DeviceType: "vfio-pci", + PolicyName: "policy-1", + ResourceName: "resource-1", + VfRange: "0-0", + }}}}, + }, + Status: sriovnetworkv1.SriovNetworkNodeStateStatus{ + Interfaces: sriovnetworkv1.InterfaceExts{{ + PciAddress: "0000:00:00.0", + NumVfs: 1, + TotalVfs: 1, + DeviceID: "1015", + Vendor: "15b3", + Name: "sriovif1", + Mtu: 1500, + Mac: "0c:42:a1:55:ee:46", + Driver: "mlx5_core", + EswitchMode: "legacy", + LinkSpeed: "25000 Mb/s", + LinkType: "ETH", + VFs: []sriovnetworkv1.VirtualFunction{{ + PciAddress: "0000:00:00.1", + DeviceID: "1016", + Vendor: "15b3", + VfID: 0, + Name: "sriovif1v0", + Mtu: 1500, + Mac: "8e:d6:2c:62:87:1b", + Driver: "mlx5_core", + }}, + }}, + }, + } + + changed, err := genericPlugin.CheckStatusChanges(networkNodeState) + Expect(err).ToNot(HaveOccurred()) + Expect(changed).To(BeTrue()) + }) + + It("should detect missing VFs on externally managed interface", func() { + networkNodeState := &sriovnetworkv1.SriovNetworkNodeState{ + Spec: sriovnetworkv1.SriovNetworkNodeStateSpec{ + Interfaces: sriovnetworkv1.Interfaces{{ + PciAddress: "0000:00:00.0", + NumVfs: 4, + ExternallyManaged: true, + VfGroups: []sriovnetworkv1.VfGroup{{ + DeviceType: "vfio-pci", + PolicyName: "policy-1", + ResourceName: "resource-1", + VfRange: "0-3", + }}}}, + }, + Status: sriovnetworkv1.SriovNetworkNodeStateStatus{ + Interfaces: sriovnetworkv1.InterfaceExts{{ + PciAddress: "0000:00:00.0", + NumVfs: 2, + TotalVfs: 8, + DeviceID: "1015", + Vendor: "15b3", + Name: "sriovif1", + Mtu: 1500, + Mac: "0c:42:a1:55:ee:46", + Driver: "mlx5_core", + EswitchMode: "legacy", + LinkSpeed: "25000 Mb/s", + LinkType: "ETH", + VFs: []sriovnetworkv1.VirtualFunction{{ + PciAddress: "0000:00:00.1", + DeviceID: "1016", + Vendor: "15b3", + VfID: 0, + Driver: "mlx5_core", + }}, + }}, + }, + } + + changed, err := genericPlugin.CheckStatusChanges(networkNodeState) + Expect(err).ToNot(HaveOccurred()) + Expect(changed).To(BeTrue()) + }) + + It("should not detect changes on externally managed interface with extra VFs and correct drivers", func() { + networkNodeState := &sriovnetworkv1.SriovNetworkNodeState{ + Spec: sriovnetworkv1.SriovNetworkNodeStateSpec{ + Interfaces: sriovnetworkv1.Interfaces{{ + PciAddress: "0000:00:00.0", + NumVfs: 1, + ExternallyManaged: true, + VfGroups: []sriovnetworkv1.VfGroup{{ + DeviceType: "vfio-pci", + PolicyName: "policy-1", + ResourceName: "resource-1", + VfRange: "0-0", + }}}}, + }, + Status: sriovnetworkv1.SriovNetworkNodeStateStatus{ + Interfaces: sriovnetworkv1.InterfaceExts{{ + PciAddress: "0000:00:00.0", + NumVfs: 2, + TotalVfs: 2, + DeviceID: "1015", + Vendor: "15b3", + Name: "sriovif1", + Mtu: 1500, + Mac: "0c:42:a1:55:ee:46", + Driver: "mlx5_core", + EswitchMode: "legacy", + LinkSpeed: "25000 Mb/s", + LinkType: "ETH", + VFs: []sriovnetworkv1.VirtualFunction{{ + PciAddress: "0000:00:00.1", + DeviceID: "1016", + Vendor: "15b3", + VfID: 0, + Name: "sriovif1v0", + Mtu: 1500, + Mac: "8e:d6:2c:62:87:1b", + Driver: "vfio-pci", + }, { + PciAddress: "0000:00:00.2", + DeviceID: "1016", + Vendor: "15b3", + VfID: 1, + Driver: "mlx5_core", + }}, + }}, + }, + } + + changed, err := genericPlugin.CheckStatusChanges(networkNodeState) + Expect(err).ToNot(HaveOccurred()) + Expect(changed).To(BeFalse()) + }) + Context("Kernel Args", func() { vfioNetworkNodeState := &sriovnetworkv1.SriovNetworkNodeState{