From d572508b6828e8e06e8e8a21005b462ec537f9b4 Mon Sep 17 00:00:00 2001 From: Andrea Panattoni Date: Thu, 28 Sep 2023 16:46:39 +0200 Subject: [PATCH] Set MAC address after renaming the interface Setting the MAC address at the end of SetupVF reduces the chances of race conditions with tools that set MAC address asynchronously (i.e. iavf). This commit solve an issue with i40e driver where calling `SetVFEffectiveMAC` after `SetVFHardwareMAC` may produce a VF with no connectivity even if the last configuration step didn't produce any error: ``` + ip link set dev ens1f0 vf 0 mac 20:04:0f:f1:88:A1 # No error + ip link set dev temp_71 address 20:04:0f:f1:88:A1 # Transient error RTNETLINK answers: Resource temporarily unavailable + ip link set dev temp_71 address 20:04:0f:f1:88:A1 # No error ``` Note: this seems to be a kernel driver regression introduced near https://github.com/torvalds/linux/commit/c34743daca0eb1dc855831a5210f0800a850088e Signed-off-by: Andrea Panattoni --- pkg/sriov/sriov.go | 43 +++++++++++++++++++++-------------------- pkg/sriov/sriov_test.go | 11 ++++++----- 2 files changed, 28 insertions(+), 26 deletions(-) diff --git a/pkg/sriov/sriov.go b/pkg/sriov/sriov.go index f34e233e4..d4bbf5e08 100644 --- a/pkg/sriov/sriov.go +++ b/pkg/sriov/sriov.go @@ -67,6 +67,9 @@ func (s *sriovManager) SetupVF(conf *sriovtypes.NetConf, podifName string, netns return fmt.Errorf("error getting VF netdevice with name %s", 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) @@ -87,23 +90,8 @@ func (s *sriovManager) SetupVF(conf *sriovtypes.NetConf, podifName string, netns return fmt.Errorf("error setting temp IF name %s for %s", tempName, linkName) } - // Save the original effective MAC address before overriding it - conf.OrigVfState.EffectiveMAC = linkObj.Attrs().HardwareAddr.String() - // 3. Set MAC address - if conf.MAC != "" { - logging.Debug("3. Set MAC address", - "func", "SetupVF", - "s.nLink", s.nLink, - "tempName", tempName, - "conf.MAC", conf.MAC) - err = utils.SetVFEffectiveMAC(s.nLink, tempName, conf.MAC) - if err != nil { - return fmt.Errorf("failed to set netlink MAC address to %s: %v", conf.MAC, err) - } - } - - // 4. Change netns - logging.Debug("4. Change netns", + // 3. Change netns + logging.Debug("3. Change netns", "func", "SetupVF", "linkObj", linkObj, "netns.Fd()", int(netns.Fd())) @@ -112,8 +100,8 @@ func (s *sriovManager) SetupVF(conf *sriovtypes.NetConf, podifName string, netns } if err := netns.Do(func(_ ns.NetNS) error { - // 5. Set Pod IF name - logging.Debug("5. Set Pod IF name", + // 4. Set Pod IF name + logging.Debug("4. Set Pod IF name", "func", "SetupVF", "linkObj", linkObj, "podifName", podifName) @@ -121,13 +109,26 @@ func (s *sriovManager) SetupVF(conf *sriovtypes.NetConf, podifName string, netns return fmt.Errorf("error setting container interface name %s for %s", linkName, tempName) } - // 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) + // 6. Set MAC address + if conf.MAC != "" { + logging.Debug("6. Set MAC address", + "func", "SetupVF", + "s.nLink", s.nLink, + "podifName", podifName, + "conf.MAC", conf.MAC) + err = utils.SetVFEffectiveMAC(s.nLink, podifName, conf.MAC) + if err != nil { + return fmt.Errorf("failed to set netlink MAC address to %s: %v", conf.MAC, err) + } + } + // 7. Bring IF up in Pod netns logging.Debug("7. Bring IF up in Pod netns", "func", "SetupVF", diff --git a/pkg/sriov/sriov_test.go b/pkg/sriov/sriov_test.go index 58a397e1e..00f705f00 100644 --- a/pkg/sriov/sriov_test.go +++ b/pkg/sriov/sriov_test.go @@ -1,9 +1,10 @@ package sriov import ( - "github.com/k8snetworkplumbingwg/sriov-cni/pkg/utils" "net" + "github.com/k8snetworkplumbingwg/sriov-cni/pkg/utils" + "github.com/containernetworking/plugins/pkg/ns" "github.com/containernetworking/plugins/pkg/testutils" "github.com/k8snetworkplumbingwg/sriov-cni/pkg/sriov/mocks" @@ -100,17 +101,17 @@ var _ = Describe("Sriov", func() { HardwareAddr: fakeMac, }} - tempLink := &utils.FakeLink{LinkAttrs: netlink.LinkAttrs{ + net1Link := &utils.FakeLink{LinkAttrs: netlink.LinkAttrs{ Index: 1000, - Name: "temp_1000", + Name: "net1", HardwareAddr: expMac, }} mocked.On("LinkByName", "enp175s6").Return(fakeLink, nil) - mocked.On("LinkByName", "temp_1000").Return(tempLink, nil) + mocked.On("LinkByName", "net1").Return(net1Link, nil) mocked.On("LinkSetDown", fakeLink).Return(nil) mocked.On("LinkSetName", fakeLink, mock.Anything).Return(nil) - mocked.On("LinkSetHardwareAddr", tempLink, expMac).Return(nil) + mocked.On("LinkSetHardwareAddr", net1Link, expMac).Return(nil) mocked.On("LinkSetNsFd", fakeLink, mock.AnythingOfType("int")).Return(nil) mocked.On("LinkSetUp", fakeLink).Return(nil) mockedPciUtils.On("EnableArpAndNdiscNotify", mock.AnythingOfType("string")).Return(nil)