bf3 isolated mode fixes

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"
 )

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 {

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())
+	})
+})

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) {

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{

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 {

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 = ""