From 22aa29b864eecd00bfb7c67cc2075030da1eb1d0 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Wed, 10 Apr 2024 13:57:24 +0200 Subject: [PATCH] Stop assuming service steps have priorities Unlike clean, deploy and verify steps, service steps cannot run automatically and thus do not have a usable notion of priority. It's not possible to provide a priority through the API but our validation code still requires it. This change gets rid of most priority handling for service steps, leaving only some foundation for future enhancements. Change-Id: I82aefc03a5c062b67e0f457612fe568399226dc8 --- ironic/conductor/steps.py | 25 ++++++------------- ironic/tests/unit/conductor/test_steps.py | 12 ++++----- .../service-priority-7482622471102c6b.yaml | 5 ++++ 3 files changed, 19 insertions(+), 23 deletions(-) create mode 100644 releasenotes/notes/service-priority-7482622471102c6b.yaml diff --git a/ironic/conductor/steps.py b/ironic/conductor/steps.py index dcdfa469cb..a1dce7eccc 100644 --- a/ironic/conductor/steps.py +++ b/ironic/conductor/steps.py @@ -132,15 +132,6 @@ def _sorted_steps(steps, sort_step_key): return sorted(steps, key=sort_step_key, reverse=True) -def _service_step_key(step): - """Sort by priority, then interface priority in event of tie. - - :param step: deploy step dict to get priority for. - """ - return (step.get('priority'), - SERVICING_INTERFACE_PRIORITY[step.get('interface')]) - - def is_equivalent(step1, step2): """Compare steps, ignoring their priority.""" return (step1.get('interface') == step2.get('interface') @@ -260,17 +251,14 @@ def _get_service_steps(task, enabled=False, sort=True): :param task: A TaskManager object :param enabled: If True, returns only enabled (priority > 0) steps. If False, returns all clean steps. - :param sort: If True, the steps are sorted from highest priority to lowest - priority. For steps having the same priority, they are sorted from - highest interface priority to lowest. + :param sort: Used for consistency, ignored. :raises: NodeServicingFailure if there was a problem getting the clean steps. :returns: A list of clean step dictionaries """ - sort_key = _service_step_key if sort else None service_steps = _get_steps(task, SERVICING_INTERFACE_PRIORITY, 'get_service_steps', enabled=enabled, - sort_step_key=sort_key) + sort_step_key=None) return service_steps @@ -621,7 +609,7 @@ def _validate_user_step(task, user_step, driver_step, step_type, 'unexpected': ', '.join(unexpected)}) errors.append(error) - if step_type == 'clean' or user_step['priority'] > 0: + if step_type != 'deploy' or user_step['priority'] > 0: # Check that all required arguments were specified by the user missing = [] for (arg_name, arg_info) in argsinfo.items(): @@ -637,10 +625,13 @@ def _validate_user_step(task, user_step, driver_step, step_type, 'miss': ', '.join(missing)}) errors.append(error) if disable_ramdisk and driver_step.get('requires_ramdisk', True): - error = _('clean step %s requires booting a ramdisk') % user_step + error = _('%(type)s step %(step)s requires booting a ramdisk') % { + 'type': step_type, + 'step': user_step + } errors.append(error) - if step_type == 'clean': + if step_type != 'deploy': # Copy fields that should not be provided by a user user_step['abortable'] = driver_step.get('abortable', False) user_step['priority'] = driver_step.get('priority', 0) diff --git a/ironic/tests/unit/conductor/test_steps.py b/ironic/tests/unit/conductor/test_steps.py index 64317c1818..2e7e0896d0 100644 --- a/ironic/tests/unit/conductor/test_steps.py +++ b/ironic/tests/unit/conductor/test_steps.py @@ -1415,17 +1415,17 @@ class NodeServiceStepsTestCase(db_base.DbTestCase): super(NodeServiceStepsTestCase, self).setUp() self.deploy_start = { - 'step': 'deploy_start', 'priority': 50, 'interface': 'deploy'} + 'step': 'deploy_start', 'interface': 'deploy'} self.power_one = { - 'step': 'power_one', 'priority': 40, 'interface': 'power'} + 'step': 'power_one', 'priority': 0, 'interface': 'power'} self.deploy_middle = { - 'step': 'deploy_middle', 'priority': 40, 'interface': 'deploy'} + 'step': 'deploy_middle', 'interface': 'deploy'} self.deploy_end = { - 'step': 'deploy_end', 'priority': 20, 'interface': 'deploy'} + 'step': 'deploy_end', 'interface': 'deploy'} self.power_disable = { 'step': 'power_disable', 'priority': 0, 'interface': 'power'} self.deploy_core = { - 'step': 'deploy', 'priority': 100, 'interface': 'deploy'} + 'step': 'deploy', 'interface': 'deploy'} # enabled steps self.service_steps = [self.deploy_start, self.power_one, self.deploy_middle, self.deploy_end] @@ -1472,7 +1472,7 @@ class NodeServiceStepsTestCase(db_base.DbTestCase): self.context, self.node.uuid, shared=False) as task: steps = conductor_steps._get_service_steps(task, enabled=False) - self.assertEqual(expected, steps) + self.assertCountEqual(expected, steps) mock_mgt_steps.assert_called_once_with(mock.ANY, task) mock_power_steps.assert_called_once_with(mock.ANY, task) mock_deploy_steps.assert_called_once_with(mock.ANY, task) diff --git a/releasenotes/notes/service-priority-7482622471102c6b.yaml b/releasenotes/notes/service-priority-7482622471102c6b.yaml new file mode 100644 index 0000000000..7758c17f3d --- /dev/null +++ b/releasenotes/notes/service-priority-7482622471102c6b.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Service step validation no longer requires a priority field, which is not + supported for servicing.