Fix multipathd dependency in NVMe-oF connections

Previously, we required multipathd to be running
to perform multipathing in NVMe-oF connections
even though we were using native NVMe multipathing
(ANA).
This was because, traditionally we only supported
iSCSI and FC connections so we only needed to check
for ``multipathd`` running. The multipathing check
happened early on while fetching the connector
when we had no info what kind of connection will
be created.
With this patch, the logic to check for multipathing
is moved to when we are discovering the volume (after
mapping) and each connector provides it's own
implementation to report if it supports multipathing
or not (by default, the value is False).
Based on the values of ``multipath``, ``enforce_multipath``
and ``supports_multipath``, we decide if we want to
proceed with the connect request, log a warning or fail.

Following are some of the cases tested with iSCSI
and NVMe-oF connector:

Enable multipathing:
target_ip_address = <primary_ip>
target_secondary_ip_addresses = <secondary_ip>

Tested with:

1. iSCSI
1.1 enforce multipathing + multipathd running
a) use_multipath_for_image_xfer = True
b) enforce_multipath_for_image_xfer = True
c) multipathd running
Result: PASS

1.2 don't enforce multipathing + multipathd stopped
a) use_multipath_for_image_xfer = True
b) enforce_multipath_for_image_xfer = False
c) multipathd stopped
Result: Log Warning

1.3 enforce_multipathing + multipathd stopped
a) use_multipath_for_image_xfer = True
b) enforce_multipath_for_image_xfer = True
c) multipathd stopped
Result: Fails with BrickException

2. NVMe-TCP

NOTE: multipathd is stopped since it shouldn't affect nvme connections now

2.1 enforce multipathing + native multipathing
a) use_multipath_for_image_xfer = True
b) enforce_multipath_for_image_xfer = True
c) native multipathing = Y
Result: PASS

2.2 don't enforce multipathing + no native multipathing
a) use_multipath_for_image_xfer = True
b) enforce_multipath_for_image_xfer = False
c) native multipathing = N
Result: Log Warning

2.3 enforce_multipathing + no native multipathing
a) use_multipath_for_image_xfer = True
b) enforce_multipath_for_image_xfer = True
c) native multipathing = N
Result: Fails with BrickException

Depends-On: https://review.opendev.org/c/openstack/cinder/+/934011

Closes-Bug: #2085013
Change-Id: I61c7d7bc040633646411758c3245d8432910f766

os_brick/initiator/connectors/base.py

@@ -112,16 +112,43 @@ class BaseLinuxConnector(initiator_connector.InitiatorConnector):
     @staticmethod
     def get_connector_properties(root_helper: str, *args, **kwargs) -> dict:
         """The generic connector properties."""
-        multipath = kwargs['multipath']
-        enforce_multipath = kwargs['enforce_multipath']
-        props = {}
+        # The 'multipath' and 'enforce_multipath' values will be used by
+        # the caller to verify multipathing in connect_volume.
+        return {
+            'multipath': kwargs['multipath'],
+            'enforce_multipath': kwargs['enforce_multipath'],
+        }
 
-        props['multipath'] = (multipath and
-                              linuxscsi.LinuxSCSI.is_multipath_running(
-                                  enforce_multipath, root_helper,
-                                  execute=kwargs.get('execute')))
+    def supports_multipath(self):
+        """Generic method to report multipath support.
 
-        return props
+        Each connector, which supports multipath, should override this
+        method and provide its own implementation of checking the
+        multipath support. See implementation in iSCSI, FC or NVMe
+        connectors for reference.
+        """
+        return False
+
+    def check_multipath(self, connection_properties):
+        LOG.debug("Connection properties %s", connection_properties)
+        multipath = self.use_multipath
+        # If we are using an old cinder, it will not contain the
+        # 'enforce_multipath' key and we will default the value to False.
+        # Unfortunately, there is is no way to know which Cinder
+        # version we are using when calling get_connector_properties to
+        # keep backward compatibility.
+        enforce_multipath = connection_properties.get(
+            'enforce_multipath', False)
+
+        if not self.supports_multipath():
+            if multipath and enforce_multipath:
+                raise exception.BrickException(
+                    "Multipathing is enforced but the host doesn't "
+                    "support multipathing.")
+            if multipath and not enforce_multipath:
+                LOG.warning(
+                    "Multipathing is requested but the host "
+                    "doesn't support multipathing.")
 
     def check_valid_device(self, path: str, run_as_root: bool = True) -> bool:
         return utils.check_valid_device(self, path)

os_brick/initiator/connectors/fibre_channel.py

@@ -203,6 +203,10 @@ class FibreChannelConnector(base.BaseLinuxConnector):
                         {'props': connection_properties})
             raise exception.VolumePathsNotFound()
 
+    def supports_multipath(self):
+        return self._linuxscsi.is_multipath_running(
+            root_helper=self._root_helper)
+
     @utils.trace
     @utils.connect_volume_prepare_result
     @base.synchronized('connect_volume', external=True)
@@ -218,6 +222,7 @@ class FibreChannelConnector(base.BaseLinuxConnector):
         target_wwn - World Wide Name
         target_lun - LUN id of the volume
         """
+        self.check_multipath(connection_properties)
         device_info = {'type': 'block'}
 
         connection_properties = self._add_targets_to_connection_properties(

os_brick/initiator/connectors/iscsi.py

@@ -510,6 +510,10 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector):
                             connection_properties)})
             raise exception.VolumePathsNotFound()
 
+    def supports_multipath(self):
+        return self._linuxscsi.is_multipath_running(
+            root_helper=self._root_helper)
+
     @utils.trace
     @utils.connect_volume_prepare_result
     @base.synchronized('connect_volume', external=True)
@@ -529,6 +533,7 @@ class ISCSIConnector(base.BaseLinuxConnector, base_iscsi.BaseISCSIConnector):
         target_lun(s) - LUN id of the volume
         Note that plural keys may be used when use_multipath=True
         """
+        self.check_multipath(connection_properties)
         try:
             if self.use_multipath:
                 return self._connect_multipath_volume(connection_properties)

os_brick/initiator/connectors/nvmeof.py

@@ -644,6 +644,7 @@ class NVMeOFConnProps(object):
         self.encrypted = conn_props.get('encrypted', False)
         self.cacheable = conn_props.get('cacheable', False)
         self.discard = conn_props.get('discard', False)
+        self.enforce_multipath = conn_props.get('enforce_multipath', False)
 
         # old connection properties format
         if REPLICAS not in conn_props and NQN not in conn_props:
@@ -852,6 +853,9 @@ class NVMeOFConnector(base.BaseLinuxConnector):
 
     # #######  Connect Volume methods ########
 
+    def supports_multipath(self):
+        return self.native_multipath_supported
+
     @utils.trace
     @utils.connect_volume_prepare_result
     @base.synchronized('connect_volume', external=True)
@@ -859,6 +863,9 @@ class NVMeOFConnector(base.BaseLinuxConnector):
     def connect_volume(
             self, connection_properties: NVMeOFConnProps) -> dict[str, str]:
         """Attach and discover the volume."""
+        self.check_multipath(
+            {'enforce_multipath': getattr(
+                connection_properties, 'enforce_multipath', False)})
         try:
             if connection_properties.is_replicated is False:
                 LOG.debug('Starting non replicated connection')

os_brick/initiator/linuxscsi.py

@@ -277,8 +277,7 @@ class LinuxSCSI(executor.Executor):
         return out.strip()
 
     @staticmethod
-    def is_multipath_running(enforce_multipath,
-                             root_helper,
+    def is_multipath_running(root_helper,
                              execute=None) -> bool:
         try:
             if execute is None:
@@ -289,13 +288,9 @@ class LinuxSCSI(executor.Executor):
             # There was a bug in multipathd where it didn't return an error
             # code and just printed the error message in stdout.
             if out and out.startswith('error receiving packet'):
-                raise putils.ProcessExecutionError('', out, 1, cmd, None)
+                return False
 
-        except putils.ProcessExecutionError as err:
-            if enforce_multipath:
-                LOG.error('multipathd is not running: exit code %(err)s',
-                          {'err': err.exit_code})
-                raise
+        except putils.ProcessExecutionError:
             return False
         return True
 
@@ -384,7 +379,7 @@ class LinuxSCSI(executor.Executor):
             multipath_running = True
         else:
             multipath_running = self.is_multipath_running(
-                enforce_multipath=False, root_helper=self._root_helper)
+                root_helper=self._root_helper)
 
         for device_name in devices_names:
             dev_path = '/dev/' + device_name

os_brick/tests/initiator/connectors/test_fibre_channel.py

@@ -219,6 +219,8 @@ class FibreChannelConnectorTestCase(test_connector.ConnectorTestCase):
                     '-fc-0x1234567890123456-lun-1']
         self.assertEqual(expected, volume_paths)
 
+    @mock.patch.object(
+        base.BaseLinuxConnector, 'check_multipath', mock.MagicMock())
     @mock.patch.object(linuxscsi.LinuxSCSI, 'wait_for_rw')
     @mock.patch.object(os.path, 'exists', return_value=True)
     @mock.patch.object(os.path, 'realpath', return_value='/dev/sdb')
@@ -290,6 +292,8 @@ class FibreChannelConnectorTestCase(test_connector.ConnectorTestCase):
                           self.connector.connect_volume,
                           connection_info['data'])
 
+    @mock.patch.object(
+        base.BaseLinuxConnector, 'check_multipath', mock.MagicMock())
     @mock.patch.object(linuxscsi.LinuxSCSI, 'find_multipath_device_path')
     @mock.patch.object(linuxscsi.LinuxSCSI, 'wait_for_mpath_device')
     def _test_connect_volume_multipath(self, get_device_info_mock,
@@ -994,3 +998,9 @@ class FibreChannelConnectorTestCase(test_connector.ConnectorTestCase):
              mock.sentinel.lun2B),
         ]
         self.assertEqual(expected, res)
+
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'is_multipath_running')
+    def test_supports_multipath(self, mock_mpath_running):
+        self.connector.supports_multipath()
+        mock_mpath_running.assert_called_once_with(
+            root_helper=self.connector._root_helper)

os_brick/tests/initiator/connectors/test_iscsi.py

@@ -19,6 +19,7 @@ import ddt
 from oslo_concurrency import processutils as putils
 
 from os_brick import exception
+from os_brick.initiator.connectors import base
 from os_brick.initiator.connectors import iscsi
 from os_brick.initiator import linuxscsi
 from os_brick.initiator import utils
@@ -396,6 +397,8 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
                            'multipath_id': FAKE_SCSI_WWN}
         self.assertEqual(expected_result, result)
 
+    @mock.patch.object(
+        base.BaseLinuxConnector, 'check_multipath', mock.MagicMock())
     @mock.patch.object(iscsi.ISCSIConnector, '_cleanup_connection')
     @mock.patch.object(iscsi.ISCSIConnector, '_connect_multipath_volume')
     @mock.patch.object(iscsi.ISCSIConnector, '_connect_single_volume')
@@ -407,6 +410,8 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
         con_mp_mock.assert_called_once_with(self.CON_PROPS)
         clean_mock.assert_not_called()
 
+    @mock.patch.object(
+        base.BaseLinuxConnector, 'check_multipath', mock.MagicMock())
     @mock.patch.object(iscsi.ISCSIConnector, '_cleanup_connection')
     @mock.patch.object(iscsi.ISCSIConnector, '_connect_multipath_volume')
     @mock.patch.object(iscsi.ISCSIConnector, '_connect_single_volume')
@@ -420,6 +425,8 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
         con_mp_mock.assert_called_once_with(self.CON_PROPS)
         clean_mock.assert_called_once_with(self.CON_PROPS, force=True)
 
+    @mock.patch.object(
+        base.BaseLinuxConnector, 'check_multipath', mock.MagicMock())
     @mock.patch.object(iscsi.ISCSIConnector, '_cleanup_connection')
     @mock.patch.object(iscsi.ISCSIConnector, '_connect_multipath_volume')
     @mock.patch.object(iscsi.ISCSIConnector, '_connect_single_volume')
@@ -431,6 +438,8 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
         con_single_mock.assert_called_once_with(self.CON_PROPS)
         clean_mock.assert_not_called()
 
+    @mock.patch.object(
+        base.BaseLinuxConnector, 'check_multipath', mock.MagicMock())
     @mock.patch.object(iscsi.ISCSIConnector, '_cleanup_connection')
     @mock.patch.object(iscsi.ISCSIConnector, '_connect_multipath_volume')
     @mock.patch.object(iscsi.ISCSIConnector, '_connect_single_volume')
@@ -489,11 +498,12 @@ class ISCSIConnectorTestCase(test_connector.ConnectorTestCase):
             # Reset to run with a different transport type
             self.cmds = list()
 
+    @mock.patch.object(priv_rootwrap, 'execute', return_value=('', ''))
     @mock.patch.object(iscsi.ISCSIConnector,
                        '_run_iscsiadm_update_discoverydb')
     @mock.patch.object(os.path, 'exists', return_value=True)
     def test_iscsi_portals_with_chap_discovery(
-            self, exists, update_discoverydb):
+            self, exists, update_discoverydb, mock_exec):
         location = '10.0.2.15:3260'
         name = 'volume-00000001'
         iqn = 'iqn.2010-10.org.openstack:%s' % name
@@ -1777,3 +1787,9 @@ Setting up iSCSI targets: unused
             (mock.sentinel.portal2, mock.sentinel.iqn2, mock.sentinel.lun2B)
         ]
         self.assertListEqual(expected, res)
+
+    @mock.patch.object(linuxscsi.LinuxSCSI, 'is_multipath_running')
+    def test_supports_multipath(self, mock_mpath_running):
+        self.connector.supports_multipath()
+        mock_mpath_running.assert_called_once_with(
+            root_helper=self.connector._root_helper)

os_brick/tests/initiator/connectors/test_nvmeof.py

@@ -2215,3 +2215,8 @@ class NVMeOFConnectorTestCase(test_connector.ConnectorTestCase):
         self.connector._try_disconnect(portal)
         mock_can_disconnect.assert_called_once_with()
         mock_execute.assert_not_called()
+
+    @ddt.data(False, True)
+    def test_supports_multipath(self, ana_support):
+        self.connector.native_multipath_supported = ana_support
+        self.assertEqual(ana_support, self.connector.supports_multipath())

os_brick/tests/initiator/test_connector.py

@@ -16,6 +16,7 @@ import platform
 import sys
 from unittest import mock
 
+import ddt
 from oslo_concurrency import processutils as putils
 from oslo_service import loopingcall
 
@@ -82,6 +83,7 @@ class ConnectorUtilsTestCase(test_base.TestCase):
                  'host': host,
                  'ip': MY_IP,
                  'multipath': multipath_result,
+                 'enforce_multipath': enforce_multipath,
                  'nvme_native_multipath': False,
                  'os_type': os_type,
                  'platform': platform,
@@ -116,40 +118,25 @@ class ConnectorUtilsTestCase(test_base.TestCase):
 
     @mock.patch.object(priv_rootwrap, 'custom_execute',
                        side_effect=OSError(2))
-    @mock.patch.object(priv_rootwrap, 'execute', return_value=('', ''))
-    def test_brick_get_connector_properties_multipath(self, mock_execute,
+    def test_brick_get_connector_properties_multipath(self,
                                                       mock_custom_execute):
         self._test_brick_get_connector_properties(True, True, True)
-        mock_execute.assert_called_once_with('multipathd', 'show', 'status',
-                                             run_as_root=True,
-                                             root_helper='sudo')
         mock_custom_execute.assert_called_once_with('nvme', 'version')
 
     @mock.patch.object(priv_rootwrap, 'custom_execute',
                        side_effect=OSError(2))
-    @mock.patch.object(priv_rootwrap, 'execute',
-                       side_effect=putils.ProcessExecutionError)
-    def test_brick_get_connector_properties_fallback(self, mock_execute,
+    def test_brick_get_connector_properties_fallback(self,
                                                      mock_custom_execute):
-        self._test_brick_get_connector_properties(True, False, False)
-        mock_execute.assert_called_once_with('multipathd', 'show', 'status',
-                                             run_as_root=True,
-                                             root_helper='sudo')
+        self._test_brick_get_connector_properties(True, False, True)
         mock_custom_execute.assert_called_once_with('nvme', 'version')
 
-    @mock.patch.object(priv_rootwrap, 'execute',
-                       side_effect=putils.ProcessExecutionError)
-    def test_brick_get_connector_properties_raise(self, mock_execute):
-        self.assertRaises(putils.ProcessExecutionError,
-                          self._test_brick_get_connector_properties,
-                          True, True, None)
-
     def test_brick_connector_properties_override_hostname(self):
         override_host = 'myhostname'
         self._test_brick_get_connector_properties(False, False, False,
                                                   host=override_host)
 
 
+@ddt.ddt
 class ConnectorTestCase(test_base.TestCase):
 
     def setUp(self):
@@ -191,7 +178,11 @@ class ConnectorTestCase(test_base.TestCase):
                 'sudo', multipath=multipath,
                 enforce_multipath=enforce_multipath)
 
-            expected_props = {'multipath': True}
+            expected_props = {
+                'multipath': multipath,
+                'enforce_multipath': enforce_multipath,
+            }
+
             self.assertEqual(expected_props, props)
 
             multipath = False
@@ -200,18 +191,12 @@ class ConnectorTestCase(test_base.TestCase):
                 'sudo', multipath=multipath,
                 enforce_multipath=enforce_multipath)
 
-            expected_props = {'multipath': False}
-            self.assertEqual(expected_props, props)
+            expected_props = {
+                'multipath': multipath,
+                'enforce_multipath': enforce_multipath,
+            }
 
-        with mock.patch.object(priv_rootwrap, 'execute',
-                               side_effect=putils.ProcessExecutionError):
-            multipath = True
-            enforce_multipath = True
-            self.assertRaises(
-                putils.ProcessExecutionError,
-                base.BaseLinuxConnector.get_connector_properties,
-                'sudo', multipath=multipath,
-                enforce_multipath=enforce_multipath)
+            self.assertEqual(expected_props, props)
 
     @mock.patch('sys.platform', 'win32')
     def test_get_connector_mapping_win32(self):
@@ -304,3 +289,39 @@ class ConnectorTestCase(test_base.TestCase):
         with mock.patch.object(self.connector, '_execute',
                                side_effect=putils.ProcessExecutionError):
             self.assertFalse(self.connector.check_valid_device('/dev'))
+
+    @ddt.data(
+        (False, False, False),
+        (False, False, True),
+        (False, True, False),
+        (False, True, True),
+        (True, False, False),
+        (True, False, True),
+        (True, True, False),
+        (True, True, True),
+    )
+    @ddt.unpack
+    @mock.patch.object(base, 'LOG')
+    @mock.patch.object(base.BaseLinuxConnector, 'supports_multipath')
+    def test_check_multipath(
+            self, mpath, enforce_mpath, supports_mpath, mock_supports_mpath,
+            mock_log):
+        self.connector = fake.FakeConnector(None)
+        fake_conn_props = self.fake_connection()
+        self.connector.use_multipath = mpath
+        mock_supports_mpath.return_value = supports_mpath
+        fake_conn_props['data']['enforce_multipath'] = enforce_mpath
+        if mpath and enforce_mpath and not supports_mpath:
+            self.assertRaises(
+                exception.BrickException,
+                self.connector.check_multipath,
+                fake_conn_props['data']
+            )
+        else:
+            self.connector.check_multipath(fake_conn_props['data'])
+            if mpath and not enforce_mpath and not supports_mpath:
+                mock_log.warning.assert_called_once_with(
+                    "Multipathing is requested but the host "
+                    "doesn't support multipathing."
+                )
+        mock_supports_mpath.assert_called_once_with()

os_brick/tests/initiator/test_linuxscsi.py

@@ -1020,34 +1020,21 @@ loop0                                     0"""
 
     @mock.patch('os_brick.privileged.rootwrap.execute', return_value=('', ''))
     def test_is_multipath_running(self, mock_exec):
-        res = linuxscsi.LinuxSCSI.is_multipath_running(False, None, mock_exec)
+        res = linuxscsi.LinuxSCSI.is_multipath_running(None, mock_exec)
         self.assertTrue(res)
         mock_exec.assert_called_once_with(
             'multipathd', 'show', 'status', run_as_root=True, root_helper=None)
 
-    @mock.patch.object(linuxscsi, 'LOG')
-    @mock.patch('os_brick.privileged.rootwrap.execute')
-    def test_is_multipath_running_failure(
-        self, mock_exec, mock_log
-    ):
-        mock_exec.side_effect = putils.ProcessExecutionError()
-        self.assertRaises(putils.ProcessExecutionError,
-                          linuxscsi.LinuxSCSI.is_multipath_running,
-                          True, None, mock_exec)
-        mock_log.error.assert_called_once()
-
     @mock.patch.object(linuxscsi, 'LOG')
     @mock.patch('os_brick.privileged.rootwrap.execute')
     def test_is_multipath_running_failure_exit_code_0(
         self, mock_exec, mock_log
     ):
         mock_exec.return_value = ('error receiving packet', '')
-        self.assertRaises(putils.ProcessExecutionError,
-                          linuxscsi.LinuxSCSI.is_multipath_running,
-                          True, None, mock_exec)
+        res = linuxscsi.LinuxSCSI.is_multipath_running(None, mock_exec)
         mock_exec.assert_called_once_with(
             'multipathd', 'show', 'status', run_as_root=True, root_helper=None)
-        mock_log.error.assert_called_once()
+        self.assertFalse(res)
 
     @mock.patch.object(linuxscsi, 'LOG')
     @mock.patch('os_brick.privileged.rootwrap.execute')
@@ -1055,7 +1042,7 @@ loop0                                     0"""
         self, mock_exec, mock_log
     ):
         mock_exec.side_effect = putils.ProcessExecutionError()
-        res = linuxscsi.LinuxSCSI.is_multipath_running(False, None, mock_exec)
+        res = linuxscsi.LinuxSCSI.is_multipath_running(None, mock_exec)
         mock_exec.assert_called_once_with(
             'multipathd', 'show', 'status', run_as_root=True, root_helper=None)
         self.assertFalse(res)
@@ -1067,7 +1054,7 @@ loop0                                     0"""
         self, mock_exec, mock_log
     ):
         mock_exec.return_value = ('error receiving packet', '')
-        res = linuxscsi.LinuxSCSI.is_multipath_running(False, None, mock_exec)
+        res = linuxscsi.LinuxSCSI.is_multipath_running(None, mock_exec)
         mock_exec.assert_called_once_with(
             'multipathd', 'show', 'status', run_as_root=True, root_helper=None)
         self.assertFalse(res)

releasenotes/notes/fix-multipathd-dependency-nvmeof-d61187f1ab3808c2.yaml

@@ -0,0 +1,9 @@
+---
+fixes:
+  - |
+    NVMe-oF connector `bug #2085013
+    <https://bugs.launchpad.net/os-brick/+bug/2085013>`_ : Removed
+    ``multipathd`` dependency for NVMe-oF connections.
+    Previously, multipathd had to be installed to leverage native
+    NVMe multipathing (ANA) but now that dependency is removed.
+