From 61b4387b95a7a147a8621dbc538ed30b3bf5c90a Mon Sep 17 00:00:00 2001 From: Josh Gachnang Date: Tue, 9 Jun 2015 15:39:38 -0700 Subject: [PATCH] Allow hardware managers to override clean step priority If two hardware managers have the same clean step, for example 'erase_devices' in the GenericHardwareManager and a custom manager, IPA must determine which step should be kept and which should be run in order to prevent running the step multiple times. This patch uses the following filtering logic to decide which step "wins": - Keep the step that belongs to HardwareManager with highest HardwareSupport (larger int) value. - If equal support level, keep the step with the higher defined priority (larger int). - If equal support level and priority, keep the step associated with the HardwareManager whose name comes earlier in the alphabet. Other than individual step priority, picking which step to keep does not actually impact the cleaning run. However, in order to make testing easier, this change ensures deterministic, predictable results. Co-Authored-By: Mario Villaplana Co-Authored-By: Jay Faulkner Co-Authored-By: Brad Morgan Change-Id: Iaeea4200c38ee22cab72ba81c1dbae3389e675e4 --- ironic_python_agent/extensions/clean.py | 81 ++++++++++++- ironic_python_agent/hardware.py | 14 ++- .../tests/unit/extensions/test_clean.py | 72 ++++++++++- .../unit/test_multi_hardware_clean_steps.py | 114 ++++++++++++++++++ 4 files changed, 268 insertions(+), 13 deletions(-) create mode 100644 ironic_python_agent/tests/unit/test_multi_hardware_clean_steps.py diff --git a/ironic_python_agent/extensions/clean.py b/ironic_python_agent/extensions/clean.py index e67bd1540..83631825c 100644 --- a/ironic_python_agent/extensions/clean.py +++ b/ironic_python_agent/extensions/clean.py @@ -12,6 +12,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +import collections + from oslo_log import log from ironic_python_agent import errors @@ -35,12 +37,15 @@ class CleanExtension(base.BaseAgentExtension): LOG.debug('Getting clean steps, called with node: %(node)s, ' 'ports: %(ports)s', {'node': node, 'ports': ports}) # Results should be a dict, not a list - steps = hardware.dispatch_to_all_managers('get_clean_steps', - node, ports) + candidate_steps = hardware.dispatch_to_all_managers('get_clean_steps', + node, ports) + + LOG.debug('Clean steps before deduplication: %s', candidate_steps) + clean_steps = _deduplicate_steps(candidate_steps) + LOG.debug('Returning clean steps: %s', clean_steps) - LOG.debug('Returning clean steps: %s', steps) return { - 'clean_steps': steps, + 'clean_steps': clean_steps, 'hardware_manager_version': _get_current_clean_version() } @@ -90,6 +95,74 @@ class CleanExtension(base.BaseAgentExtension): } +def _deduplicate_steps(candidate_steps): + """Remove duplicated clean steps + + Deduplicates clean steps returned from HardwareManagers to prevent + running a given step more than once. Other than individual step + priority, it doesn't actually impact the cleaning run which specific + steps are kept and what HardwareManager they are associated with. + However, in order to make testing easier, this method returns + deterministic results. + + Uses the following filtering logic to decide which step "wins": + - Keep the step that belongs to HardwareManager with highest + HardwareSupport (larger int) value. + - If equal support level, keep the step with the higher defined priority + (larger int). + - If equal support level and priority, keep the step associated with the + HardwareManager whose name comes earlier in the alphabet. + + :param candidate_steps: A dict containing all possible clean steps from + all managers, key=manager, value=list of clean steps + :returns: A deduplicated dictionary of {hardware_manager: + [clean-steps]} + """ + support = hardware.dispatch_to_all_managers( + 'evaluate_hardware_support') + + steps = collections.defaultdict(list) + deduped_steps = collections.defaultdict(list) + + for manager, manager_steps in candidate_steps.items(): + # We cannot deduplicate steps with unknown hardware support + if manager not in support: + LOG.warning('Unknown hardware support for %(manager)s, ' + 'dropping clean steps: %(steps)s', + {'manager': manager, 'steps': manager_steps}) + continue + + for step in manager_steps: + # build a new dict of steps that's easier to filter + step['hwm'] = {'name': manager, + 'support': support[manager]} + steps[step['step']].append(step) + + for step_name, step_list in steps.items(): + # determine the max support level among candidate steps + max_support = max([x['hwm']['support'] for x in step_list]) + # filter out any steps that are not at the max support for this step + max_support_steps = [x for x in step_list + if x['hwm']['support'] == max_support] + + # determine the max priority among remaining steps + max_priority = max([x['priority'] for x in max_support_steps]) + # filter out any steps that are not at the max priority for this step + max_priority_steps = [x for x in max_support_steps + if x['priority'] == max_priority] + + # if there are still multiple steps, sort by hwm name and take + # the first result + winning_step = sorted(max_priority_steps, + key=lambda x: x['hwm']['name'])[0] + # Remove extra metadata we added to the step for filtering + manager = winning_step.pop('hwm')['name'] + # Add winning step to deduped_steps + deduped_steps[manager].append(winning_step) + + return deduped_steps + + def _check_clean_version(clean_version=None): """Ensure the clean version hasn't changed. diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py index 0e759463e..3f0f0771e 100644 --- a/ironic_python_agent/hardware.py +++ b/ironic_python_agent/hardware.py @@ -269,7 +269,7 @@ class HardwareManager(object): def get_clean_steps(self, node, ports): """Get a list of clean steps with priority. - Returns a list of steps. Each step is represeted by a dict:: + Returns a list of steps. Each step is represented by a dict:: { 'step': the HardwareManager function to call. @@ -289,8 +289,16 @@ class HardwareManager(object): parameter, Ironic will consider False (non-abortable). } - If multiple hardware managers return the same step name, the priority - of the step will be the largest priority of steps with the same name. + + If multiple hardware managers return the same step name, the following + logic will be used to determine which manager's step "wins": + - Keep the step that belongs to HardwareManager with highest + HardwareSupport (larger int) value. + - If equal support level, keep the step with the higher defined + priority (larger int). + - If equal support level and priority, keep the step associated + with the HardwareManager whose name comes earlier in the alphabet. + The steps will be called using `hardware.dispatch_to_managers` and handled by the best suited hardware manager. If you need a step to be executed by only your hardware manager, ensure it has a unique step diff --git a/ironic_python_agent/tests/unit/extensions/test_clean.py b/ironic_python_agent/tests/unit/extensions/test_clean.py index cb110d085..11748a4f9 100644 --- a/ironic_python_agent/tests/unit/extensions/test_clean.py +++ b/ironic_python_agent/tests/unit/extensions/test_clean.py @@ -52,28 +52,88 @@ class TestCleanExtension(test_base.BaseTestCase): 'priority': 20, 'interface': 'deploy', 'reboot_requested': True - } + }, + { + 'step': 'upgrade_firmware', + 'priority': 60, + 'interface': 'deploy', + 'reboot_requested': False + }, ], 'FirmwareHardwareManager': [ { 'step': 'upgrade_firmware', - 'priority': 30, + 'priority': 10, 'interface': 'deploy', 'reboot_requested': False - } + }, + { + 'step': 'erase_devices', + 'priority': 40, + 'interface': 'deploy', + 'reboot_requested': False + }, + ], + 'DiskHardwareManager': [ + { + 'step': 'erase_devices', + 'priority': 50, + 'interface': 'deploy', + 'reboot_requested': False + }, ] } - mock_dispatch.return_value = manager_steps + expected_steps = { + 'SpecificHardwareManager': [ + # Only manager upgrading BIOS + { + 'step': 'upgrade_bios', + 'priority': 20, + 'interface': 'deploy', + 'reboot_requested': True + } + ], + 'FirmwareHardwareManager': [ + # Higher support than specific, even though lower priority + { + 'step': 'upgrade_firmware', + 'priority': 10, + 'interface': 'deploy', + 'reboot_requested': False + }, + ], + 'DiskHardwareManager': [ + # Higher support than specific, higher priority than firmware + { + 'step': 'erase_devices', + 'priority': 50, + 'interface': 'deploy', + 'reboot_requested': False + }, + ] + + } + + hardware_support = { + 'SpecificHardwareManager': 3, + 'FirmwareHardwareManager': 4, + 'DiskHardwareManager': 4 + } + + mock_dispatch.side_effect = [manager_steps, hardware_support] expected_return = { 'hardware_manager_version': self.version, - 'clean_steps': manager_steps + 'clean_steps': expected_steps } async_results = self.agent_extension.get_clean_steps(node=self.node, ports=self.ports) - self.assertEqual(expected_return, async_results.join().command_result) + # Ordering of the clean steps doesn't matter; they're sorted by + # 'priority' in Ironic + self.assertEqual(expected_return, + async_results.join().command_result) @mock.patch('ironic_python_agent.hardware.dispatch_to_managers') @mock.patch('ironic_python_agent.extensions.clean._check_clean_version') diff --git a/ironic_python_agent/tests/unit/test_multi_hardware_clean_steps.py b/ironic_python_agent/tests/unit/test_multi_hardware_clean_steps.py new file mode 100644 index 000000000..5ac9dbedf --- /dev/null +++ b/ironic_python_agent/tests/unit/test_multi_hardware_clean_steps.py @@ -0,0 +1,114 @@ +# Copyright 2013 Rackspace, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import mock +from oslotest import base as test_base +from stevedore import extension + +from ironic_python_agent.extensions import clean +from ironic_python_agent import hardware + + +def _build_clean_step(name, priority, reboot=False, abort=False): + return {'step': name, 'priority': priority, + 'reboot_requested': reboot, 'abortable': abort} + + +class AFakeMainlineHardwareManager(hardware.HardwareManager): + def evaluate_hardware_support(self): + return hardware.HardwareSupport.MAINLINE + + def get_clean_steps(self, node, ports): + return [_build_clean_step('duped_ml', 20)] + + +class AFakeGenericHardwareManager(hardware.HardwareManager): + def evaluate_hardware_support(self): + return hardware.HardwareSupport.GENERIC + + def get_clean_steps(self, node, ports): + return [_build_clean_step('duped_ml', 20), + _build_clean_step('duped_gn', 30), + _build_clean_step('ZHigherPrio', 1)] + + +class ZFakeGenericHardwareManager(hardware.HardwareManager): + def evaluate_hardware_support(self): + return hardware.HardwareSupport.GENERIC + + def get_clean_steps(self, node, ports): + return [_build_clean_step('duped_ml', 20), + _build_clean_step('duped_gn', 30), + _build_clean_step('ZHigherPrio', 100)] + + +class TestMultipleHardwareManagerCleanSteps(test_base.BaseTestCase): + def setUp(self): + super(TestMultipleHardwareManagerCleanSteps, self).setUp() + + self.agent_extension = clean.CleanExtension() + + fake_ep = mock.Mock() + fake_ep.module_name = 'fake' + fake_ep.attrs = ['fake attrs'] + self.ag_hwm = extension.Extension( + 'fake_ageneric', fake_ep, None, AFakeGenericHardwareManager()) + self.zg_hwm = extension.Extension( + 'fake_zgeneric', fake_ep, None, ZFakeGenericHardwareManager()) + self.ml_hwm = extension.Extension( + 'fake_amainline', fake_ep, None, AFakeMainlineHardwareManager()) + self.fake_ext_mgr = extension.ExtensionManager.make_test_instance( + [self.ag_hwm, self.zg_hwm, self.ml_hwm]) + + self.extension_mgr_patcher = mock.patch('stevedore.ExtensionManager') + self.mocked_extension_mgr = self.extension_mgr_patcher.start() + self.mocked_extension_mgr.return_value = self.fake_ext_mgr + hardware._global_managers = None + + def tearDown(self): + super(TestMultipleHardwareManagerCleanSteps, self).tearDown() + self.extension_mgr_patcher.stop() + + def test_clean_step_ordering(self): + as_results = self.agent_extension.get_clean_steps(node={}, ports=[]) + results = as_results.join().command_result + expected_steps = { + 'clean_steps': { + 'AFakeGenericHardwareManager': [ + {'step': 'duped_gn', + 'reboot_requested': False, + 'abortable': False, + 'priority': 30}], + 'ZFakeGenericHardwareManager': [ + {'step': 'ZHigherPrio', + 'reboot_requested': False, + 'abortable': False, + 'priority': 100}], + 'AFakeMainlineHardwareManager': [ + {'step': 'duped_ml', + 'reboot_requested': False, + 'abortable': False, + 'priority': 20}]}, + 'hardware_manager_version': { + 'AFakeGenericHardwareManager': '1.0', + 'AFakeMainlineHardwareManager': '1.0', + 'ZFakeGenericHardwareManager': '1.0'}} + + for manager, steps in results['clean_steps'].items(): + steps.sort(key=lambda x: (x['priority'], x['step'])) + + for manager, steps in expected_steps['clean_steps'].items(): + steps.sort(key=lambda x: (x['priority'], x['step'])) + + self.assertEqual(expected_steps, results)