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/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{ 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 = "" 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) + } + }) + } +}