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

@@ -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
+    :param sort: Used for consistency, ignored.
-        priority. For steps having the same priority, they are sorted from
-        highest interface priority to lowest.
     :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)

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)

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.