From 13537db293543db9449f54d7b2a85e06b8c1e1bb Mon Sep 17 00:00:00 2001 From: Adam Rozman Date: 2023年10月13日 11:31:25 +0300 Subject: [PATCH] improve multipathd error handling This commit: - Adds the ability to ignore inconsequential OS error caused by starting the multipathd service when an instance of the service is already running. Related launchpad issue https://bugs.launchpad.net/ironic-python-agent/+bug/2031092 Change-Id: Iebf486915bfdc2546451e6b38a450b4c241e43a8 --- ironic_python_agent/hardware.py | 10 ++-- .../tests/unit/test_hardware.py | 46 ++++++++++++++++++- ...handling_improvement-1669d0de4bfdbe95.yaml | 20 ++++++++ 3 files changed, 71 insertions(+), 5 deletions(-) create mode 100644 releasenotes/notes/multipath_error_handling_improvement-1669d0de4bfdbe95.yaml 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 ad5e927d1..827a2cdfc 100644 --- a/ironic_python_agent/tests/unit/test_hardware.py +++ b/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, 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).

AltStyle によって変換されたページ (->オリジナル) /