Tear down console during unprovisioning7.0.5

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 7a8b26db6b48f5593786bb92cf09fa1d6afcc1ea)

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.