Use effective MAC address for macvtap assigned VFs

This commit modifies the way sriov agent retrieves mac
addresses for macvtap assigned VFs to use the effective
mac taken from the macvtap net device instead of the
administrative mac set on the PF.

This commit rids sriov agent from depending on hypervisor/nova
to set both administrative and effective mac for macvtap ports.

Related-Bug: #1841067

Change-Id: Id499bcc49d27f13f7f03481922a3383b4a255da1

neutron/plugins/ml2/drivers/mech_sriov/agent/eswitch_manager.py

@@ -69,29 +69,46 @@ class PciOsWrapper(object):
         return os.path.isdir(cls.DEVICE_PATH % dev_name)
 
     @classmethod
-    def is_assigned_vf(cls, dev_name, vf_index):
+    def is_assigned_vf_direct(cls, dev_name, vf_index):
         """Check if VF is assigned.
 
         Checks if a given vf index of a given device name is assigned
-        by checking the relevant path in the system:
+        as PCI passthrough by checking the relevant path in the system:
         VF is assigned if:
             Direct VF: PCI_PATH does not exist.
+        @param dev_name: pf network device name
+        @param vf_index: vf index
+        @return: True if VF is assigned, False otherwise
+        """
+        path = cls.PCI_PATH % (dev_name, vf_index)
+        return not os.path.isdir(path)
+
+    @classmethod
+    def get_vf_macvtap_upper_devs(cls, dev_name, vf_index):
+        """Retrieve VF netdev upper (macvtap) devices.
+
+        @param dev_name: pf network device name
+        @param vf_index: vf index
+        @return: list of upper net devices associated with the VF
+        """
+        path = cls.PCI_PATH % (dev_name, vf_index)
+        upper_macvtap_path = os.path.join(path, "*", cls.MAC_VTAP_PREFIX)
+        devs = [os.path.basename(dev) for dev in glob.glob(upper_macvtap_path)]
+        # file name is in the format of upper_<netdev_name> extract netdev name
+        return [dev.split('_')[1] for dev in devs]
+
+    @classmethod
+    def is_assigned_vf_macvtap(cls, dev_name, vf_index):
+        """Check if VF is assigned.
+
+        Checks if a given vf index of a given device name is assigned
+        as macvtap by checking the relevant path in the system:
             Macvtap VF: upper_macvtap path exists.
         @param dev_name: pf network device name
         @param vf_index: vf index
+        @return: True if VF is assigned, False otherwise
         """
-
-        if not cls.pf_device_exists(dev_name):
-            # If the root PCI path does not exist, then the VF cannot
-            # actually have been allocated and there is no way we can
-            # manage it.
-            return False
-
-        path = cls.PCI_PATH % (dev_name, vf_index)
-        if not os.path.isdir(path):
-            return True
-        upper_macvtap_path = os.path.join(path, "*", cls.MAC_VTAP_PREFIX)
-        return bool(glob.glob(upper_macvtap_path))
+        return bool(cls.get_vf_macvtap_upper_devs(dev_name, vf_index))
 
     @classmethod
     def get_numvfs(cls, dev_name):
@@ -157,17 +174,10 @@ class EmbSwitch(object):
 
         @return: list of VF pair (mac address, pci slot)
         """
-        vf_to_pci_slot_mapping = {}
         assigned_devices_info = []
         for pci_slot, vf_index in self.pci_slot_map.items():
-            if not PciOsWrapper.is_assigned_vf(self.dev_name, vf_index):
-                continue
-            vf_to_pci_slot_mapping[vf_index] = pci_slot
-        if vf_to_pci_slot_mapping:
-            vf_to_mac_mapping = self.pci_dev_wrapper.get_assigned_macs(
-                list(vf_to_pci_slot_mapping.keys()))
-            for vf_index, mac in vf_to_mac_mapping.items():
-                pci_slot = vf_to_pci_slot_mapping[vf_index]
+            mac = self.get_pci_device(pci_slot)
+            if mac:
                 assigned_devices_info.append((mac, pci_slot))
         return assigned_devices_info
 
@@ -243,18 +253,45 @@ class EmbSwitch(object):
             raise exc.InvalidPciSlotError(pci_slot=pci_slot)
         return self.pci_dev_wrapper.set_vf_spoofcheck(vf_index, enabled)
 
+    def _get_macvtap_mac(self, vf_index):
+        upperdevs = PciOsWrapper.get_vf_macvtap_upper_devs(
+            self.dev_name, vf_index)
+        # NOTE(adrianc) although there can be many macvtap upper
+        # devices, we expect to have excatly one.
+        if len(upperdevs) > 1:
+            LOG.warning("Found more than one macvtap upper device for PF "
+                        "%(pf)s with VF index %(vf_index)s.",
+                        {"pf": self.dev_name, "vf_index": vf_index})
+        upperdev = upperdevs[0]
+        return pci_lib.PciDeviceIPWrapper(
+            upperdev).device(upperdev).link.address
+
     def get_pci_device(self, pci_slot):
         """Get mac address for given Virtual Function address
 
         @param pci_slot: pci slot
         @return: MAC address of virtual function
         """
+        if not PciOsWrapper.pf_device_exists(self.dev_name):
+            # If the root PCI path does not exist, then the VF cannot
+            # actually have been allocated and there is no way we can
+            # manage it.
+            return None
+
         vf_index = self.pci_slot_map.get(pci_slot)
         mac = None
+
         if vf_index is not None:
-            if PciOsWrapper.is_assigned_vf(self.dev_name, vf_index):
+            # NOTE(adrianc) for VF passthrough take administrative mac from PF
+            # netdevice, for macvtap take mac directly from macvtap interface.
+            # This is done to avoid relying on hypervisor [lack of] logic to
+            # keep effective and administrative mac in sync.
+            if PciOsWrapper.is_assigned_vf_direct(self.dev_name, vf_index):
                 macs = self.pci_dev_wrapper.get_assigned_macs([vf_index])
                 mac = macs.get(vf_index)
+            elif PciOsWrapper.is_assigned_vf_macvtap(
+                    self.dev_name, vf_index):
+                mac = self._get_macvtap_mac(vf_index)
         return mac
 
 

neutron/tests/unit/plugins/ml2/drivers/mech_sriov/agent/test_eswitch_manager.py

@@ -43,9 +43,6 @@ class TestCreateESwitchManager(base.BaseTestCase):
                             dev_name="p6p1", reason="device" " not found")),\
                 mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent."
                            "eswitch_manager.PciOsWrapper.pf_device_exists",
-                           return_value=True), \
-                mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent."
-                           "eswitch_manager.PciOsWrapper.is_assigned_vf",
                            return_value=True):
             eswitch_mgr = esm.ESwitchManager()
             self.addCleanup(self.cleanup)
@@ -60,9 +57,6 @@ class TestCreateESwitchManager(base.BaseTestCase):
                         return_value=self.SCANNED_DEVICES),\
                 mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent."
                            "eswitch_manager.PciOsWrapper.pf_device_exists",
-                           return_value=True), \
-                mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent."
-                           "eswitch_manager.PciOsWrapper.is_assigned_vf",
                            return_value=True):
             eswitch_mgr = esm.ESwitchManager()
             self.addCleanup(self.cleanup)
@@ -99,9 +93,6 @@ class TestESwitchManagerApi(base.BaseTestCase):
                         return_value=self.SCANNED_DEVICES), \
                  mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent."
                             "eswitch_manager.PciOsWrapper.pf_device_exists",
-                            return_value=True), \
-                 mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent."
-                            "eswitch_manager.PciOsWrapper.is_assigned_vf",
                             return_value=True):
             eswitch_mgr.discover_devices(device_mappings, None)
 
@@ -450,8 +441,11 @@ class TestEmbSwitch(base.BaseTestCase):
                         "PciDeviceIPWrapper.get_assigned_macs",
                         return_value={0: self.ASSIGNED_MAC}),\
                 mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent."
-                           "eswitch_manager.PciOsWrapper.is_assigned_vf",
-                           return_value=True):
+                           "eswitch_manager.PciOsWrapper.pf_device_exists",
+                           return_value=True), \
+                mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent."
+                           "eswitch_manager.PciOsWrapper."
+                           "is_assigned_vf_direct", return_value=True):
             result = emb_switch.get_assigned_devices_info()
             self.assertIn(self.ASSIGNED_MAC, list(result)[0])
             self.assertIn(self.PCI_SLOT, list(result)[0])
@@ -465,8 +459,8 @@ class TestEmbSwitch(base.BaseTestCase):
                         "PciDeviceIPWrapper.get_assigned_macs",
                         return_value=self.VF_TO_MAC_MAPPING),\
                 mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent."
-                           "eswitch_manager.PciOsWrapper.is_assigned_vf",
-                           return_value=True):
+                           "eswitch_manager.PciOsWrapper."
+                           "is_assigned_vf_direct", return_value=True):
             devices_info = emb_switch.get_assigned_devices_info()
             for device_info in devices_info:
                 mac = device_info[0]
@@ -476,8 +470,11 @@ class TestEmbSwitch(base.BaseTestCase):
 
     def test_get_assigned_devices_empty(self):
         with mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent."
-                        "eswitch_manager.PciOsWrapper.is_assigned_vf",
-                        return_value=False):
+                        "eswitch_manager.PciOsWrapper.is_assigned_vf_direct",
+                        return_value=False), \
+                mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent."
+                           "eswitch_manager.PciOsWrapper."
+                           "is_assigned_vf_macvtap", return_value=False):
             result = self.emb_switch.get_assigned_devices_info()
             self.assertFalse(result)
 
@@ -593,8 +590,11 @@ class TestEmbSwitch(base.BaseTestCase):
                         "PciDeviceIPWrapper.get_assigned_macs",
                         return_value={0: self.ASSIGNED_MAC}),\
                 mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent."
-                           "eswitch_manager.PciOsWrapper.is_assigned_vf",
-                           return_value=True):
+                           "eswitch_manager.PciOsWrapper."
+                           "is_assigned_vf_direct", return_value=True), \
+                mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent."
+                        "eswitch_manager.PciOsWrapper.pf_device_exists",
+                        return_value=True):
             result = self.emb_switch.get_pci_device(self.PCI_SLOT)
             self.assertEqual(self.ASSIGNED_MAC, result)
 
@@ -603,8 +603,11 @@ class TestEmbSwitch(base.BaseTestCase):
                         "PciDeviceIPWrapper.get_assigned_macs",
                         return_value=[self.ASSIGNED_MAC]),\
                 mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent."
-                           "eswitch_manager.PciOsWrapper.is_assigned_vf",
-                           return_value=True):
+                           "eswitch_manager.PciOsWrapper.pf_device_exists",
+                           return_value=True), \
+                mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent."
+                           "eswitch_manager.PciOsWrapper."
+                           "is_assigned_vf_direct", return_value=True):
             result = self.emb_switch.get_pci_device(self.WRONG_PCI_SLOT)
             self.assertIsNone(result)
 
@@ -613,6 +616,32 @@ class TestEmbSwitch(base.BaseTestCase):
         self.assertEqual([tup[0] for tup in self.SCANNED_DEVICES],
                          sorted(result))
 
+    def _test__get_macvtap_mac(self, upper_devs):
+        ip_wrapper_mock_inst = mock.MagicMock()
+        with mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent.pci_lib."
+                        "PciDeviceIPWrapper",
+                        return_value=ip_wrapper_mock_inst), \
+                mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent."
+                           "eswitch_manager.PciOsWrapper."
+                           "get_vf_macvtap_upper_devs",
+                           return_value=upper_devs), \
+                mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent."
+                           "eswitch_manager.LOG.warning") as log_mock:
+            self.emb_switch._get_macvtap_mac(0)
+            ip_wrapper_mock_inst.device.assert_called_with(upper_devs[0])
+            if len(upper_devs) > 1:
+                self.assertTrue(log_mock.called)
+            else:
+                self.assertFalse(log_mock.called)
+
+    def test__get_macvtap_mac_single_upper_dev(self):
+        upper_devs = ["macvtap0"]
+        self._test__get_macvtap_mac(upper_devs)
+
+    def test__get_macvtap_mac_multiple_upper_devs(self):
+        upper_devs = ["macvtap0", "macvtap1"]
+        self._test__get_macvtap_mac(upper_devs)
+
 
 class TestPciOsWrapper(base.BaseTestCase):
     DEV_NAME = "p7p1"
@@ -669,30 +698,26 @@ class TestPciOsWrapper(base.BaseTestCase):
             self.assertEqual([],
                              esm.PciOsWrapper.scan_vf_devices(self.DEV_NAME))
 
-    def _mock_assign_vf(self, dir_exists):
+    def _mock_assign_vf_direct(self, dir_exists):
         with mock.patch("os.path.isdir",
-                        return_value=dir_exists), \
-                mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent."
-                           "eswitch_manager.PciOsWrapper.pf_device_exists",
-                           return_value=True):
-            result = esm.PciOsWrapper.is_assigned_vf(self.DEV_NAME,
-                                                     self.VF_INDEX)
+                        return_value=dir_exists):
+            result = esm.PciOsWrapper.is_assigned_vf_direct(self.DEV_NAME,
+                                                            self.VF_INDEX)
             self.assertEqual(not dir_exists, result)
 
-    def test_is_assigned_vf_true(self):
-        self._mock_assign_vf(True)
+    def test_is_assigned_vf_direct_true(self):
+        self._mock_assign_vf_direct(True)
 
-    def test_is_assigned_vf_false(self):
-        self._mock_assign_vf(False)
+    def test_is_assigned_vf_direct_false(self):
+        self._mock_assign_vf_direct(False)
 
     def _mock_assign_vf_macvtap(self, macvtap_exists):
         def _glob(file_path):
             return ["upper_macvtap0"] if macvtap_exists else []
 
-        with mock.patch("os.path.isdir", return_value=True),\
-                mock.patch("glob.glob", side_effect=_glob):
-            result = esm.PciOsWrapper.is_assigned_vf(self.DEV_NAME,
-                                                     self.VF_INDEX)
+        with mock.patch("glob.glob", side_effect=_glob):
+            result = esm.PciOsWrapper.is_assigned_vf_macvtap(self.DEV_NAME,
+                                                             self.VF_INDEX)
             self.assertEqual(macvtap_exists, result)
 
     def test_is_assigned_vf_macvtap_true(self):
@@ -701,12 +726,19 @@ class TestPciOsWrapper(base.BaseTestCase):
     def test_is_assigned_vf_macvtap_false(self):
         self._mock_assign_vf_macvtap(False)
 
-    @mock.patch("os.path.exists", return_value=False)
-    @mock.patch("os.listdir", return_value=["eth0", "eth1"])
-    def test_is_assigned_vf_pf_disappeared(self, list_dir_mock, *args):
-        self.assertFalse(esm.PciOsWrapper.is_assigned_vf(
-            self.DEV_NAME, self.VF_INDEX))
-        self.assertFalse(list_dir_mock.called)
+    def _test_get_vf_macvtap_upper_devs(self, upper_devs):
+        with mock.patch("glob.glob", return_value=upper_devs):
+            result = esm.PciOsWrapper.get_vf_macvtap_upper_devs(self.DEV_NAME,
+                                                                self.VF_INDEX)
+            self.assertEqual([dev.split("_")[1] for dev in upper_devs], result)
+
+    def test_get_vf_macvtap_upper_devs(self):
+        upper_devs = ["upper_macvtap0", "upper_macvtap1"]
+        self._test_get_vf_macvtap_upper_devs(upper_devs)
+
+    def test_get_vf_macvtap_upper_devs_no_devs(self):
+        upper_devs = []
+        self._test_get_vf_macvtap_upper_devs(upper_devs)
 
     def test_pf_device_exists_with_no_dir(self):
         with mock.patch("os.path.isdir", return_value=False):

neutron/tests/unit/plugins/ml2/drivers/mech_sriov/agent/test_sriov_nic_agent.py

@@ -75,12 +75,6 @@ class TestSriovAgent(base.BaseTestCase):
         self.agent.scan_devices(set(), set())
         self.assertEqual(2, agent_conf['devices'])
 
-    @mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent.pci_lib."
-                "PciDeviceIPWrapper.get_assigned_macs",
-                return_value=[(DEVICE_MAC, PCI_SLOT)])
-    @mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent."
-                "eswitch_manager.PciOsWrapper.is_assigned_vf",
-                return_value=True)
     def test_treat_devices_removed_with_existed_device(self, *args):
         agent = sriov_nic_agent.SriovNicSwitchAgent({}, {}, 0, {}, {}, {})
         devices = [(DEVICE_MAC, PCI_SLOT)]
@@ -92,12 +86,6 @@ class TestSriovAgent(base.BaseTestCase):
             self.assertFalse(resync)
             self.assertTrue(fn_udd.called)
 
-    @mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent.pci_lib."
-                "PciDeviceIPWrapper.get_assigned_macs",
-                return_value=[(DEVICE_MAC, PCI_SLOT)])
-    @mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent."
-                "eswitch_manager.PciOsWrapper.is_assigned_vf",
-                return_value=True)
     def test_treat_devices_removed_with_not_existed_device(self, *args):
         agent = sriov_nic_agent.SriovNicSwitchAgent({}, {}, 0, {}, {}, {})
         devices = [(DEVICE_MAC, PCI_SLOT)]
@@ -112,12 +100,6 @@ class TestSriovAgent(base.BaseTestCase):
                 self.assertFalse(resync)
                 self.assertTrue(fn_udd.called)
 
-    @mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent.pci_lib."
-                "PciDeviceIPWrapper.get_assigned_macs",
-                return_value=[(DEVICE_MAC, PCI_SLOT)])
-    @mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent."
-                "eswitch_manager.PciOsWrapper.is_assigned_vf",
-                return_value=True)
     def test_treat_devices_removed_failed(self, *args):
         agent = sriov_nic_agent.SriovNicSwitchAgent({}, {}, 0, {}, {}, {})
         devices = [(DEVICE_MAC, PCI_SLOT)]