From 8070ac3bd903a443fbfd02abf8b0554d5d05cac1 Mon Sep 17 00:00:00 2001 From: Rajat Dhasmana Date: Tue, 21 Mar 2023 01:30:53 +0000 Subject: [PATCH] Fix iSCSI disconnect_volume when flush fails The purpose of providing force=True and ignore_errors=True is to tell os-brick that we want to disconnect the volume even if flush fails (force) and not raise any exceptions (ignore_errors). Currently, in an iSCSI multipath environment, disconnect_volume can fail when both parameters are True. The current flow when disconnecting an iSCSI volume is that if flushing a multipath device fails, we manually remove the device, logout from the target portals, and try the flush again. There are two problems here: 1) code problem: The second flush is not wrapped by ExceptionChainer. This causes it to raise the exception immediately after flush fails irrespective of the value of the ignore_errors flag. 2) conceptual problem: In this situation, there is no point in making the second flush attempt. Instead, we should just remove the multipath map from multipathd monitoring since we have already removed the paths manually. This patch fixes the conceptual problem, as we don't make a second flush call and ignore any errors on the execution ``multipathd del map`` thereby also fixing the code problem. Closes-Bug: #2012251 Change-Id: I828911495a2de550ea997e6f51cc039a7b7fa8cd --- os_brick/initiator/connectors/iscsi.py | 12 ++++++------ os_brick/initiator/linuxscsi.py | 9 +++++++++ os_brick/tests/initiator/connectors/test_iscsi.py | 9 +++++---- os_brick/tests/initiator/test_linuxscsi.py | 5 +++++ .../notes/fix-bug-2012251-381ad16653472c50.yaml | 6 ++++++ 5 files changed, 31 insertions(+), 10 deletions(-) create mode 100644 releasenotes/notes/fix-bug-2012251-381ad16653472c50.yaml diff --git a/os_brick/initiator/connectors/iscsi.py b/os_brick/initiator/connectors/iscsi.py index c59196eab..b4ebce4d6 100644 --- a/os_brick/initiator/connectors/iscsi.py +++ b/os_brick/initiator/connectors/iscsi.py @@ -943,13 +943,13 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector): self._disconnect_connection(connection_properties, disconnect, force, exc) # type:ignore - # If flushing the multipath failed before, try now after we have - # removed the devices and we may have even logged off (only reaches - # here with multipath_name if force=True). + # If flushing the multipath failed before, remove the multipath device + # map from multipathd monitoring (only reaches here with multipath_name + # if force=True). if multipath_name: - LOG.debug('Flushing again multipath %s now that we removed the ' - 'devices.', multipath_name) - self._linuxscsi.flush_multipath_device(multipath_name) + LOG.debug('Removing multipath device map %s to stop multipathd ' + 'from monitoring the device.', multipath_name) + self._linuxscsi.multipath_del_map(multipath_name) if exc: # type: ignore LOG.warning('There were errors removing %s, leftovers may remain ' diff --git a/os_brick/initiator/linuxscsi.py b/os_brick/initiator/linuxscsi.py index 298f0324a..a7dcb5620 100644 --- a/os_brick/initiator/linuxscsi.py +++ b/os_brick/initiator/linuxscsi.py @@ -759,3 +759,12 @@ class LinuxSCSI(executor.Executor): check_exit_code=False, root_helper=self._root_helper) return stdout.strip() == 'ok' + + def multipath_del_map(self, mpath_device): + """Remove a map from multipathd for monitoring.""" + stdout, stderr = self._execute('multipathd', 'del', 'map', + mpath_device, + run_as_root=True, timeout=5, + check_exit_code=False, + root_helper=self._root_helper) + return stdout.strip() == 'ok' diff --git a/os_brick/tests/initiator/connectors/test_iscsi.py b/os_brick/tests/initiator/connectors/test_iscsi.py index 00109bc82..a7e312450 100644 --- a/os_brick/tests/initiator/connectors/test_iscsi.py +++ b/os_brick/tests/initiator/connectors/test_iscsi.py @@ -721,11 +721,12 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): mock.Mock(return_value=True)) @mock.patch.object(iscsi.ISCSIConnector, '_disconnect_connection') @mock.patch.object(iscsi.ISCSIConnector, '_get_connection_devices') - @mock.patch.object(linuxscsi.LinuxSCSI, 'flush_multipath_device') @mock.patch.object(linuxscsi.LinuxSCSI, 'remove_connection', return_value=mock.sentinel.mp_name) - def test_cleanup_connection_force_failure(self, remove_mock, flush_mock, - con_devs_mock, discon_mock): + @mock.patch.object(linuxscsi.LinuxSCSI, 'multipath_del_map') + def test_cleanup_connection_force_failure(self, remove_map_mock, + remove_mock, con_devs_mock, + discon_mock): # Return an ordered dicts instead of normal dict for discon_mock.assert con_devs_mock.return_value = collections.OrderedDict(( @@ -749,7 +750,7 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase): self.CON_PROPS, [('ip1:port1', 'tgt1'), ('ip3:port3', 'tgt3')], mock.sentinel.force, mock.ANY) - flush_mock.assert_called_once_with(mock.sentinel.mp_name) + remove_map_mock.assert_called_once_with(mock.sentinel.mp_name) def test_cleanup_connection_no_data_discoverydb(self): self.connector.use_multipath = True diff --git a/os_brick/tests/initiator/test_linuxscsi.py b/os_brick/tests/initiator/test_linuxscsi.py index 89899b728..4dc7a8f6b 100644 --- a/os_brick/tests/initiator/test_linuxscsi.py +++ b/os_brick/tests/initiator/test_linuxscsi.py @@ -1299,6 +1299,11 @@ loop0 0""" self.linuxscsi.multipath_del_path('/dev/sda') self.assertEqual(['multipathd del path /dev/sda'], self.cmds) + @ddt.data('mpatha', '3600140598d2e10957eb444ab09805555') + def test_multipath_del_map(self, mpath_device): + self.linuxscsi.multipath_del_map(mpath_device) + self.assertEqual(['multipathd del map %s' % (mpath_device)], self.cmds) + @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/fix-bug-2012251-381ad16653472c50.yaml b/releasenotes/notes/fix-bug-2012251-381ad16653472c50.yaml new file mode 100644 index 000000000..1ae1e4b3b --- /dev/null +++ b/releasenotes/notes/fix-bug-2012251-381ad16653472c50.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + `Bug #2012251 `_: Fixed + issue when disconnecting iSCSI volume when ``force`` and ``ignore_errors`` + are set to ``True`` and flushing multipath device fails.