From ba2100a83f674fb5d86471afa829ec4dd29653d0 Mon Sep 17 00:00:00 2001 From: Kendall Nelson Date: Tue, 12 Jan 2016 14:12:27 -0600 Subject: [PATCH] Remove multipath -l logic from ISCSI connector This patch continues work on making the connect_volume methods more efficient. Using multipath -l to find the available multipath devices can take a lot of time listing all of the potential devices. Previously, the FC connector had been modified to skip the use of multipath -l. This patch takes the FC code and makes it into a generic method to be used in the ISCSI connector and the FC connector and updates tests. I also fixed argument order in assertEquals() in the test_connect_volume_with_multipath() function. Closes-Bug: #1487169 Change-Id: If480df7c17a6f1c2ecebc0e00f5938d3056776fd --- os_brick/initiator/connector.py | 93 +++++++++++++--------- os_brick/tests/initiator/test_connector.py | 80 ++++++++++++++----- 2 files changed, 113 insertions(+), 60 deletions(-) diff --git a/os_brick/initiator/connector.py b/os_brick/initiator/connector.py index 9926deaa8..afcf24270 100644 --- a/os_brick/initiator/connector.py +++ b/os_brick/initiator/connector.py @@ -153,6 +153,7 @@ class InitiatorConnector(executor.Executor): driver = host_driver.HostDriver() self.set_driver(driver) self.device_scan_attempts = device_scan_attempts + self._linuxscsi = linuxscsi.LinuxSCSI(root_helper, execute=execute) def set_driver(self, driver): """The driver is used to find used LUNs.""" @@ -286,6 +287,45 @@ class InitiatorConnector(executor.Executor): return False return True + def _discover_mpath_device(self, device_wwn, + connection_properties): + """This method discovers a multipath device. + + Discover a multipath device based on a defined connection_property + and a device_wwn and return the multipath_id and path of the multipath + enabled device if there is one. + """ + + path = self._linuxscsi.find_multipath_device_path(device_wwn) + device_path = None + multipath_id = None + + if path is None: + mpath_info = self._linuxscsi.find_multipath_device( + self.device_name) + if mpath_info: + device_path = mpath_info['device'] + multipath_id = device_wwn + else: + # we didn't find a multipath device. + # so we assume the kernel only sees 1 device + device_path = self.host_device + LOG.debug("Unable to find multipath device name for " + "volume. Using path %(device)s for volume.", + {'device': self.host_device}) + else: + device_path = path + multipath_id = device_wwn + if connection_properties.get('access_mode', '') != 'ro': + try: + # Sometimes the multipath devices will show up as read only + # initially and need additional time/rescans to get to RW. + self._linuxscsi.wait_for_rw(device_wwn, device_path) + except exception.BlockDeviceReadOnly: + LOG.warning(_LW('Block device %s is still read-only. ' + 'Continuing anyway.'), device_path) + return device_path, multipath_id + @abc.abstractmethod def connect_volume(self, connection_properties): """Connect to a volume. @@ -797,20 +837,18 @@ class ISCSIConnector(InitiatorConnector): # Choose an accessible host device host_device = next(dev for dev in host_devices if os.path.exists(dev)) - if self.use_multipath: - # We use the multipath device instead of the single path device - self._rescan_multipath() - multipath_device = self._get_multipath_device_name(host_device) - if multipath_device is not None: - host_device = multipath_device - LOG.debug("Found multipath device name for " - "volume. Using path %(device)s " - "for volume.", {'device': host_device}) - # find out the WWN of the device device_wwn = self._linuxscsi.get_scsi_wwn(host_device) LOG.debug("Device WWN = '%(wwn)s'", {'wwn': device_wwn}) device_info['scsi_wwn'] = device_wwn + + if self.use_multipath: + (host_device, multipath_id) = (super( + ISCSIConnector, self)._discover_mpath_device( + device_wwn, connection_properties)) + if multipath_id: + device_info['multipath_id'] = multipath_id + device_info['path'] = host_device LOG.debug("connect_volume returning %s", device_info) @@ -1334,37 +1372,14 @@ class FibreChannelConnector(InitiatorConnector): # see if the new drive is part of a multipath # device. If so, we'll use the multipath device. if self.use_multipath: + (device_path, multipath_id) = (super( + FibreChannelConnector, self)._discover_mpath_device( + device_wwn, connection_properties)) + if multipath_id: + device_info['multipath_id'] = multipath_id - path = self._linuxscsi.find_multipath_device_path(device_wwn) - if path is not None: - LOG.debug("Multipath device path discovered %(device)s", - {'device': path}) - device_path = path - # for temporary backwards compatibility + if device_path and device_info['multipath_id'] is None: device_info['multipath_id'] = device_wwn - else: - mpath_info = self._linuxscsi.find_multipath_device( - self.device_name) - if mpath_info: - device_path = mpath_info['device'] - # for temporary backwards compatibility - device_info['multipath_id'] = device_wwn - else: - # we didn't find a multipath device. - # so we assume the kernel only sees 1 device - device_path = self.host_device - LOG.debug("Unable to find multipath device name for " - "volume. Using path %(device)s for volume.", - {'device': self.host_device}) - - if connection_properties.get('access_mode', '') != 'ro': - try: - # Sometimes the multipath devices will show up as read only - # initially and need additional time/rescans to get to RW. - self._linuxscsi.wait_for_rw(device_wwn, device_path) - except exception.BlockDeviceReadOnly: - LOG.warning(_LW('Block device %s is still read-only. ' - 'Continuing anyway.'), device_path) else: device_path = self.host_device diff --git a/os_brick/tests/initiator/test_connector.py b/os_brick/tests/initiator/test_connector.py index d0aecc10d..f00827ac8 100644 --- a/os_brick/tests/initiator/test_connector.py +++ b/os_brick/tests/initiator/test_connector.py @@ -334,6 +334,31 @@ class ISCSIConnectorTestCase(ConnectorTestCase): connection_properties['data']) self.assertEqual(expected, volume_paths) + @mock.patch.object(linuxscsi.LinuxSCSI, 'find_multipath_device_path') + @mock.patch.object(linuxscsi.LinuxSCSI, 'find_multipath_device') + def test_discover_mpath_device(self, mock_multipath_device, + mock_multipath_device_path): + location1 = '10.0.2.15:3260' + location2 = '[2001:db8::1]:3260' + name1 = 'volume-00000001-1' + name2 = 'volume-00000001-2' + iqn1 = 'iqn.2010-10.org.openstack:%s' % name1 + iqn2 = 'iqn.2010-10.org.openstack:%s' % name2 + fake_multipath_dev = '/dev/mapper/fake-multipath-dev' + vol = {'id': 1, 'name': name1} + connection_properties = self.iscsi_connection_multipath( + vol, [location1, location2], [iqn1, iqn2], [1, 2]) + mock_multipath_device_path.return_value = fake_multipath_dev + mock_multipath_device.return_value = FAKE_SCSI_WWN + (result_path, result_mpath_id) = ( + self.connector_with_multipath._discover_mpath_device( + FAKE_SCSI_WWN, + connection_properties['data'])) + result = {'path': result_path, 'multipath_id': result_mpath_id} + expected_result = {'path': fake_multipath_dev, + 'multipath_id': FAKE_SCSI_WWN} + self.assertEqual(expected_result, result) + def _test_connect_volume(self, extra_props, additional_commands, transport=None, disconnect_mock=None): # for making sure the /dev/disk/by-path is gone @@ -509,16 +534,19 @@ class ISCSIConnectorTestCase(ConnectorTestCase): @mock.patch.object(connector.ISCSIConnector, '_rescan_multipath') @mock.patch.object(connector.ISCSIConnector, '_get_multipath_device_name') @mock.patch.object(os.path, 'exists', return_value=True) + @mock.patch.object(connector.InitiatorConnector, '_discover_mpath_device') def test_connect_volume_with_multipath( - self, exists_mock, get_device_mock, rescan_multipath_mock, - rescan_iscsi_mock, connect_to_mock, portals_mock, iscsiadm_mock, - mock_iscsi_wwn): + self, mock_discover_mpath_device, exists_mock, get_device_mock, + rescan_multipath_mock, rescan_iscsi_mock, connect_to_mock, + portals_mock, iscsiadm_mock, mock_iscsi_wwn): mock_iscsi_wwn.return_value = FAKE_SCSI_WWN location = '10.0.2.15:3260' name = 'volume-00000001' iqn = 'iqn.2010-10.org.openstack:%s' % name vol = {'id': 1, 'name': name} connection_properties = self.iscsi_connection(vol, location, iqn) + mock_discover_mpath_device.return_value = ( + 'iqn.2010-10.org.openstack:%s' % name, FAKE_SCSI_WWN) self.connector_with_multipath = \ connector.ISCSIConnector(None, use_multipath=True) @@ -528,10 +556,11 @@ class ISCSIConnectorTestCase(ConnectorTestCase): result = self.connector_with_multipath.connect_volume( connection_properties['data']) - expected_result = {'path': 'iqn.2010-10.org.openstack:volume-00000001', + expected_result = {'multipath_id': FAKE_SCSI_WWN, + 'path': 'iqn.2010-10.org.openstack:volume-00000001', 'type': 'block', 'scsi_wwn': FAKE_SCSI_WWN} - self.assertEqual(result, expected_result) + self.assertEqual(expected_result, result) @mock.patch.object(connector.ISCSIConnector, '_run_iscsiadm_update_discoverydb') @@ -585,13 +614,13 @@ class ISCSIConnectorTestCase(ConnectorTestCase): @mock.patch.object(connector.ISCSIConnector, '_get_multipath_device_map', return_value={}) @mock.patch.object(connector.ISCSIConnector, '_get_iscsi_devices') - @mock.patch.object(connector.ISCSIConnector, '_rescan_multipath') @mock.patch.object(connector.ISCSIConnector, '_run_multipath') @mock.patch.object(connector.ISCSIConnector, '_get_multipath_device_name') @mock.patch.object(connector.ISCSIConnector, '_get_multipath_iqns') + @mock.patch.object(connector.InitiatorConnector, '_discover_mpath_device') def test_connect_volume_with_multiple_portals( - self, mock_get_iqn, mock_device_name, mock_run_multipath, - mock_rescan_multipath, mock_iscsi_devices, + self, mock_discover_mpath_device, mock_get_iqn, mock_device_name, + mock_run_multipath, mock_iscsi_devices, mock_get_device_map, mock_devices, mock_exists, mock_scsi_wwn): mock_scsi_wwn.return_value = FAKE_SCSI_WWN location1 = '10.0.2.15:3260' @@ -609,12 +638,14 @@ class ISCSIConnectorTestCase(ConnectorTestCase): '/dev/disk/by-path/ip-%s-iscsi-%s-lun-2' % (dev_loc2, iqn2)] mock_devices.return_value = devs mock_iscsi_devices.return_value = devs - mock_device_name.return_value = fake_multipath_dev mock_get_iqn.return_value = [iqn1, iqn2] + mock_discover_mpath_device.return_value = (fake_multipath_dev, + FAKE_SCSI_WWN) result = self.connector_with_multipath.connect_volume( connection_properties['data']) - expected_result = {'path': fake_multipath_dev, 'type': 'block', + expected_result = {'multipath_id': FAKE_SCSI_WWN, + 'path': fake_multipath_dev, 'type': 'block', 'scsi_wwn': FAKE_SCSI_WWN} cmd_format = 'iscsiadm -m node -T %s -p %s --%s' expected_commands = [cmd_format % (iqn1, location1, 'login'), @@ -622,7 +653,6 @@ class ISCSIConnectorTestCase(ConnectorTestCase): self.assertEqual(expected_result, result) for command in expected_commands: self.assertIn(command, self.cmds) - mock_device_name.assert_called_once_with(devs[0]) self.cmds = [] self.connector_with_multipath.disconnect_volume( @@ -643,11 +673,12 @@ class ISCSIConnectorTestCase(ConnectorTestCase): @mock.patch.object(connector.ISCSIConnector, '_get_multipath_device_name') @mock.patch.object(connector.ISCSIConnector, '_get_multipath_iqns') @mock.patch.object(connector.ISCSIConnector, '_run_iscsiadm') + @mock.patch.object(connector.InitiatorConnector, '_discover_mpath_device') def test_connect_volume_with_multiple_portals_primary_error( - self, mock_iscsiadm, mock_get_iqn, mock_device_name, - mock_run_multipath, mock_rescan_multipath, mock_iscsi_devices, - mock_get_multipath_device_map, mock_devices, mock_exists, - mock_scsi_wwn): + self, mock_discover_mpath_device, mock_iscsiadm, mock_get_iqn, + mock_device_name, mock_run_multipath, mock_rescan_multipath, + mock_iscsi_devices, mock_get_multipath_device_map, mock_devices, + mock_exists, mock_scsi_wwn): mock_scsi_wwn.return_value = FAKE_SCSI_WWN location1 = '10.0.2.15:3260' location2 = '[2001:db8::1]:3260' @@ -675,15 +706,17 @@ class ISCSIConnectorTestCase(ConnectorTestCase): mock_device_name.return_value = fake_multipath_dev mock_get_iqn.return_value = [iqn2] mock_iscsiadm.side_effect = fake_run_iscsiadm + mock_discover_mpath_device.return_value = (fake_multipath_dev, + FAKE_SCSI_WWN) props = connection_properties['data'].copy() result = self.connector_with_multipath.connect_volume( connection_properties['data']) - expected_result = {'path': fake_multipath_dev, 'type': 'block', + expected_result = {'multipath_id': FAKE_SCSI_WWN, + 'path': fake_multipath_dev, 'type': 'block', 'scsi_wwn': FAKE_SCSI_WWN} self.assertEqual(expected_result, result) - mock_device_name.assert_called_once_with(dev2) props['target_portal'] = location1 props['target_iqn'] = iqn1 mock_iscsiadm.assert_any_call(props, ('--login',), @@ -717,10 +750,12 @@ class ISCSIConnectorTestCase(ConnectorTestCase): @mock.patch.object(connector.ISCSIConnector, '_rescan_multipath') @mock.patch.object(connector.ISCSIConnector, '_run_multipath') @mock.patch.object(connector.ISCSIConnector, '_get_multipath_device_name') + @mock.patch.object(connector.InitiatorConnector, '_discover_mpath_device') def test_connect_volume_with_multipath_connecting( - self, mock_device_name, mock_run_multipath, - mock_rescan_multipath, mock_iscsi_devices, mock_devices, - mock_connect, mock_portals, mock_exists, mock_scsi_wwn): + self, mock_discover_mpath_device, mock_device_name, + mock_run_multipath, mock_rescan_multipath, mock_iscsi_devices, + mock_devices, mock_connect, mock_portals, mock_exists, + mock_scsi_wwn): mock_scsi_wwn.return_value = FAKE_SCSI_WWN location1 = '10.0.2.15:3260' location2 = '[2001:db8::1]:3260' @@ -739,10 +774,13 @@ class ISCSIConnectorTestCase(ConnectorTestCase): mock_device_name.return_value = fake_multipath_dev mock_portals.return_value = [[location1, iqn1], [location2, iqn1], [location2, iqn2]] + mock_discover_mpath_device.return_value = (fake_multipath_dev, + FAKE_SCSI_WWN) result = self.connector_with_multipath.connect_volume( connection_properties['data']) - expected_result = {'path': fake_multipath_dev, 'type': 'block', + expected_result = {'multipath_id': FAKE_SCSI_WWN, + 'path': fake_multipath_dev, 'type': 'block', 'scsi_wwn': FAKE_SCSI_WWN} props1 = connection_properties['data'].copy() props2 = connection_properties['data'].copy()