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 0c91acc87..3f0f0771e 100644 --- a/ironic_python_agent/hardware.py +++ b/ironic_python_agent/hardware.py @@ -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)