From 7a8b26db6b48f5593786bb92cf09fa1d6afcc1ea Mon Sep 17 00:00:00 2001 From: Jim Rollenhagen Date: Thu, 10 May 2018 08:06:19 -0400 Subject: [PATCH] Tear down console during unprovisioning Before this patch, when unprovisioning a node, the console was left running. This allowed a user to view the console even after the instance was gone. Stop the console during unprovisioning to block this. ConductorManager._set_console_mode will now bubble up any exceptions raised, so that we can explode as needed during unprovisioning. This does not have any side effects, as elsewhere it is run in it's own thread and execution was complete after handling the exception. Also change a few mock.ANY in the relevant unit tests to the actual task object, as cleanup. Change-Id: Iec128444d692e0b0fbc1737eb21b0e6f69b7ec1e Partial-Bug: #1769817 Story: #2002000 Task: #19634 --- doc/source/admin/notifications.rst | 13 +++-- ironic/conductor/manager.py | 39 ++++++++------- ironic/tests/unit/conductor/test_manager.py | 47 +++++++++++++++++-- ...e-during-unprovision-a29d8facb3f03be5.yaml | 7 +++ 4 files changed, 78 insertions(+), 28 deletions(-) create mode 100644 releasenotes/notes/stop-console-during-unprovision-a29d8facb3f03be5.yaml 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.