commit 76f7565b99e637d74878955a0033f35e9eb0e13f Author: Mamduh Date: Tue Aug 18 12:40:10 2020 +0300 Refactor code of linux_net to more cleaner and increase performace The patch adds new functions '_get_phys_port_name' for reading physical port name of the SR-IOV port and '_get_phys_switch_id' for reading physical port switch ID of the SR-IOV port, in addition to refactoring 'get_representor_port' to use the new functions and decrease calls for "_get_pf_func" and netdevs associated with the PF will now be processed in the loop, however it will not be matching 'phys_port_name' which ensures the correct behaviour. In addition to updating the unit test for linux_net and remove not needed mocks Related-Bug: #1892132 Change-Id: I3fdbea4f48cb79ebfd03a4da21e2232ccafb7a76 diff --git a/vif_plug_ovs/linux_net.py b/vif_plug_ovs/linux_net.py index 0d677a6..448b08c 100644 --- a/vif_plug_ovs/linux_net.py +++ b/vif_plug_ovs/linux_net.py @@ -239,46 +239,35 @@ def get_representor_port(pf_ifname, vf_num): VF number in the phys_port_name. That interface is the representor for the requested VF. """ - pf_path = "/sys/class/net/%s" % pf_ifname - pf_sw_id_file = os.path.join(pf_path, "phys_switch_id") pf_sw_id = None try: - with open(pf_sw_id_file, 'r') as fd: - pf_sw_id = fd.readline().rstrip() + pf_sw_id = _get_phys_switch_id(pf_ifname) except (OSError, IOError): raise exception.RepresentorNotFound(ifname=pf_ifname, vf_num=vf_num) - pf_subsystem_file = os.path.join(pf_path, "subsystem") + pf_subsystem_file = "/sys/class/net/%s/subsystem" % pf_ifname try: devices = os.listdir(pf_subsystem_file) except (OSError, IOError): raise exception.RepresentorNotFound(ifname=pf_ifname, vf_num=vf_num) - for device in devices: - address_str, pf = get_function_by_ifname(device) - if pf: - continue + ifname_pf_func = _get_pf_func(pf_ifname) + if ifname_pf_func is None: + raise exception.RepresentorNotFound(ifname=pf_ifname, vf_num=vf_num) - device_path = "/sys/class/net/%s" % device - device_sw_id_file = os.path.join(device_path, "phys_switch_id") + for device in devices: try: - with open(device_sw_id_file, 'r') as fd: - device_sw_id = fd.readline().rstrip() + device_sw_id = _get_phys_switch_id(device) + if not device_sw_id or device_sw_id != pf_sw_id: + continue except (OSError, IOError): continue - if device_sw_id != pf_sw_id: - continue - device_port_name_file = ( - os.path.join(device_path, 'phys_port_name')) - - if not os.path.isfile(device_port_name_file): - continue - try: - with open(device_port_name_file, 'r') as fd: - phys_port_name = fd.readline().rstrip() + phys_port_name = _get_phys_port_name(device) + if phys_port_name is None: + continue except (OSError, IOError): continue @@ -287,9 +276,6 @@ def get_representor_port(pf_ifname, vf_num): # the PCI func number of pf_ifname. rep_parent_pf_func = _parse_pf_number(phys_port_name) if rep_parent_pf_func is not None: - ifname_pf_func = _get_pf_func(pf_ifname) - if ifname_pf_func is None: - continue if int(rep_parent_pf_func) != int(ifname_pf_func): continue @@ -321,9 +307,7 @@ def _get_sysfs_netdev_path(pci_addr, pf_interface): def _is_switchdev(netdev): """Returns True if a netdev has a readable phys_switch_id""" try: - sw_id_file = "/sys/class/net/%s/phys_switch_id" % netdev - with open(sw_id_file, 'r') as fd: - phys_switch_id = fd.readline().rstrip() + phys_switch_id = _get_phys_switch_id(netdev) if phys_switch_id != "" and phys_switch_id is not None: return True except (OSError, IOError): @@ -389,3 +373,33 @@ def get_pf_pci_from_vf(vf_pci): """ physfn_path = os.readlink("/sys/bus/pci/devices/%s/physfn" % vf_pci) return os.path.basename(physfn_path) + + +def _get_phys_port_name(ifname): + """Get the interface name and return its phys_port_name + + :param ifname: The interface name + :return: The phys_port_name of the given ifname + """ + phys_port_name_path = "/sys/class/net/%s/phys_port_name" % ifname + + if not os.path.isfile(phys_port_name_path): + return None + + with open(phys_port_name_path, 'r') as fd: + return fd.readline().strip() + + +def _get_phys_switch_id(ifname): + """Get the interface name and return its phys_switch_id + + :param ifname: The interface name + :return: The phys_switch_id of the given ifname + """ + phys_port_name_path = "/sys/class/net/%s/phys_switch_id" % ifname + + if not os.path.isfile(phys_port_name_path): + return None + + with open(phys_port_name_path, 'r') as fd: + return fd.readline().strip() diff --git a/vif_plug_ovs/tests/unit/test_linux_net.py b/vif_plug_ovs/tests/unit/test_linux_net.py index 659b1a2..63ee393 100644 --- a/vif_plug_ovs/tests/unit/test_linux_net.py +++ b/vif_plug_ovs/tests/unit/test_linux_net.py @@ -133,47 +133,22 @@ class LinuxNetTest(testtools.TestCase): linux_net.add_bridge_port("br0", "vnet1") mock_set.assert_called_once_with("vnet1", master="br0") - @mock.patch('builtins.open') - @mock.patch.object(os.path, 'isfile') - def test_is_switchdev_ioerror(self, mock_isfile, mock_open): - mock_isfile.side_effect = [True] - mock_open.return_value.__enter__ = lambda s: s - readline_mock = mock_open.return_value.readline - readline_mock.side_effect = ( - [IOError()]) + @mock.patch.object(linux_net, '_get_phys_switch_id') + def test_is_switchdev_ioerror(self, mock__get_phys_switch_id): + mock__get_phys_switch_id.side_effect = ([IOError()]) test_switchdev = linux_net._is_switchdev('pf_ifname') self.assertEqual(test_switchdev, False) - @mock.patch('builtins.open') - @mock.patch.object(os.path, 'isfile') - def test_is_switchdev_empty(self, mock_isfile, mock_open): - mock_isfile.side_effect = [True] - mock_open.return_value.__enter__ = lambda s: s - readline_mock = mock_open.return_value.readline - readline_mock.side_effect = ( - ['']) - open_calls = ( - [mock.call('/sys/class/net/pf_ifname/phys_switch_id', 'r'), - mock.call().readline(), - mock.call().__exit__(None, None, None)]) + @mock.patch.object(linux_net, '_get_phys_switch_id') + def test_is_switchdev_empty(self, mock__get_phys_switch_id): + mock__get_phys_switch_id.return_value = '' test_switchdev = linux_net._is_switchdev('pf_ifname') - mock_open.assert_has_calls(open_calls) self.assertEqual(test_switchdev, False) - @mock.patch('builtins.open') - @mock.patch.object(os.path, 'isfile') - def test_is_switchdev_positive(self, mock_isfile, mock_open): - mock_isfile.side_effect = [True] - mock_open.return_value.__enter__ = lambda s: s - readline_mock = mock_open.return_value.readline - readline_mock.side_effect = ( - ['pf_sw_id']) - open_calls = ( - [mock.call('/sys/class/net/pf_ifname/phys_switch_id', 'r'), - mock.call().readline(), - mock.call().__exit__(None, None, None)]) + @mock.patch.object(linux_net, '_get_phys_switch_id') + def test_is_switchdev_positive(self, mock__get_phys_switch_id): + mock__get_phys_switch_id.return_value = 'pf_sw_id' test_switchdev = linux_net._is_switchdev('pf_ifname') - mock_open.assert_has_calls(open_calls) self.assertEqual(test_switchdev, True) def test_parse_vf_number(self): @@ -192,184 +167,115 @@ class LinuxNetTest(testtools.TestCase): self.assertEqual(linux_net._parse_pf_number("pf31"), "31") self.assertIsNone(linux_net._parse_pf_number("g4rbl3d")) - @mock.patch('builtins.open') - @mock.patch.object(os.path, 'isfile') @mock.patch.object(os, 'listdir') - @mock.patch.object(linux_net, "get_function_by_ifname") - def test_get_representor_port(self, mock_get_function_by_ifname, - mock_listdir, mock_isfile, mock_open): + @mock.patch.object(linux_net, "_get_pf_func") + @mock.patch.object(linux_net, "_get_phys_port_name") + @mock.patch.object(linux_net, '_get_phys_switch_id') + def test_get_representor_port(self, mock__get_phys_switch_id, + mock__get_phys_port_name, + mock__get_pf_func, + mock_listdir): mock_listdir.return_value = [ 'pf_ifname', 'rep_vf_1', 'rep_vf_2' ] - mock_isfile.side_effect = [True, True] - mock_open.return_value.__enter__ = lambda s: s - readline_mock = mock_open.return_value.readline - readline_mock.side_effect = ( - ['pf_sw_id', 'pf_sw_id', '1', 'pf_sw_id', 'pf0vf2']) - # PCI IDs mocked: - # PF0: 0000:0a:00.0 - # PF0VF1: 0000:0a:02.1 PF0VF2: 0000:0a:02.2 - mock_get_function_by_ifname.side_effect = ( - [("0000:0a:00.0", True), - ("0000:0a:02.1", False), - ("0000:0a:02.2", False), ("0000:0a:00.0", True)]) - open_calls = ( - [mock.call('/sys/class/net/pf_ifname/phys_switch_id', 'r'), - mock.call().readline(), - mock.call().__exit__(None, None, None), - mock.call('/sys/class/net/rep_vf_1/phys_switch_id', 'r'), - mock.call().readline(), - mock.call().__exit__(None, None, None), - mock.call('/sys/class/net/rep_vf_1/phys_port_name', 'r'), - mock.call().readline(), - mock.call().__exit__(None, None, None), - mock.call('/sys/class/net/rep_vf_2/phys_switch_id', 'r'), - mock.call().readline(), - mock.call().__exit__(None, None, None), - mock.call('/sys/class/net/rep_vf_2/phys_port_name', 'r'), - mock.call().readline(), - mock.call().__exit__(None, None, None)]) + mock__get_phys_switch_id.return_value = 'pf_sw_id' + mock__get_pf_func.return_value = "0" + mock__get_phys_port_name.side_effect = (['1', "pf0vf1", "pf0vf2"]) ifname = linux_net.get_representor_port('pf_ifname', '2') - mock_open.assert_has_calls(open_calls) self.assertEqual('rep_vf_2', ifname) - @mock.patch('builtins.open') - @mock.patch.object(os.path, 'isfile') @mock.patch.object(os, 'listdir') - @mock.patch.object(linux_net, "get_function_by_ifname") + @mock.patch.object(linux_net, "_get_pf_func") + @mock.patch.object(linux_net, "_get_phys_port_name") + @mock.patch.object(linux_net, "_get_phys_switch_id") def test_get_representor_port_2_pfs( - self, mock_get_function_by_ifname, mock_listdir, mock_isfile, - mock_open): + self, mock__get_phys_switch_id, mock__get_phys_port_name, + mock__get_pf_func, mock_listdir): mock_listdir.return_value = [ 'pf_ifname1', 'pf_ifname2', 'rep_pf1_vf_1', 'rep_pf1_vf_2', 'rep_pf2_vf_1', 'rep_pf2_vf_2', ] - mock_isfile.side_effect = [True, True, True, True] - mock_open.return_value.__enter__ = lambda s: s - readline_mock = mock_open.return_value.readline - readline_mock.side_effect = ( - ['pf_sw_id', - 'pf_sw_id', 'VF1@PF1', 'pf_sw_id', 'vf2@pf1', - 'pf_sw_id', 'pf2vf1', 'pf_sw_id', 'pf2vf2']) - # PCI IDs mocked: - # PF1: 0000:0a:00.1 PF2: 0000:0a:00.2 - # PF1VF1: 0000:0a:02.1 PF1VF2: 0000:0a:02.2 - # PF2VF1: 0000:0a:04.1 PF2VF2: 0000:0a:04.2 - mock_get_function_by_ifname.side_effect = ( - [("0000:0a:00.1", True), ("0000:0a:00.2", True), - ("0000:0a:02.1", False), ("0000:0a:00.2", True), - ("0000:0a:02.2", False), ("0000:0a:00.2", True), - ("0000:0a:04.1", False), ("0000:0a:00.2", True), - ("0000:0a:04.2", False), ("0000:0a:00.2", True)]) + mock__get_phys_switch_id.return_value = 'pf_sw_id' + mock__get_pf_func.return_value = "2" + mock__get_phys_port_name.side_effect = ( + ["p1", "p2", "VF1@PF1", "pf2vf1", "vf2@pf1", "pf2vf2"]) ifname = linux_net.get_representor_port('pf_ifname2', '2') self.assertEqual('rep_pf2_vf_2', ifname) - @mock.patch('builtins.open') - @mock.patch.object(os.path, 'isfile') @mock.patch.object(os, 'listdir') - @mock.patch.object(linux_net, "get_function_by_ifname") + @mock.patch.object(linux_net, "_get_pf_func") + @mock.patch.object(linux_net, "_get_phys_switch_id") + @mock.patch.object(linux_net, "_get_phys_port_name") def test_get_representor_port_not_found( - self, mock_get_function_by_ifname, mock_listdir, mock_isfile, - mock_open): + self, mock__get_phys_port_name, mock__get_phys_switch_id, + mock__get_pf_func, mock_listdir): mock_listdir.return_value = [ 'pf_ifname', 'rep_vf_1', 'rep_vf_2' ] - mock_isfile.side_effect = [True, True] - mock_open.return_value.__enter__ = lambda s: s - readline_mock = mock_open.return_value.readline - readline_mock.side_effect = ( - ['pf_sw_id', 'pf_sw_id', '1', 'pf_sw_id', '2']) - # PCI IDs mocked: - # PF0: 0000:0a:00.0 - # PF0VF1: 0000:0a:02.1 PF0VF2: 0000:0a:02.2 - mock_get_function_by_ifname.side_effect = ( - [("0000:0a:00.0", True), - ("0000:0a:02.1", False), - ("0000:0a:02.2", False)]) + mock__get_phys_switch_id.return_value = 'pf_sw_id' + mock__get_pf_func.return_value = "0" + mock__get_phys_port_name.side_effect = ( + ["p0", "1", "2"]) self.assertRaises( exception.RepresentorNotFound, linux_net.get_representor_port, 'pf_ifname', '3'), - @mock.patch('builtins.open') - @mock.patch.object(os.path, 'isfile') @mock.patch.object(os, 'listdir') - @mock.patch.object(linux_net, "get_function_by_ifname") + @mock.patch.object(linux_net, "_get_pf_func") + @mock.patch.object(linux_net, "_get_phys_port_name") + @mock.patch.object(linux_net, "_get_phys_switch_id") def test_get_representor_port_exception_io_error( - self, mock_get_function_by_ifname, mock_listdir, mock_isfile, - mock_open): + self, mock__get_phys_switch_id, mock__get_phys_port_name, + mock__get_pf_func, mock_listdir): mock_listdir.return_value = [ 'pf_ifname', 'rep_vf_1', 'rep_vf_2' ] - mock_isfile.side_effect = [True, True] - mock_open.return_value.__enter__ = lambda s: s - readline_mock = mock_open.return_value.readline - readline_mock.side_effect = ( + mock__get_phys_switch_id.side_effect = ( ['pf_sw_id', 'pf_sw_id', IOError(), 'pf_sw_id', '2']) - # PCI IDs mocked: - # PF0: 0000:0a:00.0 - # PF0VF1: 0000:0a:02.1 PF0VF2: 0000:0a:02.2 - mock_get_function_by_ifname.side_effect = ( - [("0000:0a:00.0", True), - ("0000:0a:02.1", False), - ("0000:0a:02.2", False), ("0000:0a:00.0", True)]) + mock__get_pf_func.return_value = "0" + mock__get_phys_port_name.side_effect = ( + ["p0", "pf0vf0", "pf0vf1"]) self.assertRaises( exception.RepresentorNotFound, linux_net.get_representor_port, 'pf_ifname', '3') - @mock.patch('builtins.open') - @mock.patch.object(os.path, 'isfile') @mock.patch.object(os, 'listdir') - @mock.patch.object(linux_net, "get_function_by_ifname") + @mock.patch.object(linux_net, "_get_pf_func") + @mock.patch.object(linux_net, "_get_phys_port_name") + @mock.patch.object(linux_net, "_get_phys_switch_id") def test_get_representor_port_exception_value_error( - self, mock_get_function_by_ifname, mock_listdir, mock_isfile, - mock_open): + self, mock__get_phys_switch_id, mock__get_phys_port_name, + mock__get_pf_func, mock_listdir): mock_listdir.return_value = [ 'pf_ifname', 'rep_vf_1', 'rep_vf_2' ] - mock_isfile.side_effect = [True, True] - mock_open.return_value.__enter__ = lambda s: s - readline_mock = mock_open.return_value.readline - readline_mock.side_effect = ( - ['pf_sw_id', 'pf_sw_id', '1', 'pf_sw_id', 'a']) - # PCI IDs mocked: - # PF0: 0000:0a:00.0 - # PF0VF1: 0000:0a:02.1 PF0VF2: 0000:0a:02.2 - mock_get_function_by_ifname.side_effect = ( - [("0000:0a:00.0", True), - ("0000:0a:02.1", False), - ("0000:0a:02.2", False)]) + mock__get_phys_switch_id.return_value = 'pf_sw_id' + mock__get_phys_port_name.side_effect = (['p0', '1', 'a']) + mock__get_pf_func.return_value = "0" self.assertRaises( exception.RepresentorNotFound, linux_net.get_representor_port, 'pf_ifname', '3') - @mock.patch('builtins.open') - @mock.patch.object(os.path, 'isfile') @mock.patch.object(os, 'listdir') - def test_physical_function_inferface_name( - self, mock_listdir, mock_isfile, mock_open): + @mock.patch.object(linux_net, '_get_phys_switch_id') + def test_physical_function_interface_name( + self, mock__get_phys_switch_id, mock_listdir): mock_listdir.return_value = ['foo', 'bar'] - mock_isfile.side_effect = [True, True] - mock_open.return_value.__enter__ = lambda s: s - readline_mock = mock_open.return_value.readline - readline_mock.side_effect = ( + mock__get_phys_switch_id.side_effect = ( ['', 'valid_switch']) ifname = linux_net.get_ifname_by_pci_address( '0000:00:00.1', pf_interface=True, switchdev=False) self.assertEqual(ifname, 'foo') - @mock.patch('builtins.open') - @mock.patch.object(os.path, 'isfile') @mock.patch.object(os, 'listdir') - def test_physical_function_inferface_name_with_switchdev( - self, mock_listdir, mock_isfile, mock_open): + @mock.patch.object(linux_net, '_get_phys_switch_id') + def test_physical_function_interface_name_with_switchdev( + self, mock__get_phys_switch_id, mock_listdir): mock_listdir.return_value = ['foo', 'bar'] - mock_isfile.side_effect = [True, True] - mock_open.return_value.__enter__ = lambda s: s - readline_mock = mock_open.return_value.readline - readline_mock.side_effect = ( + mock__get_phys_switch_id.side_effect = ( ['', 'valid_switch']) ifname = linux_net.get_ifname_by_pci_address( '0000:00:00.1', pf_interface=True, switchdev=True) @@ -420,3 +326,35 @@ class LinuxNetTest(testtools.TestCase): linux_net.get_vf_num_by_pci_address, '0000:00:00.1' ) + + @mock.patch('builtins.open') + @mock.patch.object(os.path, 'isfile') + def test__get_phys_port_name(self, mock_isfile, mock_open): + mock_open.return_value.__enter__ = lambda s: s + readline_mock = mock_open.return_value.readline + readline_mock.return_value = 'pf0vf0' + mock_isfile.return_value = True + phys_port_name = linux_net._get_phys_port_name("vf_ifname") + self.assertEqual(phys_port_name, 'pf0vf0') + + @mock.patch.object(os.path, 'isfile') + def test__get_phys_port_name_not_found(self, mock_isfile): + mock_isfile.return_value = False + phys_port_name = linux_net._get_phys_port_name("vf_ifname") + self.assertIsNone(phys_port_name) + + @mock.patch('builtins.open') + @mock.patch.object(os.path, 'isfile') + def test__get_phys_switch_id(self, mock_isfile, mock_open): + mock_open.return_value.__enter__ = lambda s: s + readline_mock = mock_open.return_value.readline + readline_mock.return_value = '66e40000039b0398' + mock_isfile.return_value = True + phys_port_name = linux_net._get_phys_switch_id("ifname") + self.assertEqual(phys_port_name, '66e40000039b0398') + + @mock.patch.object(os.path, 'isfile') + def test__get_phys_switch_id_not_found(self, mock_isfile): + mock_isfile.return_value = False + phys_port_name = linux_net._get_phys_switch_id("ifname") + self.assertIsNone(phys_port_name)