From 83ce7c4264a57658fb749c740e6b32e3eb6a906b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aija=20Jaunt=C4=93va?= Date: Fri, 2 Oct 2020 11:04:01 -0400 Subject: [PATCH] Fix idrac-wsman BIOS step async 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. And improve coverage to test this error handling and also happy paths. Story: 2008307 Task: 41197 Change-Id: I1e957c2b526abc37920212b6431b11eedc9f89be --- ironic/drivers/modules/drac/bios.py | 20 +++-- .../unit/drivers/modules/drac/test_bios.py | 82 ++++++++++++++++++- ...-step-error-handling-80cd30c54c71c595.yaml | 8 ++ 3 files changed, 101 insertions(+), 9 deletions(-) create mode 100644 releasenotes/notes/fix-wsman-bios-async-step-error-handling-80cd30c54c71c595.yaml diff --git a/ironic/drivers/modules/drac/bios.py b/ironic/drivers/modules/drac/bios.py index bc09c0176c..db4a497a10 100644 --- a/ironic/drivers/modules/drac/bios.py +++ b/ironic/drivers/modules/drac/bios.py @@ -261,14 +261,18 @@ class DracWSManBIOS(base.BIOSInterface): :param task: a TaskManager instance with node to act on :param config_job: a python-dracclient Job object (named tuple) """ - LOG.error("BIOS configuration job failed for node %(node)s. " - "Failed config job: %(config_job_id)s. " - "Message: '%(message)s'.", - {'node': task.node_uuid, 'config_job_id': config_job.id, - 'message': config_job.message}) - task.node.last_error = config_job.message - # tell conductor to handle failure of clean/deploy step - task.process_event('fail') + error_msg = (_("Failed config job: %(config_job_id)s. " + "Message: '%(message)s'.") % + {'config_job_id': config_job.id, + 'message': config_job.message}) + log_msg = ("BIOS configuration job failed for node %(node)s. " + "%(error)s " % + {'node': task.node.uuid, + 'error': error_msg}) + if task.node.clean_step: + manager_utils.cleaning_error_handler(task, log_msg, error_msg) + else: + manager_utils.deploying_error_handler(task, log_msg, error_msg) def _resume_current_operation(self, task): """Continue cleaning/deployment of the node. diff --git a/ironic/tests/unit/drivers/modules/drac/test_bios.py b/ironic/tests/unit/drivers/modules/drac/test_bios.py index 6fdca0684d..3903d979c4 100644 --- a/ironic/tests/unit/drivers/modules/drac/test_bios.py +++ b/ironic/tests/unit/drivers/modules/drac/test_bios.py @@ -1,6 +1,6 @@ # -*- coding: utf-8 -*- # -# Copyright (c) 2015-2016 Dell Inc. or its subsidiaries. +# Copyright (c) 2015-2021 Dell Inc. or its subsidiaries. # All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"); you may @@ -26,6 +26,7 @@ from dracclient import exceptions as drac_exceptions from ironic.common import exception from ironic.common import states from ironic.conductor import task_manager +from ironic.conductor import utils as manager_utils from ironic.drivers.modules import deploy_utils from ironic.drivers.modules.drac import bios as drac_bios from ironic.drivers.modules.drac import common as drac_common @@ -238,6 +239,85 @@ class DracWSManBIOSConfigurationTestCase(test_utils.BaseDracTest): self.assertRaises(exception.DracOperationError, task.driver.bios.factory_reset, task) + @mock.patch.object(manager_utils, 'notify_conductor_resume_clean', + autospec=True) + @mock.patch.object(drac_job, 'get_job', spec_set=True, + autospec=True) + def test__check_node_bios_jobs(self, mock_get_job, + mock_notify_conductor_resume_clean): + mock_job = mock.Mock() + mock_job.status = 'Completed' + mock_get_job.return_value = mock_job + + with task_manager.acquire(self.context, self.node.uuid) as task: + driver_internal_info = task.node.driver_internal_info + driver_internal_info['bios_config_job_ids'] = ['123', '789'] + task.node.driver_internal_info = driver_internal_info + task.node.clean_step = {'priority': 100, 'interface': 'bios', + 'step': 'factory_reset', 'argsinfo': {}} + task.node.save() + mock_cache = mock.Mock() + task.driver.bios.cache_bios_settings = mock_cache + + task.driver.bios._check_node_bios_jobs(task) + + self.assertEqual([], task.node.driver_internal_info.get( + 'bios_config_job_ids')) + mock_cache.assert_called_once_with(task) + mock_notify_conductor_resume_clean.assert_called_once_with(task) + + @mock.patch.object(drac_job, 'get_job', spec_set=True, + autospec=True) + def test__check_node_bios_jobs_still_running(self, mock_get_job): + mock_job = mock.Mock() + mock_job.status = 'Running' + mock_get_job.return_value = mock_job + + with task_manager.acquire(self.context, self.node.uuid) as task: + driver_internal_info = task.node.driver_internal_info + driver_internal_info['bios_config_job_ids'] = ['123'] + task.node.driver_internal_info = driver_internal_info + task.node.save() + mock_resume = mock.Mock() + task.driver.bios._resume_current_operation = mock_resume + mock_cache = mock.Mock() + task.driver.bios.cache_bios_settings = mock_cache + + task.driver.bios._check_node_bios_jobs(task) + + self.assertEqual(['123'], + task.node.driver_internal_info.get( + 'bios_config_job_ids')) + mock_cache.assert_not_called() + mock_resume.assert_not_called() + + @mock.patch.object(manager_utils, 'cleaning_error_handler', autospec=True) + @mock.patch.object(drac_job, 'get_job', spec_set=True, + autospec=True) + def test__check_node_bios_jobs_failed(self, mock_get_job, + mock_cleaning_error_handler): + mock_job = mock.Mock() + mock_job.status = 'Failed' + mock_job.id = '123' + mock_job.message = 'Invalid' + mock_get_job.return_value = mock_job + + with task_manager.acquire(self.context, self.node.uuid) as task: + driver_internal_info = task.node.driver_internal_info + driver_internal_info['bios_config_job_ids'] = ['123'] + task.node.driver_internal_info = driver_internal_info + task.node.clean_step = {'priority': 100, 'interface': 'bios', + 'step': 'factory_reset', 'argsinfo': {}} + task.node.save() + + task.driver.bios._check_node_bios_jobs(task) + + self.assertEqual([], + task.node.driver_internal_info.get( + 'bios_config_job_ids')) + mock_cleaning_error_handler.assert_called_once_with( + task, mock.ANY, "Failed config job: 123. Message: 'Invalid'.") + class DracBIOSConfigurationTestCase(test_utils.BaseDracTest): diff --git a/releasenotes/notes/fix-wsman-bios-async-step-error-handling-80cd30c54c71c595.yaml b/releasenotes/notes/fix-wsman-bios-async-step-error-handling-80cd30c54c71c595.yaml new file mode 100644 index 0000000000..ca39a1512d --- /dev/null +++ b/releasenotes/notes/fix-wsman-bios-async-step-error-handling-80cd30c54c71c595.yaml @@ -0,0 +1,8 @@ +--- +fixes: + - | + Fixes ``idrac-wsman`` BIOS ``apply_configuration`` and ``factory_reset`` + clean and deploy steps to fail correctly in case of error when checking + completed jobs. Before the fix when BIOS job failed, then node clean or + deploy failed with timeout instead of actual error in cleaning or + deploying step.