diff --git a/doc/source/admin/notifications.rst b/doc/source/admin/notifications.rst index ff0131ff2b..52f7ed50ec 100644 --- a/doc/source/admin/notifications.rst +++ b/doc/source/admin/notifications.rst @@ -425,13 +425,12 @@ a node console are: * ``baremetal.node.console_restore.end`` * ``baremetal.node.console_restore.error`` -``console_set`` action is used when start or stop console is initiated via API -request. The ``console_restore`` action is used when the console was already -enabled, but a driver must restart the console because an ironic-conductor was -restarted. This may also be sent when an ironic-conductor takes over a node -that was being managed by another ironic-conductor. "start" and "end" -notifications have INFO level, "error" has ERROR. Example of node console -notification:: +``console_set`` action is used when start or stop console is initiated. The +``console_restore`` action is used when the console was already enabled, but a +driver must restart the console because an ironic-conductor was restarted. This +may also be sent when an ironic-conductor takes over a node that was being +managed by another ironic-conductor. "start" and "end" notifications have INFO +level, "error" has ERROR. Example of node console notification:: { "priority": "info", diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 801f7808c7..06cd928638 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -905,6 +905,11 @@ class ConductorManager(base_manager.BaseConductorManager): # rescuing network as well. task.driver.rescue.clean_up(task) + # stop the console + # do it in this thread since we're already out of the main + # conductor thread. + if node.console_enabled: + self._set_console_mode(task, False) task.driver.deploy.clean_up(task) task.driver.deploy.tear_down(task) except Exception as e: @@ -2168,23 +2173,25 @@ class ConductorManager(base_manager.BaseConductorManager): # take_over() right now. else: task.driver.console.stop_console(task) + except Exception as e: - op = _('enabling') if enabled else _('disabling') - msg = (_('Error %(op)s the console on node %(node)s. ' - 'Reason: %(error)s') % {'op': op, - 'node': node.uuid, - 'error': e}) - node.last_error = msg - LOG.error(msg) - node.save() - notify_utils.emit_console_notification( - task, 'console_set', fields.NotificationStatus.ERROR) - else: - node.console_enabled = enabled - node.last_error = None - node.save() - notify_utils.emit_console_notification( - task, 'console_set', fields.NotificationStatus.END) + with excutils.save_and_reraise_exception(): + op = _('enabling') if enabled else _('disabling') + msg = (_('Error %(op)s the console on node %(node)s. ' + 'Reason: %(error)s') % {'op': op, + 'node': node.uuid, + 'error': e}) + node.last_error = msg + LOG.error(msg) + node.save() + notify_utils.emit_console_notification( + task, 'console_set', fields.NotificationStatus.ERROR) + + node.console_enabled = enabled + node.last_error = None + node.save() + notify_utils.emit_console_notification( + task, 'console_set', fields.NotificationStatus.END) @METRICS.timer('ConductorManager.create_port') @messaging.expected_exceptions(exception.NodeLocked, diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index 7efbdc6816..7777f62e8a 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -1805,21 +1805,48 @@ class DoNodeDeployTearDownTestCase(mgr_utils.ServiceSetUpMixin, self.assertIsNotNone(node.last_error) # Assert instance_info was erased self.assertEqual({}, node.instance_info) - mock_tear_down.assert_called_once_with(mock.ANY) + mock_tear_down.assert_called_once_with(task) + + @mock.patch('ironic.conductor.manager.ConductorManager._set_console_mode') + def test_do_node_tear_down_console_raises_error(self, mock_console): + # test when _set_console_mode raises exception + node = obj_utils.create_test_node( + self.context, driver='fake', provision_state=states.DELETING, + target_provision_state=states.AVAILABLE, + instance_info={'foo': 'bar'}, + console_enabled=True, + driver_internal_info={'is_whole_disk_image': False}) + + task = task_manager.TaskManager(self.context, node.uuid) + self._start_service() + mock_console.side_effect = exception.ConsoleError('test') + self.assertRaises(exception.ConsoleError, + self.service._do_node_tear_down, task, + node.provision_state) + node.refresh() + self.assertEqual(states.ERROR, node.provision_state) + self.assertEqual(states.NOSTATE, node.target_provision_state) + self.assertIsNotNone(node.last_error) + # Assert instance_info was erased + self.assertEqual({}, node.instance_info) + mock_console.assert_called_once_with(task, False) # TODO(TheJulia): Since we're functionally bound to neutron support # by default, the fake drivers still invoke neutron. + @mock.patch('ironic.conductor.manager.ConductorManager._set_console_mode') @mock.patch('ironic.common.neutron.unbind_neutron_port') @mock.patch('ironic.conductor.manager.ConductorManager._do_node_clean') @mock.patch('ironic.drivers.modules.fake.FakeDeploy.tear_down') - def test__do_node_tear_down_ok(self, mock_tear_down, mock_clean, - mock_unbind): + def _test__do_node_tear_down_ok(self, mock_tear_down, mock_clean, + mock_unbind, mock_console, + enabled_console=False): # test when driver.deploy.tear_down succeeds node = obj_utils.create_test_node( self.context, driver='fake', provision_state=states.DELETING, target_provision_state=states.AVAILABLE, instance_uuid=uuidutils.generate_uuid(), instance_info={'foo': 'bar'}, + console_enabled=enabled_console, driver_internal_info={'is_whole_disk_image': False, 'clean_steps': {}, 'root_uuid_or_disk_id': 'foo', @@ -1843,10 +1870,20 @@ class DoNodeDeployTearDownTestCase(mgr_utils.ServiceSetUpMixin, self.assertNotIn('clean_steps', node.driver_internal_info) self.assertNotIn('root_uuid_or_disk_id', node.driver_internal_info) self.assertNotIn('is_whole_disk_image', node.driver_internal_info) - mock_tear_down.assert_called_once_with(mock.ANY) - mock_clean.assert_called_once_with(mock.ANY) + mock_tear_down.assert_called_once_with(task) + mock_clean.assert_called_once_with(task) self.assertEqual({}, port.internal_info) mock_unbind.assert_called_once_with('foo', context=mock.ANY) + if enabled_console: + mock_console.assert_called_once_with(task, False) + else: + mock_console.assert_not_called() + + def test__do_node_tear_down_ok_without_console(self): + self._test__do_node_tear_down_ok(enabled_console=False) + + def test__do_node_tear_down_ok_with_console(self): + self._test__do_node_tear_down_ok(enabled_console=True) @mock.patch('ironic.drivers.modules.fake.FakeRescue.clean_up') @mock.patch('ironic.conductor.manager.ConductorManager._do_node_clean') diff --git a/releasenotes/notes/stop-console-during-unprovision-a29d8facb3f03be5.yaml b/releasenotes/notes/stop-console-during-unprovision-a29d8facb3f03be5.yaml new file mode 100644 index 0000000000..71b035abba --- /dev/null +++ b/releasenotes/notes/stop-console-during-unprovision-a29d8facb3f03be5.yaml @@ -0,0 +1,7 @@ +--- +security: + - | + Fixes an issue where an enabled console could be left running after a node + was unprovisioned. This allowed a user to view the console even after the + instance was gone. Ironic now stops the console during unprovisioning to + block this.