From b6275912c2ee07a5544d45806f80830bf8852123 Mon Sep 17 00:00:00 2001
From: Julia Kreger <juliaashleykreger@gmail.com>
Date: Wed, 15 Jan 2025 15:21:37 -0800
Subject: [PATCH] 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                   |  9 +++++++-
 ironic/conductor/steps.py                     |  3 ++-
 ironic/tests/unit/conductor/test_manager.py   | 21 ++++++++++++++++++-
 ironic/tests/unit/conductor/test_steps.py     |  4 +++-
 ...d-wait-service-steps-37dc91fd7393b180.yaml |  5 +++++
 5 files changed, 38 insertions(+), 4 deletions(-)
 create mode 100644 releasenotes/notes/fix-hold-wait-service-steps-37dc91fd7393b180.yaml

diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py
index 88338db2dd..304f95c5ce 100644
--- a/ironic/conductor/manager.py
+++ b/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:
diff --git a/ironic/conductor/steps.py b/ironic/conductor/steps.py
index a1dce7eccc..79a23edeff 100644
--- a/ironic/conductor/steps.py
+++ b/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
diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py
index 529bcbaa6f..0f2b553f2e 100644
--- a/ironic/tests/unit/conductor/test_manager.py
+++ b/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,
diff --git a/ironic/tests/unit/conductor/test_steps.py b/ironic/tests/unit/conductor/test_steps.py
index 2e7e0896d0..8782c4a62c 100644
--- a/ironic/tests/unit/conductor/test_steps.py
+++ b/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,
diff --git a/releasenotes/notes/fix-hold-wait-service-steps-37dc91fd7393b180.yaml b/releasenotes/notes/fix-hold-wait-service-steps-37dc91fd7393b180.yaml
new file mode 100644
index 0000000000..6d10fdbec3
--- /dev/null
+++ b/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.