From 70b7ca345f39b5126b5e68aa507298b623932dd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aija=20Jaunt=C4=93va?= Date: Fri, 2 Oct 2020 07:43:33 -0400 Subject: [PATCH] Fix redfish BIOS apply config error handling Instead of using process_event('fail') use error_handlers, otherwise in case of failure node gets stuck and fails because of timeout, instead of failing earlier due to step failure. Besides adding new unit tests, also update related unit tests to test for success correctly and have realistic data. Story: 2008307 Task: 41196 Change-Id: If28ccb252a87610e3fd3dc78e1ed75bb8ca1cdcf --- ironic/drivers/modules/redfish/bios.py | 10 +-- .../unit/drivers/modules/redfish/test_bios.py | 74 +++++++++++++++++-- ...ation-error-handling-464695b09e4f81ac.yaml | 8 ++ 3 files changed, 79 insertions(+), 13 deletions(-) create mode 100644 releasenotes/notes/fix-redfish-bios-apply-configuration-error-handling-464695b09e4f81ac.yaml diff --git a/ironic/drivers/modules/redfish/bios.py b/ironic/drivers/modules/redfish/bios.py index 018a8d9555..f9f0a8c5f4 100644 --- a/ironic/drivers/modules/redfish/bios.py +++ b/ironic/drivers/modules/redfish/bios.py @@ -332,8 +332,8 @@ class RedfishBIOS(base.BIOSInterface): last_error = (_('Redfish BIOS apply_configuration step failed. ' 'Attributes %(attrs)s are not updated.') % {'attrs': attrs_not_updated}) - LOG.error(error_msg) - task.node.last_error = last_error - if task.node.provision_state in [states.CLEANING, states.CLEANWAIT, - states.DEPLOYING, states.DEPLOYWAIT]: - task.process_event('fail') + if task.node.provision_state in [states.CLEANING, states.CLEANWAIT]: + LOG.error(error_msg) + manager_utils.cleaning_error_handler(task, last_error) + if task.node.provision_state in [states.DEPLOYING, states.DEPLOYWAIT]: + manager_utils.deploying_error_handler(task, error_msg, last_error) diff --git a/ironic/tests/unit/drivers/modules/redfish/test_bios.py b/ironic/tests/unit/drivers/modules/redfish/test_bios.py index 95c61ee762..30aab95876 100644 --- a/ironic/tests/unit/drivers/modules/redfish/test_bios.py +++ b/ironic/tests/unit/drivers/modules/redfish/test_bios.py @@ -236,7 +236,8 @@ class RedfishBiosTestCase(db_base.DbTestCase): self._test_step_pre_reboot() @mock.patch.object(redfish_utils, 'get_system', autospec=True) - def _test_step_post_reboot(self, mock_get_system): + def _test_step_post_reboot(self, mock_get_system, + attributes_after_reboot=None): if self.node.deploy_step: step_data = self.node.deploy_step else: @@ -246,6 +247,14 @@ class RedfishBiosTestCase(db_base.DbTestCase): if step == 'factory_reset': check_fields = ['post_factory_reset_reboot_requested'] if step == 'apply_configuration': + mock_bios = mock.Mock() + # if attributes after reboot not provided then mimic success + # by returning the same as requested + mock_bios.attributes = attributes_after_reboot \ + or self.node.driver_internal_info['requested_bios_attrs'] + mock_system = mock.Mock() + mock_system.bios = mock_bios + mock_get_system.return_value = mock_system check_fields = ['post_config_reboot_requested', 'requested_bios_attrs'] with task_manager.acquire(self.context, self.node.uuid, @@ -279,13 +288,56 @@ class RedfishBiosTestCase(db_base.DbTestCase): node.save() self._test_step_post_reboot() - def test_apply_conf_post_reboot_cleaning(self): - data = [{'name': 'ProcTurboMode', 'value': 'Disabled'}, + @mock.patch.object(manager_utils, 'cleaning_error_handler', autospec=True) + @mock.patch.object(manager_utils, 'deploying_error_handler', autospec=True) + def test_apply_conf_post_reboot_cleaning(self, + mock_deploying_error_handler, + mock_cleaning_error_handler): + data = [{'name': 'ProcTurboMode', 'value': 'Enabled'}, {'name': 'NicBoot1', 'value': 'NetworkBoot'}] + self.node.clean_step = {'priority': 100, 'interface': 'bios', + 'step': 'apply_configuration', + 'argsinfo': {'settings': data}} + requested_attrs = {'ProcTurboMode': 'Enabled', + 'NicBoot1': 'NetworkBoot'} + node = self.node + driver_internal_info = node.driver_internal_info + driver_internal_info['post_config_reboot_requested'] = True + driver_internal_info['requested_bios_attrs'] = requested_attrs + self.node.driver_internal_info = driver_internal_info + self.node.save() + self._test_step_post_reboot() + mock_cleaning_error_handler.assert_not_called() + mock_deploying_error_handler.assert_not_called() + + @mock.patch.object(manager_utils, 'cleaning_error_handler', autospec=True) + def test_apply_conf_post_reboot_cleaning_failed( + self, mock_cleaning_error_handler): + data = [{'name': 'ProcTurboMode', 'value': 'Enabled'}] self.node.clean_step = {'priority': 100, 'interface': 'bios', 'step': 'apply_configuration', 'argsinfo': {'settings': data}} requested_attrs = {'ProcTurboMode': 'Enabled'} + attributes_after_reboot = {'ProcTurboMode': 'Disabled'} + node = self.node + driver_internal_info = node.driver_internal_info + driver_internal_info['post_config_reboot_requested'] = True + driver_internal_info['requested_bios_attrs'] = requested_attrs + self.node.driver_internal_info = driver_internal_info + self.node.provision_state = states.CLEANING + self.node.save() + self._test_step_post_reboot( + attributes_after_reboot=attributes_after_reboot) + mock_cleaning_error_handler.assert_called_once() + + def test_apply_conf_post_reboot_deploying(self): + data = [{'name': 'ProcTurboMode', 'value': 'Disabled'}, + {'name': 'NicBoot1', 'value': 'NetworkBoot'}] + self.node.deploy_step = {'priority': 100, 'interface': 'bios', + 'step': 'apply_configuration', + 'argsinfo': {'settings': data}} + requested_attrs = {'ProcTurboMode': 'Enabled', + 'NicBoot1': 'NetworkBoot'} node = self.node driver_internal_info = node.driver_internal_info driver_internal_info['post_config_reboot_requested'] = True @@ -294,20 +346,25 @@ class RedfishBiosTestCase(db_base.DbTestCase): self.node.save() self._test_step_post_reboot() - def test_apply_conf_post_reboot_deploying(self): - data = [{'name': 'ProcTurboMode', 'value': 'Disabled'}, - {'name': 'NicBoot1', 'value': 'NetworkBoot'}] + @mock.patch.object(manager_utils, 'deploying_error_handler', autospec=True) + def test_apply_conf_post_reboot_deploying_failed( + self, mock_deploying_error_handler): + data = [{'name': 'ProcTurboMode', 'value': 'Enabled'}] self.node.deploy_step = {'priority': 100, 'interface': 'bios', 'step': 'apply_configuration', 'argsinfo': {'settings': data}} requested_attrs = {'ProcTurboMode': 'Enabled'} + attributes_after_reboot = {'ProcTurboMode': 'Disabled'} node = self.node driver_internal_info = node.driver_internal_info driver_internal_info['post_config_reboot_requested'] = True driver_internal_info['requested_bios_attrs'] = requested_attrs self.node.driver_internal_info = driver_internal_info + self.node.provision_state = states.DEPLOYWAIT self.node.save() - self._test_step_post_reboot() + self._test_step_post_reboot( + attributes_after_reboot=attributes_after_reboot) + mock_deploying_error_handler.assert_called_once() @mock.patch.object(redfish_utils, 'get_system', autospec=True) def test_factory_reset_fail(self, mock_get_system): @@ -345,7 +402,8 @@ class RedfishBiosTestCase(db_base.DbTestCase): def test_check_bios_attrs(self, mock_get_system): settings = [{'name': 'ProcTurboMode', 'value': 'Disabled'}, {'name': 'NicBoot1', 'value': 'NetworkBoot'}] - requested_attrs = {'ProcTurboMode': 'Enabled'} + requested_attrs = {'ProcTurboMode': 'Enabled', + 'NicBoot1': 'NetworkBoot'} with task_manager.acquire(self.context, self.node.uuid, shared=False) as task: attributes = mock_get_system(task.node).bios.attributes diff --git a/releasenotes/notes/fix-redfish-bios-apply-configuration-error-handling-464695b09e4f81ac.yaml b/releasenotes/notes/fix-redfish-bios-apply-configuration-error-handling-464695b09e4f81ac.yaml new file mode 100644 index 0000000000..ecae3d9331 --- /dev/null +++ b/releasenotes/notes/fix-redfish-bios-apply-configuration-error-handling-464695b09e4f81ac.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + Fixes ``redfish`` BIOS ``apply_configuration`` clean and deploy step to + fail correctly in case of error when checking if BIOS updates are + successfully applied. Before the fix when BIOS updates were unsuccessful, + then node cleaning or deploying failed with timeout instead of actual error + in clean or deploy step.