diff --git a/os_brick/exception.py b/os_brick/exception.py index 294eb3751..a2377b3d7 100644 --- a/os_brick/exception.py +++ b/os_brick/exception.py @@ -120,7 +120,7 @@ class TargetPortalNotFound(BrickException): message = _("Unable to find target portal %(target_portal)s.") -class TargetPortalsNotFound(BrickException): +class TargetPortalsNotFound(TargetPortalNotFound): message = _("Unable to find target portal in %(target_portals)s.") diff --git a/os_brick/initiator/connectors/base_iscsi.py b/os_brick/initiator/connectors/base_iscsi.py index b91a592a3..f81579341 100644 --- a/os_brick/initiator/connectors/base_iscsi.py +++ b/os_brick/initiator/connectors/base_iscsi.py @@ -29,13 +29,18 @@ class BaseISCSIConnector(initiator_connector.InitiatorConnector): props.pop(key, None) yield props + @staticmethod + def _get_luns(con_props, iqns=None): + luns = con_props.get('target_luns') + num_luns = len(con_props['target_iqns']) if iqns is None else len(iqns) + return luns or [con_props['target_lun']] * num_luns + def _get_all_targets(self, connection_properties): - if all([key in connection_properties for key in ('target_portals', - 'target_iqns', - 'target_luns')]): - return zip(connection_properties['target_portals'], - connection_properties['target_iqns'], - connection_properties['target_luns']) + if all(key in connection_properties for key in ('target_portals', + 'target_iqns')): + return list(zip(connection_properties['target_portals'], + connection_properties['target_iqns'], + self._get_luns(connection_properties))) return [(connection_properties['target_portal'], connection_properties['target_iqn'], diff --git a/os_brick/initiator/connectors/iscsi.py b/os_brick/initiator/connectors/iscsi.py index f7b1c7a56..ba178c05b 100644 --- a/os_brick/initiator/connectors/iscsi.py +++ b/os_brick/initiator/connectors/iscsi.py @@ -167,35 +167,45 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): # entry: [tcp, [1], 192.168.121.250:3260,1 ...] return [entry[2] for entry in self._get_iscsi_sessions_full()] - def _get_ips_iqns_luns(self, connection_properties, discover=True): + def _get_ips_iqns_luns(self, connection_properties, discover=True, + is_disconnect_call=False): """Build a list of ips, iqns, and luns. - Used only when doing multipath, we have 3 cases: + Used when doing singlepath and multipath, and we have 4 cases: - All information is in the connection properties - We have to do an iSCSI discovery to get the information - We don't want to do another discovery and we query the discoverydb + - Discovery failed because it was actually a single pathed attachment :param connection_properties: The dictionary that describes all of the target volume attributes. :type connection_properties: dict :param discover: Whether doing an iSCSI discovery is acceptable. :type discover: bool + :param is_disconnect_call: Whether this is a call coming from a user + disconnect_volume call or a call from some + other operation's cleanup. + :type is_disconnect_call: bool :returns: list of tuples of (ip, iqn, lun) """ + # There are cases where we don't know if the local attach was done + # using multipathing or single pathing, so assume multipathing. try: if ('target_portals' in connection_properties and 'target_iqns' in connection_properties): # Use targets specified by connection_properties - ips_iqns_luns = list( - zip(connection_properties['target_portals'], - connection_properties['target_iqns'], - self._get_luns(connection_properties))) + ips_iqns_luns = self._get_all_targets(connection_properties) else: method = (self._discover_iscsi_portals if discover else self._get_discoverydb_portals) ips_iqns_luns = method(connection_properties) - except exception.TargetPortalsNotFound: + except exception.TargetPortalNotFound: + # Discovery failed, on disconnect this will happen if we + # are detaching a single pathed connection, so we use the + # connection properties to return the tuple. + if is_disconnect_call: + return self._get_all_targets(connection_properties) raise except Exception: LOG.exception('Exception encountered during portal discovery') @@ -311,12 +321,6 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): def _get_transport(self): return self.transport - @staticmethod - def _get_luns(con_props, iqns=None): - luns = con_props.get('target_luns') - num_luns = len(con_props['target_iqns']) if iqns is None else len(iqns) - return luns or [con_props.get('target_lun')] * num_luns - def _get_discoverydb_portals(self, connection_properties): """Retrieve iscsi portals information from the discoverydb. @@ -787,7 +791,7 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): mpath) def _get_connection_devices(self, connection_properties, - ips_iqns_luns=None): + ips_iqns_luns=None, is_disconnect_call=False): """Get map of devices by sessions from our connection. For each of the TCP sessions that correspond to our connection @@ -807,14 +811,15 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): connection properties (sendtargets was used on connect) so we may have to retrieve the info from the discoverydb. Call _get_ips_iqns_luns to do the right things. + + This method currently assumes that it's only called by the + _cleanup_conection method. """ if not ips_iqns_luns: - if self.use_multipath: - # We are only called from disconnect, so don't discover - ips_iqns_luns = self._get_ips_iqns_luns(connection_properties, - discover=False) - else: - ips_iqns_luns = self._get_all_targets(connection_properties) + # This is a cleanup, don't do discovery + ips_iqns_luns = self._get_ips_iqns_luns( + connection_properties, discover=False, + is_disconnect_call=is_disconnect_call) LOG.debug('Getting connected devices for (ips,iqns,luns)=%s', ips_iqns_luns) nodes = self._get_iscsi_nodes() @@ -874,11 +879,12 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): """ return self._cleanup_connection(connection_properties, force=force, ignore_errors=ignore_errors, - device_info=device_info) + device_info=device_info, + is_disconnect_call=True) def _cleanup_connection(self, connection_properties, ips_iqns_luns=None, force=False, ignore_errors=False, - device_info=None): + device_info=None, is_disconnect_call=False): """Cleans up connection flushing and removing devices and multipath. :param connection_properties: The dictionary that describes all @@ -895,13 +901,18 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): ignore errors or raise an exception once finished the operation. Default is False. :param device_info: Attached device information. + :param is_disconnect_call: Whether this is a call coming from a user + disconnect_volume call or a call from some + other operation's cleanup. + :type is_disconnect_call: bool :type ignore_errors: bool """ exc = exception.ExceptionChainer() try: devices_map = self._get_connection_devices(connection_properties, - ips_iqns_luns) - except exception.TargetPortalsNotFound as exc: + ips_iqns_luns, + is_disconnect_call) + except exception.TargetPortalNotFound as exc: # When discovery sendtargets failed on connect there is no # information in the discoverydb, so there's nothing to clean. LOG.debug('Skipping cleanup %s', exc) @@ -917,8 +928,7 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): was_multipath = (path_used.startswith('/dev/dm-') or 'mpath' in path_used) multipath_name = self._linuxscsi.remove_connection( - remove_devices, self.use_multipath, force, exc, - path_used, was_multipath) + remove_devices, force, exc, path_used, was_multipath) # Disconnect sessions and remove nodes that are left without devices disconnect = [conn for conn, (__, keep) in devices_map.items() diff --git a/os_brick/initiator/linuxscsi.py b/os_brick/initiator/linuxscsi.py index c1498e5cb..480503f70 100644 --- a/os_brick/initiator/linuxscsi.py +++ b/os_brick/initiator/linuxscsi.py @@ -280,12 +280,11 @@ class LinuxSCSI(executor.Executor): # instead it maps to /dev/mapped/crypt-XYZ return not was_multipath and '/dev' != os.path.split(path_used)[0] - def remove_connection(self, devices_names, is_multipath, force=False, - exc=None, path_used=None, was_multipath=False): + def remove_connection(self, devices_names, force=False, exc=None, + path_used=None, was_multipath=False): """Remove LUNs and multipath associated with devices names. :param devices_names: Iterable with real device names ('sda', 'sdb') - :param is_multipath: Whether this is a multipath connection or not :param force: Whether to forcefully disconnect even if flush fails. :param exc: ExceptionChainer where to add exceptions if forcing :param path_used: What path was used by Nova/Cinder for I/O @@ -294,19 +293,17 @@ class LinuxSCSI(executor.Executor): """ if not devices_names: return - multipath_name = None exc = exception.ExceptionChainer() if exc is None else exc - LOG.debug('Removing %(type)s devices %(devices)s', - {'type': 'multipathed' if is_multipath else 'single pathed', - 'devices': ', '.join(devices_names)}) - if is_multipath: - multipath_dm = self.find_sysfs_multipath_dm(devices_names) - multipath_name = multipath_dm and self.get_dm_name(multipath_dm) - if multipath_name: - with exc.context(force, 'Flushing %s failed', multipath_name): - self.flush_multipath_device(multipath_name) - multipath_name = None + multipath_dm = self.find_sysfs_multipath_dm(devices_names) + LOG.debug('Removing %(type)s devices %(devices)s', + {'type': 'multipathed' if multipath_dm else 'single pathed', + 'devices': ', '.join(devices_names)}) + multipath_name = multipath_dm and self.get_dm_name(multipath_dm) + if multipath_name: + with exc.context(force, 'Flushing %s failed', multipath_name): + self.flush_multipath_device(multipath_name) + multipath_name = None for device_name in devices_names: dev_path = '/dev/' + device_name diff --git a/os_brick/tests/initiator/connectors/test_base_iscsi.py b/os_brick/tests/initiator/connectors/test_base_iscsi.py index 26408c36e..aee246845 100644 --- a/os_brick/tests/initiator/connectors/test_base_iscsi.py +++ b/os_brick/tests/initiator/connectors/test_base_iscsi.py @@ -51,16 +51,29 @@ class BaseISCSIConnectorTestCase(test_base.TestCase): self.assertEqual(expected_props, list_props[0]) def test_get_all_targets(self): - connection_properties = { - 'target_portals': [mock.sentinel.target_portals], - 'target_iqns': [mock.sentinel.target_iqns], - 'target_luns': [mock.sentinel.target_luns]} + portals = [mock.sentinel.portals1, mock.sentinel.portals2] + iqns = [mock.sentinel.iqns1, mock.sentinel.iqns2] + luns = [mock.sentinel.luns1, mock.sentinel.luns2] + connection_properties = {'target_portals': portals, + 'target_iqns': iqns, + 'target_luns': luns} all_targets = self.connector._get_all_targets(connection_properties) - expected_targets = zip([mock.sentinel.target_portals], - [mock.sentinel.target_iqns], - [mock.sentinel.target_luns]) + expected_targets = zip(portals, iqns, luns) + self.assertEqual(list(expected_targets), list(all_targets)) + + def test_get_all_targets_no_target_luns(self): + portals = [mock.sentinel.portals1, mock.sentinel.portals2] + iqns = [mock.sentinel.iqns1, mock.sentinel.iqns2] + lun = mock.sentinel.luns + connection_properties = {'target_portals': portals, + 'target_iqns': iqns, + 'target_lun': lun} + + all_targets = self.connector._get_all_targets(connection_properties) + + expected_targets = zip(portals, iqns, [lun, lun]) self.assertEqual(list(expected_targets), list(all_targets)) def test_get_all_targets_single_target(self): diff --git a/os_brick/tests/initiator/connectors/test_iscsi.py b/os_brick/tests/initiator/connectors/test_iscsi.py index 0d3d09b62..48c9c024e 100644 --- a/os_brick/tests/initiator/connectors/test_iscsi.py +++ b/os_brick/tests/initiator/connectors/test_iscsi.py @@ -140,7 +140,6 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_nodes') def test_get_connection_devices(self, nodes_mock, sessions_mock, glob_mock, iql_mock): - self.connector.use_multipath = True iql_mock.return_value = self.connector._get_all_targets(self.CON_PROPS) # List sessions from other targets and non tcp sessions @@ -166,7 +165,8 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): ('ip2:port2', 'tgt2'): ({'sdb'}, {'sdc'}), ('ip3:port3', 'tgt3'): (set(), set())} self.assertDictEqual(expected, res) - iql_mock.assert_called_once_with(self.CON_PROPS, discover=False) + iql_mock.assert_called_once_with(self.CON_PROPS, discover=False, + is_disconnect_call=False) @mock.patch('glob.glob') @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_sessions_full') @@ -560,7 +560,8 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): mock.sentinel.con_props, force=mock.sentinel.Force, ignore_errors=mock.sentinel.ignore_errors, - device_info=mock.sentinel.dev_info) + device_info=mock.sentinel.dev_info, + is_disconnect_call=True) @ddt.data(True, False) @mock.patch.object(iscsi.ISCSIConnector, '_get_transport') @@ -692,19 +693,17 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): (('ip2:port2', 'tgt2'), ({'sdb'}, {'sdc'})), (('ip3:port3', 'tgt3'), (set(), set())))) - with mock.patch.object(self.connector, - 'use_multipath') as use_mp_mock: - self.connector._cleanup_connection( - self.CON_PROPS, ips_iqns_luns=mock.sentinel.ips_iqns_luns, - force=False, ignore_errors=False, - device_info=mock.sentinel.device_info) + self.connector._cleanup_connection( + self.CON_PROPS, ips_iqns_luns=mock.sentinel.ips_iqns_luns, + force=False, ignore_errors=False, + device_info=mock.sentinel.device_info) get_dev_path_mock.called_once_with(self.CON_PROPS, mock.sentinel.device_info) con_devs_mock.assert_called_once_with(self.CON_PROPS, - mock.sentinel.ips_iqns_luns) - remove_mock.assert_called_once_with({'sda', 'sdb'}, use_mp_mock, - False, mock.ANY, + mock.sentinel.ips_iqns_luns, + False) + remove_mock.assert_called_once_with({'sda', 'sdb'}, False, mock.ANY, path_used, was_multipath) discon_mock.assert_called_once_with( self.CON_PROPS, @@ -730,17 +729,16 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): (('ip2:port2', 'tgt2'), ({'sdb'}, {'sdc'})), (('ip3:port3', 'tgt3'), (set(), set())))) - with mock.patch.object(self.connector, 'use_multipath', - wraps=True) as use_mp_mock: - self.assertRaises(exception.ExceptionChainer, - self.connector._cleanup_connection, - self.CON_PROPS, - ips_iqns_luns=mock.sentinel.ips_iqns_luns, - force=mock.sentinel.force, ignore_errors=False) + self.assertRaises(exception.ExceptionChainer, + self.connector._cleanup_connection, + self.CON_PROPS, + ips_iqns_luns=mock.sentinel.ips_iqns_luns, + force=mock.sentinel.force, ignore_errors=False) con_devs_mock.assert_called_once_with(self.CON_PROPS, - mock.sentinel.ips_iqns_luns) - remove_mock.assert_called_once_with({'sda', 'sdb'}, use_mp_mock, + mock.sentinel.ips_iqns_luns, + False) + remove_mock.assert_called_once_with({'sda', 'sdb'}, mock.sentinel.force, mock.ANY, '', False) discon_mock.assert_called_once_with( @@ -976,6 +974,21 @@ Setting up iSCSI targets: unused db_portals_mock.assert_called_once_with(self.SINGLE_CON_PROPS) discover_mock.assert_not_called() + @mock.patch.object(iscsi.ISCSIConnector, '_get_all_targets') + @mock.patch.object(iscsi.ISCSIConnector, '_get_discoverydb_portals') + @mock.patch.object(iscsi.ISCSIConnector, '_discover_iscsi_portals') + def test_get_ips_iqns_luns_disconnect_single_path(self, discover_mock, + db_portals_mock, + get_targets_mock): + db_portals_mock.side_effect = exception.TargetPortalsNotFound + res = self.connector._get_ips_iqns_luns(self.SINGLE_CON_PROPS, + discover=False, + is_disconnect_call=True) + db_portals_mock.assert_called_once_with(self.SINGLE_CON_PROPS) + discover_mock.assert_not_called() + get_targets_mock.assert_called_once_with(self.SINGLE_CON_PROPS) + self.assertEqual(get_targets_mock.return_value, res) + @mock.patch.object(iscsi.ISCSIConnector, '_discover_iscsi_portals') def test_get_ips_iqns_luns_no_target_iqns_share_iqn(self, discover_mock): discover_mock.return_value = [('ip1:port1', 'tgt1', '1'), @@ -1249,7 +1262,9 @@ Setting up iSCSI targets: unused connect_mock.side_effect = my_connect - res = self.connector._connect_multipath_volume(self.CON_PROPS) + with mock.patch.object(self.connector, + 'use_multipath'): + res = self.connector._connect_multipath_volume(self.CON_PROPS) expected = {'type': 'block', 'scsi_wwn': '', 'multipath_id': '', 'path': '/dev/dm-0'} diff --git a/os_brick/tests/initiator/connectors/test_iser.py b/os_brick/tests/initiator/connectors/test_iser.py index 5c546a599..6ba3ed72a 100644 --- a/os_brick/tests/initiator/connectors/test_iser.py +++ b/os_brick/tests/initiator/connectors/test_iser.py @@ -56,7 +56,8 @@ class ISERConnectorTestCase(test_connector.ConnectorTestCase): res = self.connector._get_connection_devices(self.connection_data) expected = {('ip:port', 'target_1'): ({'sda'}, set())} self.assertDictEqual(expected, res) - iql_mock.assert_called_once_with(self.connection_data, discover=False) + iql_mock.assert_called_once_with(self.connection_data, discover=False, + is_disconnect_call=False) @mock.patch.object(iscsi.ISCSIConnector, '_get_iscsi_sessions_full') @mock.patch.object(iscsi.ISCSIConnector, '_execute') diff --git a/os_brick/tests/initiator/test_linuxscsi.py b/os_brick/tests/initiator/test_linuxscsi.py index 32ee94e9b..7b4e21268 100644 --- a/os_brick/tests/initiator/test_linuxscsi.py +++ b/os_brick/tests/initiator/test_linuxscsi.py @@ -246,7 +246,6 @@ class LinuxSCSITestCase(base.TestCase): devices_names = ('sda', 'sdb') exc = exception.ExceptionChainer() mp_name = self.linuxscsi.remove_connection(devices_names, - is_multipath=True, force=mock.sentinel.Force, exc=exc) find_dm_mock.assert_called_once_with(devices_names) @@ -276,8 +275,7 @@ class LinuxSCSITestCase(base.TestCase): exc = exception.ExceptionChainer() self.assertRaises(exception.ExceptionChainer, self.linuxscsi.remove_connection, - devices_names, is_multipath=True, - force=False, exc=exc) + devices_names, force=False, exc=exc) find_dm_mock.assert_called_once_with(devices_names) get_dm_name_mock.assert_called_once_with(find_dm_mock.return_value) flush_mp_mock.assert_called_once_with(get_dm_name_mock.return_value) @@ -286,36 +284,49 @@ class LinuxSCSITestCase(base.TestCase): remove_link_mock.assert_not_called() self.assertTrue(bool(exc)) + @mock.patch.object(linuxscsi.LinuxSCSI, 'find_sysfs_multipath_dm') @mock.patch.object(linuxscsi.LinuxSCSI, '_remove_scsi_symlinks') @mock.patch.object(linuxscsi.LinuxSCSI, 'wait_for_volumes_removal') @mock.patch.object(linuxscsi.LinuxSCSI, 'remove_scsi_device') - def test_remove_connection_singlepath(self, remove_mock, wait_mock, - remove_link_mock): + def test_remove_connection_singlepath_no_path(self, remove_mock, wait_mock, + remove_link_mock, + find_dm_mock): + # Test remove connection when we didn't form a multipath and didn't + # even use any of the devices that were found. This means that we + # don't flush any of the single paths when removing them. + find_dm_mock.return_value = None devices_names = ('sda', 'sdb') exc = exception.ExceptionChainer() - self.linuxscsi.remove_connection(devices_names, is_multipath=False, + self.linuxscsi.remove_connection(devices_names, force=mock.sentinel.Force, exc=exc) + find_dm_mock.assert_called_once_with(devices_names) remove_mock.assert_has_calls( [mock.call('/dev/sda', mock.sentinel.Force, exc, False), mock.call('/dev/sdb', mock.sentinel.Force, exc, False)]) wait_mock.assert_called_once_with(devices_names) remove_link_mock.assert_called_once_with(devices_names) + @mock.patch.object(linuxscsi.LinuxSCSI, 'find_sysfs_multipath_dm') @mock.patch.object(linuxscsi.LinuxSCSI, '_remove_scsi_symlinks') @mock.patch.object(linuxscsi.LinuxSCSI, 'wait_for_volumes_removal') @mock.patch.object(linuxscsi.LinuxSCSI, 'remove_scsi_device') def test_remove_connection_singlepath_used(self, remove_mock, wait_mock, - remove_link_mock): + remove_link_mock, find_dm_mock): + # Test remove connection when we didn't form a multipath and just used + # one of the single paths that were found. This means that we don't + # flush any of the single paths when removing them. + find_dm_mock.return_value = None devices_names = ('sda', 'sdb') exc = exception.ExceptionChainer() # realpath was mocked on test setup with mock.patch('os.path.realpath', side_effect=self.realpath): - self.linuxscsi.remove_connection(devices_names, is_multipath=True, + self.linuxscsi.remove_connection(devices_names, force=mock.sentinel.Force, exc=exc, path_used='/dev/sdb', was_multipath=False) + find_dm_mock.assert_called_once_with(devices_names) remove_mock.assert_has_calls( [mock.call('/dev/sda', mock.sentinel.Force, exc, False), mock.call('/dev/sdb', mock.sentinel.Force, exc, True)]) diff --git a/releasenotes/notes/disconnect-multipath-cfg-changed-637abc5ecf44fb10.yaml b/releasenotes/notes/disconnect-multipath-cfg-changed-637abc5ecf44fb10.yaml new file mode 100644 index 000000000..beb6dbeb3 --- /dev/null +++ b/releasenotes/notes/disconnect-multipath-cfg-changed-637abc5ecf44fb10.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + `Bug #1921381 `_: Fix + disconnecting volumes when the use_multipath value is changed from the + connect_volume call to the disconnect_volume call. diff --git a/releasenotes/notes/wallaby-extra-prelude-d8de88e3e11a7b9f.yaml b/releasenotes/notes/wallaby-extra-prelude-d8de88e3e11a7b9f.yaml new file mode 100644 index 000000000..52fd96537 --- /dev/null +++ b/releasenotes/notes/wallaby-extra-prelude-d8de88e3e11a7b9f.yaml @@ -0,0 +1,5 @@ +--- +prelude: > + This release fixes an issue that could cause data loss when the + configuration enabling/disabling multipathing is changed on a compute + when volumes are currently attached.