openstack/ironic-python-agent
Code Issues Proposed changes

Split hardware manager initialize out of evaluate_hardware_support

Browse Source 
The current code in GenericHardware.evaluate_hardware_support ends up
using hardware manager calls, which then use partly initialized hardware
manager list and can even cause a recursion.

This change introduces a new optional call initialize() which is
guaranteed to run:
1) After all hardware managers have been evaluated
2) After the hardware manager cache is populated
3) In the order of the support level of hardware managers

Change-Id: I068d3d73483c161062aa3b48f3154a2d99941382
Signed-off-by: Dmitry Tantsur <dtantsur@protonmail.com>
This commit is contained in:
Dmitry Tantsur
2025-07-04 16:13:16 +02:00
parent 883e3cf057
commit 9426df9ab3
5 changed files with 97 additions and 60 deletions
Download Patch File Download Diff File Expand all files Collapse all files

31
ironic_python_agent/hardware.py
View File

@@ -926,7 +926,25 @@ class BootInfo(encoding.SerializableComparable):
class HardwareManager(object, metaclass=abc.ABCMeta):
@abc.abstractmethod
def evaluate_hardware_support(self):
pass
"""Evaluate the level of support for this hardware manager.
See the HardwareSupport object for more documentation.
:returns: One of the constants from the HardwareSupport object.
"""
def initialize(self):
"""Initialize the hardware manager.
This method is invoked for all hardware managers in the order of their
support level after their evaluate_hardware_support returns a value
greater than NONE.
Be careful when making hardware manager calls from initialize: other
hardware manager with the same or lower support level may not be
initialized yet. It's only safe when you're sure that the current
hardware manager provides the call.
"""
def list_network_interfaces(self):
raise errors.IncompatibleHardwareMethodError
@@ -1344,7 +1362,9 @@ class GenericHardwareManager(HardwareManager):
self._lshw_cache = None
def evaluate_hardware_support(self):
# Do some initialization before we declare ourself ready
return HardwareSupport.GENERIC
def initialize(self):
_check_for_iscsi()
_md_scan_and_assemble()
_load_ipmi_modules()
@@ -1353,7 +1373,6 @@ class GenericHardwareManager(HardwareManager):
MULTIPATH_ENABLED = _enable_multipath()
self.wait_for_disks()
return HardwareSupport.GENERIC
def list_hardware_info(self):
"""Return full hardware inventory as a serializable dict.
@@ -3588,6 +3607,12 @@ def get_managers_detail():
_global_managers = hwms
# NOTE(dtantsur): do not call initialize until all hardware managers
# are probed and properly cached!
for hwm in hwms:
LOG.debug('Initializing hardware manager %s', hwm['name'])
hwm['manager'].initialize()
return _global_managers

14
ironic_python_agent/hardware_managers/cna.py
View File

@@ -77,13 +77,15 @@ class IntelCnaHardwareManager(hardware.HardwareManager):
def evaluate_hardware_support(self):
if _detect_cna_card():
LOG.debug('Found Intel CNA network card')
# On Intel CNA cards, in order to make LLDP info collecting
# possible, the embedded LLDP agent, which runs inside that
# card, needs to be turned off.
if CONF.collect_lldp:
LOG.info('Disable CNA network card embedded lldp agent now')
_disable_embedded_lldp_agent_in_cna_card()
return hardware.HardwareSupport.MAINLINE
else:
LOG.debug('No Intel CNA network card found')
return hardware.HardwareSupport.NONE
def initialize(self):
# On Intel CNA cards, in order to make LLDP info collecting
# possible, the embedded LLDP agent, which runs inside that
# card, needs to be turned off.
if CONF.collect_lldp:
LOG.info('Disable CNA network card embedded lldp agent now')
_disable_embedded_lldp_agent_in_cna_card()

36
ironic_python_agent/tests/unit/hardware_managers/test_cna.py
View File

@@ -118,7 +118,7 @@ class TestIntelCnaHardwareManager(base.IronicAgentTest):
@mock.patch.object(cna, 'LOG', autospec=True)
@mock.patch.object(cna, '_detect_cna_card', autospec=True)
def test_evaluate_hardware_support_with_collect_lldp_disabled(
def test_evaluate_hardware_support(
self, mock_detect_card, mock_log):
mock_detect_card.return_value = True
expected_support = hardware.HardwareSupport.MAINLINE
@@ -126,21 +126,6 @@ class TestIntelCnaHardwareManager(base.IronicAgentTest):
self.assertEqual(expected_support, actual_support)
mock_log.debug.assert_called_once()
@mock.patch.object(cna, 'LOG', autospec=True)
@mock.patch.object(cna, '_detect_cna_card', autospec=True)
@mock.patch.object(cna, '_disable_embedded_lldp_agent_in_cna_card',
autospec=True)
def test_evaluate_hardware_support_with_collect_lldp_enabled(
self, mock_disable_lldp_agent, mock_detect_card, mock_log):
self.config(collect_lldp=True)
mock_detect_card.return_value = True
expected_support = hardware.HardwareSupport.MAINLINE
actual_support = self.hardware.evaluate_hardware_support()
self.assertEqual(expected_support, actual_support)
mock_log.debug.assert_called_once()
mock_log.info.assert_called_once()
mock_disable_lldp_agent.assert_called_once()
@mock.patch.object(cna, 'LOG', autospec=True)
@mock.patch.object(cna, '_detect_cna_card', autospec=True)
def test_evaluate_hardware_support_no_cna_card_detected(self,
@@ -151,3 +136,22 @@ class TestIntelCnaHardwareManager(base.IronicAgentTest):
actual_support = self.hardware.evaluate_hardware_support()
self.assertEqual(expected_support, actual_support)
mock_log.debug.assert_called_once()
@mock.patch.object(cna, 'LOG', autospec=True)
@mock.patch.object(cna, '_disable_embedded_lldp_agent_in_cna_card',
autospec=True)
def test_initialize_with_collect_lldp_disabled(
self, mock_disable_lldp_agent, mock_log):
self.hardware.initialize()
mock_log.info.assert_not_called()
mock_disable_lldp_agent.assert_not_called()
@mock.patch.object(cna, 'LOG', autospec=True)
@mock.patch.object(cna, '_disable_embedded_lldp_agent_in_cna_card',
autospec=True)
def test_initialize_with_collect_lldp_enabled(
self, mock_disable_lldp_agent, mock_log):
self.config(collect_lldp=True)
self.hardware.initialize()
mock_log.info.assert_called_once()
mock_disable_lldp_agent.assert_called_once()

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

@@ -5626,12 +5626,12 @@ class TestGenericHardwareManager(base.IronicAgentTest):
@mock.patch.object(hardware, '_md_scan_and_assemble', autospec=True)
@mock.patch.object(hardware, '_check_for_iscsi', autospec=True)
@mock.patch.object(time, 'sleep', autospec=True)
class TestEvaluateHardwareSupport(base.IronicAgentTest):
class TestInitializeSupport(base.IronicAgentTest):
def setUp(self):
super(TestEvaluateHardwareSupport, self).setUp()
super().setUp()
self.hardware = hardware.GenericHardwareManager()
def test_evaluate_hw_waits_for_disks(
def test_initialize_waits_for_disks(
self, mocked_sleep, mocked_check_for_iscsi,
mocked_md_assemble, mocked_get_inst_dev,
mocked_load_ipmi_modules, mocked_enable_mpath):
@@ -5640,33 +5640,31 @@ class TestEvaluateHardwareSupport(base.IronicAgentTest):
None
]
result = self.hardware.evaluate_hardware_support()
self.hardware.initialize()
self.assertTrue(mocked_load_ipmi_modules.called)
self.assertTrue(mocked_check_for_iscsi.called)
self.assertTrue(mocked_md_assemble.called)
self.assertEqual(hardware.HardwareSupport.GENERIC, result)
mocked_get_inst_dev.assert_called_with(mock.ANY)
self.assertEqual(2, mocked_get_inst_dev.call_count)
mocked_sleep.assert_called_once_with(CONF.disk_wait_delay)
@mock.patch.object(hardware, 'LOG', autospec=True)
def test_evaluate_hw_no_wait_for_disks(
def test_initialize_no_wait_for_disks(
self, mocked_log, mocked_sleep, mocked_check_for_iscsi,
mocked_md_assemble, mocked_get_inst_dev,
mocked_load_ipmi_modules, mocked_enable_mpath):
CONF.set_override('disk_wait_attempts', '0')
result = self.hardware.evaluate_hardware_support()
self.hardware.initialize()
self.assertTrue(mocked_check_for_iscsi.called)
self.assertEqual(hardware.HardwareSupport.GENERIC, result)
self.assertFalse(mocked_get_inst_dev.called)
self.assertFalse(mocked_sleep.called)
self.assertFalse(mocked_log.called)
@mock.patch.object(hardware, 'LOG', autospec=True)
def test_evaluate_hw_waits_for_disks_nonconfigured(
def test_initialize_waits_for_disks_nonconfigured(
self, mocked_log, mocked_sleep, mocked_check_for_iscsi,
mocked_md_assemble, mocked_get_inst_dev,
mocked_load_ipmi_modules, mocked_enable_mpath):
@@ -5685,7 +5683,7 @@ class TestEvaluateHardwareSupport(base.IronicAgentTest):
None
]
self.hardware.evaluate_hardware_support()
self.hardware.initialize()
mocked_get_inst_dev.assert_called_with(mock.ANY)
self.assertEqual(10, mocked_get_inst_dev.call_count)
@@ -5696,13 +5694,13 @@ class TestEvaluateHardwareSupport(base.IronicAgentTest):
CONF.disk_wait_delay * 9)
@mock.patch.object(hardware, 'LOG', autospec=True)
def test_evaluate_hw_waits_for_disks_configured(self, mocked_log,
mocked_sleep,
mocked_check_for_iscsi,
mocked_md_assemble,
mocked_get_inst_dev,
mocked_load_ipmi_modules,
mocked_enable_mpath):
def test_initialize_waits_for_disks_configured(self, mocked_log,
mocked_sleep,
mocked_check_for_iscsi,
mocked_md_assemble,
mocked_get_inst_dev,
mocked_load_ipmi_modules,
mocked_enable_mpath):
CONF.set_override('disk_wait_attempts', '1')
mocked_get_inst_dev.side_effect = [
@@ -5711,7 +5709,7 @@ class TestEvaluateHardwareSupport(base.IronicAgentTest):
None
]
self.hardware.evaluate_hardware_support()
self.hardware.initialize()
mocked_get_inst_dev.assert_called_with(mock.ANY)
self.assertEqual(1, mocked_get_inst_dev.call_count)
@@ -5719,36 +5717,35 @@ class TestEvaluateHardwareSupport(base.IronicAgentTest):
mocked_log.warning.assert_called_once_with(
'The root device was not detected')
def test_evaluate_hw_disks_timeout_unconfigured(self, mocked_sleep,
mocked_check_for_iscsi,
mocked_md_assemble,
mocked_get_inst_dev,
mocked_load_ipmi_modules,
mocked_enable_mpath):
def test_initialize_disks_timeout_unconfigured(self, mocked_sleep,
mocked_check_for_iscsi,
mocked_md_assemble,
mocked_get_inst_dev,
mocked_load_ipmi_modules,
mocked_enable_mpath):
mocked_get_inst_dev.side_effect = errors.DeviceNotFound('boom')
self.hardware.evaluate_hardware_support()
self.hardware.initialize()
mocked_sleep.assert_called_with(3)
def test_evaluate_hw_disks_timeout_configured(self, mocked_sleep,
mocked_check_for_iscsi,
mocked_md_assemble,
mocked_root_dev,
mocked_load_ipmi_modules,
mocked_enable_mpath):
def test_initialize_disks_timeout_configured(self, mocked_sleep,
mocked_check_for_iscsi,
mocked_md_assemble,
mocked_root_dev,
mocked_load_ipmi_modules,
mocked_enable_mpath):
CONF.set_override('disk_wait_delay', '5')
mocked_root_dev.side_effect = errors.DeviceNotFound('boom')
self.hardware.evaluate_hardware_support()
self.hardware.initialize()
mocked_sleep.assert_called_with(5)
def test_evaluate_hw_disks_timeout(
def test_initialize_disks_timeout(
self, mocked_sleep, mocked_check_for_iscsi,
mocked_md_assemble, mocked_get_inst_dev,
mocked_load_ipmi_modules,
mocked_enable_mpath):
mocked_get_inst_dev.side_effect = errors.DeviceNotFound('boom')
result = self.hardware.evaluate_hardware_support()
self.assertEqual(hardware.HardwareSupport.GENERIC, result)
self.hardware.initialize()
mocked_get_inst_dev.assert_called_with(mock.ANY)
self.assertEqual(CONF.disk_wait_attempts,
mocked_get_inst_dev.call_count)

9
releasenotes/notes/hwmgr-init-8f9c9b6205f64966.yaml Normal file
View File

@@ -0,0 +1,9 @@
---
deprecations:
- |
Some hardware managers include complex initialization logic in their
``evaluate_hardware_support`` calls. This behavior could cause various
issues (such as recursion on attempt to invoke other hardware manager
calls) and is now deprecated. Please move the initialization login into
the new ``initialize`` call, which is guaranteed to run after all
hardware managers have been cached and only for enabled hardware managers.