From 111b3931a2db1d5be4ebe704bf26c34fa9408483 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Wed, 1 Mar 2023 13:08:16 +0100 Subject: [PATCH] Support force disconnect for FC This patch adds support for the force and ignore_errors on the disconnect_volume of the FC connector like we have in the iSCSI connector. Related-Bug: #2004555 Change-Id: Ia74ecfba03ba23de9d30eb33706245a7f85e1d66 (cherry picked from commit 570df49db9de3030e658619138588b836c007f8c) Conflicts: os_brick/initiator/connectors/fibre_channel.py --- .../initiator/connectors/fibre_channel.py | 35 +++-- .../connectors/fibre_channel_s390x.py | 11 +- os_brick/initiator/linuxscsi.py | 18 +++ .../connectors/test_fibre_channel.py | 129 +++++++++++++++++- .../connectors/test_fibre_channel_s390x.py | 25 +++- os_brick/tests/initiator/test_linuxscsi.py | 58 ++++++++ .../fc-force-disconnect-1a33cf46c233dd04.yaml | 5 + 7 files changed, 266 insertions(+), 15 deletions(-) create mode 100644 releasenotes/notes/fc-force-disconnect-1a33cf46c233dd04.yaml diff --git a/os_brick/initiator/connectors/fibre_channel.py b/os_brick/initiator/connectors/fibre_channel.py index 8e85338..c3119bc 100644 --- a/os_brick/initiator/connectors/fibre_channel.py +++ b/os_brick/initiator/connectors/fibre_channel.py @@ -332,7 +332,7 @@ class FibreChannelConnector(base.BaseLinuxConnector): target_wwn - World Wide Name target_lun - LUN id of the volume """ - + exc = exception.ExceptionChainer() devices = [] wwn = None @@ -348,14 +348,30 @@ class FibreChannelConnector(base.BaseLinuxConnector): wwn = self._linuxscsi.get_scsi_wwn(path) mpath_path = self._linuxscsi.find_multipath_device_path(wwn) if mpath_path: - self._linuxscsi.flush_multipath_device(mpath_path) + with exc.context(force, 'Flushing %s failed', mpath_path): + self._linuxscsi.flush_multipath_device(mpath_path) dev_info = self._linuxscsi.get_device_info(real_path) devices.append(dev_info) + # If flush failed, then remove it forcefully since force=True + if mpath_path and exc: + with exc.context(force, 'Removing multipath %s failed', + mpath_path): + mpath_name = os.path.basename(os.path.realpath(mpath_path)) + self._linuxscsi.multipath_del_map(mpath_name) + LOG.debug("devices to remove = %s", devices) - self._remove_devices(connection_properties, devices, device_info) + self._remove_devices(connection_properties, devices, device_info, + force, exc) + + if exc: # type: ignore + LOG.warning('There were errors removing %s, leftovers may remain ' + 'in the system', volume_paths) + if not ignore_errors: + raise exc # type: ignore - def _remove_devices(self, connection_properties, devices, device_info): + def _remove_devices(self, connection_properties, devices, device_info, + force, exc): # There may have been more than 1 device mounted # by the kernel for this volume. We have to remove # all of them @@ -370,11 +386,12 @@ class FibreChannelConnector(base.BaseLinuxConnector): # paths, whereas for multipaths we have multiple link formats. was_multipath = '/pci-' not in path_used and was_symlink for device in devices: - device_path = device['device'] - flush = self._linuxscsi.requires_flush(device_path, - path_used, - was_multipath) - self._linuxscsi.remove_scsi_device(device_path, flush=flush) + with exc.context(force, 'Removing device %s failed', device): + device_path = device['device'] + flush = self._linuxscsi.requires_flush(device_path, + path_used, + was_multipath) + self._linuxscsi.remove_scsi_device(device_path, flush=flush) def _get_pci_num(self, hba): # NOTE(walter-boring) diff --git a/os_brick/initiator/connectors/fibre_channel_s390x.py b/os_brick/initiator/connectors/fibre_channel_s390x.py index 57f72d3..f44ca19 100644 --- a/os_brick/initiator/connectors/fibre_channel_s390x.py +++ b/os_brick/initiator/connectors/fibre_channel_s390x.py @@ -86,12 +86,15 @@ class FibreChannelConnectorS390X(fibre_channel.FibreChannelConnector): ] return host_device - def _remove_devices(self, connection_properties, devices, device_info): + def _remove_devices(self, connection_properties, devices, device_info, + force, exc): hbas = self._linuxfc.get_fc_hbas_info() targets = connection_properties['targets'] possible_devs = self._get_possible_devices(hbas, targets) for platform, pci_num, target_wwn, lun in possible_devs: target_lun = self._get_lun_string(lun) - self._linuxfc.deconfigure_scsi_device(pci_num, - target_wwn, - target_lun) + with exc.context(force, 'Removing device %s:%s:%s failed', + pci_num, target_wwn, target_lun): + self._linuxfc.deconfigure_scsi_device(pci_num, + target_wwn, + target_lun) diff --git a/os_brick/initiator/linuxscsi.py b/os_brick/initiator/linuxscsi.py index ddbaf02..3adebb9 100644 --- a/os_brick/initiator/linuxscsi.py +++ b/os_brick/initiator/linuxscsi.py @@ -740,3 +740,21 @@ class LinuxSCSI(executor.Executor): check_exit_code=False, root_helper=self._root_helper) return stdout.strip() == 'ok' + + @utils.retry((putils.ProcessExecutionError, exception.BrickException), + retries=3) + def multipath_del_map(self, mpath): + """Stop monitoring a multipath given its device name (eg: dm-7). + + Method ensures that the multipath device mapper actually dissapears + from sysfs. + """ + map_name = self.get_dm_name(mpath) + if map_name: + self._execute('multipathd', 'del', 'map', map_name, + run_as_root=True, timeout=5, + root_helper=self._root_helper) + + if map_name and self.get_dm_name(mpath): + raise exception.BrickException("Multipath doesn't go away") + LOG.debug('Multipath %s no longer present', mpath) diff --git a/os_brick/tests/initiator/connectors/test_fibre_channel.py b/os_brick/tests/initiator/connectors/test_fibre_channel.py index 063058e..163dec6 100644 --- a/os_brick/tests/initiator/connectors/test_fibre_channel.py +++ b/os_brick/tests/initiator/connectors/test_fibre_channel.py @@ -743,10 +743,13 @@ class FibreChannelConnectorTestCase(test_connector.ConnectorTestCase): @mock.patch('os_brick.utils.get_dev_path') def test__remove_devices(self, path_used, was_multipath, get_dev_path_mock, flush_mock, remove_mock): + exc = exception.ExceptionChainer() get_dev_path_mock.return_value = path_used self.connector._remove_devices(mock.sentinel.con_props, [{'device': '/dev/sda'}], - mock.sentinel.device_info) + mock.sentinel.device_info, + force=False, exc=exc) + self.assertFalse(bool(exc)) get_dev_path_mock.assert_called_once_with(mock.sentinel.con_props, mock.sentinel.device_info) flush_mock.assert_called_once_with('/dev/sda', path_used, @@ -754,6 +757,39 @@ class FibreChannelConnectorTestCase(test_connector.ConnectorTestCase): remove_mock.assert_called_once_with('/dev/sda', flush=flush_mock.return_value) + @ddt.data(('/dev/mapper/', True), + ('/dev/mapper/mpath0', True), + # Check real devices are properly detected as non multipaths + ('/dev/sda', False), + ('/dev/disk/by-path/pci-1-fc-1-lun-1', False)) + @ddt.unpack + @mock.patch('os_brick.initiator.linuxscsi.LinuxSCSI.remove_scsi_device') + @mock.patch('os_brick.initiator.linuxscsi.LinuxSCSI.requires_flush') + @mock.patch('os_brick.utils.get_dev_path') + def test__remove_devices_fails(self, path_used, was_multipath, + get_dev_path_mock, flush_mock, remove_mock): + exc = exception.ExceptionChainer() + get_dev_path_mock.return_value = path_used + remove_mock.side_effect = Exception + self.connector._remove_devices(mock.sentinel.con_props, + [{'device': '/dev/sda'}, + {'device': '/dev/sdb'}], + mock.sentinel.device_info, + force=True, exc=exc) + self.assertTrue(bool(exc)) + get_dev_path_mock.assert_called_once_with(mock.sentinel.con_props, + mock.sentinel.device_info) + + expect_flush = [mock.call('/dev/sda', path_used, was_multipath), + mock.call('/dev/sdb', path_used, was_multipath)] + self.assertEqual(len(expect_flush), flush_mock.call_count) + flush_mock.assert_has_calls(expect_flush) + + expect_remove = [mock.call('/dev/sda', flush=flush_mock.return_value), + mock.call('/dev/sdb', flush=flush_mock.return_value)] + self.assertEqual(len(expect_remove), remove_mock.call_count) + remove_mock.assert_has_calls(expect_remove) + @mock.patch.object(linuxscsi.LinuxSCSI, 'find_multipath_device') @mock.patch.object(linuxscsi.LinuxSCSI, 'wait_for_rw') @mock.patch.object(os.path, 'exists', return_value=True) @@ -815,3 +851,94 @@ class FibreChannelConnectorTestCase(test_connector.ConnectorTestCase): 'tee -a /sys/block/sdc/device/delete', ] self.assertEqual(expected_commands, self.cmds) + + @ddt.data((False, Exception), (True, Exception), (False, None)) + @ddt.unpack + @mock.patch.object(fibre_channel.FibreChannelConnector, '_remove_devices') + @mock.patch.object(linuxscsi.LinuxSCSI, 'multipath_del_map') + @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_multipath_device') + @mock.patch.object(linuxscsi.LinuxSCSI, 'wait_for_rw') + @mock.patch.object(os.path, 'exists', return_value=True) + @mock.patch.object(os.path, 'realpath') + @mock.patch.object(linuxfc.LinuxFibreChannel, 'get_fc_hbas') + @mock.patch.object(linuxfc.LinuxFibreChannel, 'get_fc_hbas_info') + @mock.patch.object(linuxscsi.LinuxSCSI, 'get_scsi_wwn') + @mock.patch.object(linuxscsi.LinuxSCSI, 'get_device_info') + @mock.patch.object(linuxscsi.LinuxSCSI, 'find_multipath_device_path') + @mock.patch.object(base.BaseLinuxConnector, 'check_valid_device') + def test_disconnect_volume_fails(self, ignore_exc, side_effect, + check_valid_device_mock, + find_mp_device_path_mock, + get_device_info_mock, + get_scsi_wwn_mock, + get_fc_hbas_info_mock, + get_fc_hbas_mock, + realpath_mock, + exists_mock, + wait_for_rw_mock, + flush_mock, + del_map_mock, + remove_mock): + + flush_mock.side_effect = side_effect + del_map_mock.side_effect = side_effect + + check_valid_device_mock.return_value = True + self.connector.use_multipath = True + get_fc_hbas_mock.side_effect = self.fake_get_fc_hbas + get_fc_hbas_info_mock.side_effect = self.fake_get_fc_hbas_info + + wwn = '360002ac00000000000000b860000741c' + multipath_devname = f'/dev/disk/by-id/dm-uuid-mpath-{wwn}' + realpath_mock.return_value = '/dev/dm-1' + devices = {"device": multipath_devname, + "id": wwn, + "devices": [{'device': '/dev/sdb', + 'address': '1:0:0:1', + 'host': 1, 'channel': 0, + 'id': 0, 'lun': 1}, + {'device': '/dev/sdc', + 'address': '1:0:0:2', + 'host': 1, 'channel': 0, + 'id': 0, 'lun': 1}]} + get_device_info_mock.side_effect = devices['devices'] + get_scsi_wwn_mock.return_value = wwn + + location = '10.0.2.15:3260' + name = 'volume-00000001' + vol = {'id': 1, 'name': name} + initiator_wwn = ['1234567890123456', '1234567890123457'] + + find_mp_device_path_mock.return_value = '/dev/mapper/mpatha' + + connection_info = self.fibrechan_connection(vol, location, + initiator_wwn) + if side_effect and not ignore_exc: + self.assertRaises(exception.ExceptionChainer, + self.connector.disconnect_volume, + connection_info['data'], devices['devices'][0], + force=True, ignore_errors=ignore_exc) + else: + self.connector.disconnect_volume(connection_info['data'], + devices['devices'][0], + force=True, + ignore_errors=ignore_exc) + + flush_mock.assert_called_once_with( + find_mp_device_path_mock.return_value) + + expected = [ + mock.call(f'/dev/disk/by-path/pci-0000:05:00.2-fc-0x{wwn}-lun-1') + for wwn in initiator_wwn] + if side_effect: + del_map_mock.assert_called_once_with('dm-1') + expected.append(mock.call(find_mp_device_path_mock.return_value)) + else: + del_map_mock.assert_not_called() + + remove_mock.assert_called_once_with(connection_info['data'], + devices['devices'], + devices['devices'][0], + True, mock.ANY) + self.assertEqual(len(expected), realpath_mock.call_count) + realpath_mock.assert_has_calls(expected) diff --git a/os_brick/tests/initiator/connectors/test_fibre_channel_s390x.py b/os_brick/tests/initiator/connectors/test_fibre_channel_s390x.py index aeb250b..4a7c654 100644 --- a/os_brick/tests/initiator/connectors/test_fibre_channel_s390x.py +++ b/os_brick/tests/initiator/connectors/test_fibre_channel_s390x.py @@ -13,6 +13,7 @@ # under the License. from unittest import mock +from os_brick import exception from os_brick.initiator.connectors import fibre_channel_s390x from os_brick.initiator import linuxfc from os_brick.tests.initiator import test_connector @@ -66,10 +67,32 @@ class FibreChannelConnectorS390XTestCase(test_connector.ConnectorTestCase): 'deconfigure_scsi_device') def test_remove_devices(self, mock_deconfigure_scsi_device, mock_get_fc_hbas_info, mock_get_possible_devices): + exc = exception.ExceptionChainer() connection_properties = {'targets': [5, 2]} self.connector._remove_devices(connection_properties, devices=None, - device_info=None) + device_info=None, force=False, exc=exc) mock_deconfigure_scsi_device.assert_called_with(3, 5, "0x0002000000000000") mock_get_fc_hbas_info.assert_called_once_with() mock_get_possible_devices.assert_called_once_with([], [5, 2]) + self.assertFalse(bool(exc)) + + @mock.patch.object(fibre_channel_s390x.FibreChannelConnectorS390X, + '_get_possible_devices', return_value=[('', 3, 5, 2), ]) + @mock.patch.object(linuxfc.LinuxFibreChannelS390X, 'get_fc_hbas_info', + return_value=[]) + @mock.patch.object(linuxfc.LinuxFibreChannelS390X, + 'deconfigure_scsi_device') + def test_remove_devices_force(self, mock_deconfigure_scsi_device, + mock_get_fc_hbas_info, + mock_get_possible_devices): + exc = exception.ExceptionChainer() + mock_deconfigure_scsi_device.side_effect = Exception + connection_properties = {'targets': [5, 2]} + self.connector._remove_devices(connection_properties, devices=None, + device_info=None, force=True, exc=exc) + mock_deconfigure_scsi_device.assert_called_with(3, 5, + "0x0002000000000000") + mock_get_fc_hbas_info.assert_called_once_with() + mock_get_possible_devices.assert_called_once_with([], [5, 2]) + self.assertTrue(bool(exc)) diff --git a/os_brick/tests/initiator/test_linuxscsi.py b/os_brick/tests/initiator/test_linuxscsi.py index 87aacc5..8bd7f0e 100644 --- a/os_brick/tests/initiator/test_linuxscsi.py +++ b/os_brick/tests/initiator/test_linuxscsi.py @@ -1231,6 +1231,64 @@ loop0 0""" self.linuxscsi.multipath_del_path('/dev/sda') self.assertEqual(['multipathd del path /dev/sda'], self.cmds) + @mock.patch.object(linuxscsi.LinuxSCSI, 'get_dm_name', return_value=None) + def test_multipath_del_map_not_present(self, name_mock): + self.linuxscsi.multipath_del_map('dm-7') + self.assertEqual([], self.cmds) + name_mock.assert_called_once_with('dm-7') + + @mock.patch.object(linuxscsi.LinuxSCSI, '_execute') + @mock.patch.object(linuxscsi.LinuxSCSI, 'get_dm_name', return_value=None) + def test_multipath_del_map(self, name_mock, exec_mock): + exec_mock.side_effect = [putils.ProcessExecutionError, None] + + mpath_name = '3600d0230000000000e13955cc3757800' + name_mock.side_effect = [mpath_name, mpath_name, None] + self.linuxscsi.multipath_del_map('dm-7') + + self.assertEqual(2, exec_mock.call_count) + exec_mock.assert_has_calls( + [mock.call('multipathd', 'del', 'map', mpath_name, + run_as_root=True, timeout=5, + root_helper=self.linuxscsi._root_helper)] * 2) + self.assertEqual(3, name_mock.call_count) + name_mock.assert_has_calls([mock.call('dm-7')] * 3) + + @mock.patch.object(linuxscsi.LinuxSCSI, '_execute') + @mock.patch.object(linuxscsi.LinuxSCSI, 'get_dm_name') + def test_multipath_del_map_retries_cmd_fails(self, name_mock, exec_mock): + exec_mock.side_effect = putils.ProcessExecutionError + mpath_name = '3600d0230000000000e13955cc3757800' + name_mock.return_value = mpath_name + self.assertRaises(putils.ProcessExecutionError, + self.linuxscsi.multipath_del_map, 'dm-7') + + self.assertEqual(3, exec_mock.call_count) + exec_mock.assert_has_calls( + [mock.call('multipathd', 'del', 'map', mpath_name, + run_as_root=True, timeout=5, + root_helper=self.linuxscsi._root_helper)] * 3) + + self.assertEqual(3, name_mock.call_count) + name_mock.assert_has_calls([mock.call('dm-7')] * 3) + + @mock.patch.object(linuxscsi.LinuxSCSI, '_execute') + @mock.patch.object(linuxscsi.LinuxSCSI, 'get_dm_name') + def test_multipath_del_map_retries_remains(self, name_mock, exec_mock): + mpath_name = '3600d0230000000000e13955cc3757800' + name_mock.return_value = mpath_name + self.assertRaises(exception.BrickException, + self.linuxscsi.multipath_del_map, 'dm-7') + + self.assertEqual(3, exec_mock.call_count) + exec_mock.assert_has_calls( + [mock.call('multipathd', 'del', 'map', mpath_name, + run_as_root=True, timeout=5, + root_helper=self.linuxscsi._root_helper)] * 3) + + self.assertEqual(6, name_mock.call_count) + name_mock.assert_has_calls([mock.call('dm-7')] * 6) + @ddt.data(('/dev/sda', '/dev/sda', False, True, None), # This checks that we ignore the was_multipath parameter if it # doesn't make sense (because the used path is the one we are diff --git a/releasenotes/notes/fc-force-disconnect-1a33cf46c233dd04.yaml b/releasenotes/notes/fc-force-disconnect-1a33cf46c233dd04.yaml new file mode 100644 index 0000000..07bc577 --- /dev/null +++ b/releasenotes/notes/fc-force-disconnect-1a33cf46c233dd04.yaml @@ -0,0 +1,5 @@ +--- +features: + - | + FC connector: Added support to ``force`` and ``ignore_errors`` parameters + on ``disconnect_volume`` method. -- 2.38.1