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.

Conflicts:
	ironic/tests/unit/conductor/test_manager.py

Change-Id: Iec128444d692e0b0fbc1737eb21b0e6f69b7ec1e
Partial-Bug: #1769817
Story: #2002000
Task: #19634
(cherry picked from commit 7a8b26db6b)

doc/source/admin/notifications.rst

@@ -416,13 +416,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
+``console_set`` action is used when start or stop console is initiated. The
-request. The ``console_restore`` action is used when the console was already
+``console_restore`` action is used when the console was already enabled, but a
-enabled, but a driver must restart the console because an ironic-conductor was
+driver must restart the console because an ironic-conductor was restarted. This
-restarted. This may also be sent when an ironic-conductor takes over a node
+may also be sent when an ironic-conductor takes over a node that was being
-that was being managed by another ironic-conductor. "start" and "end"
+managed by another ironic-conductor. "start" and "end" notifications have INFO
-notifications have INFO level, "error" has ERROR. Example of node console
+level, "error" has ERROR. Example of node console notification::
-notification::
 
    {
     "priority": "info",

ironic/conductor/manager.py

@@ -665,6 +665,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:
@@ -1848,23 +1853,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')
+            with excutils.save_and_reraise_exception():
-            msg = (_('Error %(op)s the console on node %(node)s. '
+                op = _('enabling') if enabled else _('disabling')
-                     'Reason: %(error)s') % {'op': op,
+                msg = (_('Error %(op)s the console on node %(node)s. '
-                                             'node': node.uuid,
+                         'Reason: %(error)s') % {'op': op,
-                                             'error': e})
+                                                 'node': node.uuid,
-            node.last_error = msg
+                                                 'error': e})
-            LOG.error(msg)
+                node.last_error = msg
-            node.save()
+                LOG.error(msg)
-            notify_utils.emit_console_notification(
+                node.save()
-                task, 'console_set', fields.NotificationStatus.ERROR)
+                notify_utils.emit_console_notification(
-        else:
+                    task, 'console_set', fields.NotificationStatus.ERROR)
-            node.console_enabled = enabled
+
-            node.last_error = None
+        node.console_enabled = enabled
-            node.save()
+        node.last_error = None
-            notify_utils.emit_console_notification(
+        node.save()
-                task, 'console_set', fields.NotificationStatus.END)
+        notify_utils.emit_console_notification(
+            task, 'console_set', fields.NotificationStatus.END)
 
     @METRICS.timer('ConductorManager.create_port')
     @messaging.expected_exceptions(exception.NodeLocked,

ironic/tests/unit/conductor/test_manager.py

@@ -1707,17 +1707,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}})
 
@@ -1734,6 +1760,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')

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.