From 9426df9ab35387d680fc258fa8e81ade7166519a Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Fri, 4 Jul 2025 16:13:16 +0200 Subject: [PATCH] Split hardware manager initialize out of evaluate_hardware_support 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 --- ironic_python_agent/hardware.py | 31 ++++++++- ironic_python_agent/hardware_managers/cna.py | 14 ++-- .../tests/unit/hardware_managers/test_cna.py | 36 +++++----- .../tests/unit/test_hardware.py | 67 +++++++++---------- .../notes/hwmgr-init-8f9c9b6205f64966.yaml | 9 +++ 5 files changed, 97 insertions(+), 60 deletions(-) create mode 100644 releasenotes/notes/hwmgr-init-8f9c9b6205f64966.yaml diff --git a/ironic_python_agent/hardware.py b/ironic_python_agent/hardware.py index 965c0fabf..4c1aac3fb 100644 --- a/ironic_python_agent/hardware.py +++ b/ironic_python_agent/hardware.py @@ -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 diff --git a/ironic_python_agent/hardware_managers/cna.py b/ironic_python_agent/hardware_managers/cna.py index a4021c10f..082bc8e8f 100644 --- a/ironic_python_agent/hardware_managers/cna.py +++ b/ironic_python_agent/hardware_managers/cna.py @@ -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() diff --git a/ironic_python_agent/tests/unit/hardware_managers/test_cna.py b/ironic_python_agent/tests/unit/hardware_managers/test_cna.py index 29c456a7f..6f76765dd 100644 --- a/ironic_python_agent/tests/unit/hardware_managers/test_cna.py +++ b/ironic_python_agent/tests/unit/hardware_managers/test_cna.py @@ -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() diff --git a/ironic_python_agent/tests/unit/test_hardware.py b/ironic_python_agent/tests/unit/test_hardware.py index 6cb38bf6a..7da6aa222 100644 --- a/ironic_python_agent/tests/unit/test_hardware.py +++ b/ironic_python_agent/tests/unit/test_hardware.py @@ -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) diff --git a/releasenotes/notes/hwmgr-init-8f9c9b6205f64966.yaml b/releasenotes/notes/hwmgr-init-8f9c9b6205f64966.yaml new file mode 100644 index 000000000..0b4ea2fec --- /dev/null +++ b/releasenotes/notes/hwmgr-init-8f9c9b6205f64966.yaml @@ -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.