From 528a9ce0a0e95500ffd4d97db19e9fed5ab2b4f5 Mon Sep 17 00:00:00 2001 From: Adrian Chiris Date: Thu, 15 Aug 2019 17:01:54 +0300 Subject: [PATCH] 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 (cherry picked from commit 6f7b88d0808bee36b9d089ce8d3c0f98ae0b2dea) --- .../mech_sriov/agent/eswitch_manager.py | 85 +++++++++---- .../mech_sriov/agent/test_eswitch_manager.py | 112 +++++++++++------- .../mech_sriov/agent/test_sriov_nic_agent.py | 18 --- 3 files changed, 133 insertions(+), 82 deletions(-) diff --git a/neutron/plugins/ml2/drivers/mech_sriov/agent/eswitch_manager.py b/neutron/plugins/ml2/drivers/mech_sriov/agent/eswitch_manager.py index 42f2a5b3a39..7b242cc4a2a 100644 --- a/neutron/plugins/ml2/drivers/mech_sriov/agent/eswitch_manager.py +++ b/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_ 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 diff --git a/neutron/tests/unit/plugins/ml2/drivers/mech_sriov/agent/test_eswitch_manager.py b/neutron/tests/unit/plugins/ml2/drivers/mech_sriov/agent/test_eswitch_manager.py index 703493745f3..aa8643aa635 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/mech_sriov/agent/test_eswitch_manager.py +++ b/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): diff --git a/neutron/tests/unit/plugins/ml2/drivers/mech_sriov/agent/test_sriov_nic_agent.py b/neutron/tests/unit/plugins/ml2/drivers/mech_sriov/agent/test_sriov_nic_agent.py index 1571ba45254..33de9e617fe 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/mech_sriov/agent/test_sriov_nic_agent.py +++ b/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)]