From c5fb191393c9a7a7c958828360ff7034369848a0 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Tue, 9 Nov 2021 13:50:47 +0100 Subject: [PATCH] Simplify error messages when running clean/deploy step The caller knows what step it invokes, there is no point in repeating it in the error message. There is also no need to wrap the exception if it's a RESTError or an ironic-lib exception already since they are normally detailed enough. Only leave a detailed message when an unexpected exception happens. Change-Id: I1d8ca1e7ed1462159e4ae5f0bcf58686f6a2681c --- ironic_python_agent/extensions/clean.py | 10 +++++-- ironic_python_agent/extensions/deploy.py | 10 +++++-- .../tests/unit/extensions/test_clean.py | 26 ++++++++++++++++++- .../tests/unit/extensions/test_deploy.py | 26 ++++++++++++++++++- 4 files changed, 66 insertions(+), 6 deletions(-) diff --git a/ironic_python_agent/extensions/clean.py b/ironic_python_agent/extensions/clean.py index 87d2a4fe7..1b6275341 100644 --- a/ironic_python_agent/extensions/clean.py +++ b/ironic_python_agent/extensions/clean.py @@ -12,6 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +from ironic_lib import exception as il_exc from oslo_log import log from ironic_python_agent import errors @@ -73,9 +74,14 @@ class CleanExtension(base.BaseAgentExtension): raise ValueError(msg) try: result = hardware.dispatch_to_managers(step['step'], node, ports) + except (errors.RESTError, il_exc.IronicException): + LOG.exception('Error performing clean step %s', step['step']) + raise except Exception as e: - msg = ('Error performing clean step %(step)s: %(err)s' % - {'step': step['step'], 'err': e}) + msg = ('Unexpected exception performing clean step %(step)s. ' + '%(cls)s: %(err)s' % {'step': step['step'], + 'cls': e.__class__.__name__, + 'err': e}) LOG.exception(msg) raise errors.CleaningError(msg) diff --git a/ironic_python_agent/extensions/deploy.py b/ironic_python_agent/extensions/deploy.py index 333668914..d9dc8490e 100644 --- a/ironic_python_agent/extensions/deploy.py +++ b/ironic_python_agent/extensions/deploy.py @@ -10,6 +10,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +from ironic_lib import exception as il_exc from oslo_log import log from ironic_python_agent import errors @@ -75,9 +76,14 @@ class DeployExtension(base.BaseAgentExtension): try: result = hardware.dispatch_to_managers(step['step'], node, ports, **kwargs) + except (errors.RESTError, il_exc.IronicException): + LOG.exception('Error performing deploy step %s', step['step']) + raise except Exception as e: - msg = ('Error performing deploy step %(step)s: %(err)s' % - {'step': step['step'], 'err': e}) + msg = ('Unexpected exception performing deploy step %(step)s. ' + '%(cls)s: %(err)s' % {'step': step['step'], + 'cls': e.__class__.__name__, + 'err': e}) LOG.exception(msg) raise errors.DeploymentError(msg) diff --git a/ironic_python_agent/tests/unit/extensions/test_clean.py b/ironic_python_agent/tests/unit/extensions/test_clean.py index 43cd8ec90..50299ce39 100644 --- a/ironic_python_agent/tests/unit/extensions/test_clean.py +++ b/ironic_python_agent/tests/unit/extensions/test_clean.py @@ -209,7 +209,8 @@ class TestCleanExtension(base.IronicAgentTest): autospec=True) def test_execute_clean_step_fail(self, mock_version, mock_dispatch, mock_cache_node): - mock_dispatch.side_effect = RuntimeError + err = errors.BlockDeviceError("I'm a teapot") + mock_dispatch.side_effect = err async_result = self.agent_extension.execute_clean_step( step=self.step['GenericHardwareManager'][0], node=self.node, @@ -217,6 +218,29 @@ class TestCleanExtension(base.IronicAgentTest): async_result.join() self.assertEqual('FAILED', async_result.command_status) + self.assertEqual(err, async_result.command_error) + + mock_version.assert_called_once_with(self.version) + mock_dispatch.assert_called_once_with( + self.step['GenericHardwareManager'][0]['step'], + self.node, self.ports) + mock_cache_node.assert_called_once_with(self.node) + + @mock.patch('ironic_python_agent.hardware.dispatch_to_managers', + autospec=True) + @mock.patch('ironic_python_agent.hardware.check_versions', + autospec=True) + def test_execute_clean_step_exception(self, mock_version, mock_dispatch, + mock_cache_node): + mock_dispatch.side_effect = RuntimeError('boom') + + async_result = self.agent_extension.execute_clean_step( + step=self.step['GenericHardwareManager'][0], node=self.node, + ports=self.ports, clean_version=self.version) + async_result.join() + + self.assertEqual('FAILED', async_result.command_status) + self.assertIn('RuntimeError: boom', str(async_result.command_error)) mock_version.assert_called_once_with(self.version) mock_dispatch.assert_called_once_with( diff --git a/ironic_python_agent/tests/unit/extensions/test_deploy.py b/ironic_python_agent/tests/unit/extensions/test_deploy.py index 32cd0f997..5cd52b329 100644 --- a/ironic_python_agent/tests/unit/extensions/test_deploy.py +++ b/ironic_python_agent/tests/unit/extensions/test_deploy.py @@ -235,7 +235,8 @@ class TestDeployExtension(base.IronicAgentTest): autospec=True) def test_execute_deploy_step_fail(self, mock_version, mock_dispatch, mock_cache_node): - mock_dispatch.side_effect = RuntimeError + err = errors.BlockDeviceError("I'm a teapot") + mock_dispatch.side_effect = err async_result = self.agent_extension.execute_deploy_step( step=self.step['GenericHardwareManager'][0], node=self.node, @@ -243,6 +244,29 @@ class TestDeployExtension(base.IronicAgentTest): async_result.join() self.assertEqual('FAILED', async_result.command_status) + self.assertEqual(err, async_result.command_error) + + mock_version.assert_called_once_with(self.version) + mock_dispatch.assert_called_once_with( + self.step['GenericHardwareManager'][0]['step'], + self.node, self.ports) + mock_cache_node.assert_called_once_with(self.node) + + @mock.patch('ironic_python_agent.hardware.dispatch_to_managers', + autospec=True) + @mock.patch('ironic_python_agent.hardware.check_versions', + autospec=True) + def test_execute_deploy_step_exception(self, mock_version, mock_dispatch, + mock_cache_node): + mock_dispatch.side_effect = RuntimeError('boom') + + async_result = self.agent_extension.execute_deploy_step( + step=self.step['GenericHardwareManager'][0], node=self.node, + ports=self.ports, deploy_version=self.version) + async_result.join() + + self.assertEqual('FAILED', async_result.command_status) + self.assertIn('RuntimeError: boom', str(async_result.command_error)) mock_version.assert_called_once_with(self.version) mock_dispatch.assert_called_once_with(