From c9c9b3100d3bd8983ca53a75c4a4e5f9c7f122b9 Mon Sep 17 00:00:00 2001 From: Kaifeng Wang Date: Sun, 25 Dec 2022 12:14:06 +0800 Subject: [PATCH] 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 | 4 -- ironic/drivers/modules/ipmitool.py | 6 +++ ironic/tests/unit/conductor/test_manager.py | 38 ------------------- .../unit/drivers/modules/test_ipmitool.py | 10 +++++ ...onsole-port-conflict-6dc19688079e2c7f.yaml | 8 ++++ 5 files changed, 24 insertions(+), 42 deletions(-) create mode 100644 releasenotes/notes/fix-console-port-conflict-6dc19688079e2c7f.yaml diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index cf49889582..1d3e7eb0bf 100644 --- a/ironic/conductor/manager.py +++ b/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: diff --git a/ironic/drivers/modules/ipmitool.py b/ironic/drivers/modules/ipmitool.py index 87420f369d..218f90a5c0 100644 --- a/ironic/drivers/modules/ipmitool.py +++ b/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( diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index fd206e36d3..b4a18c8c2f 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/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): diff --git a/ironic/tests/unit/drivers/modules/test_ipmitool.py b/ironic/tests/unit/drivers/modules/test_ipmitool.py index b982e0cb2f..016b9d6ed4 100644 --- a/ironic/tests/unit/drivers/modules/test_ipmitool.py +++ b/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: diff --git a/releasenotes/notes/fix-console-port-conflict-6dc19688079e2c7f.yaml b/releasenotes/notes/fix-console-port-conflict-6dc19688079e2c7f.yaml new file mode 100644 index 0000000000..32b4199151 --- /dev/null +++ b/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 + `_.