openstack/ironic-python-agent
Code Issues Proposed changes

Simplify error messages when running clean/deploy step

Browse Source 
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
This commit is contained in:
Dmitry Tantsur 2021-11-09 13:50:47 +01:00
parent dc8c1f16f9
commit c5fb191393
4 changed files with 66 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
ironic_python_agent/extensions/clean.py
View File

@ -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)

10
ironic_python_agent/extensions/deploy.py
View File

@ -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)

26
ironic_python_agent/tests/unit/extensions/test_clean.py
View File

@ -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(

26
ironic_python_agent/tests/unit/extensions/test_deploy.py
View File

@ -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(