openstack/ironic-python-agent
Code Issues Proposed changes

Call evaluate_hardware_support exactly once per hwm

Browse Source 
Fixes an issue where we could call evaluate_hardware_support multiple
times each run. Now, instead, we cache the values and use the cache
where needed.

Adds unit test coverage for get_managers and the new method.
Fixes issue where we were caching hardware managers between unit tests.

Closes-bug: 2066308
Change-Id: Iebc5b6d2440bfc9f23daa322493379bbe69e84d0
(cherry picked from commit c39517b044)
This commit is contained in:
Jay Faulkner 2024-05-21 15:30:26 -07:00
parent c1581df331
commit e9c0578c7d
7 changed files with 148 additions and 58 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

67
ironic_python_agent/hardware.py
View File

@ -3148,46 +3148,65 @@ def _collect_udev(io_dict):
io_dict[f'udev/{kname}'] = buf
def _compare_extensions(ext1, ext2):
mgr1 = ext1.obj
mgr2 = ext2.obj
return mgr2.evaluate_hardware_support() - mgr1.evaluate_hardware_support()
def _compare_managers(hwm1, hwm2):
return hwm2['support'] - hwm1['support']
def _get_extensions():
return stevedore.ExtensionManager(
namespace='ironic_python_agent.hardware_managers',
invoke_on_load=True
)
def get_managers():
"""Get a list of hardware managers in priority order.
Use stevedore to find all eligible hardware managers, sort them based on
self-reported (via evaluate_hardware_support()) priorities, and return them
in a list. The resulting list is cached in _global_managers.
This exists as a backwards compatibility shim, returning a simple list
of managers where expected. New usages should use get_managers_detail.
:returns: Priority-sorted list of hardware managers
:raises HardwareManagerNotFound: if no valid hardware managers found
"""
return [hwm['manager'] for hwm in get_managers_detail()]
def get_managers_detail():
"""Get detailed information about hardware managers
Use stevedore to find all eligible hardware managers, sort them based on
self-reported (via evaluate_hardware_support()) priorities, and return a
dict containing the manager object, it's class name, and hardware support
value. The resulting list is cached in _global_managers.
:returns: list of dictionaries representing hardware managers and metadata
:raises HardwareManagerNotFound: if no valid hardware managers found
"""
global _global_managers
if not _global_managers:
extension_manager = stevedore.ExtensionManager(
namespace='ironic_python_agent.hardware_managers',
invoke_on_load=True)
# There will always be at least one extension available (the
# GenericHardwareManager).
extensions = sorted(extension_manager,
key=functools.cmp_to_key(_compare_extensions))
preferred_managers = []
for extension in extensions:
if extension.obj.evaluate_hardware_support() > 0:
preferred_managers.append(extension.obj)
for extension in _get_extensions():
hwm = extension.obj
hardware_support = hwm.evaluate_hardware_support()
if hardware_support > 0:
preferred_managers.append({
'name': hwm.__class__.__name__,
'manager': hwm,
'support': hardware_support
})
LOG.info('Hardware manager found: %s',
extension.entry_point_target)
if not preferred_managers:
raise errors.HardwareManagerNotFound
_global_managers = preferred_managers
hwms = sorted(preferred_managers,
key=functools.cmp_to_key(_compare_managers))
_global_managers = hwms
return _global_managers
@ -3381,20 +3400,14 @@ def deduplicate_steps(candidate_steps):
all managers, key=manager, value=list of steps
:returns: A deduplicated dictionary of {hardware_manager: [steps]}
"""
support = dispatch_to_all_managers(
'evaluate_hardware_support')
support = {hwm['name']: hwm['support']
for hwm in get_managers_detail()}
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 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,

1
ironic_python_agent/tests/unit/base.py
View File

@ -63,6 +63,7 @@ class IronicAgentTest(test_base.BaseTestCase):
ext_base._EXT_MANAGER = None
hardware._CACHED_HW_INFO = None
hardware._global_managers = None
def _set_config(self):
self.cfg_fixture = self.useFixture(config_fixture.Config(CONF))

20
ironic_python_agent/tests/unit/extensions/test_clean.py
View File

@ -34,12 +34,14 @@ class TestCleanExtension(base.IronicAgentTest):
}
self.version = {'generic': '1', 'specific': '1'}
@mock.patch('ironic_python_agent.hardware.get_managers_detail',
autospec=True)
@mock.patch('ironic_python_agent.hardware.get_current_versions',
autospec=True)
@mock.patch('ironic_python_agent.hardware.dispatch_to_all_managers',
autospec=True)
def test_get_clean_steps(self, mock_dispatch, mock_version,
mock_cache_node):
mock_managers, mock_cache_node):
mock_version.return_value = self.version
manager_steps = {
@ -118,13 +120,17 @@ class TestCleanExtension(base.IronicAgentTest):
}
hardware_support = {
'SpecificHardwareManager': 3,
'FirmwareHardwareManager': 4,
'DiskHardwareManager': 4
}
# NOTE(JayF): The real dict also has manager: hwm-object
# but we don't use it in the code under test
hwms = [
{'name': 'SpecificHardwareManager', 'support': 3},
{'name': 'FirmwareHardwareManager', 'support': 4},
{'name': 'DiskHardwareManager', 'support': 4},
]
mock_dispatch.side_effect = [manager_steps]
mock_managers.return_value = hwms
mock_dispatch.side_effect = [manager_steps, hardware_support]
expected_return = {
'hardware_manager_version': self.version,
'clean_steps': expected_steps

20
ironic_python_agent/tests/unit/extensions/test_deploy.py
View File

@ -32,12 +32,14 @@ class TestDeployExtension(base.IronicAgentTest):
}
self.version = {'generic': '1', 'specific': '1'}
@mock.patch('ironic_python_agent.hardware.get_managers_detail',
autospec=True)
@mock.patch('ironic_python_agent.hardware.get_current_versions',
autospec=True)
@mock.patch('ironic_python_agent.hardware.dispatch_to_all_managers',
autospec=True)
def test_get_deploy_steps(self, mock_dispatch, mock_version,
mock_cache_node):
mock_managers, mock_cache_node):
mock_version.return_value = self.version
manager_steps = {
@ -116,13 +118,17 @@ class TestDeployExtension(base.IronicAgentTest):
}
hardware_support = {
'SpecificHardwareManager': 3,
'FirmwareHardwareManager': 4,
'DiskHardwareManager': 4
}
# NOTE(JayF): The real dict also has manager: hwm-object
# but we don't use it in the code under test
hwms = [
{'name': 'SpecificHardwareManager', 'support': 3},
{'name': 'FirmwareHardwareManager', 'support': 4},
{'name': 'DiskHardwareManager', 'support': 4},
]
mock_dispatch.side_effect = [manager_steps]
mock_managers.return_value = hwms
mock_dispatch.side_effect = [manager_steps, hardware_support]
expected_return = {
'hardware_manager_version': self.version,
'deploy_steps': expected_steps

20
ironic_python_agent/tests/unit/extensions/test_service.py
View File

@ -34,12 +34,14 @@ class TestServiceExtension(base.IronicAgentTest):
}
self.version = {'generic': '1', 'specific': '1'}
@mock.patch('ironic_python_agent.hardware.get_managers_detail',
autospec=True)
@mock.patch('ironic_python_agent.hardware.get_current_versions',
autospec=True)
@mock.patch('ironic_python_agent.hardware.dispatch_to_all_managers',
autospec=True)
def test_get_service_steps(self, mock_dispatch, mock_version,
mock_cache_node):
mock_managers, mock_cache_node):
mock_version.return_value = self.version
manager_steps = {
@ -118,13 +120,17 @@ class TestServiceExtension(base.IronicAgentTest):
}
hardware_support = {
'SpecificHardwareManager': 3,
'FirmwareHardwareManager': 4,
'DiskHardwareManager': 4
}
# NOTE(JayF): The real dict also has manager: hwm-object
# but we don't use it in the code under test
hwms = [
{'name': 'SpecificHardwareManager', 'support': 3},
{'name': 'FirmwareHardwareManager', 'support': 4},
{'name': 'DiskHardwareManager', 'support': 4},
]
mock_dispatch.side_effect = [manager_steps]
mock_managers.return_value = hwms
mock_dispatch.side_effect = [manager_steps, hardware_support]
expected_return = {
'hardware_manager_version': self.version,
'service_steps': expected_steps

71
ironic_python_agent/tests/unit/test_hardware.py
View File

@ -81,12 +81,22 @@ BLK_DEVICE_TEMPLATE_PARTUUID_DEVICE = [
]
class FakeHardwareManager(hardware.GenericHardwareManager):
def __init__(self, hardware_support):
self._hardware_support = hardware_support
class FakeHardwareManager(hardware.HardwareManager):
def evaluate_hardware_support(self):
return self._hardware_support
return self.support
def _create_mock_hwm(name, support):
def set_support(self, x):
self.support = x
# note(JayF): This code creates a subclass of FakeHardwareManager with
# a unique name. Since we actually use the class name in IPA
# code as an identifier, we need to have a new class for each
# mock.
hwm = type(name, (FakeHardwareManager,), {'_set_support': set_support})()
hwm._set_support(support)
return hwm
class TestHardwareManagerLoading(base.IronicAgentTest):
@ -104,17 +114,58 @@ class TestHardwareManagerLoading(base.IronicAgentTest):
fake_ep.attrs = ['fake attrs']
ext1 = extension.Extension(
'fake_generic0', fake_ep, None,
FakeHardwareManager(hardware.HardwareSupport.GENERIC))
_create_mock_hwm("fake_generic0",
hardware.HardwareSupport.GENERIC))
ext2 = extension.Extension(
'fake_mainline0', fake_ep, None,
FakeHardwareManager(hardware.HardwareSupport.MAINLINE))
_create_mock_hwm("fake_mainline0",
hardware.HardwareSupport.MAINLINE))
ext3 = extension.Extension(
'fake_generic1', fake_ep, None,
FakeHardwareManager(hardware.HardwareSupport.GENERIC))
self.correct_hw_manager = ext2.obj
'fake_serviceprovider0', fake_ep, None,
_create_mock_hwm("fake_serviceprovider0",
hardware.HardwareSupport.SERVICE_PROVIDER))
# Note(JayF): Ensure these are added in an order other than priority
# order or else you may invalidate the entire test :)
self.fake_ext_mgr = extension.ExtensionManager.make_test_instance([
ext1, ext2, ext3
])
self.expected_detail_response = [
{'name': 'fake_serviceprovider0',
'support': hardware.HardwareSupport.SERVICE_PROVIDER,
'manager': ext3.obj},
{'name': 'fake_mainline0',
'support': hardware.HardwareSupport.MAINLINE,
'manager': ext2.obj},
{'name': 'fake_generic0',
'support': hardware.HardwareSupport.GENERIC,
'manager': ext1.obj},
]
self.expected_get_managers_response = [ext3.obj, ext2.obj, ext1.obj]
@mock.patch.object(hardware, '_get_extensions', autospec=True)
def test_get_managers(self, mock_extensions):
"""Test to ensure get_managers sorts and returns a list of HWMs.
The most meaningful part of this test is ensuring HWMs are in priority
order, with the highest hardware support value coming earlier in the
list of classes.
"""
mock_extensions.return_value = self.fake_ext_mgr
expected_names = [x.__class__.__name__
for x in self.expected_get_managers_response]
actual_names = [x.__class__.__name__
for x in hardware.get_managers()]
self.assertEqual(actual_names, expected_names)
@mock.patch.object(hardware, '_get_extensions', autospec=True)
def test_get_managers_detail(self, mock_extensions):
"""ensure get_manager_details returns a list of HWMs + metadata
These also need to be sorted in priority order
"""
mock_extensions.return_value = self.fake_ext_mgr
self.assertEqual(hardware.get_managers_detail(),
self.expected_detail_response)
@mock.patch.object(hardware, '_udev_settle', lambda *_: None)

7
releasenotes/notes/only-run-evaluate-hardware-support-once-9ec1ae327b4e03f2.yaml Normal file
View File

@ -0,0 +1,7 @@
---
fixes:
- |
Fixes bug 2066308, an issue where Ironic Python Agent would call
evaluate_hardware_support multiple times on hardware manager plugins.
Scanning for hardware and disks is time consuming, and caused timeouts
on badly-performing nodes.