Use interface index instead of name

It's possible to have a race in the VFIsReady function. vf netdevice can have a default eth0 device name and be the time we call the netlink syscall to get the device information eth0 can be a different device. this cause duplicate mac allocation on vf admin mac address Signed-off-by: Sebastian Sch <sebassch@gmail.com>
k8snetworkplumbingwg · Jun 26, 2024 · 511016d · 511016d
1 parent 43bfa6c
commit 511016d
Show file tree

Hide file tree

Showing 10 changed files with 111 additions and 11 deletions.
diff --git a/pkg/helper/mock/mock_helper.go b/pkg/helper/mock/mock_helper.go
diff --git a/pkg/host/internal/lib/dputils/dputils.go b/pkg/host/internal/lib/dputils/dputils.go
@@ -12,6 +12,8 @@ func New() DPUtilsLib {
 type DPUtilsLib interface {
 	// GetNetNames returns host net interface names as string for a PCI device from its pci address
 	GetNetNames(pciAddr string) ([]string, error)
+	// GetNetIndex returns host net interface index as int for a PCI device from its pci address
+	GetNetIndex(pciAddr string) (int, error)
 	// GetDriverName returns current driver attached to a pci device from its pci address
 	GetDriverName(pciAddr string) (string, error)
 	// GetVFID returns VF ID index (within specific PF) based on PCI address
@@ -37,6 +39,10 @@ func (w *libWrapper) GetNetNames(pciAddr string) ([]string, error) {
 	return dputils.GetNetNames(pciAddr)
 }
 
+func (w *libWrapper) GetNetIndex(pciAddr string) (int, error) {
+	return dputils.GetNetIndex(pciAddr)
+}
+
 // GetDriverName returns current driver attached to a pci device from its pci address
 func (w *libWrapper) GetDriverName(pciAddr string) (string, error) {
 	return dputils.GetDriverName(pciAddr)

diff --git a/pkg/host/internal/lib/dputils/mock/mock_dputils.go b/pkg/host/internal/lib/dputils/mock/mock_dputils.go
diff --git a/pkg/host/internal/lib/netlink/mock/mock_netlink.go b/pkg/host/internal/lib/netlink/mock/mock_netlink.go
diff --git a/pkg/host/internal/lib/netlink/netlink.go b/pkg/host/internal/lib/netlink/netlink.go
@@ -24,6 +24,8 @@ type NetlinkLib interface {
 	LinkSetVfPortGUID(link Link, vf int, portguid net.HardwareAddr) error
 	// LinkByName finds a link by name and returns a pointer to the object.
 	LinkByName(name string) (Link, error)
+	// LinkByName finds a link by index and returns a pointer to the object.
+	LinkByIndex(index int) (Link, error)
 	// LinkSetVfHardwareAddr sets the hardware address of a vf for the link.
 	// Equivalent to: `ip link set $link vf $vf mac $hwaddr`
 	LinkSetVfHardwareAddr(link Link, vf int, hwaddr net.HardwareAddr) error
@@ -79,11 +81,16 @@ func (w *libWrapper) LinkSetVfPortGUID(link Link, vf int, portguid net.HardwareA
 	return netlink.LinkSetVfPortGUID(link, vf, portguid)
 }
 
-// LinkByName finds a link by name and returns a pointer to the object.// LinkByName finds a link by name and returns a pointer to the object.
+// LinkByName finds a link by name and returns a pointer to the object.
 func (w *libWrapper) LinkByName(name string) (Link, error) {
 	return netlink.LinkByName(name)
 }
 
+// LinkByIndex finds a link by index and returns a pointer to the object.
+func (w *libWrapper) LinkByIndex(index int) (Link, error) {
+	return netlink.LinkByIndex(index)
+}
+
 // LinkSetVfHardwareAddr sets the hardware address of a vf for the link.
 // Equivalent to: `ip link set $link vf $vf mac $hwaddr`
 func (w *libWrapper) LinkSetVfHardwareAddr(link Link, vf int, hwaddr net.HardwareAddr) error {

diff --git a/pkg/host/internal/network/network.go b/pkg/host/internal/network/network.go
@@ -75,6 +75,7 @@ func (n *network) TryToGetVirtualInterfaceName(pciAddr string) string {
 func (n *network) TryGetInterfaceName(pciAddr string) string {
 	names, err := n.dputilsLib.GetNetNames(pciAddr)
 	if err != nil || len(names) < 1 {
+		log.Log.Error(err, "TryGetInterfaceName(): failed to get interface name")
 		return ""
 	}
 	netDevName := names[0]
@@ -95,10 +96,20 @@ func (n *network) TryGetInterfaceName(pciAddr string) string {
 		return name
 	}
 
-	log.Log.V(2).Info("tryGetInterfaceName()", "name", netDevName)
+	log.Log.V(2).Info("TryGetInterfaceName()", "name", netDevName)
 	return netDevName
 }
 
+// TryGetInterfaceName tries to find the SR-IOV virtual interface index base on pci address
+func (n *network) TryGetInterfaceIndex(pciAddr string) int {
+	ifIndex, err := n.dputilsLib.GetNetIndex(pciAddr)
+	if err != nil {
+		log.Log.Error(err, "TryGetInterfaceIndex(): failed to get interface index")
+		return -1
+	}
+	return ifIndex
+}
+
 func (n *network) GetPhysSwitchID(name string) (string, error) {
 	swIDFile := filepath.Join(vars.FilesystemRoot, consts.SysClassNet, name, "phys_switch_id")
 	physSwitchID, err := os.ReadFile(swIDFile)

diff --git a/pkg/host/internal/sriov/sriov.go b/pkg/host/internal/sriov/sriov.go
@@ -189,12 +189,13 @@ func (s *sriov) VFIsReady(pciAddr string) (netlink.Link, error) {
 	var err error
 	var vfLink netlink.Link
 	err = wait.PollImmediate(time.Second, 10*time.Second, func() (bool, error) {
-		vfName := s.networkHelper.TryGetInterfaceName(pciAddr)
-		vfLink, err = s.netlinkLib.LinkByName(vfName)
+		vfIndex := s.networkHelper.TryGetInterfaceIndex(pciAddr)
+		vfLink, err = s.netlinkLib.LinkByIndex(vfIndex)
 		if err != nil {
 			log.Log.Error(err, "VFIsReady(): unable to get VF link", "device", pciAddr)
+			return false, nil
 		}
-		return err == nil, nil
+		return true, nil
 	})
 	if err != nil {
 		return vfLink, err

diff --git a/pkg/host/internal/sriov/sriov_test.go b/pkg/host/internal/sriov/sriov_test.go
@@ -235,11 +235,11 @@ var _ = Describe("SRIOV", func() {
 			hostMock.EXPECT().UnbindDriverIfNeeded("0000:d8:00.2", true).Return(nil)
 			hostMock.EXPECT().BindDefaultDriver("0000:d8:00.2").Return(nil)
 			hostMock.EXPECT().SetNetdevMTU("0000:d8:00.2", 2000).Return(nil)
-			hostMock.EXPECT().TryGetInterfaceName("0000:d8:00.2").Return("enp216s0f0_0")
+			hostMock.EXPECT().TryGetInterfaceIndex("0000:d8:00.2").Return(42).Times(1).Times(2)
 			vf0LinkMock := netlinkMockPkg.NewMockLink(testCtrl)
 			vf0Mac, _ := net.ParseMAC("02:42:19:51:2f:af")
-			vf0LinkMock.EXPECT().Attrs().Return(&netlink.LinkAttrs{HardwareAddr: vf0Mac})
-			netlinkLibMock.EXPECT().LinkByName("enp216s0f0_0").Return(vf0LinkMock, nil)
+			vf0LinkMock.EXPECT().Attrs().Return(&netlink.LinkAttrs{Name: "enp216s0f0_0", HardwareAddr: vf0Mac}).AnyTimes()
+			netlinkLibMock.EXPECT().LinkByIndex(42).Return(vf0LinkMock, nil)
 			netlinkLibMock.EXPECT().LinkSetVfHardwareAddr(vf0LinkMock, 0, vf0Mac).Return(nil)
 
 			dputilsLibMock.EXPECT().GetVFID("0000:d8:00.3").Return(1, nil)
@@ -359,11 +359,11 @@ var _ = Describe("SRIOV", func() {
 			hostMock.EXPECT().UnbindDriverIfNeeded("0000:d8:00.2", true).Return(nil)
 			hostMock.EXPECT().BindDefaultDriver("0000:d8:00.2").Return(nil)
 			hostMock.EXPECT().SetNetdevMTU("0000:d8:00.2", 2000).Return(nil)
-			hostMock.EXPECT().TryGetInterfaceName("0000:d8:00.2").Return("enp216s0f0_0")
+			hostMock.EXPECT().TryGetInterfaceIndex("0000:d8:00.2").Return(42).AnyTimes()
 			vf0LinkMock := netlinkMockPkg.NewMockLink(testCtrl)
 			vf0Mac, _ := net.ParseMAC("02:42:19:51:2f:af")
-			vf0LinkMock.EXPECT().Attrs().Return(&netlink.LinkAttrs{HardwareAddr: vf0Mac})
-			netlinkLibMock.EXPECT().LinkByName("enp216s0f0_0").Return(vf0LinkMock, nil)
+			vf0LinkMock.EXPECT().Attrs().Return(&netlink.LinkAttrs{Name: "enp216s0f0_0", HardwareAddr: vf0Mac}).Times(3)
+			netlinkLibMock.EXPECT().LinkByIndex(42).Return(vf0LinkMock, nil).AnyTimes()
 			netlinkLibMock.EXPECT().LinkSetVfHardwareAddr(vf0LinkMock, 0, vf0Mac).Return(nil)
 			hostMock.EXPECT().GetPhysPortName("enp216s0f0np0").Return("p0", nil)
 			hostMock.EXPECT().GetPhysSwitchID("enp216s0f0np0").Return("7cfe90ff2cc0", nil)
@@ -537,6 +537,21 @@ var _ = Describe("SRIOV", func() {
 			helpers.GinkgoAssertFileContentsEquals("/sys/bus/pci/devices/0000:d8:00.0/sriov_numvfs", "2")
 		})
 	})
+
+	Context("VfIsReady", func() {
+		It("Should retry if interface index is -1", func() {
+			hostMock.EXPECT().TryGetInterfaceIndex("0000:d8:00.2").Return(-1).Times(1)
+			hostMock.EXPECT().TryGetInterfaceIndex("0000:d8:00.2").Return(42).Times(1)
+			vf0LinkMock := netlinkMockPkg.NewMockLink(testCtrl)
+			vf0Mac, _ := net.ParseMAC("02:42:19:51:2f:af")
+			vf0LinkMock.EXPECT().Attrs().Return(&netlink.LinkAttrs{Name: "enp216s0f0_0", HardwareAddr: vf0Mac})
+			netlinkLibMock.EXPECT().LinkByIndex(-1).Return(vf0LinkMock, fmt.Errorf("interface not found"))
+			netlinkLibMock.EXPECT().LinkByIndex(42).Return(vf0LinkMock, nil).Times(1)
+			vfLink, err := s.VFIsReady("0000:d8:00.2")
+			Expect(err).ToNot(HaveOccurred())
+			Expect(vfLink.Attrs().HardwareAddr).To(Equal(vf0Mac))
+		})
+	})
 })
 
 func getTestPCIDevices() []*ghw.PCIDevice {

diff --git a/pkg/host/mock/mock_host.go b/pkg/host/mock/mock_host.go
diff --git a/pkg/host/types/interfaces.go b/pkg/host/types/interfaces.go
@@ -81,6 +81,8 @@ type NetworkInterface interface {
 	TryToGetVirtualInterfaceName(pciAddr string) string
 	// TryGetInterfaceName tries to find the SR-IOV virtual interface name base on pci address
 	TryGetInterfaceName(pciAddr string) string
+	// TryGetInterfaceName tries to find the SR-IOV virtual interface index base on pci address
+	TryGetInterfaceIndex(pciAddr string) int
 	// GetPhysSwitchID returns the physical switch ID for a specific pci address
 	GetPhysSwitchID(name string) (string, error)
 	// GetPhysPortName returns the physical port name for a specific pci address