From ad8697674b4ca8bdb0ba8873ac2e7f353910606a Mon Sep 17 00:00:00 2001
From: Dmitry Tantsur <dtantsur@protonmail.com>
Date: Mon, 27 Jul 2020 17:50:51 +0200
Subject: [PATCH] Make the final deploy step validation actually fail deploy

Since continue_node_deploy is an async RPC call, currently it ends up
just logging InvalidParameterValue, making deployment hang.

Related Story: #2006963

Change-Id: I2231de30778a2ab3adffa8a5b68ff7216717534c
(cherry picked from commit a1e079caec1a6a9026d687cb87acc6e228f8e4bf)
---
 ironic/conductor/manager.py                   | 12 ++++++--
 ironic/tests/unit/conductor/test_manager.py   | 29 +++++++++++++++++++
 ...deploy-step-validate-76b2aa97e02ba669.yaml |  5 ++++
 3 files changed, 43 insertions(+), 3 deletions(-)
 create mode 100644 releasenotes/notes/deploy-step-validate-76b2aa97e02ba669.yaml

diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py
index ea8e3daea7..036bae990e 100644
--- a/ironic/conductor/manager.py
+++ b/ironic/conductor/manager.py
@@ -875,9 +875,15 @@ class ConductorManager(base_manager.BaseConductorManager):
 
             # Agent is now running, we're ready to validate the remaining steps
             if not info.get('steps_validated'):
-                conductor_steps.validate_deploy_templates(task)
-                conductor_steps.set_node_deployment_steps(
-                    task, reset_current=False)
+                try:
+                    conductor_steps.validate_deploy_templates(task)
+                    conductor_steps.set_node_deployment_steps(
+                        task, reset_current=False)
+                except exception.IronicException as exc:
+                    msg = _('Failed to validate the final deploy steps list '
+                            'for node %(node)s: %(exc)s') % {'node': node.uuid,
+                                                             'exc': exc}
+                    return utils.deploying_error_handler(task, msg)
                 info['steps_validated'] = True
                 save_required = True
 
diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py
index 06cf9bd02f..311493e17a 100644
--- a/ironic/tests/unit/conductor/test_manager.py
+++ b/ironic/tests/unit/conductor/test_manager.py
@@ -1851,6 +1851,35 @@ class ContinueNodeDeployTestCase(mgr_utils.ServiceSetUpMixin,
                                       deployments.do_next_deploy_step,
                                       mock.ANY, 1, mock.ANY)
 
+    @mock.patch.object(conductor_steps, 'validate_deploy_templates',
+                       autospec=True)
+    @mock.patch('ironic.conductor.manager.ConductorManager._spawn_worker',
+                autospec=True)
+    def test_continue_node_steps_validation(self, mock_spawn, mock_validate):
+        prv_state = states.DEPLOYWAIT
+        tgt_prv_state = states.ACTIVE
+        mock_validate.side_effect = exception.InvalidParameterValue('boom')
+        driver_info = {'deploy_steps': self.deploy_steps,
+                       'deploy_step_index': 0,
+                       'steps_validated': False}
+        node = obj_utils.create_test_node(self.context, driver='fake-hardware',
+                                          provision_state=prv_state,
+                                          target_provision_state=tgt_prv_state,
+                                          last_error=None,
+                                          driver_internal_info=driver_info,
+                                          deploy_step=self.deploy_steps[0])
+        self._start_service()
+        mock_spawn.reset_mock()
+        self.service.continue_node_deploy(self.context, node.uuid)
+        self._stop_service()
+        node.refresh()
+        self.assertEqual(states.DEPLOYFAIL, node.provision_state)
+        self.assertIn('Failed to validate the final deploy steps',
+                      node.last_error)
+        self.assertIn('boom', node.last_error)
+        self.assertEqual(tgt_prv_state, node.target_provision_state)
+        self.assertFalse(mock_spawn.called)
+
 
 @mgr_utils.mock_record_keepalive
 class CheckTimeoutsTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
diff --git a/releasenotes/notes/deploy-step-validate-76b2aa97e02ba669.yaml b/releasenotes/notes/deploy-step-validate-76b2aa97e02ba669.yaml
new file mode 100644
index 0000000000..1dea110a8d
--- /dev/null
+++ b/releasenotes/notes/deploy-step-validate-76b2aa97e02ba669.yaml
@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    Fixes deployment hanging on an invalid in-band deploy step in a deploy
+    templates.