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

4 files changed, 66 insertions, 6 deletions

diff --git a/ironic_python_agent/extensions/clean.py b/ironic_python_agent/extensions/clean.py
index 87d2a4fe..1b627534 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 33366891..d9dc8490 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 43cd8ec9..50299ce3 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 32cd0f99..5cd52b32 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(