From aa5e1a1e7e249d5d1725be4a69410ba62c7a5184 Mon Sep 17 00:00:00 2001 From: Sebastian Sch Date: Thu, 27 Jun 2024 20:23:46 +0300 Subject: [PATCH] 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 --- pkg/helper/mock/mock_helper.go | 14 +++++++++ .../internal/lib/netlink/mock/mock_netlink.go | 15 +++++++++ pkg/host/internal/lib/netlink/netlink.go | 9 +++++- pkg/host/internal/network/network.go | 26 +++++++++++++++- pkg/host/internal/network/network_test.go | 31 +++++++++++++++++++ pkg/host/internal/sriov/sriov.go | 11 +++++-- pkg/host/internal/sriov/sriov_test.go | 26 ++++++++++++---- pkg/host/mock/mock_host.go | 14 +++++++++ pkg/host/types/interfaces.go | 2 ++ 9 files changed, 137 insertions(+), 11 deletions(-) diff --git a/pkg/helper/mock/mock_helper.go b/pkg/helper/mock/mock_helper.go index 2b2ff4e338..34eebe8b8a 100644 --- a/pkg/helper/mock/mock_helper.go +++ b/pkg/helper/mock/mock_helper.go @@ -1127,6 +1127,20 @@ func (mr *MockHostHelpersInterfaceMockRecorder) TryEnableVhostNet() *gomock.Call return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "TryEnableVhostNet", reflect.TypeOf((*MockHostHelpersInterface)(nil).TryEnableVhostNet)) } +// TryGetInterfaceIndex mocks base method. +func (m *MockHostHelpersInterface) TryGetInterfaceIndex(pciAddr string) int { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "TryGetInterfaceIndex", pciAddr) + ret0, _ := ret[0].(int) + return ret0 +} + +// TryGetInterfaceIndex indicates an expected call of TryGetInterfaceIndex. +func (mr *MockHostHelpersInterfaceMockRecorder) TryGetInterfaceIndex(pciAddr interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "TryGetInterfaceIndex", reflect.TypeOf((*MockHostHelpersInterface)(nil).TryGetInterfaceIndex), pciAddr) +} + // TryGetInterfaceName mocks base method. func (m *MockHostHelpersInterface) TryGetInterfaceName(pciAddr string) string { m.ctrl.T.Helper() diff --git a/pkg/host/internal/lib/netlink/mock/mock_netlink.go b/pkg/host/internal/lib/netlink/mock/mock_netlink.go index e3e7d649f0..367f57018a 100644 --- a/pkg/host/internal/lib/netlink/mock/mock_netlink.go +++ b/pkg/host/internal/lib/netlink/mock/mock_netlink.go @@ -159,6 +159,21 @@ func (mr *MockNetlinkLibMockRecorder) IsLinkAdminStateUp(link interface{}) *gomo return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsLinkAdminStateUp", reflect.TypeOf((*MockNetlinkLib)(nil).IsLinkAdminStateUp), link) } +// LinkByIndex mocks base method. +func (m *MockNetlinkLib) LinkByIndex(index int) (netlink.Link, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "LinkByIndex", index) + ret0, _ := ret[0].(netlink.Link) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// LinkByIndex indicates an expected call of LinkByIndex. +func (mr *MockNetlinkLibMockRecorder) LinkByIndex(index interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "LinkByIndex", reflect.TypeOf((*MockNetlinkLib)(nil).LinkByIndex), index) +} + // LinkByName mocks base method. func (m *MockNetlinkLib) LinkByName(name string) (netlink.Link, error) { m.ctrl.T.Helper() diff --git a/pkg/host/internal/lib/netlink/netlink.go b/pkg/host/internal/lib/netlink/netlink.go index 95b620bba0..e53ee85bf9 100644 --- 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 index 5c026f217a..28145677c4 100644 --- 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,33 @@ 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 } +// TryGetInterfaceIndex tries to find the SR-IOV virtual interface index base on pci address +func (n *network) TryGetInterfaceIndex(pciAddr string) int { + ifName := n.TryGetInterfaceName(pciAddr) + if ifName == "" { + return -1 + } + + // read the ifindex file from the interface folder + indexFile := filepath.Join(vars.FilesystemRoot, consts.SysBusPciDevices, pciAddr, "net", ifName, "ifindex") + ifIndex, err := os.ReadFile(indexFile) + if err != nil { + log.Log.Error(err, "TryGetInterfaceIndex(): failed to read ifindex file", "indexFile", indexFile) + return -1 + } + + intIfIndex, err := strconv.Atoi(strings.TrimSpace(string(ifIndex))) + if err != nil { + log.Log.Error(err, "TryGetInterfaceIndex(): failed to parse ifindex file content", "ifIndex", string(ifIndex)) + return -1 + } + return intIfIndex +} + 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/network/network_test.go b/pkg/host/internal/network/network_test.go index ef181ddba6..5f3a20eb86 100644 --- a/pkg/host/internal/network/network_test.go +++ b/pkg/host/internal/network/network_test.go @@ -238,4 +238,35 @@ var _ = Describe("Network", func() { Expect(n.GetNetDevNodeGUID("0000:4b:00.3")).To(Equal("1122:3344:5566:7788")) }) }) + Context("TryGetInterfaceIndex", func() { + It("should return valid index", func() { + helpers.GinkgoConfigureFakeFS(&fakefilesystem.FS{ + Dirs: []string{ + "/sys/bus/pci/devices/0000:4b:00.3/net/eth0/", + "/sys/class/net/eth0/", + }, + Files: map[string][]byte{ + "/sys/bus/pci/devices/0000:4b:00.3/net/eth0/ifindex": []byte("42"), + "/sys/class/net/eth0/phys_switch_id": {}, + }, + }) + dputilsLibMock.EXPECT().GetNetNames("0000:4b:00.3").Return([]string{"eth0"}, nil) + index := n.TryGetInterfaceIndex("0000:4b:00.3") + Expect(index).To(Equal(42)) + }) + It("should return invalid index", func() { + helpers.GinkgoConfigureFakeFS(&fakefilesystem.FS{ + Dirs: []string{ + "/sys/bus/pci/devices/0000:4b:00.3/net/eth0", + "/sys/class/net/eth0/", + }, + Files: map[string][]byte{ + "/sys/class/net/eth0/phys_switch_id": {}, + }, + }) + dputilsLibMock.EXPECT().GetNetNames("0000:4b:00.3").Return([]string{"eth0"}, nil) + index := n.TryGetInterfaceIndex("0000:4b:00.3") + Expect(index).To(Equal(-1)) + }) + }) }) diff --git a/pkg/host/internal/sriov/sriov.go b/pkg/host/internal/sriov/sriov.go index 1b8d99091d..996acf97d4 100644 --- a/pkg/host/internal/sriov/sriov.go +++ b/pkg/host/internal/sriov/sriov.go @@ -189,12 +189,17 @@ 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) + if vfIndex == -1 { + log.Log.Error(nil, "VFIsReady(): invalid index number -1") + return false, nil + } + 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 index 8c4e8ef6ff..c311733d81 100644 --- 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) 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}) + 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,20 @@ 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(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 index e9ccdb9cd2..5a09c1440c 100644 --- a/pkg/host/mock/mock_host.go +++ b/pkg/host/mock/mock_host.go @@ -970,6 +970,20 @@ func (mr *MockHostManagerInterfaceMockRecorder) TryEnableVhostNet() *gomock.Call return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "TryEnableVhostNet", reflect.TypeOf((*MockHostManagerInterface)(nil).TryEnableVhostNet)) } +// TryGetInterfaceIndex mocks base method. +func (m *MockHostManagerInterface) TryGetInterfaceIndex(pciAddr string) int { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "TryGetInterfaceIndex", pciAddr) + ret0, _ := ret[0].(int) + return ret0 +} + +// TryGetInterfaceIndex indicates an expected call of TryGetInterfaceIndex. +func (mr *MockHostManagerInterfaceMockRecorder) TryGetInterfaceIndex(pciAddr interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "TryGetInterfaceIndex", reflect.TypeOf((*MockHostManagerInterface)(nil).TryGetInterfaceIndex), pciAddr) +} + // TryGetInterfaceName mocks base method. func (m *MockHostManagerInterface) TryGetInterfaceName(pciAddr string) string { m.ctrl.T.Helper() diff --git a/pkg/host/types/interfaces.go b/pkg/host/types/interfaces.go index 8867760324..2265b01379 100644 --- 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 + // TryGetInterfaceIndex 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