Fixes console port conflict occurs in certain path

The dynamically allocated console port for a node is saved
into database and reused on subsequent console operations.
In certain code path the port record cann't be trusted and
we should do a re-allocation.

This patch fixes the issue by ignores previous allocation
record. The extra cleanup in the takeover is not required
anymore and removed as well.

Change-Id: I1a07ea9b30a2c760af7a6a4e39f3ff227df28fff
Story: 2010489
Task: 47061

ironic/conductor/manager.py

@@ -1773,10 +1773,6 @@ class ConductorManager(base_manager.BaseConductorManager):
         if task.node.console_enabled:
             notify_utils.emit_console_notification(
                 task, 'console_restore', fields.NotificationStatus.START)
-            # NOTE(kaifeng) Clear allocated_ipmi_terminal_port if exists,
-            # so current conductor can allocate a new free port from local
-            # resources.
-            task.node.del_driver_internal_info('allocated_ipmi_terminal_port')
             try:
                 task.driver.console.start_console(task)
             except Exception as err:

ironic/drivers/modules/ipmitool.py

@@ -1556,6 +1556,9 @@ class IPMIShellinaboxConsole(IPMIConsole):
                  created
         :raises: ConsoleSubprocessFailed when invoking the subprocess failed
         """
+        # Dealloc allocated port if any, so the same host can never has
+        # duplicated port.
+        _release_allocated_port(task)
         driver_info = _parse_driver_info(task.node)
         if not driver_info['port']:
             driver_info['port'] = _allocate_port(task)
@@ -1611,6 +1614,9 @@ class IPMISocatConsole(IPMIConsole):
                  created
         :raises: ConsoleSubprocessFailed when invoking the subprocess failed
         """
+        # Dealloc allocated port if any, so the same host can never has
+        # duplicated port.
+        _release_allocated_port(task)
         driver_info = _parse_driver_info(task.node)
         if not driver_info['port']:
             driver_info['port'] = _allocate_port(

ironic/tests/unit/conductor/test_manager.py

@@ -7244,44 +7244,6 @@ class DoNodeTakeOverTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):
              mock.call(task, 'console_restore',
                        obj_fields.NotificationStatus.ERROR)])
 
-    @mock.patch.object(notification_utils, 'emit_console_notification',
-                       autospec=True)
-    @mock.patch('ironic.drivers.modules.fake.FakeConsole.start_console',
-                autospec=True)
-    @mock.patch('ironic.drivers.modules.fake.FakeDeploy.take_over',
-                autospec=True)
-    @mock.patch('ironic.drivers.modules.fake.FakeDeploy.prepare',
-                autospec=True)
-    def test__do_takeover_with_console_port_cleaned(self, mock_prepare,
-                                                    mock_take_over,
-                                                    mock_start_console,
-                                                    mock_notify):
-        self._start_service(start_consoles=False)
-        node = obj_utils.create_test_node(self.context, driver='fake-hardware',
-                                          console_enabled=True)
-        di_info = node.driver_internal_info
-        di_info['allocated_ipmi_terminal_port'] = 12345
-        node.driver_internal_info = di_info
-        node.save()
-
-        task = task_manager.TaskManager(self.context, node.uuid)
-
-        self.service._do_takeover(task)
-        node.refresh()
-        self.assertIsNone(node.last_error)
-        self.assertTrue(node.console_enabled)
-        self.assertIsNone(
-            node.driver_internal_info.get('allocated_ipmi_terminal_port',
-                                          None))
-        mock_prepare.assert_called_once_with(task.driver.deploy, task)
-        mock_take_over.assert_called_once_with(task.driver.deploy, task)
-        mock_start_console.assert_called_once_with(task.driver.console, task)
-        mock_notify.assert_has_calls(
-            [mock.call(task, 'console_restore',
-                       obj_fields.NotificationStatus.START),
-             mock.call(task, 'console_restore',
-                       obj_fields.NotificationStatus.END)])
-
 
 @mgr_utils.mock_record_keepalive
 class DoNodeAdoptionTestCase(mgr_utils.ServiceSetUpMixin, db_base.DbTestCase):

ironic/tests/unit/drivers/modules/test_ipmitool.py

@@ -3255,6 +3255,11 @@ class IPMIToolShellinaboxTestCase(db_base.DbTestCase):
         mock_start.return_value = None
         mock_info.return_value = {'port': None}
         mock_alloc.return_value = 1234
+        # Ensure allocated port is not re-used
+        dii = self.node.driver_internal_info
+        dii['allocated_ipmi_terminal_port'] = 4321
+        self.node.driver_internal_info = dii
+        self.node.save()
 
         with task_manager.acquire(self.context,
                                   self.node.uuid) as task:
@@ -3468,6 +3473,11 @@ class IPMIToolSocatDriverTestCase(IPMIToolShellinaboxTestCase):
         mock_start.return_value = None
         mock_info.return_value = {'port': None}
         mock_alloc.return_value = 1234
+        # Ensure allocated port is not re-used
+        dii = self.node.driver_internal_info
+        dii['allocated_ipmi_terminal_port'] = 4321
+        self.node.driver_internal_info = dii
+        self.node.save()
 
         with task_manager.acquire(self.context,
                                   self.node.uuid) as task:

releasenotes/notes/fix-console-port-conflict-6dc19688079e2c7f.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    Fixes issues that auto-allocated console port could conflict on the same
+    host under certain circumstances related to conductor takeover.
+
+    For more information, see `story 2010489
+    <https://storyboard.openstack.org/#!/story/2010489>`_.