From fa3e1c629fb6847d5fd11494436cfd922c3cc10c Mon Sep 17 00:00:00 2001 From: Adrian Chiris Date: Sun, 18 Aug 2019 14:58:14 +0300 Subject: [PATCH] SR-IOV: macvtap assigned vf check using sysfs This reverts commits: 9a022e7d7b85b7c21cf26698fe59c818c4577194 d7604169988726d121cdc9727accfeb6e29f4aed As the issue is relevant only for old kernels and almost 4 years have passed since this fix was introduced, it is safe to undo the workaround to use ip link show command for determining whether or not a VF is assigned with macvtap(and followup patch that fixed excessive use of ip link show commans). reverting this commit will benefit the agent by reducing the amount of ip link calls. While it is possible to perform a revert-per-commit, this change should really go in as one commit for clarity and avoid introducing a performance degradation in case one is merged without the other. Conflicts: neutron/plugins/ml2/drivers/mech_sriov/agent/eswitch_manager.py neutron/plugins/ml2/drivers/mech_sriov/agent/pci_lib.py neutron/tests/unit/plugins/ml2/drivers/mech_sriov/agent/test_eswitch_manager.py neutron/tests/unit/plugins/ml2/drivers/mech_sriov/agent/test_pci_lib.py Conflicts details: In eswitch_manager: merge tool was unable to properly revert is_assigned_vf() due to changes in later patches. In pci_lib: conflicts with later changes to is_macvtap_assigned() and link_show() that are no longer required and revert of IPCommandDeviceError definition. In unit tests: conflicts due to newly added tests and reverting the IPCommandDeviceError exception definition. Sorting out mocks as link_show() method was removed Related-bug: #1523083 Change-Id: I04ea8eb63de07a6e1e51c2790c5920b086b8542c (cherry picked from commit 76de8a715d0f48b24fccc06459e9413b96a0449c) --- .../mech_sriov/agent/common/exceptions.py | 6 +- .../mech_sriov/agent/eswitch_manager.py | 30 ++----- .../ml2/drivers/mech_sriov/agent/pci_lib.py | 40 ++------- .../mech_sriov/agent/test_eswitch_manager.py | 87 ++++++++----------- .../drivers/mech_sriov/agent/test_pci_lib.py | 40 ++------- ...3.13-removed-support-8bb00902dd607746.yaml | 8 ++ 6 files changed, 63 insertions(+), 148 deletions(-) create mode 100644 releasenotes/notes/sriov-agent-kernel-3.13-removed-support-8bb00902dd607746.yaml diff --git a/neutron/plugins/ml2/drivers/mech_sriov/agent/common/exceptions.py b/neutron/plugins/ml2/drivers/mech_sriov/agent/common/exceptions.py index 37b9935b64f..8b0bf3bc8ba 100644 --- a/neutron/plugins/ml2/drivers/mech_sriov/agent/common/exceptions.py +++ b/neutron/plugins/ml2/drivers/mech_sriov/agent/common/exceptions.py @@ -27,7 +27,7 @@ class InvalidDeviceError(SriovNicError): class IpCommandError(SriovNicError): - message = _("ip command failed: %(reason)s") + message = _("ip command failed on device %(dev_name)s: %(reason)s") class IpCommandOperationNotSupportedError(SriovNicError): @@ -36,7 +36,3 @@ class IpCommandOperationNotSupportedError(SriovNicError): class InvalidPciSlotError(SriovNicError): message = _("Invalid pci slot %(pci_slot)s") - - -class IpCommandDeviceError(SriovNicError): - message = _("ip command failed on device %(dev_name)s: %(reason)s") 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 8e5a97de13f..42f2a5b3a39 100644 --- a/neutron/plugins/ml2/drivers/mech_sriov/agent/eswitch_manager.py +++ b/neutron/plugins/ml2/drivers/mech_sriov/agent/eswitch_manager.py @@ -13,6 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import glob import os import re @@ -36,6 +37,7 @@ class PciOsWrapper(object): NUMVFS_PATH = "/sys/class/net/%s/device/sriov_numvfs" VIRTFN_FORMAT = r"^virtfn(?P\d+)" VIRTFN_REG_EX = re.compile(VIRTFN_FORMAT) + MAC_VTAP_PREFIX = "upper_macvtap*" @classmethod def scan_vf_devices(cls, dev_name): @@ -67,17 +69,16 @@ class PciOsWrapper(object): return os.path.isdir(cls.DEVICE_PATH % dev_name) @classmethod - def is_assigned_vf(cls, dev_name, vf_index, ip_link_show_output): + def is_assigned_vf(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: VF is assigned if: Direct VF: PCI_PATH does not exist. - Macvtap VF: macvtap@ interface exists in ip link show + Macvtap VF: upper_macvtap path exists. @param dev_name: pf network device name @param vf_index: vf index - @param ip_link_show_output: 'ip link show' output """ if not cls.pf_device_exists(dev_name): @@ -87,21 +88,10 @@ class PciOsWrapper(object): return False path = cls.PCI_PATH % (dev_name, vf_index) - - try: - ifname_list = os.listdir(path) - except OSError: - # PCI_PATH does not exist means that the DIRECT VF assigned + if not os.path.isdir(path): return True - - # Note(moshele) kernel < 3.13 doesn't create symbolic link - # for macvtap interface. Therefore we workaround it - # by parsing ip link show and checking if macvtap interface exists - for ifname in ifname_list: - if pci_lib.PciDeviceIPWrapper.is_macvtap_assigned( - ifname, ip_link_show_output): - return True - return False + upper_macvtap_path = os.path.join(path, "*", cls.MAC_VTAP_PREFIX) + return bool(glob.glob(upper_macvtap_path)) @classmethod def get_numvfs(cls, dev_name): @@ -169,9 +159,8 @@ class EmbSwitch(object): """ vf_to_pci_slot_mapping = {} assigned_devices_info = [] - ls = self.pci_dev_wrapper.link_show() for pci_slot, vf_index in self.pci_slot_map.items(): - if not PciOsWrapper.is_assigned_vf(self.dev_name, vf_index, ls): + 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: @@ -263,8 +252,7 @@ class EmbSwitch(object): vf_index = self.pci_slot_map.get(pci_slot) mac = None if vf_index is not None: - ls = self.pci_dev_wrapper.link_show() - if PciOsWrapper.is_assigned_vf(self.dev_name, vf_index, ls): + if PciOsWrapper.is_assigned_vf(self.dev_name, vf_index): macs = self.pci_dev_wrapper.get_assigned_macs([vf_index]) mac = macs.get(vf_index) return mac diff --git a/neutron/plugins/ml2/drivers/mech_sriov/agent/pci_lib.py b/neutron/plugins/ml2/drivers/mech_sriov/agent/pci_lib.py index f84a4583194..42f875620cf 100644 --- a/neutron/plugins/ml2/drivers/mech_sriov/agent/pci_lib.py +++ b/neutron/plugins/ml2/drivers/mech_sriov/agent/pci_lib.py @@ -39,11 +39,9 @@ class PciDeviceIPWrapper(ip_lib.IPWrapper): MAC_PATTERN = r"MAC\s+(?P[a-fA-F0-9:]+)," STATE_PATTERN = r"\s+link-state\s+(?P\w+)" ANY_PATTERN = ".*," - MACVTAP_PATTERN = r".*macvtap[0-9]+@(?P[a-zA-Z0-9_]+):" VF_LINE_FORMAT = VF_PATTERN + MAC_PATTERN + ANY_PATTERN + STATE_PATTERN VF_DETAILS_REG_EX = re.compile(VF_LINE_FORMAT) - MACVTAP_REG_EX = re.compile(MACVTAP_PATTERN) IP_LINK_OP_NOT_SUPPORTED = 'RTNETLINK answers: Operation not supported' @@ -71,8 +69,8 @@ class PciDeviceIPWrapper(ip_lib.IPWrapper): raise exc.IpCommandOperationNotSupportedError( dev_name=self.dev_name) else: - raise exc.IpCommandDeviceError(dev_name=self.dev_name, - reason=str(e)) + raise exc.IpCommandError(dev_name=self.dev_name, + reason=str(e)) def get_assigned_macs(self, vf_list): """Get assigned mac addresses for vf list. @@ -84,8 +82,8 @@ class PciDeviceIPWrapper(ip_lib.IPWrapper): out = self._as_root([], "link", ("show", self.dev_name)) except Exception as e: LOG.exception("Failed executing ip command") - raise exc.IpCommandDeviceError(dev_name=self.dev_name, - reason=e) + raise exc.IpCommandError(dev_name=self.dev_name, + reason=e) vf_to_mac_mapping = {} vf_lines = self._get_vf_link_show(vf_list, out) if vf_lines: @@ -106,8 +104,8 @@ class PciDeviceIPWrapper(ip_lib.IPWrapper): out = self._as_root([], "link", ("show", self.dev_name)) except Exception as e: LOG.exception("Failed executing ip command") - raise exc.IpCommandDeviceError(dev_name=self.dev_name, - reason=e) + raise exc.IpCommandError(dev_name=self.dev_name, + reason=e) vf_lines = self._get_vf_link_show([vf_index], out) if vf_lines: vf_details = self._parse_vf_link_show(vf_lines[0]) @@ -187,29 +185,3 @@ class PciDeviceIPWrapper(ip_lib.IPWrapper): "for %(device)s", {'line': vf_line, 'device': self.dev_name}) return vf_details - - def link_show(self): - try: - out = self._as_root([], "link", ("show", )) - except Exception as e: - LOG.error("Failed executing ip command: %s", e) - raise exc.IpCommandError(reason=e) - return out - - @classmethod - def is_macvtap_assigned(cls, ifname, ip_link_show_output): - """Check if vf has macvtap interface assigned - - Parses the output of ip link show command and checks - if macvtap[0-9]+@ regex matches the - output. - @param ifname: vf interface name - @param ip_link_show_output: 'ip link show' result to parse - @return: True on match otherwise False - """ - for line in ip_link_show_output.splitlines(): - pattern_match = cls.MACVTAP_REG_EX.match(line) - if pattern_match: - if ifname == pattern_match.group('vf_interface'): - return True - return False 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 91497e07988..703493745f3 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 @@ -130,10 +130,7 @@ class TestESwitchManagerApi(base.BaseTestCase): def test_get_assigned_devices_info(self): with mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent." "eswitch_manager.EmbSwitch.get_assigned_devices_info", - return_value=[(self.ASSIGNED_MAC, self.PCI_SLOT)]),\ - mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent." - "pci_lib.PciDeviceIPWrapper.link_show", - return_value=''): + return_value=[(self.ASSIGNED_MAC, self.PCI_SLOT)]): result = self.eswitch_mgr.get_assigned_devices_info() self.assertIn(self.ASSIGNED_MAC, list(result)[0]) self.assertIn(self.PCI_SLOT, list(result)[0]) @@ -343,9 +340,6 @@ class TestESwitchManagerApi(base.BaseTestCase): mock.patch('neutron.plugins.ml2.drivers.mech_sriov.agent.' 'pci_lib.PciDeviceIPWrapper.get_assigned_macs', return_value=mac_address), \ - mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent." - "pci_lib.PciDeviceIPWrapper.link_show", - return_value=''), \ mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent." "eswitch_manager.PciOsWrapper.pf_device_exists", return_value=True): @@ -457,10 +451,7 @@ class TestEmbSwitch(base.BaseTestCase): return_value={0: self.ASSIGNED_MAC}),\ mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent." "eswitch_manager.PciOsWrapper.is_assigned_vf", - return_value=True), \ - mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent." - "pci_lib.PciDeviceIPWrapper.link_show", - return_value=''): + 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]) @@ -475,10 +466,7 @@ class TestEmbSwitch(base.BaseTestCase): 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),\ - mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent." - "pci_lib.PciDeviceIPWrapper.link_show", - return_value=''): + return_value=True): devices_info = emb_switch.get_assigned_devices_info() for device_info in devices_info: mac = device_info[0] @@ -489,10 +477,7 @@ 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), \ - mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent." - "pci_lib.PciDeviceIPWrapper.link_show", - return_value=''): + return_value=False): result = self.emb_switch.get_assigned_devices_info() self.assertFalse(result) @@ -609,10 +594,7 @@ class TestEmbSwitch(base.BaseTestCase): return_value={0: self.ASSIGNED_MAC}),\ mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent." "eswitch_manager.PciOsWrapper.is_assigned_vf", - return_value=True),\ - mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent." - "pci_lib.PciDeviceIPWrapper.link_show", - return_value=''): + return_value=True): result = self.emb_switch.get_pci_device(self.PCI_SLOT) self.assertEqual(self.ASSIGNED_MAC, result) @@ -687,44 +669,43 @@ class TestPciOsWrapper(base.BaseTestCase): self.assertEqual([], esm.PciOsWrapper.scan_vf_devices(self.DEV_NAME)) - @mock.patch("os.listdir", side_effect=OSError()) - def test_is_assigned_vf_true(self, *args): - with mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent." - "eswitch_manager.PciOsWrapper.pf_device_exists", - return_value=True): - self.assertTrue(esm.PciOsWrapper.is_assigned_vf( - self.DEV_NAME, self.VF_INDEX, '')) + def _mock_assign_vf(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) + self.assertEqual(not dir_exists, result) - @mock.patch("os.path.exists", return_value=True) - @mock.patch("os.listdir", return_value=[DEV_NAME, "eth1"]) - @mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent.pci_lib." - "PciDeviceIPWrapper.is_macvtap_assigned", return_value=False) - def test_is_assigned_vf_false(self, *args): - self.assertFalse(esm.PciOsWrapper.is_assigned_vf( - self.DEV_NAME, self.VF_INDEX, '')) + def test_is_assigned_vf_true(self): + self._mock_assign_vf(True) - @mock.patch("os.path.exists", return_value=True) - @mock.patch("os.listdir", return_value=["eth0", "eth1"]) - @mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent.pci_lib." - "PciDeviceIPWrapper.is_macvtap_assigned", return_value=True) - def test_is_assigned_vf_macvtap(self, mock_is_macvtap_assigned, *args): - esm.PciOsWrapper.is_assigned_vf(self.DEV_NAME, self.VF_INDEX, '') - mock_is_macvtap_assigned.called_with(self.VF_INDEX, "eth0") + def test_is_assigned_vf_false(self): + self._mock_assign_vf(False) - @mock.patch("os.path.exists", return_value=True) - @mock.patch("os.listdir", side_effect=OSError()) - @mock.patch("neutron.plugins.ml2.drivers.mech_sriov.agent.pci_lib." - "PciDeviceIPWrapper.is_macvtap_assigned") - def test_is_assigned_vf_macvtap_failure(self, mock_is_macvtap_assigned, - *args): - esm.PciOsWrapper.is_assigned_vf(self.DEV_NAME, self.VF_INDEX, '') - self.assertFalse(mock_is_macvtap_assigned.called) + 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) + self.assertEqual(macvtap_exists, result) + + def test_is_assigned_vf_macvtap_true(self): + self._mock_assign_vf_macvtap(True) + + 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.DEV_NAME, self.VF_INDEX)) self.assertFalse(list_dir_mock.called) def test_pf_device_exists_with_no_dir(self): diff --git a/neutron/tests/unit/plugins/ml2/drivers/mech_sriov/agent/test_pci_lib.py b/neutron/tests/unit/plugins/ml2/drivers/mech_sriov/agent/test_pci_lib.py index ad958d54822..0cfda31a009 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/mech_sriov/agent/test_pci_lib.py +++ b/neutron/tests/unit/plugins/ml2/drivers/mech_sriov/agent/test_pci_lib.py @@ -38,17 +38,6 @@ class TestPciLib(base.BaseTestCase): ' checking off, link-state enable') VF_LINK_SHOW = '\n'.join((PF_LINK_SHOW, PF_MAC, VF_0_LINK_SHOW, VF_1_LINK_SHOW, VF_2_LINK_SHOW)) - MACVTAP_LINK_SHOW = ('63: macvtap1@enp129s0f1: mtu ' - '1500 qdisc noop state DOWN mode DEFAULT group ' - 'default qlen 500 link/ether 4a:9b:6d:de:65:b5 brd ' - 'ff:ff:ff:ff:ff:ff') - MACVTAP_LINK_SHOW2 = ('64: macvtap2@p1p2_1: mtu ' - '1500 qdisc noop state DOWN mode DEFAULT group ' - 'default qlen 500 link/ether 4a:9b:6d:de:65:b5 brd ' - 'ff:ff:ff:ff:ff:ff') - - IP_LINK_SHOW_WITH_MACVTAP = '\n'.join((VF_LINK_SHOW, MACVTAP_LINK_SHOW)) - IP_LINK_SHOW_WITH_MACVTAP2 = '\n'.join((VF_LINK_SHOW, MACVTAP_LINK_SHOW2)) MAC_MAPPING = { 0: "fa:16:3e:b4:81:ac", @@ -72,7 +61,7 @@ class TestPciLib(base.BaseTestCase): with mock.patch.object(self.pci_wrapper, "_as_root") as mock_as_root: mock_as_root.side_effect = Exception() - self.assertRaises(exc.IpCommandDeviceError, + self.assertRaises(exc.IpCommandError, self.pci_wrapper.get_assigned_macs, [self.VF_INDEX]) @@ -94,7 +83,7 @@ class TestPciLib(base.BaseTestCase): with mock.patch.object(self.pci_wrapper, "_as_root") as mock_as_root: mock_as_root.side_effect = Exception() - self.assertRaises(exc.IpCommandDeviceError, + self.assertRaises(exc.IpCommandError, self.pci_wrapper.get_vf_state, self.VF_INDEX) @@ -108,7 +97,7 @@ class TestPciLib(base.BaseTestCase): with mock.patch.object(self.pci_wrapper, "_as_root") as mock_as_root: mock_as_root.side_effect = Exception() - self.assertRaises(exc.IpCommandDeviceError, + self.assertRaises(exc.IpCommandError, self.pci_wrapper.set_vf_state, self.VF_INDEX, True) @@ -123,7 +112,7 @@ class TestPciLib(base.BaseTestCase): with mock.patch.object(self.pci_wrapper, "_as_root") as mock_as_root: mock_as_root.side_effect = Exception() - self.assertRaises(exc.IpCommandDeviceError, + self.assertRaises(exc.IpCommandError, self.pci_wrapper.set_vf_spoofcheck, self.VF_INDEX, True) @@ -143,7 +132,7 @@ class TestPciLib(base.BaseTestCase): else: with mock.patch.object(self.pci_wrapper, "_as_root", side_effect=Exception()): - self.assertRaises(exc.IpCommandDeviceError, + self.assertRaises(exc.IpCommandError, self.pci_wrapper.set_vf_rate, self.VF_INDEX, rate, @@ -174,22 +163,3 @@ class TestPciLib(base.BaseTestCase): self.pci_wrapper.set_vf_state, self.VF_INDEX, state=True) - - def test_is_macvtap_assigned(self): - self.assertTrue(pci_lib.PciDeviceIPWrapper.is_macvtap_assigned( - 'enp129s0f1', self.IP_LINK_SHOW_WITH_MACVTAP)) - - def test_is_macvtap_assigned_interface_with_underscore(self): - self.assertTrue(pci_lib.PciDeviceIPWrapper.is_macvtap_assigned( - 'p1p2_1', self.IP_LINK_SHOW_WITH_MACVTAP2)) - - def test_is_macvtap_assigned_not_assigned(self): - self.assertFalse(pci_lib.PciDeviceIPWrapper.is_macvtap_assigned( - 'enp129s0f2', self.IP_LINK_SHOW_WITH_MACVTAP)) - - def test_link_show_command_failed(self): - with mock.patch.object(pci_lib.PciDeviceIPWrapper, - "_as_root") as mock_as_root: - mock_as_root.side_effect = Exception() - self.assertRaises(exc.IpCommandError, - self.pci_wrapper.link_show) diff --git a/releasenotes/notes/sriov-agent-kernel-3.13-removed-support-8bb00902dd607746.yaml b/releasenotes/notes/sriov-agent-kernel-3.13-removed-support-8bb00902dd607746.yaml new file mode 100644 index 00000000000..d6be602bf89 --- /dev/null +++ b/releasenotes/notes/sriov-agent-kernel-3.13-removed-support-8bb00902dd607746.yaml @@ -0,0 +1,8 @@ +--- +upgrade: + - | + SR-IOV agent code no longer supports old kernels (<3.13) for MacVtap + ports. This change is not expected to affect existing deployments since + most OS distributions already have the relevant kernel patches. + In addition, latest major release of all Supported distributions already + have a newer kernel.