From 1861c936500f5aac6cd36f25762f817007bf9ba0 Mon Sep 17 00:00:00 2001 From: Shivanand Tendulker Date: Wed, 23 Sep 2020 10:07:41 -0400 Subject: [PATCH] Use correct error handler instead of calling process_event('fail') Drivers should be using error handler deploying_error_handler() or cleaning_error_handler() from conductor.utils for handling the failures of deploy or clean step instead of directly calling process_event('fail'). These handlers perform necessary cleaning before moving the node to failed state. Conflicts: ironic/drivers/modules/ilo/management.py ironic/tests/unit/drivers/modules/ilo/test_management.py ironic/tests/unit/drivers/modules/ilo/test_raid.py NOTE(stendulker): The conflict is due to not having following changes in Train: 9fb4074bfef7b6e4ead7d3cedf7120668966eaa1 d927dd0d0af615d26a8ab662f88b058ace6c6798 Change-Id: I5d9b6831137cdef171a9929f144956f741390953 (cherry picked from commit 76a6fca8d7c662779097ceeab5acc869d7a08437) --- ironic/drivers/modules/ilo/management.py | 9 +-------- ironic/drivers/modules/ilo/raid.py | 6 ++++-- .../drivers/modules/ilo/test_management.py | 8 ++++---- .../unit/drivers/modules/ilo/test_raid.py | 20 +++++++++++++------ 4 files changed, 23 insertions(+), 20 deletions(-) diff --git a/ironic/drivers/modules/ilo/management.py b/ironic/drivers/modules/ilo/management.py index b8cf400d07..1c5bd48d69 100644 --- a/ironic/drivers/modules/ilo/management.py +++ b/ironic/drivers/modules/ilo/management.py @@ -701,13 +701,6 @@ class Ilo5Management(IloManagement): task.node.driver_internal_info = driver_internal_info task.node.save() - def _set_clean_failed(self, task, msg): - LOG.error("Out-of-band sanitize disk erase job failed for node " - "%(node)s. Message: '%(message)s'.", - {'node': task.node.uuid, 'message': msg}) - task.node.last_error = msg - task.process_event('fail') - def _wait_for_disk_erase_status(self, node): """Wait for out-of-band sanitize disk erase to be completed.""" interval = CONF.ilo.oob_erase_devices_job_status_interval @@ -842,4 +835,4 @@ class Ilo5Management(IloManagement): 'ilo_disk_erase_ssd_check', 'cleaning_reboot', 'skip_current_clean_step') - self._set_clean_failed(task, ilo_exception) + manager_utils.cleaning_error_handler(task, ilo_exception) diff --git a/ironic/drivers/modules/ilo/raid.py b/ironic/drivers/modules/ilo/raid.py index 1b528fc0ea..4390bc1934 100644 --- a/ironic/drivers/modules/ilo/raid.py +++ b/ironic/drivers/modules/ilo/raid.py @@ -72,8 +72,10 @@ class Ilo5RAID(base.RAIDInterface): LOG.error("RAID configuration job failed for node %(node)s. " "Message: '%(message)s'.", {'node': task.node.uuid, 'message': msg}) - task.node.last_error = msg - task.process_event('fail') + if task.node.provision_state == states.DEPLOYING: + manager_utils.deploying_error_handler(task, msg, msg) + else: + manager_utils.cleaning_error_handler(task, msg) def _set_driver_internal_true_value(self, task, *keys): driver_internal_info = task.node.driver_internal_info diff --git a/ironic/tests/unit/drivers/modules/ilo/test_management.py b/ironic/tests/unit/drivers/modules/ilo/test_management.py index 13623bfa54..06c8e1550e 100644 --- a/ironic/tests/unit/drivers/modules/ilo/test_management.py +++ b/ironic/tests/unit/drivers/modules/ilo/test_management.py @@ -1375,8 +1375,9 @@ class Ilo5ManagementTestCase(db_base.DbTestCase): task, erase_pattern={'ssd': 'xyz'}) @mock.patch.object(ilo_common, 'get_ilo_object', autospec=True) - @mock.patch.object(ilo_management.Ilo5Management, '_set_clean_failed') - def test_erase_devices_hdd_ilo_error(self, set_clean_failed_mock, + @mock.patch.object(manager_utils, 'cleaning_error_handler', + autospec=True) + def test_erase_devices_hdd_ilo_error(self, clean_err_handler_mock, ilo_mock): ilo_mock_object = ilo_mock.return_value ilo_mock_object.get_available_disk_types.return_value = ['HDD'] @@ -1395,5 +1396,4 @@ class Ilo5ManagementTestCase(db_base.DbTestCase): task.node.driver_internal_info) self.assertNotIn('skip_current_clean_step', task.node.driver_internal_info) - set_clean_failed_mock.assert_called_once_with( - task, exc) + clean_err_handler_mock.assert_called_once_with(task, exc) diff --git a/ironic/tests/unit/drivers/modules/ilo/test_raid.py b/ironic/tests/unit/drivers/modules/ilo/test_raid.py index 7e89c2160f..257d6272e8 100644 --- a/ironic/tests/unit/drivers/modules/ilo/test_raid.py +++ b/ironic/tests/unit/drivers/modules/ilo/test_raid.py @@ -387,38 +387,46 @@ class Ilo5RAIDTestCase(db_base.DbTestCase): self.node.save() self._test_create_configuration_skip_root_skip_non_root() - @mock.patch.object(ilo_raid.Ilo5RAID, '_set_step_failed') + @mock.patch.object(manager_utils, 'cleaning_error_handler', + autospec=True) + @mock.patch.object(manager_utils, 'deploying_error_handler', + autospec=True) @mock.patch.object(ilo_common, 'get_ilo_object', autospec=True) def _test_create_configuration_ilo_error(self, ilo_mock, - set_step_failed_mock): + deploy_err_handler_mock, + clean_err_handler_mock): ilo_mock_object = ilo_mock.return_value exc = ilo_error.IloError('error') ilo_mock_object.create_raid_configuration.side_effect = exc + msg = ('Failed to create raid configuration on ' + 'node %s' % self.node.uuid) with task_manager.acquire(self.context, self.node.uuid) as task: task.driver.raid.create_configuration( task, create_nonroot_volumes=False) - set_step_failed_mock.assert_called_once_with( - task, - 'Failed to create raid configuration ' - 'on node %s' % self.node.uuid, exc) self.assertNotIn('ilo_raid_create_in_progress', task.node.driver_internal_info) if task.node.clean_step: self.assertNotIn('skip_current_clean_step', task.node.driver_internal_info) + clean_err_handler_mock.assert_called_once_with( + task, msg) else: self.assertNotIn('skip_current_deploy_step', task.node.driver_internal_info) + deploy_err_handler_mock.assert_called_once_with( + task, msg, msg) def test_create_configuration_ilo_error_cleaning(self): self.node.clean_step = {'step': 'create_configuration', 'interface': 'raid'} + self.node.provision_state = states.CLEANING self.node.save() self._test_create_configuration_ilo_error() def test_create_configuration_ilo_error_cleaning_deploying(self): self.node.deploy_step = {'step': 'create_configuration', 'interface': 'raid'} + self.node.provision_state = states.DEPLOYING self.node.save() self._test_create_configuration_ilo_error()