diff --git a/pkg/sriov/sriov.go b/pkg/sriov/sriov.go index 916a80fa..ebcc5717 100644 --- a/pkg/sriov/sriov.go +++ b/pkg/sriov/sriov.go @@ -66,82 +66,100 @@ func NewSriovManager() Manager { // SetupVF sets up a VF in Pod netns func (s *sriovManager) SetupVF(conf *sriovtypes.NetConf, podifName string, netns ns.NetNS) error { linkName := conf.OrigVfState.HostIFName + // Save the original NS in case we need to restore it + // after an error occurs + initns, err := ns.GetCurrentNS() + if err != nil { + return fmt.Errorf("failed to get current NS: %v", err) + } + tempNS, err := ns.TempNetNS() + if err != nil { + return fmt.Errorf("failed to create tempNS: %v", err) + } + + defer func() { + if cerr := tempNS.Close(); cerr != nil { + logging.Warning("failed to close temporary netns; VF may remain accessible in temp namespace", + "func", "SetupVF", + "linkName", linkName, + "error", cerr) + } + }() linkObj, err := s.nLink.LinkByName(linkName) if err != nil { - return fmt.Errorf("error getting VF netdevice with name %s", linkName) + return fmt.Errorf("error: %v. Failed to get VF netdevice with name %s", err, linkName) } // Save the original effective MAC address before overriding it conf.OrigVfState.EffectiveMAC = linkObj.Attrs().HardwareAddr.String() - // tempName used as intermediary name to avoid name conflicts - tempName := fmt.Sprintf("%s%d", "temp_", linkObj.Attrs().Index) - - // 1. Set link down - logging.Debug("1. Set link down", + // 1.Move interface to tempNS + logging.Debug("1. Move the interface to tempNS", "func", "SetupVF", "linkObj", linkObj) - if err := s.nLink.LinkSetDown(linkObj); err != nil { - return fmt.Errorf("failed to down vf device %q: %v", linkName, err) - } - - // 2. Set temp name - logging.Debug("2. Set temp name", - "func", "SetupVF", - "linkObj", linkObj, - "tempName", tempName) - if err := s.nLink.LinkSetName(linkObj, tempName); err != nil { - return fmt.Errorf("error setting temp IF name %s for %s", tempName, linkName) + if err = s.nLink.LinkSetNsFd(linkObj, int(tempNS.Fd())); err != nil { + return fmt.Errorf("failed to move %q to tempNS: %v", linkName, err) } + err = tempNS.Do(func(linkNS ns.NetNS) error { + // lookup the device in tempNS (index might have changed) + tempNSLinkObj, err := s.nLink.LinkByName(linkName) + if err != nil { + return fmt.Errorf("failed to find %q in tempNS: %v", linkName, err) + } + // Rename the interface to pod interface name + if err = s.nLink.LinkSetName(tempNSLinkObj, podifName); err != nil { + return fmt.Errorf("failed to rename host device %q to %q: %v", linkName, podifName, err) + } - // 3. Remove alt name from the nic - logging.Debug("3. Remove interface original name from alt names", - "func", "SetupVF", - "linkObj", linkObj, - "OriginalLinkName", linkName, - "tempName", tempName) - linkObj, err = s.nLink.LinkByName(tempName) - if err != nil { - return fmt.Errorf("error getting VF netdevice with name %s: %v", tempName, err) - } - for _, altName := range linkObj.Attrs().AltNames { - if altName == linkName { - if err := s.nLink.LinkDelAltName(linkObj, linkName); err != nil { - return fmt.Errorf("error removing VF altname %s: %v", linkName, err) + // 3. Remove alt name from the nic + logging.Debug("3. Remove interface original name from alt names", + "func", "SetupVF", + "tempNSObj", tempNSLinkObj, + "OriginalLinkName", linkName) + for _, altName := range tempNSLinkObj.Attrs().AltNames { + if altName == linkName { + if err = s.nLink.LinkDelAltName(tempNSLinkObj, linkName); err != nil { + return fmt.Errorf("error removing VF altname %s: %v", linkName, err) + } } } - } - // 4. Change netns - logging.Debug("4. Change netns", - "func", "SetupVF", - "linkObj", linkObj, - "netns.Fd()", int(netns.Fd())) - if err := s.nLink.LinkSetNsFd(linkObj, int(netns.Fd())); err != nil { - return fmt.Errorf("failed to move IF %s to netns: %q", tempName, err) + // 4. Change netns + logging.Debug("4. Change netns", + "func", "SetupVF", + "tempNSObj", tempNSLinkObj, + "netns.Fd()", int(netns.Fd())) + if err = s.nLink.LinkSetNsFd(tempNSLinkObj, int(netns.Fd())); err != nil { + return fmt.Errorf("failed to move IF %s to netns: %w", podifName, err) + } + return nil + }) + if err != nil { + logging.Error("Move the interface back to initNS because of ", "error", err) + moveAndRenameLinkErr := s.moveAndRenameLink(tempNS, podifName, linkName, initns) + if moveAndRenameLinkErr != nil { + return fmt.Errorf("setupVF failed: %v; rollback failed: %v", err, moveAndRenameLinkErr) + } + return fmt.Errorf("setupVF failed: %v", err) } - if err := netns.Do(func(_ ns.NetNS) error { - // 5. Set Pod IF name - logging.Debug("5. Set Pod IF name", - "func", "SetupVF", - "linkObj", linkObj, - "podifName", podifName) - if err := s.nLink.LinkSetName(linkObj, podifName); err != nil { - return fmt.Errorf("error setting container interface name %s for %s", linkName, tempName) + err = netns.Do(func(_ ns.NetNS) error { + netNSLinkObj, err := s.nLink.LinkByName(podifName) + if err != nil { + return fmt.Errorf("error: %v. Failed to get VF netdevice with name %s", err, podifName) } - // 6. Enable IPv4 ARP notify and IPv6 Network Discovery notify + // 5. Enable IPv4 ARP notify and IPv6 Network Discovery notify // Error is ignored here because enabling this feature is only a performance enhancement. - logging.Debug("6. Enable IPv4 ARP notify and IPv6 Network Discovery notify", + logging.Debug("5. Enable IPv4 ARP notify and IPv6 Network Discovery notify", "func", "SetupVF", "podifName", podifName) _ = s.utils.EnableArpAndNdiscNotify(podifName) - // 7. Set MAC address + // 6. Set MAC address if conf.MAC != "" { - logging.Debug("7. Set MAC address", + logging.Debug("6. Set MAC address", "func", "SetupVF", "s.nLink", s.nLink, "podifName", podifName, @@ -152,20 +170,21 @@ func (s *sriovManager) SetupVF(conf *sriovtypes.NetConf, podifName string, netns } } - logging.Debug("8. Enable Optimistic DAD for IPv6 addresses", "func", "SetupVF", - "linkObj", linkObj) + logging.Debug("7. Enable Optimistic DAD for IPv6 addresses", "func", "SetupVF", + "linkObj", netNSLinkObj) _ = s.utils.EnableOptimisticDad(podifName) - // 9. Bring IF up in Pod netns - logging.Debug("9. Bring IF up in Pod netns", + // 8. Bring IF up in Pod netns + logging.Debug("8. Bring IF up in Pod netns", "func", "SetupVF", - "linkObj", linkObj) - if err := s.nLink.LinkSetUp(linkObj); err != nil { + "linkObj", netNSLinkObj) + if err = s.nLink.LinkSetUp(netNSLinkObj); err != nil { return fmt.Errorf("error bringing interface up in container ns: %q", err) } return nil - }); err != nil { + }) + if err != nil { return fmt.Errorf("error setting up interface in container namespace: %q", err) } @@ -173,10 +192,33 @@ func (s *sriovManager) SetupVF(conf *sriovtypes.NetConf, podifName string, netns // and use it as a pointer vfMTU := linkObj.Attrs().MTU conf.MTU = &vfMTU - return nil } +// moveAndRenameLink restores the original link name and moves it back to initns +func (s *sriovManager) moveAndRenameLink(sourceNM ns.NetNS, podifName, linkName string, initns ns.NetNS) error { + err := sourceNM.Do(func(_ ns.NetNS) error { + // Restore the original link name in case of error in renaming + if nsLinkObj, err := s.nLink.LinkByName(podifName); err == nil { + linkSetNameError := s.nLink.LinkSetName(nsLinkObj, linkName) + if linkSetNameError != nil { + logging.Warning("LinkSetName failed when trying to restore original name", "error", linkSetNameError) + return fmt.Errorf("error bringing interface up in container ns: %q", linkSetNameError) + } + // Try to move interface back to initns + if nsLinkObj, err := s.nLink.LinkByName(linkName); err == nil { + linkSetNsFdError := s.nLink.LinkSetNsFd(nsLinkObj, int(initns.Fd())) + if linkSetNsFdError != nil { + logging.Warning("LinkSetNsFd failed when trying to move back to initns in case of an error", "error", linkSetNsFdError) + return fmt.Errorf("failed to move IF %s back to initns: %w", linkName, linkSetNsFdError) + } + } + } + return nil + }) + return err +} + // ReleaseVF reset a VF from Pod netns and return it to init netns func (s *sriovManager) ReleaseVF(conf *sriovtypes.NetConf, podifName string, netns ns.NetNS) error { initns, err := ns.GetCurrentNS() @@ -273,7 +315,6 @@ func (s *sriovManager) ApplyVFConfig(conf *sriovtypes.NetConf) error { return fmt.Errorf("failed to set vf %d vlan configuration - id %d, qos %d and proto %s: %v", conf.VFID, *conf.Vlan, *conf.VlanQoS, *conf.VlanProto, err) } } - // 2. Set mac address if conf.MAC != "" { // when we restore the original hardware mac address we may get a device or resource busy. so we introduce retry diff --git a/pkg/sriov/sriov_test.go b/pkg/sriov/sriov_test.go index 7f8afccd..d66b224a 100644 --- a/pkg/sriov/sriov_test.go +++ b/pkg/sriov/sriov_test.go @@ -1,6 +1,7 @@ package sriov import ( + "errors" "net" . "github.com/onsi/ginkgo/v2" @@ -46,7 +47,6 @@ var _ = Describe("Sriov", func() { }) It("Assuming existing interface", func() { - var targetNetNS ns.NetNS targetNetNS, err := testutils.NewNS() defer func() { if targetNetNS != nil { @@ -67,7 +67,6 @@ var _ = Describe("Sriov", func() { }} mocked.On("LinkByName", mock.AnythingOfType("string")).Return(fakeLink, nil) - mocked.On("LinkSetDown", fakeLink).Return(nil) mocked.On("LinkSetName", fakeLink, mock.Anything).Return(nil) mocked.On("LinkSetNsFd", fakeLink, mock.AnythingOfType("int")).Return(nil) mocked.On("LinkSetUp", fakeLink).Return(nil) @@ -78,8 +77,8 @@ var _ = Describe("Sriov", func() { Expect(err).NotTo(HaveOccurred()) Expect(netconf.OrigVfState.EffectiveMAC).To(Equal("6e:16:06:0e:b7:e9")) }) + It("Setting VF's MAC address", func() { - var targetNetNS ns.NetNS targetNetNS, err := testutils.NewNS() defer func() { if targetNetNS != nil { @@ -108,30 +107,21 @@ var _ = Describe("Sriov", func() { HardwareAddr: expMac, }} - net2Link := &utils.FakeLink{LinkAttrs: netlink.LinkAttrs{ - Index: 1000, - Name: "temp_1000", - HardwareAddr: expMac, - }} - mocked.On("LinkByName", "enp175s6").Return(fakeLink, nil) - mocked.On("LinkByName", "temp_1000").Return(net2Link, nil) mocked.On("LinkByName", "net1").Return(net1Link, nil) - mocked.On("LinkSetDown", fakeLink).Return(nil) mocked.On("LinkSetName", fakeLink, mock.Anything).Return(nil) - mocked.On("LinkSetName", net2Link, mock.Anything).Return(nil) mocked.On("LinkSetHardwareAddr", net1Link, expMac).Return(nil) - mocked.On("LinkSetNsFd", net2Link, mock.AnythingOfType("int")).Return(nil) - mocked.On("LinkSetUp", net2Link).Return(nil) + mocked.On("LinkSetNsFd", fakeLink, mock.AnythingOfType("int")).Return(nil) mockedPciUtils.On("EnableArpAndNdiscNotify", mock.AnythingOfType("string")).Return(nil) + mocked.On("LinkSetUp", net1Link).Return(nil) mockedPciUtils.On("EnableOptimisticDad", mock.AnythingOfType("string")).Return(nil) sm := sriovManager{nLink: mocked, utils: mockedPciUtils} err = sm.SetupVF(netconf, podifName, targetNetNS) Expect(err).NotTo(HaveOccurred()) mocked.AssertExpectations(t) }) - It("Remove altName", func() { - var targetNetNS ns.NetNS + + It("Setting VF's MAC address failed", func() { targetNetNS, err := testutils.NewNS() defer func() { if targetNetNS != nil { @@ -160,23 +150,55 @@ var _ = Describe("Sriov", func() { HardwareAddr: expMac, }} - net2Link := &utils.FakeLink{LinkAttrs: netlink.LinkAttrs{ + mocked.On("LinkByName", "enp175s6").Return(fakeLink, nil) + mocked.On("LinkSetName", fakeLink, podifName).Return(nil) + mocked.On("LinkByName", "net1").Return(net1Link, nil) + mocked.On("LinkSetHardwareAddr", net1Link, expMac).Return(errors.New("LinkSetHardwareAddr failed")) + mocked.On("LinkSetNsFd", fakeLink, mock.AnythingOfType("int")).Return(nil) + mockedPciUtils.On("EnableArpAndNdiscNotify", mock.AnythingOfType("string")).Return(nil) + mockedPciUtils.On("EnableOptimisticDad", mock.AnythingOfType("string")).Return(nil) + sm := sriovManager{nLink: mocked, utils: mockedPciUtils} + err = sm.SetupVF(netconf, podifName, targetNetNS) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("failed to set netlink MAC address to %s: LinkSetHardwareAddr failed", expMac.String())) + mocked.AssertExpectations(t) + }) + + It("Remove altName", func() { + targetNetNS, err := testutils.NewNS() + defer func() { + if targetNetNS != nil { + targetNetNS.Close() + } + }() + Expect(err).NotTo(HaveOccurred()) + mocked := &mocks_utils.NetlinkManager{} + mockedPciUtils := &mocks.PciUtils{} + fakeMac, err := net.ParseMAC("6e:16:06:0e:b7:e9") + Expect(err).NotTo(HaveOccurred()) + + netconf.MAC = "e4:11:22:33:44:55" + expMac, err := net.ParseMAC(netconf.MAC) + Expect(err).NotTo(HaveOccurred()) + + fakeLink := &utils.FakeLink{LinkAttrs: netlink.LinkAttrs{ Index: 1000, - Name: "temp_1000", + Name: "dummylink", + HardwareAddr: fakeMac, + }} + + net1Link := &utils.FakeLink{LinkAttrs: netlink.LinkAttrs{ + Index: 1000, + Name: "net1", HardwareAddr: expMac, - AltNames: []string{"enp175s6"}, }} mocked.On("LinkByName", "enp175s6").Return(fakeLink, nil) - mocked.On("LinkByName", "temp_1000").Return(net2Link, nil) mocked.On("LinkByName", "net1").Return(net1Link, nil) - mocked.On("LinkSetDown", fakeLink).Return(nil) mocked.On("LinkSetName", fakeLink, mock.Anything).Return(nil) - mocked.On("LinkSetName", net2Link, mock.Anything).Return(nil) - mocked.On("LinkDelAltName", net2Link, "enp175s6").Return(nil) mocked.On("LinkSetHardwareAddr", net1Link, expMac).Return(nil) - mocked.On("LinkSetNsFd", net2Link, mock.AnythingOfType("int")).Return(nil) - mocked.On("LinkSetUp", net2Link).Return(nil) + mocked.On("LinkSetNsFd", fakeLink, mock.AnythingOfType("int")).Return(nil) + mocked.On("LinkSetUp", net1Link).Return(nil) mockedPciUtils.On("EnableArpAndNdiscNotify", mock.AnythingOfType("string")).Return(nil) mockedPciUtils.On("EnableOptimisticDad", mock.AnythingOfType("string")).Return(nil) sm := sriovManager{nLink: mocked, utils: mockedPciUtils} @@ -184,8 +206,8 @@ var _ = Describe("Sriov", func() { Expect(err).NotTo(HaveOccurred()) mocked.AssertExpectations(t) }) + It("Return MTU from PF", func() { - var targetNetNS ns.NetNS targetNetNS, err := testutils.NewNS() defer func() { if targetNetNS != nil { @@ -216,31 +238,133 @@ var _ = Describe("Sriov", func() { MTU: 1500, }} - net2Link := &utils.FakeLink{LinkAttrs: netlink.LinkAttrs{ + mocked.On("LinkByName", "enp175s6").Return(fakeLink, nil) + mocked.On("LinkByName", "net1").Return(net1Link, nil) + mocked.On("LinkSetName", fakeLink, mock.Anything).Return(nil) + mocked.On("LinkSetHardwareAddr", net1Link, expMac).Return(nil) + mocked.On("LinkSetNsFd", fakeLink, mock.AnythingOfType("int")).Return(nil) + mocked.On("LinkSetUp", net1Link).Return(nil) + mockedPciUtils.On("EnableArpAndNdiscNotify", mock.AnythingOfType("string")).Return(nil) + mockedPciUtils.On("EnableOptimisticDad", mock.AnythingOfType("string")).Return(nil) + sm := sriovManager{nLink: mocked, utils: mockedPciUtils} + err = sm.SetupVF(netconf, podifName, targetNetNS) + Expect(err).NotTo(HaveOccurred()) + Expect(*netconf.MTU).To(Equal(1500)) + mocked.AssertExpectations(t) + }) + + It("Bring IF up in Pod netns fails", func() { + targetNetNS, err := testutils.NewNS() + defer func() { + if targetNetNS != nil { + targetNetNS.Close() + } + }() + Expect(err).NotTo(HaveOccurred()) + mocked := &mocks_utils.NetlinkManager{} + mockedPciUtils := &mocks.PciUtils{} + fakeMac, err := net.ParseMAC("6e:16:06:0e:b7:e9") + Expect(err).NotTo(HaveOccurred()) + + netconf.MAC = "e4:11:22:33:44:55" + expMac, err := net.ParseMAC(netconf.MAC) + Expect(err).NotTo(HaveOccurred()) + + fakeLink := &utils.FakeLink{LinkAttrs: netlink.LinkAttrs{ + Index: 1000, + Name: "dummylink", + HardwareAddr: fakeMac, + MTU: 1500, + }} + + net1Link := &utils.FakeLink{LinkAttrs: netlink.LinkAttrs{ Index: 1000, - Name: "temp_1000", + Name: "net1", HardwareAddr: expMac, MTU: 1500, }} mocked.On("LinkByName", "enp175s6").Return(fakeLink, nil) - mocked.On("LinkByName", "temp_1000").Return(net2Link, nil) mocked.On("LinkByName", "net1").Return(net1Link, nil) - mocked.On("LinkSetDown", fakeLink).Return(nil) - mocked.On("LinkSetName", fakeLink, mock.Anything).Return(nil) - mocked.On("LinkSetName", net2Link, mock.Anything).Return(nil) + mocked.On("LinkSetName", fakeLink, podifName).Return(nil) mocked.On("LinkSetHardwareAddr", net1Link, expMac).Return(nil) - mocked.On("LinkSetNsFd", net2Link, mock.AnythingOfType("int")).Return(nil) - mocked.On("LinkSetUp", net2Link).Return(nil) + mocked.On("LinkSetNsFd", fakeLink, mock.AnythingOfType("int")).Return(nil) + mocked.On("LinkSetUp", net1Link).Return(errors.New("failed to set link up")) mockedPciUtils.On("EnableArpAndNdiscNotify", mock.AnythingOfType("string")).Return(nil) mockedPciUtils.On("EnableOptimisticDad", mock.AnythingOfType("string")).Return(nil) sm := sriovManager{nLink: mocked, utils: mockedPciUtils} err = sm.SetupVF(netconf, podifName, targetNetNS) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("error bringing interface up in container ns:")) + Expect(err.Error()).To(ContainSubstring("failed to set link up")) + mocked.AssertExpectations(t) + }) + + It("LinkSetNsFd fails link not moved to targetNetNS", func() { + targetNetNS, err := testutils.NewNS() + defer func() { + if targetNetNS != nil { + targetNetNS.Close() + } + }() Expect(err).NotTo(HaveOccurred()) - Expect(*netconf.MTU).To(Equal(1500)) + + tempNS, err := ns.TempNetNS() + defer func() { + if tempNS != nil { + tempNS.Close() + } + }() + Expect(err).NotTo(HaveOccurred()) + mocked := &mocks_utils.NetlinkManager{} + mockedPciUtils := &mocks.PciUtils{} + fakeMac, err := net.ParseMAC("6e:16:06:0e:b7:e9") + Expect(err).NotTo(HaveOccurred()) + + netconf.MAC = "e4:11:22:33:44:55" + // expMac, err := net.ParseMAC(netconf.MAC) + Expect(err).NotTo(HaveOccurred()) + + initns, err := ns.GetCurrentNS() + Expect(err).NotTo(HaveOccurred()) + + fakeLink := &utils.FakeLink{LinkAttrs: netlink.LinkAttrs{ + Index: 1000, + Name: "dummylink", + HardwareAddr: fakeMac, + Namespace: initns.Fd(), + }} + + mocked.On("LinkByName", "enp175s6").Return(fakeLink, nil) + mocked.On("LinkSetNsFd", fakeLink, mock.AnythingOfType("int")).Return(errors.New("move to netns failed")) + sm := sriovManager{nLink: mocked, utils: mockedPciUtils} + err = sm.SetupVF(netconf, podifName, targetNetNS) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("failed to move \"enp175s6\" to tempNS: move to netns failed")) mocked.AssertExpectations(t) }) + + It("returns error when LinkByName fails", func() { + targetNetNS, err := testutils.NewNS() + defer func() { + if targetNetNS != nil { + targetNetNS.Close() + } + }() + Expect(err).NotTo(HaveOccurred()) + mocked := &mocks_utils.NetlinkManager{} + mockedPciUtils := &mocks.PciUtils{} + + // Simulate LinkByName failure + mocked.On("LinkByName", mock.AnythingOfType("string")).Return(nil, errors.New("device not found")) + + sm := sriovManager{nLink: mocked, utils: mockedPciUtils} + err = sm.SetupVF(netconf, podifName, targetNetNS) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("error: device not found. Failed to get VF netdevice with name")) + }) }) + Context("Checking ApplyVFConfig function", func() { var ( netconf *sriovtypes.NetConf @@ -317,7 +441,6 @@ var _ = Describe("Sriov", func() { } }) It("Assuming existing interface", func() { - var targetNetNS ns.NetNS targetNetNS, err := testutils.NewNS() defer func() { if targetNetNS != nil { @@ -334,8 +457,8 @@ var _ = Describe("Sriov", func() { mocked.On("LinkByName", podifName).Return(fakeLink, nil) mocked.On("LinkSetDown", fakeLink).Return(nil) mocked.On("LinkSetName", fakeLink, netconf.OrigVfState.HostIFName).Return(nil) - mocked.On("LinkSetMTU", fakeLink, 1500).Return(nil) mocked.On("LinkSetNsFd", fakeLink, mock.AnythingOfType("int")).Return(nil) + mocked.On("LinkSetMTU", fakeLink, 1500).Return(nil) sm := sriovManager{nLink: mocked} err = sm.ReleaseVF(netconf, podifName, targetNetNS) Expect(err).NotTo(HaveOccurred()) @@ -361,7 +484,6 @@ var _ = Describe("Sriov", func() { } }) It("Should not restores Effective MAC address when it is not provided in netconf", func() { - var targetNetNS ns.NetNS targetNetNS, err := testutils.NewNS() defer func() { if targetNetNS != nil { @@ -384,7 +506,6 @@ var _ = Describe("Sriov", func() { It("Restores Effective MAC address when provided in netconf", func() { netconf.MAC = "aa:f3:8d:65:1b:d4" - var targetNetNS ns.NetNS targetNetNS, err := testutils.NewNS() defer func() { if targetNetNS != nil {