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). Signed-off-by: Andrea Panattoni <[email protected]>
k8snetworkplumbingwg · Oct 12, 2023 · 03df0d8 · 03df0d8
1 parent 1bf2f23
commit 03df0d8
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 26 deletions.
diff --git 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,22 +100,35 @@ 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)
 		if err := s.nLink.LinkSetName(linkObj, podifName); err != nil {
 			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
@@ -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)