diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py index 2da80160f..c7b7e71fb 100644 --- a/ironic_python_agent/hardware.py +++ b/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): diff --git a/ironic_python_agent/tests/unit/test_hardware.py b/ironic_python_agent/tests/unit/test_hardware.py index c8c96e877..ed1877bc8 100644 --- a/ironic_python_agent/tests/unit/test_hardware.py +++ b/ironic_python_agent/tests/unit/test_hardware.py @@ -5246,6 +5246,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] @@ -5283,13 +5323,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, diff --git a/releasenotes/notes/multipath_error_handling_improvement-1669d0de4bfdbe95.yaml b/releasenotes/notes/multipath_error_handling_improvement-1669d0de4bfdbe95.yaml new file mode 100644 index 000000000..c52d99e62 --- /dev/null +++ b/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).