From 1523ae1ce41bd115be09ceebe16f7c8367ea1b5d Mon Sep 17 00:00:00 2001 From: Jacob Anders Date: Fri, 31 Jul 2020 20:15:26 +1000 Subject: [PATCH] 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 | 16 +++ ironic/conductor/steps.py | 32 ++++- ironic/conf/conductor.py | 13 +++ ironic/tests/unit/conductor/test_steps.py | 109 ++++++++++++++++++ .../notes/releasenote-b3b25c13ea1e2844.yaml | 6 + 5 files changed, 173 insertions(+), 3 deletions(-) create mode 100644 releasenotes/notes/releasenote-b3b25c13ea1e2844.yaml diff --git a/doc/source/admin/cleaning.rst b/doc/source/admin/cleaning.rst index 67c6c9e9fc..f36e2f0b90 100644 --- a/doc/source/admin/cleaning.rst +++ b/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? ------------------------------ diff --git a/ironic/conductor/steps.py b/ironic/conductor/steps.py index a6663db01d..dcc40c2134 100644 --- a/ironic/conductor/steps.py +++ b/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', - enabled=enabled, sort_step_key=sort_key) + if CONF.conductor.clean_step_priority_override: + 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): diff --git a/ironic/conf/conductor.py b/ironic/conf/conductor.py index 494317f8d2..cb52c45d44 100644 --- a/ironic/conf/conductor.py +++ b/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.')), ] diff --git a/ironic/tests/unit/conductor/test_steps.py b/ironic/tests/unit/conductor/test_steps.py index 845d639d43..8d6954a414 100644 --- a/ironic/tests/unit/conductor/test_steps.py +++ b/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) diff --git a/releasenotes/notes/releasenote-b3b25c13ea1e2844.yaml b/releasenotes/notes/releasenote-b3b25c13ea1e2844.yaml new file mode 100644 index 0000000000..7967d0e8cb --- /dev/null +++ b/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.