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
torvalds/linux@c34743d

Signed-off-by: Andrea Panattoni <[email protected]>

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",

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)