From 0e7c6f97884599d2825f871812d25e85304ce93c Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Mon, 13 Mar 2023 19:02:33 +0100 Subject: Refactoring: create ironic.conductor.inspection ... to reduce the already frightening size of ironic.conductor.manager and make space for more inspection additions. While here, fix up log messages for clarity and brevity. Change-Id: I5196d58016ae094f17e0aad187a11d9cceaab04b --- ironic/conductor/inspection.py | 108 ++++++++++++++++++++++ ironic/conductor/manager.py | 83 +---------------- ironic/tests/unit/conductor/test_inspection.py | 118 +++++++++++++++++++++++++ ironic/tests/unit/conductor/test_manager.py | 116 +----------------------- 4 files changed, 232 insertions(+), 193 deletions(-) create mode 100644 ironic/conductor/inspection.py create mode 100644 ironic/tests/unit/conductor/test_inspection.py (limited to 'ironic') diff --git a/ironic/conductor/inspection.py b/ironic/conductor/inspection.py new file mode 100644 index 000000000..53c76e99d --- /dev/null +++ b/ironic/conductor/inspection.py @@ -0,0 +1,108 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +"""Inspection implementation for the conductor.""" + +from oslo_log import log +from oslo_utils import excutils + +from ironic.common import exception +from ironic.common.i18n import _ +from ironic.common import states +from ironic.conductor import task_manager +from ironic.conductor import utils + +LOG = log.getLogger(__name__) + + +@task_manager.require_exclusive_lock +def inspect_hardware(task): + """Initiates inspection. + + :param task: a TaskManager instance with an exclusive lock + on its node. + :raises: HardwareInspectionFailure if driver doesn't + return the state as states.MANAGEABLE, states.INSPECTWAIT. + + """ + node = task.node + + def handle_failure(e, log_func=LOG.error): + utils.node_history_record(task.node, event=e, + event_type=states.INTROSPECTION, + error=True, user=task.context.user_id) + task.process_event('fail') + log_func("Failed to inspect node %(node)s: %(err)s", + {'node': node.uuid, 'err': e}) + + # Inspection cannot start in fast-track mode, wipe token and URL. + utils.wipe_token_and_url(task) + + try: + new_state = task.driver.inspect.inspect_hardware(task) + except exception.IronicException as e: + with excutils.save_and_reraise_exception(): + error = str(e) + handle_failure(error) + except Exception as e: + error = (_('Unexpected exception of type %(type)s: %(msg)s') % + {'type': type(e).__name__, 'msg': e}) + handle_failure(error, log_func=LOG.exception) + raise exception.HardwareInspectionFailure(error=error) + + if new_state == states.MANAGEABLE: + task.process_event('done') + LOG.info('Successfully inspected node %(node)s', + {'node': node.uuid}) + elif new_state == states.INSPECTWAIT: + task.process_event('wait') + LOG.info('Successfully started introspection on node %(node)s', + {'node': node.uuid}) + else: + error = (_("During inspection, driver returned unexpected " + "state %(state)s") % {'state': new_state}) + handle_failure(error) + raise exception.HardwareInspectionFailure(error=error) + + +@task_manager.require_exclusive_lock +def abort_inspection(task): + """Abort inspection for the node.""" + node = task.node + + try: + task.driver.inspect.abort(task) + except exception.UnsupportedDriverExtension: + with excutils.save_and_reraise_exception(): + LOG.error('Inspect interface "%(intf)s" does not support abort ' + 'operation for node %(node)s', + {'intf': node.inspect_interface, 'node': node.uuid}) + except Exception as e: + with excutils.save_and_reraise_exception(): + LOG.exception('Error when aborting inspection of node %(node)s', + {'node': node.uuid}) + error = _('Failed to abort inspection: %s') % e + utils.node_history_record(task.node, event=error, + event_type=states.INTROSPECTION, + error=True, + user=task.context.user_id) + node.save() + + error = _('Inspection was aborted by request.') + utils.node_history_record(task.node, event=error, + event_type=states.INTROSPECTION, + error=True, + user=task.context.user_id) + utils.wipe_token_and_url(task) + task.process_event('abort') + LOG.info('Successfully aborted inspection of node %(node)s', + {'node': node.uuid}) diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 74e3192cf..bbd2355bd 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -63,6 +63,7 @@ from ironic.conductor import allocations from ironic.conductor import base_manager from ironic.conductor import cleaning from ironic.conductor import deployments +from ironic.conductor import inspection from ironic.conductor import notification_utils as notify_utils from ironic.conductor import periodics from ironic.conductor import steps as conductor_steps @@ -1365,35 +1366,7 @@ class ConductorManager(base_manager.BaseConductorManager): return if node.provision_state == states.INSPECTWAIT: - try: - task.driver.inspect.abort(task) - except exception.UnsupportedDriverExtension: - with excutils.save_and_reraise_exception(): - intf_name = task.driver.inspect.__class__.__name__ - LOG.error('Inspect interface %(intf)s does not ' - 'support abort operation when aborting ' - 'inspection of node %(node)s', - {'intf': intf_name, 'node': node.uuid}) - except Exception as e: - with excutils.save_and_reraise_exception(): - LOG.exception('Error in aborting the inspection of ' - 'node %(node)s', {'node': node.uuid}) - error = _('Failed to abort inspection: %s') % e - utils.node_history_record(task.node, event=error, - event_type=states.INTROSPECTION, - error=True, - user=task.context.user_id) - node.save() - error = _('Inspection was aborted by request.') - utils.node_history_record(task.node, event=error, - event_type=states.INTROSPECTION, - error=True, - user=task.context.user_id) - utils.wipe_token_and_url(task) - task.process_event('abort') - LOG.info('Successfully aborted inspection of node %(node)s', - {'node': node.uuid}) - return + return inspection.abort_inspection(task) @METRICS.timer('ConductorManager._sync_power_states') @periodics.periodic(spacing=CONF.conductor.sync_power_state_interval, @@ -3038,7 +3011,7 @@ class ConductorManager(base_manager.BaseConductorManager): task.process_event( 'inspect', callback=self._spawn_worker, - call_args=(_do_inspect_hardware, task), + call_args=(inspection.inspect_hardware, task), err_handler=utils.provisioning_error_handler) except exception.InvalidState: @@ -3858,53 +3831,3 @@ def do_sync_power_state(task, count): task, old_power_state) return count - - -@task_manager.require_exclusive_lock -def _do_inspect_hardware(task): - """Initiates inspection. - - :param task: a TaskManager instance with an exclusive lock - on its node. - :raises: HardwareInspectionFailure if driver doesn't - return the state as states.MANAGEABLE, states.INSPECTWAIT. - - """ - node = task.node - - def handle_failure(e, log_func=LOG.error): - utils.node_history_record(task.node, event=e, - event_type=states.INTROSPECTION, - error=True, user=task.context.user_id) - task.process_event('fail') - log_func("Failed to inspect node %(node)s: %(err)s", - {'node': node.uuid, 'err': e}) - - # Inspection cannot start in fast-track mode, wipe token and URL. - utils.wipe_token_and_url(task) - - try: - new_state = task.driver.inspect.inspect_hardware(task) - except exception.IronicException as e: - with excutils.save_and_reraise_exception(): - error = str(e) - handle_failure(error) - except Exception as e: - error = (_('Unexpected exception of type %(type)s: %(msg)s') % - {'type': type(e).__name__, 'msg': e}) - handle_failure(error, log_func=LOG.exception) - raise exception.HardwareInspectionFailure(error=error) - - if new_state == states.MANAGEABLE: - task.process_event('done') - LOG.info('Successfully inspected node %(node)s', - {'node': node.uuid}) - elif new_state == states.INSPECTWAIT: - task.process_event('wait') - LOG.info('Successfully started introspection on node %(node)s', - {'node': node.uuid}) - else: - error = (_("During inspection, driver returned unexpected " - "state %(state)s") % {'state': new_state}) - handle_failure(error) - raise exception.HardwareInspectionFailure(error=error) diff --git a/ironic/tests/unit/conductor/test_inspection.py b/ironic/tests/unit/conductor/test_inspection.py new file mode 100644 index 000000000..c64b883e4 --- /dev/null +++ b/ironic/tests/unit/conductor/test_inspection.py @@ -0,0 +1,118 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from unittest import mock + +from ironic.common import exception +from ironic.common import states +from ironic.conductor import inspection +from ironic.conductor import task_manager +from ironic.tests.unit.db import base as db_base +from ironic.tests.unit.objects import utils as obj_utils + + +@mock.patch('ironic.drivers.modules.fake.FakeInspect.inspect_hardware', + autospec=True) +class TestInspectHardware(db_base.DbTestCase): + + def test_inspect_hardware_ok(self, mock_inspect): + node = obj_utils.create_test_node( + self.context, driver='fake-hardware', + provision_state=states.INSPECTING, + driver_internal_info={'agent_url': 'url', + 'agent_secret_token': 'token'}) + task = task_manager.TaskManager(self.context, node.uuid) + mock_inspect.return_value = states.MANAGEABLE + inspection.inspect_hardware(task) + node.refresh() + self.assertEqual(states.MANAGEABLE, node.provision_state) + self.assertEqual(states.NOSTATE, node.target_provision_state) + self.assertIsNone(node.last_error) + mock_inspect.assert_called_once_with(task.driver.inspect, task) + task.node.refresh() + self.assertNotIn('agent_url', task.node.driver_internal_info) + self.assertNotIn('agent_secret_token', task.node.driver_internal_info) + + def test_inspect_hardware_return_inspecting(self, mock_inspect): + node = obj_utils.create_test_node(self.context, driver='fake-hardware', + provision_state=states.INSPECTING) + task = task_manager.TaskManager(self.context, node.uuid) + mock_inspect.return_value = states.INSPECTING + self.assertRaises(exception.HardwareInspectionFailure, + inspection.inspect_hardware, task) + + node.refresh() + self.assertIn('driver returned unexpected state', node.last_error) + self.assertEqual(states.INSPECTFAIL, node.provision_state) + self.assertEqual(states.MANAGEABLE, node.target_provision_state) + mock_inspect.assert_called_once_with(task.driver.inspect, task) + + def test_inspect_hardware_return_inspect_wait(self, mock_inspect): + node = obj_utils.create_test_node(self.context, driver='fake-hardware', + provision_state=states.INSPECTING) + task = task_manager.TaskManager(self.context, node.uuid) + mock_inspect.return_value = states.INSPECTWAIT + inspection.inspect_hardware(task) + node.refresh() + self.assertEqual(states.INSPECTWAIT, node.provision_state) + self.assertEqual(states.MANAGEABLE, node.target_provision_state) + self.assertIsNone(node.last_error) + mock_inspect.assert_called_once_with(task.driver.inspect, task) + + @mock.patch.object(inspection, 'LOG', autospec=True) + def test_inspect_hardware_return_other_state(self, log_mock, mock_inspect): + node = obj_utils.create_test_node(self.context, driver='fake-hardware', + provision_state=states.INSPECTING) + task = task_manager.TaskManager(self.context, node.uuid) + mock_inspect.return_value = None + self.assertRaises(exception.HardwareInspectionFailure, + inspection.inspect_hardware, task) + node.refresh() + self.assertEqual(states.INSPECTFAIL, node.provision_state) + self.assertEqual(states.MANAGEABLE, node.target_provision_state) + self.assertIsNotNone(node.last_error) + mock_inspect.assert_called_once_with(task.driver.inspect, task) + self.assertTrue(log_mock.error.called) + + def test_inspect_hardware_raises_error(self, mock_inspect): + mock_inspect.side_effect = exception.HardwareInspectionFailure('test') + state = states.MANAGEABLE + node = obj_utils.create_test_node(self.context, driver='fake-hardware', + provision_state=states.INSPECTING, + target_provision_state=state) + task = task_manager.TaskManager(self.context, node.uuid) + + self.assertRaisesRegex(exception.HardwareInspectionFailure, '^test$', + inspection.inspect_hardware, task) + node.refresh() + self.assertEqual(states.INSPECTFAIL, node.provision_state) + self.assertEqual(states.MANAGEABLE, node.target_provision_state) + self.assertEqual('test', node.last_error) + self.assertTrue(mock_inspect.called) + + def test_inspect_hardware_unexpected_error(self, mock_inspect): + mock_inspect.side_effect = RuntimeError('x') + state = states.MANAGEABLE + node = obj_utils.create_test_node(self.context, driver='fake-hardware', + provision_state=states.INSPECTING, + target_provision_state=state) + task = task_manager.TaskManager(self.context, node.uuid) + + self.assertRaisesRegex(exception.HardwareInspectionFailure, + 'Unexpected exception of type RuntimeError: x', + inspection.inspect_hardware, task) + node.refresh() + self.assertEqual(states.INSPECTFAIL, node.provision_state) + self.assertEqual(states.MANAGEABLE, node.target_provision_state) + self.assertEqual('Unexpected exception of type RuntimeError: x', + node.last_error) + self.assertTrue(mock_inspect.called) diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index 6a6f7e08f..7278486a5 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -46,6 +46,7 @@ from ironic.common import nova from ironic.common import states from ironic.conductor import cleaning from ironic.conductor import deployments +from ironic.conductor import inspection from ironic.conductor import manager from ironic.conductor import notification_utils from ironic.conductor import steps as conductor_steps @@ -6461,77 +6462,6 @@ class ManagerSyncLocalStateTestCase(mgr_utils.CommonMixIn, db_base.DbTestCase): @mgr_utils.mock_record_keepalive class NodeInspectHardware(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): - @mock.patch('ironic.drivers.modules.fake.FakeInspect.inspect_hardware', - autospec=True) - def test_inspect_hardware_ok(self, mock_inspect): - self._start_service() - node = obj_utils.create_test_node( - self.context, driver='fake-hardware', - provision_state=states.INSPECTING, - driver_internal_info={'agent_url': 'url', - 'agent_secret_token': 'token'}) - task = task_manager.TaskManager(self.context, node.uuid) - mock_inspect.return_value = states.MANAGEABLE - manager._do_inspect_hardware(task) - node.refresh() - self.assertEqual(states.MANAGEABLE, node.provision_state) - self.assertEqual(states.NOSTATE, node.target_provision_state) - self.assertIsNone(node.last_error) - mock_inspect.assert_called_once_with(task.driver.inspect, task) - task.node.refresh() - self.assertNotIn('agent_url', task.node.driver_internal_info) - self.assertNotIn('agent_secret_token', task.node.driver_internal_info) - - @mock.patch('ironic.drivers.modules.fake.FakeInspect.inspect_hardware', - autospec=True) - def test_inspect_hardware_return_inspecting(self, mock_inspect): - self._start_service() - node = obj_utils.create_test_node(self.context, driver='fake-hardware', - provision_state=states.INSPECTING) - task = task_manager.TaskManager(self.context, node.uuid) - mock_inspect.return_value = states.INSPECTING - self.assertRaises(exception.HardwareInspectionFailure, - manager._do_inspect_hardware, task) - - node.refresh() - self.assertIn('driver returned unexpected state', node.last_error) - self.assertEqual(states.INSPECTFAIL, node.provision_state) - self.assertEqual(states.MANAGEABLE, node.target_provision_state) - mock_inspect.assert_called_once_with(task.driver.inspect, task) - - @mock.patch('ironic.drivers.modules.fake.FakeInspect.inspect_hardware', - autospec=True) - def test_inspect_hardware_return_inspect_wait(self, mock_inspect): - self._start_service() - node = obj_utils.create_test_node(self.context, driver='fake-hardware', - provision_state=states.INSPECTING) - task = task_manager.TaskManager(self.context, node.uuid) - mock_inspect.return_value = states.INSPECTWAIT - manager._do_inspect_hardware(task) - node.refresh() - self.assertEqual(states.INSPECTWAIT, node.provision_state) - self.assertEqual(states.MANAGEABLE, node.target_provision_state) - self.assertIsNone(node.last_error) - mock_inspect.assert_called_once_with(task.driver.inspect, task) - - @mock.patch.object(manager, 'LOG', autospec=True) - @mock.patch('ironic.drivers.modules.fake.FakeInspect.inspect_hardware', - autospec=True) - def test_inspect_hardware_return_other_state(self, mock_inspect, log_mock): - self._start_service() - node = obj_utils.create_test_node(self.context, driver='fake-hardware', - provision_state=states.INSPECTING) - task = task_manager.TaskManager(self.context, node.uuid) - mock_inspect.return_value = None - self.assertRaises(exception.HardwareInspectionFailure, - manager._do_inspect_hardware, task) - node.refresh() - self.assertEqual(states.INSPECTFAIL, node.provision_state) - self.assertEqual(states.MANAGEABLE, node.target_provision_state) - self.assertIsNotNone(node.last_error) - mock_inspect.assert_called_once_with(task.driver.inspect, task) - self.assertTrue(log_mock.error.called) - def test__check_inspect_wait_timeouts(self): self._start_service() CONF.set_override('inspect_wait_timeout', 1, group='conductor') @@ -6609,46 +6539,6 @@ class NodeInspectHardware(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): def test_inspect_hardware_power_validate_fail(self, mock_validate): self._test_inspect_hardware_validate_fail(mock_validate) - @mock.patch('ironic.drivers.modules.fake.FakeInspect.inspect_hardware', - autospec=True) - def test_inspect_hardware_raises_error(self, mock_inspect): - self._start_service() - mock_inspect.side_effect = exception.HardwareInspectionFailure('test') - state = states.MANAGEABLE - node = obj_utils.create_test_node(self.context, driver='fake-hardware', - provision_state=states.INSPECTING, - target_provision_state=state) - task = task_manager.TaskManager(self.context, node.uuid) - - self.assertRaisesRegex(exception.HardwareInspectionFailure, '^test$', - manager._do_inspect_hardware, task) - node.refresh() - self.assertEqual(states.INSPECTFAIL, node.provision_state) - self.assertEqual(states.MANAGEABLE, node.target_provision_state) - self.assertEqual('test', node.last_error) - self.assertTrue(mock_inspect.called) - - @mock.patch('ironic.drivers.modules.fake.FakeInspect.inspect_hardware', - autospec=True) - def test_inspect_hardware_unexpected_error(self, mock_inspect): - self._start_service() - mock_inspect.side_effect = RuntimeError('x') - state = states.MANAGEABLE - node = obj_utils.create_test_node(self.context, driver='fake-hardware', - provision_state=states.INSPECTING, - target_provision_state=state) - task = task_manager.TaskManager(self.context, node.uuid) - - self.assertRaisesRegex(exception.HardwareInspectionFailure, - 'Unexpected exception of type RuntimeError: x', - manager._do_inspect_hardware, task) - node.refresh() - self.assertEqual(states.INSPECTFAIL, node.provision_state) - self.assertEqual(states.MANAGEABLE, node.target_provision_state) - self.assertEqual('Unexpected exception of type RuntimeError: x', - node.last_error) - self.assertTrue(mock_inspect.called) - @mock.patch.object(conductor_utils, 'node_history_record', mock.Mock(spec=conductor_utils.node_history_record)) @@ -8247,7 +8137,7 @@ class NodeTraitsTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): class DoNodeInspectAbortTestCase(mgr_utils.CommonMixIn, mgr_utils.ServiceSetUpMixin, db_base.DbTestCase): - @mock.patch.object(manager, 'LOG', autospec=True) + @mock.patch.object(inspection, 'LOG', autospec=True) @mock.patch('ironic.drivers.modules.fake.FakeInspect.abort', autospec=True) @mock.patch('ironic.conductor.task_manager.acquire', autospec=True) def test_do_inspect_abort_interface_not_support(self, mock_acquire, @@ -8268,7 +8158,7 @@ class DoNodeInspectAbortTestCase(mgr_utils.CommonMixIn, exc.exc_info[0]) self.assertTrue(mock_log.error.called) - @mock.patch.object(manager, 'LOG', autospec=True) + @mock.patch.object(inspection, 'LOG', autospec=True) @mock.patch('ironic.drivers.modules.fake.FakeInspect.abort', autospec=True) @mock.patch('ironic.conductor.task_manager.acquire', autospec=True) def test_do_inspect_abort_interface_return_failed(self, mock_acquire, -- cgit v1.2.1