diff --git a/ironic/drivers/modules/inspector/agent.py b/ironic/drivers/modules/inspector/agent.py index b253481075..7b7a227ada 100644 --- a/ironic/drivers/modules/inspector/agent.py +++ b/ironic/drivers/modules/inspector/agent.py @@ -79,7 +79,7 @@ class AgentInspect(common.Common): if inspect_utils.clear_lookup_addresses(task.node): task.node.save() - common.clean_up(task, finish=False) + common.clean_up(task, finish=False, always_power_off=True) def continue_inspection(self, task, inventory, plugin_data): """Continue in-band hardware inspection. @@ -90,8 +90,7 @@ class AgentInspect(common.Common): """ # Run the inspection hooks run_inspection_hooks(task, inventory, plugin_data, self.hooks) - - common.clean_up(task, finish=False) + common.clean_up(task, finish=False, always_power_off=True) def _store_logs(plugin_data, node): @@ -115,7 +114,6 @@ def run_inspection_hooks(task, inventory, plugin_data, hooks): try: _run_post_hooks(task, inventory, plugin_data, hooks) - _power_off_node(task) except exception.HardwareInspectionFailure: with excutils.save_and_reraise_exception(): _store_logs(plugin_data, task.node) @@ -180,20 +178,3 @@ def _run_post_hooks(task, inventory, plugin_data, hooks): LOG.debug('Running inspection hook %(hook)s for node %(node)s', {'hook': hook.name, 'node': task.node.uuid}) hook.obj.__call__(task, inventory, plugin_data) - - -def _power_off_node(task): - power_off = CONF.inspector.power_off - if not power_off: - return - - node = task.node - LOG.debug('Forcing power off of node %(node)s', {'node': node.uuid}) - try: - cond_utils.node_power_action(task, states.POWER_OFF) - except Exception as exc: - msg = _("Failed to power off node %(node)s after inspection. Check " - "its power management configuration. Error: %(error)s" % - {'node': node.uuid, "error": exc}) - raise exception.HardwareInspectionFailure(msg) - LOG.info('Node %(node)s powered off', {'node': node.uuid}) diff --git a/ironic/drivers/modules/inspector/interface.py b/ironic/drivers/modules/inspector/interface.py index aa71ce1787..2eb985d9df 100644 --- a/ironic/drivers/modules/inspector/interface.py +++ b/ironic/drivers/modules/inspector/interface.py @@ -62,29 +62,30 @@ def _get_callback_endpoint(client): parts.query, parts.fragment)) -def tear_down_managed_boot(task): +def tear_down_managed_boot(task, always_power_off=False): errors = [] ironic_manages_boot = utils.pop_node_nested_field( task.node, 'driver_internal_info', _IRONIC_MANAGES_BOOT) - if not ironic_manages_boot: - return errors - try: - task.driver.boot.clean_up_ramdisk(task) - except Exception as exc: - errors.append(_('unable to clean up ramdisk boot: %s') % exc) - LOG.exception('Unable to clean up ramdisk boot for node %s', - task.node.uuid) - try: - with cond_utils.power_state_for_network_configuration(task): - task.driver.network.remove_inspection_network(task) - except Exception as exc: - errors.append(_('unable to remove inspection ports: %s') % exc) - LOG.exception('Unable to remove inspection network for node %s', - task.node.uuid) + if ironic_manages_boot: + try: + task.driver.boot.clean_up_ramdisk(task) + except Exception as exc: + errors.append(_('unable to clean up ramdisk boot: %s') % exc) + LOG.exception('Unable to clean up ramdisk boot for node %s', + task.node.uuid) + try: + with cond_utils.power_state_for_network_configuration(task): + task.driver.network.remove_inspection_network(task) + except Exception as exc: + errors.append(_('unable to remove inspection ports: %s') % exc) + LOG.exception('Unable to remove inspection network for node %s', + task.node.uuid) - if CONF.inspector.power_off and not utils.fast_track_enabled(task.node): + if ((ironic_manages_boot or always_power_off) + and CONF.inspector.power_off + and not utils.fast_track_enabled(task.node)): try: cond_utils.node_power_action(task, states.POWER_OFF) except Exception as exc: @@ -363,8 +364,8 @@ def _check_status(task): task.context) -def clean_up(task, finish=True): - errors = tear_down_managed_boot(task) +def clean_up(task, finish=True, always_power_off=False): + errors = tear_down_managed_boot(task, always_power_off=always_power_off) if errors: errors = ', '.join(errors) LOG.error('Inspection clean up failed for node %(uuid)s: %(err)s', diff --git a/ironic/tests/unit/drivers/modules/inspector/test_agent.py b/ironic/tests/unit/drivers/modules/inspector/test_agent.py index 8aab6004c4..6399a48f6c 100644 --- a/ironic/tests/unit/drivers/modules/inspector/test_agent.py +++ b/ironic/tests/unit/drivers/modules/inspector/test_agent.py @@ -20,7 +20,9 @@ from ironic.conf import CONF from ironic.drivers.modules import deploy_utils from ironic.drivers.modules import inspect_utils from ironic.drivers.modules.inspector import agent as inspector +from ironic.drivers.modules.inspector.hooks import base as hooks_base from ironic.drivers.modules.inspector import interface as common +from ironic.drivers import utils as driver_utils from ironic.tests.unit.db import base as db_base from ironic.tests.unit.objects import utils as obj_utils @@ -132,12 +134,56 @@ class ContinueInspectionTestCase(db_base.DbTestCase): mock_inspection_hooks.assert_called_once_with( task, mock.sentinel.inventory, mock.sentinel.plugin_data, self.iface.hooks) - mock_tear_down.assert_called_once_with(task) + mock_tear_down.assert_called_once_with(task, always_power_off=True) self.assertEqual(states.INSPECTING, task.node.provision_state) self.assertIsNone(result) +@mock.patch.object(driver_utils, 'store_ramdisk_logs', autospec=True) +class RunInspectionHooksTestCase(db_base.DbTestCase): + def setUp(self): + super().setUp() + CONF.set_override('enabled_inspect_interfaces', + ['agent', 'no-inspect']) + self.node = obj_utils.create_test_node( + self.context, + inspect_interface='agent', + provision_state=states.INSPECTING) + self.task = mock.Mock(spec=task_manager.TaskManager, node=self.node) + self.hooks = [ + mock.Mock(name=str(i), + obj=mock.MagicMock(spec=hooks_base.InspectionHook)) + for i in range(2) + ] + self.inventory = {"interfaces": [42]} + self.plugin_data = {"logs": "abcd"} + + def test(self, mock_ramdisk_logs): + inspector.run_inspection_hooks(self.task, self.inventory, + self.plugin_data, self.hooks) + for hook in self.hooks: + hook.obj.preprocess.assert_called_once_with( + self.task, self.inventory, self.plugin_data) + hook.obj.assert_called_once_with( + self.task, self.inventory, self.plugin_data) + + mock_ramdisk_logs.assert_not_called() + + def test_always_collect_logs(self, mock_ramdisk_logs): + CONF.set_override('deploy_logs_collect', 'always', group='agent') + inspector.run_inspection_hooks(self.task, self.inventory, + self.plugin_data, self.hooks) + for hook in self.hooks: + hook.obj.preprocess.assert_called_once_with( + self.task, self.inventory, self.plugin_data) + hook.obj.assert_called_once_with( + self.task, self.inventory, self.plugin_data) + + mock_ramdisk_logs.assert_called_once_with( + self.node, "abcd", label="inspect") + + @mock.patch.object(common, 'tear_down_managed_boot', autospec=True) class AbortInspectionTestCase(db_base.DbTestCase): def setUp(self): @@ -157,7 +203,7 @@ class AbortInspectionTestCase(db_base.DbTestCase): mock_tear_down.return_value = None with task_manager.acquire(self.context, self.node.id) as task: self.iface.abort(task) - mock_tear_down.assert_called_once_with(task) + mock_tear_down.assert_called_once_with(task, always_power_off=True) self.node.refresh() # Keep the state - it will be changed on the conductor level @@ -169,7 +215,7 @@ class AbortInspectionTestCase(db_base.DbTestCase): mock_tear_down.return_value = ['boom'] with task_manager.acquire(self.context, self.node.id) as task: self.iface.abort(task) - mock_tear_down.assert_called_once_with(task) + mock_tear_down.assert_called_once_with(task, always_power_off=True) self.node.refresh() self.assertEqual(states.INSPECTFAIL, self.node.provision_state) diff --git a/ironic/tests/unit/drivers/modules/inspector/test_interface.py b/ironic/tests/unit/drivers/modules/inspector/test_interface.py index 42bb55f2b5..d9a356a5b9 100644 --- a/ironic/tests/unit/drivers/modules/inspector/test_interface.py +++ b/ironic/tests/unit/drivers/modules/inspector/test_interface.py @@ -323,6 +323,89 @@ class InspectHardwareTestCase(BaseTestCase): self.task, 'power off', timeout=None) +class TearDownManagedInspectionTestCase(BaseTestCase): + + def test_unmanaged(self): + inspector.tear_down_managed_boot(self.task) + + self.assertFalse(self.driver.network.remove_inspection_network.called) + self.assertFalse(self.driver.boot.clean_up_ramdisk.called) + self.assertFalse(self.driver.power.set_power_state.called) + + def test_unmanaged_force_power_off(self): + inspector.tear_down_managed_boot(self.task, always_power_off=True) + + self.assertFalse(self.driver.network.remove_inspection_network.called) + self.assertFalse(self.driver.boot.clean_up_ramdisk.called) + self.driver.power.set_power_state.assert_called_once_with( + self.task, 'power off', timeout=None) + + def test_managed(self): + utils.set_node_nested_field(self.node, 'driver_internal_info', + 'inspector_manage_boot', True) + self.node.save() + + inspector.tear_down_managed_boot(self.task) + + self.driver.network.remove_inspection_network.assert_called_once_with( + self.task) + self.driver.boot.clean_up_ramdisk.assert_called_once_with(self.task) + self.driver.power.set_power_state.assert_called_once_with( + self.task, 'power off', timeout=None) + + def test_managed_no_power_off(self): + CONF.set_override('power_off', False, group='inspector') + utils.set_node_nested_field(self.node, 'driver_internal_info', + 'inspector_manage_boot', True) + self.node.save() + + inspector.tear_down_managed_boot(self.task) + + self.driver.network.remove_inspection_network.assert_called_once_with( + self.task) + self.driver.boot.clean_up_ramdisk.assert_called_once_with(self.task) + self.assertFalse(self.driver.power.set_power_state.called) + + def test_managed_no_power_off_on_fast_track(self): + CONF.set_override('fast_track', True, group='deploy') + utils.set_node_nested_field(self.node, 'driver_internal_info', + 'inspector_manage_boot', True) + self.node.save() + + inspector.tear_down_managed_boot(self.task) + + self.driver.network.remove_inspection_network.assert_called_once_with( + self.task) + self.driver.boot.clean_up_ramdisk.assert_called_once_with(self.task) + self.assertFalse(self.driver.power.set_power_state.called) + + def _test_clean_up_failed(self): + utils.set_node_nested_field(self.node, 'driver_internal_info', + 'inspector_manage_boot', True) + self.node.save() + + result = inspector.tear_down_managed_boot(self.task) + + self.assertIn("boom", result[0]) + + def test_boot_clean_up_failed(self): + self.driver.boot.clean_up_ramdisk.side_effect = RuntimeError('boom') + + self._test_clean_up_failed() + + self.driver.boot.clean_up_ramdisk.assert_called_once_with(self.task) + + def test_network_clean_up_failed(self): + self.driver.network.remove_inspection_network.side_effect = \ + RuntimeError('boom') + + self._test_clean_up_failed() + + self.driver.network.remove_inspection_network.assert_called_once_with( + self.task) + self.driver.boot.clean_up_ramdisk.assert_called_once_with(self.task) + + @mock.patch.object(client, 'get_client', autospec=True) class CheckStatusTestCase(BaseTestCase): def setUp(self): @@ -363,119 +446,34 @@ class CheckStatusTestCase(BaseTestCase): mock_get.assert_called_once_with(self.node.uuid) self.assertFalse(self.task.process_event.called) - def test_status_ok(self, mock_client): + @mock.patch.object(inspector, 'tear_down_managed_boot', autospec=True) + def test_status_ok(self, mock_tear_down, mock_client): mock_get = mock_client.return_value.get_introspection mock_get.return_value = mock.Mock(is_finished=True, error=None, spec=['is_finished', 'error']) + mock_tear_down.return_value = [] inspector._check_status(self.task) mock_get.assert_called_once_with(self.node.uuid) self.task.process_event.assert_called_once_with('done') - self.assertFalse(self.driver.network.remove_inspection_network.called) - self.assertFalse(self.driver.boot.clean_up_ramdisk.called) - self.assertFalse(self.driver.power.set_power_state.called) + mock_tear_down.assert_called_once_with( + self.task, always_power_off=False) - def test_status_ok_managed(self, mock_client): - utils.set_node_nested_field(self.node, 'driver_internal_info', - 'inspector_manage_boot', True) - self.node.save() - mock_get = mock_client.return_value.get_introspection - mock_get.return_value = mock.Mock(is_finished=True, - error=None, - spec=['is_finished', 'error']) - inspector._check_status(self.task) - mock_get.assert_called_once_with(self.node.uuid) - self.task.process_event.assert_called_once_with('done') - self.driver.network.remove_inspection_network.assert_called_once_with( - self.task) - self.driver.boot.clean_up_ramdisk.assert_called_once_with(self.task) - self.driver.power.set_power_state.assert_called_once_with( - self.task, 'power off', timeout=None) - - def test_status_ok_managed_no_power_off(self, mock_client): - CONF.set_override('power_off', False, group='inspector') - utils.set_node_nested_field(self.node, 'driver_internal_info', - 'inspector_manage_boot', True) - self.node.save() - mock_get = mock_client.return_value.get_introspection - mock_get.return_value = mock.Mock(is_finished=True, - error=None, - spec=['is_finished', 'error']) - inspector._check_status(self.task) - mock_get.assert_called_once_with(self.node.uuid) - self.task.process_event.assert_called_once_with('done') - self.driver.network.remove_inspection_network.assert_called_once_with( - self.task) - self.driver.boot.clean_up_ramdisk.assert_called_once_with(self.task) - self.assertFalse(self.driver.power.set_power_state.called) - - def test_status_ok_managed_no_power_off_on_fast_track(self, mock_client): - CONF.set_override('fast_track', True, group='deploy') - utils.set_node_nested_field(self.node, 'driver_internal_info', - 'inspector_manage_boot', True) - self.node.save() - mock_get = mock_client.return_value.get_introspection - mock_get.return_value = mock.Mock(is_finished=True, - error=None, - spec=['is_finished', 'error']) - inspector._check_status(self.task) - mock_get.assert_called_once_with(self.node.uuid) - self.task.process_event.assert_called_once_with('done') - self.driver.network.remove_inspection_network.assert_called_once_with( - self.task) - self.driver.boot.clean_up_ramdisk.assert_called_once_with(self.task) - self.assertFalse(self.driver.power.set_power_state.called) - - def test_status_error(self, mock_client): + @mock.patch.object(inspector, 'tear_down_managed_boot', autospec=True) + def test_status_error(self, mock_tear_down, mock_client): mock_get = mock_client.return_value.get_introspection mock_get.return_value = mock.Mock(is_finished=True, error='boom', spec=['is_finished', 'error']) + mock_tear_down.return_value = [] inspector._check_status(self.task) mock_get.assert_called_once_with(self.node.uuid) self.task.process_event.assert_called_once_with('fail') self.assertIn('boom', self.node.last_error) - self.assertFalse(self.driver.network.remove_inspection_network.called) - self.assertFalse(self.driver.boot.clean_up_ramdisk.called) - self.assertFalse(self.driver.power.set_power_state.called) + mock_tear_down.assert_called_once_with(self.task) - def test_status_error_managed(self, mock_client): - utils.set_node_nested_field(self.node, 'driver_internal_info', - 'inspector_manage_boot', True) - self.node.save() - mock_get = mock_client.return_value.get_introspection - mock_get.return_value = mock.Mock(is_finished=True, - error='boom', - spec=['is_finished', 'error']) - inspector._check_status(self.task) - mock_get.assert_called_once_with(self.node.uuid) - self.task.process_event.assert_called_once_with('fail') - self.assertIn('boom', self.node.last_error) - self.driver.network.remove_inspection_network.assert_called_once_with( - self.task) - self.driver.boot.clean_up_ramdisk.assert_called_once_with(self.task) - self.driver.power.set_power_state.assert_called_once_with( - self.task, 'power off', timeout=None) - - def test_status_error_managed_no_power_off(self, mock_client): - CONF.set_override('power_off', False, group='inspector') - utils.set_node_nested_field(self.node, 'driver_internal_info', - 'inspector_manage_boot', True) - self.node.save() - mock_get = mock_client.return_value.get_introspection - mock_get.return_value = mock.Mock(is_finished=True, - error='boom', - spec=['is_finished', 'error']) - inspector._check_status(self.task) - mock_get.assert_called_once_with(self.node.uuid) - self.task.process_event.assert_called_once_with('fail') - self.assertIn('boom', self.node.last_error) - self.driver.network.remove_inspection_network.assert_called_once_with( - self.task) - self.driver.boot.clean_up_ramdisk.assert_called_once_with(self.task) - self.assertFalse(self.driver.power.set_power_state.called) - - def _test_status_clean_up_failed(self, mock_client): + @mock.patch.object(inspector, 'tear_down_managed_boot', autospec=True) + def test_status_clean_up_failed(self, mock_tear_down, mock_client): utils.set_node_nested_field(self.node, 'driver_internal_info', 'inspector_manage_boot', True) self.node.save() @@ -483,27 +481,13 @@ class CheckStatusTestCase(BaseTestCase): mock_get.return_value = mock.Mock(is_finished=True, error=None, spec=['is_finished', 'error']) + mock_tear_down.return_value = ["boom"] inspector._check_status(self.task) mock_get.assert_called_once_with(self.node.uuid) self.task.process_event.assert_called_once_with('fail') self.assertIn('boom', self.node.last_error) - - def test_status_boot_clean_up_failed(self, mock_client): - self.driver.boot.clean_up_ramdisk.side_effect = RuntimeError('boom') - - self._test_status_clean_up_failed(mock_client) - - self.driver.boot.clean_up_ramdisk.assert_called_once_with(self.task) - - def test_status_network_clean_up_failed(self, mock_client): - self.driver.network.remove_inspection_network.side_effect = \ - RuntimeError('boom') - - self._test_status_clean_up_failed(mock_client) - - self.driver.network.remove_inspection_network.assert_called_once_with( - self.task) - self.driver.boot.clean_up_ramdisk.assert_called_once_with(self.task) + mock_tear_down.assert_called_once_with( + self.task, always_power_off=False) @mock.patch.object(inspect_utils, 'store_inspection_data', autospec=True) def test_status_ok_store_inventory(self, mock_store_data, mock_client): diff --git a/releasenotes/notes/inspect-off-099e3c73edaf6082.yaml b/releasenotes/notes/inspect-off-099e3c73edaf6082.yaml new file mode 100644 index 0000000000..1a46dd6215 --- /dev/null +++ b/releasenotes/notes/inspect-off-099e3c73edaf6082.yaml @@ -0,0 +1,9 @@ +--- +fixes: + - | + The configuration option ``[inspector]power_off`` is now actually ignored + for nodes with fast track enabled, as documented in its help. + - | + Fixes the built-in in-band inspection implementation to power off the + node after aborting inspection on user's request, unless the node is + in the fast track mode or ``[inspector]power_off`` is set to ``False``.