Generic way to configure clean step priorites

This change adds a generic method of configuring clean step
priorities instead of making changes in Ironic code every time a new
clean step is introduced.

Change-Id: I56b9a878724d27af2ac05232a1680017de4d8df5
Story: 1618014

doc/source/admin/cleaning.rst

@@ -282,6 +282,22 @@ the number of iterations, use the following configuration option::
   [deploy]
   erase_devices_iterations=1
 
+Overriding step priority
+------------------------
+
+``[conductor]clean_step_priority_override`` is a new configuration option
+which allows specifying priority of each step using multiple configuration
+values:
+
+.. code-block:: ini
+
+  [conductor]
+  clean_step_priority_override=deploy.erase_devices_metadata:123
+  clean_step_priority_override=management.reset_bios_to_default:234
+  clean_step_priority_override=management.clean_priority_reset_ilo:345
+
+This parameter can be specified as many times as required to define priorities
+for several cleaning steps - the values will be combined.
 
 What cleaning step is running?
 ------------------------------

ironic/conductor/steps.py

@@ -93,7 +93,7 @@ def find_step(steps, step):
 
 
 def _get_steps(task, interfaces, get_method, enabled=False,
-               sort_step_key=None):
+               sort_step_key=None, prio_overrides=None):
     """Get steps for task.node.
 
     :param task: A TaskManager object
@@ -108,6 +108,10 @@ def _get_steps(task, interfaces, get_method, enabled=False,
     :param sort_step_key: If set, this is a method (key) used to sort the steps
         from highest priority to lowest priority. For steps having the same
         priority, they are sorted from highest interface priority to lowest.
+    :param prio_overrides: An optional dictionary of priority overrides for
+        steps, e.g:
+        {'deploy.erase_devices_metadata': '123',
+         'management.reset_bios_to_default': '234'}
     :raises: NodeCleaningFailure or InstanceDeployFailure if there was a
         problem getting the steps.
     :returns: A list of step dictionaries
@@ -120,6 +124,12 @@ def _get_steps(task, interfaces, get_method, enabled=False,
             interface_steps = [x for x in getattr(interface, get_method)(task)
                                if not enabled or x['priority'] > 0]
             steps.extend(interface_steps)
+    if prio_overrides is not None:
+        for step in steps:
+            override_key = '%(interface)s.%(step)s' % step
+            override_value = prio_overrides.get(override_key)
+            if override_value:
+                step["priority"] = int(override_value)
     if sort_step_key:
         steps = _sorted_steps(steps, sort_step_key)
     return steps
@@ -139,8 +149,24 @@ def _get_cleaning_steps(task, enabled=False, sort=True):
     :returns: A list of clean step dictionaries
     """
     sort_key = _clean_step_key if sort else None
-    return _get_steps(task, CLEANING_INTERFACE_PRIORITY, 'get_clean_steps',
+    if CONF.conductor.clean_step_priority_override:
-                      enabled=enabled, sort_step_key=sort_key)
+        csp_override = {}
+        for element in CONF.conductor.clean_step_priority_override:
+            csp_override.update(element)
+
+        cleaning_steps = _get_steps(task, CLEANING_INTERFACE_PRIORITY,
+                                    'get_clean_steps', enabled=enabled,
+                                    sort_step_key=sort_key,
+                                    prio_overrides=csp_override)
+
+        LOG.debug("cleaning_steps after applying "
+                  "clean_step_priority_override for node %(node)s: %(step)s",
+                  task.node.uuid, cleaning_steps)
+    else:
+        cleaning_steps = _get_steps(task, CLEANING_INTERFACE_PRIORITY,
+                                    'get_clean_steps', enabled=enabled,
+                                    sort_step_key=sort_key)
+    return cleaning_steps
 
 
 def _get_deployment_steps(task, enabled=False, sort=True):

ironic/conf/conductor.py

@@ -16,6 +16,7 @@
 #    under the License.
 
 from oslo_config import cfg
+from oslo_config import types
 
 from ironic.common.i18n import _
 
@@ -270,6 +271,18 @@ opts = [
                       'will be used by ironic when building UEFI-bootable ISO '
                       'out of kernel and ramdisk. Required for UEFI boot from '
                       'partition images.')),
+    cfg.MultiOpt('clean_step_priority_override',
+                 item_type=types.Dict(),
+                 default={},
+                 help=_('Priority to run automated clean steps for both '
+                        'in-band and out of band clean steps, provided in '
+                        'interface.step_name:priority format, e.g. '
+                        'deploy.erase_devices_metadata:123. The option can '
+                        'be specified multiple times to define priorities '
+                        'for multiple steps. If set to 0, this specific step '
+                        'will not run during cleaning. If unset for an '
+                        'inband clean step, will use the priority set in the '
+                        'ramdisk.')),
 ]
 
 

ironic/tests/unit/conductor/test_steps.py

@@ -575,6 +575,115 @@ class NodeCleaningStepsTestCase(db_base.DbTestCase):
 
         self.assertEqual(self.clean_steps, steps)
 
+    @mock.patch('ironic.drivers.modules.fake.FakeDeploy.get_clean_steps',
+                autospec=True)
+    @mock.patch('ironic.drivers.modules.fake.FakePower.get_clean_steps',
+                autospec=True)
+    def test__get_cleaning_steps_priority_override_ok(self, mock_power_steps,
+                                                      mock_deploy_steps):
+        # Test clean_step_priority_override
+        cfg.CONF.set_override('clean_step_priority_override',
+                              ["deploy.erase_disks:123",
+                               "power.update_firmware:234",
+                               "deploy.update_firmware:456", ],
+                              'conductor')
+
+        node = obj_utils.create_test_node(
+            self.context, driver='fake-hardware',
+            provision_state=states.CLEANING,
+            target_provision_state=states.AVAILABLE)
+
+        mock_power_steps.return_value = [self.power_update]
+        mock_deploy_steps.return_value = [self.deploy_erase,
+                                          self.deploy_update,
+                                          self.deploy_raid]
+
+        with task_manager.acquire(
+                self.context, node.uuid, shared=True) as task:
+            steps = conductor_steps._get_cleaning_steps(task, enabled=True)
+
+        expected_step_priorities = [{'interface': 'deploy', 'priority': 456,
+                                     'step': 'update_firmware'},
+                                    {'interface': 'power', 'priority': 234,
+                                     'step': 'update_firmware'},
+                                    {'abortable': True,
+                                     'interface': 'deploy',
+                                     'priority': 123,
+                                     'step': 'erase_disks'}]
+
+        self.assertEqual(expected_step_priorities, steps)
+
+    @mock.patch('ironic.drivers.modules.fake.FakeDeploy.get_clean_steps',
+                autospec=True)
+    @mock.patch('ironic.drivers.modules.fake.FakePower.get_clean_steps',
+                autospec=True)
+    def test__get_cleaning_steps_priority_override_fail(self,
+                                                        mock_power_steps,
+                                                        mock_deploy_steps):
+        # Test clean_step_priority_override
+        cfg.CONF.set_override('clean_step_priority_override',
+                              ["deploy.erase_disks:123",
+                               "power.update_firmware:234",
+                               "deploy.update_firmware:456", ],
+                              'conductor')
+
+        node = obj_utils.create_test_node(
+            self.context, driver='fake-hardware',
+            provision_state=states.CLEANING,
+            target_provision_state=states.AVAILABLE)
+
+        mock_power_steps.return_value = [self.power_update]
+        mock_deploy_steps.return_value = [self.deploy_erase,
+                                          self.deploy_update,
+                                          self.deploy_raid]
+
+        with task_manager.acquire(
+                self.context, node.uuid, shared=True) as task:
+            steps = conductor_steps._get_cleaning_steps(task, enabled=True)
+
+        expected_step_priorities = [{'interface': 'deploy', 'priority': 333,
+                                     'step': 'update_firmware'},
+                                    {'interface': 'power', 'priority': 222,
+                                     'step': 'update_firmware'},
+                                    {'abortable': True,
+                                     'interface': 'deploy',
+                                     'priority': 111,
+                                     'step': 'erase_disks'}]
+
+        self.assertNotEqual(expected_step_priorities, steps)
+
+    @mock.patch('ironic.drivers.modules.fake.FakeDeploy.get_clean_steps',
+                autospec=True)
+    @mock.patch('ironic.drivers.modules.fake.FakePower.get_clean_steps',
+                autospec=True)
+    def test__get_cleaning_steps_priority_no_override(self, mock_power_steps,
+                                                      mock_deploy_steps):
+
+        node = obj_utils.create_test_node(
+            self.context, driver='fake-hardware',
+            provision_state=states.CLEANING,
+            target_provision_state=states.AVAILABLE)
+
+        mock_power_steps.return_value = [self.power_update]
+        mock_deploy_steps.return_value = [self.deploy_erase,
+                                          self.deploy_update,
+                                          self.deploy_raid]
+
+        with task_manager.acquire(
+                self.context, node.uuid, shared=True) as task:
+            steps = conductor_steps._get_cleaning_steps(task, enabled=True)
+
+        expected_step_priorities = [{'abortable': True,
+                                     'interface': 'deploy',
+                                     'priority': 20,
+                                     'step': 'erase_disks'},
+                                    {'interface': 'power', 'priority': 10,
+                                     'step': 'update_firmware'},
+                                    {'interface': 'deploy', 'priority': 10,
+                                     'step': 'update_firmware'}]
+
+        self.assertEqual(expected_step_priorities, steps)
+
     @mock.patch.object(conductor_steps, '_validate_user_clean_steps',
                        autospec=True)
     @mock.patch.object(conductor_steps, '_get_cleaning_steps', autospec=True)

releasenotes/notes/releasenote-b3b25c13ea1e2844.yaml

@@ -0,0 +1,6 @@
+---
+features:
+  - |
+    Adds ``[conductor]clean_step_priority_override`` configuration parameter
+    which allows the operator to define a custom order in which the cleaning
+    steps are to run.