Fix hold/wait step logic in step validation

Somehow... the hold and wait steps were dropped or were lost in
from when hold/wait step logic was developed. This fixes it and
adds them to a test which exercises the validation logic.

Also takes into account the unhold verb call from Dmitry's change
in https://review.opendev.org/c/openstack/ironic/+/913707 and
adds a test accordingly.

Change-Id: I8c23db46b4a5772d907f6c73ed5b975fdaaf80c8

ironic/conductor/manager.py

@@ -1362,7 +1362,14 @@ class ConductorManager(base_manager.BaseConductorManager):
                 # up and it continue it's operation.
                 task.process_event('unhold')
                 return
-
+            if (action == states.VERBS['unhold']
+                    and node.provision_state == states.SERVICEHOLD):
+                task.process_event(
+                    'unhold',
+                    callback=self._spawn_worker,
+                    call_args=(servicing.continue_node_service, task),
+                    err_handler=utils.servicing_error_handler)
+                return
             try:
                 task.process_event(action)
             except exception.InvalidState:

ironic/conductor/steps.py

@@ -722,7 +722,8 @@ def _validate_user_steps(task, user_steps, driver_steps, step_type,
             # as we have the original API request context to leverage
             # for RBAC validation.
             continue
-        if user_step.get('step') in ['power_on', 'power_off', 'reboot']:
+        if user_step.get('step') in ['power_on', 'power_off', 'reboot',
+                                     'hold', 'wait']:
             # NOTE(TheJulia): These are flow related steps the conductor
             # resolves internally.
             continue

ironic/tests/unit/conductor/test_manager.py

@@ -3422,7 +3422,26 @@ class DoNodeServiceTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
             call_args=(servicing.do_node_service, mock.ANY,
                        {'foo': 'bar'}, False),
             err_handler=mock.ANY, target_state='active')
-# end legacy
+
+    @mock.patch('ironic.conductor.manager.ConductorManager._spawn_worker',
+                autospec=True)
+    def test_do_node_provision_action_unhold_service(self, mock_spawn):
+        node = obj_utils.create_test_node(
+            self.context, driver='fake-hardware',
+            provision_state=states.SERVICEHOLD,
+            target_provision_state=states.ACTIVE,
+            driver_internal_info={
+                'service_steps': [
+                    {'step': 'hold', 'priority': 9, 'interface': 'power'},
+                ],
+                'clean_step_index': 1
+            }
+        )
+        self._start_service()
+        self.service.do_provisioning_action(self.context, node.uuid, 'unhold')
+        mock_spawn.assert_called_with(
+            self.service, servicing.continue_node_service, mock.ANY)
+        self._stop_service()
 
 
 class DoNodeRescueTestCase(mgr_utils.CommonMixIn, mgr_utils.ServiceSetUpMixin,

ironic/tests/unit/conductor/test_steps.py

@@ -885,7 +885,9 @@ class NodeCleaningStepsTestCase(db_base.DbTestCase):
         mock_steps.return_value = self.clean_steps
 
         user_steps = [{'step': 'update_firmware', 'interface': 'power'},
-                      {'step': 'erase_disks', 'interface': 'deploy'}]
+                      {'step': 'erase_disks', 'interface': 'deploy'},
+                      {'step': 'hold', 'interface': 'deploy'},
+                      {'step': 'wait', 'interface': 'deploy'}]
 
         with task_manager.acquire(self.context, node.uuid) as task:
             result = conductor_steps._validate_user_clean_steps(task,

releasenotes/notes/fix-hold-wait-service-steps-37dc91fd7393b180.yaml

@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    Fixes step validation where some of the reserved step names, ``hold``,
+    and ``wait``, were not being properly handled by the step validation code.