Merge "improve multipathd error handling"

ironic_python_agent/hardware.py

@@ -213,21 +213,25 @@ def _enable_multipath():
         # NOTE(TheJulia): Testing locally, a prior running multipathd, the
         # explicit multipathd start just appears to silently exit with a
         # result code of 0.
-        il_utils.execute('multipathd')
+        # NOTE(rozzix): This could cause an OS error:
+        # "process is already running failed to create pid file" depending on
+        # the multipathd version in case multipathd is already running.
+        # I suggest muting the OS error in addition to the execution error.
+        il_utils.try_execute('multipathd')
         # This is mainly to get the system to actually do the needful and
         # identify/enumerate paths by combining what it can detect and what
         # it already knows. This may be useful, and in theory this should be
         # logged in the IPA log should it be needed.
         il_utils.execute('multipath', '-ll')
-        return True
     except FileNotFoundError as e:
         LOG.warning('Attempted to determine if multipath tools were present. '
                     'Not detected. Error recorded: %s', e)
         return False
-    except processutils.ProcessExecutionError as e:
+    except (processutils.ProcessExecutionError, OSError) as e:
         LOG.warning('Attempted to invoke multipath utilities, but we '
                     'encountered an error: %s', e)
         return False
+    return True
 
 
 def _get_multipath_parent_device(device):

ironic_python_agent/tests/unit/test_hardware.py

@@ -5350,6 +5350,46 @@ class TestMultipathEnabled(base.IronicAgentTest):
             mock.call('multipath', '-ll'),
         ])
 
+    @mock.patch.object(os.path, 'isfile', autospec=True)
+    def test_enable_multipath_already_running(self,
+                                              mock_isfile,
+                                              mocked_execute):
+        mock_isfile.side_effect = [True, True]
+        mocked_execute.side_effect = [
+            ('', ''),
+            ('', ''),
+            (OSError),
+            ('', ''),
+        ]
+        self.assertTrue(hardware._enable_multipath())
+        self.assertEqual(4, mocked_execute.call_count)
+        mocked_execute.assert_has_calls([
+            mock.call('modprobe', 'dm_multipath'),
+            mock.call('modprobe', 'multipath'),
+            mock.call('multipathd'),
+            mock.call('multipath', '-ll'),
+        ])
+
+    @mock.patch.object(os.path, 'isfile', autospec=True)
+    def test_enable_multipath_ll_fails(self,
+                                       mock_isfile,
+                                       mocked_execute):
+        mock_isfile.side_effect = [True, True]
+        mocked_execute.side_effect = [
+            ('', ''),
+            ('', ''),
+            ('', ''),
+            (OSError),
+        ]
+        self.assertFalse(hardware._enable_multipath())
+        self.assertEqual(4, mocked_execute.call_count)
+        mocked_execute.assert_has_calls([
+            mock.call('modprobe', 'dm_multipath'),
+            mock.call('modprobe', 'multipath'),
+            mock.call('multipathd'),
+            mock.call('multipath', '-ll'),
+        ])
+
     @mock.patch.object(os.path, 'isfile', autospec=True)
     def test_enable_multipath_mpathconf(self, mock_isfile, mocked_execute):
         mock_isfile.side_effect = [True, False]
@@ -5387,13 +5427,15 @@ class TestMultipathEnabled(base.IronicAgentTest):
         ])
 
     @mock.patch.object(hardware, '_load_multipath_modules', autospec=True)
+    @mock.patch.object(il_utils, 'try_execute', autospec=True)
     def test_enable_multipath_not_found_mpath_config(self,
+                                                     mock_try_exec,
                                                      mock_modules,
                                                      mocked_execute):
-        mocked_execute.side_effect = FileNotFoundError()
+        mock_modules.side_effect = FileNotFoundError()
         self.assertFalse(hardware._enable_multipath())
-        self.assertEqual(1, mocked_execute.call_count)
         self.assertEqual(1, mock_modules.call_count)
+        self.assertEqual(0, mock_try_exec.call_count)
 
     @mock.patch.object(hardware, '_load_multipath_modules', autospec=True)
     def test_enable_multipath_lacking_support(self,

releasenotes/notes/multipath_error_handling_improvement-1669d0de4bfdbe95.yaml

@@ -0,0 +1,20 @@
+---
+fixes:
+  - |
+    The error handling of the multipathd service startup/discovery process.
+    IPA handles both scenario when the multipathd service is already started
+    and the scenario when the service has not been started and in the second
+    scenario IPA will try to start the service. IPA is not pre checking whether
+    multipathd is running already or not, it will start the multipathd service
+    even if it is already running and expects 0 error code . It has been
+    noticed that with certain combinations of Linux distros and multipathd
+    versions the error code is not 0 when IPA tries to start multipathd in
+    case an instance of multipathd is already running.
+    When the expected return code is not 0 an exception will be thrown and that
+    will cause the multipath device discovery to terminate prematurely and
+    if the selected root device is a multipath device then IPA won't be
+    able to provision.
+    This fix discards the exception that is caused by the non 0 error code
+    returned by the multipathd startup process. In case there is a genuine
+    issue with the multipath service, that would be caught when the actual
+    multipath device listing command is executed (multipath -ll).