diff options
| -rw-r--r-- | doc/source/deploy/notifications.rst | 13 | ||||
| -rw-r--r-- | ironic/conductor/manager.py | 39 | ||||
| -rw-r--r-- | ironic/tests/unit/conductor/test_manager.py | 40 | ||||
| -rw-r--r-- | releasenotes/notes/stop-console-during-unprovision-a29d8facb3f03be5.yaml | 7 |
4 files changed, 74 insertions, 25 deletions
diff --git a/doc/source/deploy/notifications.rst b/doc/source/deploy/notifications.rst index 0fd705f17..a7baea36f 100644 --- a/doc/source/deploy/notifications.rst +++ b/doc/source/deploy/notifications.rst @@ -293,13 +293,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 eb1bdbbf2..a1f091cf1 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -651,6 +651,11 @@ class ConductorManager(base_manager.BaseConductorManager): """Internal RPC method to tear down an existing node deployment.""" node = task.node try: + # 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: @@ -1822,23 +1827,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.update_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 49d3de941..c7b23396d 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -1568,17 +1568,43 @@ 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.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) + @mock.patch('ironic.conductor.manager.ConductorManager._set_console_mode') @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): + def _test__do_node_tear_down_ok(self, mock_tear_down, mock_clean, + 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, 'instance': {'ephemeral_gb': 10}}) @@ -1595,6 +1621,16 @@ class DoNodeDeployTearDownTestCase(mgr_utils.ServiceSetUpMixin, self.assertNotIn('instance', node.driver_internal_info) mock_tear_down.assert_called_once_with(mock.ANY) mock_clean.assert_called_once_with(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.conductor.manager.ConductorManager._do_node_clean') @mock.patch('ironic.drivers.modules.fake.FakeDeploy.tear_down') 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 000000000..71b035abb --- /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. |