diff --git a/ironic/common/exception.py b/ironic/common/exception.py index 9c949df4d4..26147edde1 100644 --- a/ironic/common/exception.py +++ b/ironic/common/exception.py @@ -715,3 +715,8 @@ class ClientSideError(wsme.exc.ClientSideError): class NodeIsRetired(Invalid): _msg_fmt = _("The %(op)s operation can't be performed on node " "%(node)s because it is retired.") + + +class NoFreeIPMITerminalPorts(TemporaryFailure): + _msg_fmt = _("Unable to allocate a free port on host %(host)s for IPMI " + "terminal, not enough free ports.") diff --git a/ironic/conductor/manager.py b/ironic/conductor/manager.py index 509fb2af38..5e901ed4a1 100644 --- a/ironic/conductor/manager.py +++ b/ironic/conductor/manager.py @@ -2143,6 +2143,13 @@ 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. + internal_info = task.node.driver_internal_info + if 'allocated_ipmi_terminal_port' in internal_info: + internal_info.pop('allocated_ipmi_terminal_port') + task.node.driver_internal_info = internal_info try: task.driver.console.start_console(task) except Exception as err: diff --git a/ironic/conf/console.py b/ironic/conf/console.py index 2c361c1816..566f3bf1f7 100644 --- a/ironic/conf/console.py +++ b/ironic/conf/console.py @@ -53,6 +53,13 @@ opts = [ default='$my_ip', help=_('IP address of Socat service running on the host of ' 'ironic conductor. Used only by Socat console.')), + cfg.StrOpt('port_range', + regex=r'^\d+:\d+$', + sample_default='10000:20000', + help=_('A range of ports available to be used for the console ' + 'proxy service running on the host of ironic ' + 'conductor, in the form of :. This option ' + 'is used by both Shellinabox and Socat console')), ] diff --git a/ironic/drivers/modules/console_utils.py b/ironic/drivers/modules/console_utils.py index 59e52fb369..00e74c0c67 100644 --- a/ironic/drivers/modules/console_utils.py +++ b/ironic/drivers/modules/console_utils.py @@ -23,10 +23,12 @@ import errno import fcntl import os import signal +import socket import subprocess import time from ironic_lib import utils as ironic_utils +from oslo_concurrency import lockutils from oslo_log import log as logging from oslo_service import loopingcall from oslo_utils import fileutils @@ -40,6 +42,9 @@ from ironic.conf import CONF LOG = logging.getLogger(__name__) +ALLOCATED_PORTS = set() # in-memory set of already allocated ports +SERIAL_LOCK = 'ironic-console-lock' + def _get_console_pid_dir(): """Return the directory for the pid file.""" @@ -145,6 +150,57 @@ def make_persistent_password_file(path, password): raise exception.PasswordFileFailedToCreate(error=e) +def _get_port_range(): + config_range = CONF.console.port_range + + start, stop = map(int, config_range.split(':')) + if start >= stop: + msg = _("[console]port_range should be in the " + "format : and start < stop") + raise exception.InvalidParameterValue(msg) + return start, stop + + +def _verify_port(port): + """Check whether specified port is in use.""" + s = socket.socket() + try: + s.bind((CONF.host, port)) + except socket.error: + raise exception.Conflict() + finally: + s.close() + + +@lockutils.synchronized(SERIAL_LOCK) +def acquire_port(): + """Returns a free TCP port on current host. + + Find and returns a free TCP port in the range + of 'CONF.console.port_range'. + """ + + start, stop = _get_port_range() + + for port in range(start, stop): + if port in ALLOCATED_PORTS: + continue + try: + _verify_port(port) + ALLOCATED_PORTS.add(port) + return port + except exception.Conflict: + pass + + raise exception.NoFreeIPMITerminalPorts(host=CONF.host) + + +@lockutils.synchronized(SERIAL_LOCK) +def release_port(port): + """Release specified TCP port.""" + ALLOCATED_PORTS.discard(port) + + def get_shellinabox_console_url(port): """Get a url to access the console via shellinaboxd. diff --git a/ironic/drivers/modules/ipmitool.py b/ironic/drivers/modules/ipmitool.py index 16c7218f23..666e48b45d 100644 --- a/ironic/drivers/modules/ipmitool.py +++ b/ironic/drivers/modules/ipmitool.py @@ -274,6 +274,7 @@ def _parse_driver_info(node): """ info = node.driver_info or {} + internal_info = node.driver_internal_info or {} bridging_types = ['single', 'dual'] missing_info = [key for key in REQUIRED_PROPERTIES if not info.get(key)] if missing_info: @@ -286,7 +287,8 @@ def _parse_driver_info(node): password = str(info.get('ipmi_password', '')) hex_kg_key = info.get('ipmi_hex_kg_key') dest_port = info.get('ipmi_port') - port = info.get('ipmi_terminal_port') + port = (info.get('ipmi_terminal_port') or + internal_info.get('allocated_ipmi_terminal_port')) priv_level = info.get('ipmi_priv_level', 'ADMINISTRATOR') bridging_type = info.get('ipmi_bridging', 'no') local_address = info.get('ipmi_local_address') @@ -828,6 +830,27 @@ def _constructor_checks(driver): _check_temp_dir() +def _allocate_port(task): + node = task.node + dii = node.driver_internal_info or {} + allocated_port = console_utils.acquire_port() + dii['allocated_ipmi_terminal_port'] = allocated_port + node.driver_internal_info = dii + node.save() + return allocated_port + + +def _release_allocated_port(task): + node = task.node + dii = node.driver_internal_info or {} + allocated_port = dii.get('allocated_ipmi_terminal_port') + if allocated_port: + dii.pop('allocated_ipmi_terminal_port') + node.driver_internal_info = dii + node.save() + console_utils.release_port(allocated_port) + + class IPMIPower(base.PowerInterface): def __init__(self): @@ -1292,10 +1315,10 @@ class IPMIConsole(base.ConsoleInterface): """ driver_info = _parse_driver_info(task.node) - if not driver_info['port']: + if not driver_info['port'] and CONF.console.port_range is None: raise exception.MissingParameterValue(_( - "Missing 'ipmi_terminal_port' parameter in node's" - " driver_info.")) + "Either missing 'ipmi_terminal_port' parameter in node's " + "driver_info or [console]port_range is not configured")) if driver_info['protocol_version'] != '2.0': raise exception.InvalidParameterValue(_( @@ -1367,6 +1390,9 @@ class IPMIShellinaboxConsole(IPMIConsole): :raises: ConsoleSubprocessFailed when invoking the subprocess failed """ driver_info = _parse_driver_info(task.node) + if not driver_info['port']: + driver_info['port'] = _allocate_port(task) + self._start_console(driver_info, console_utils.start_shellinabox_console) @@ -1382,6 +1408,7 @@ class IPMIShellinaboxConsole(IPMIConsole): finally: ironic_utils.unlink_without_raise( _console_pwfile_path(task.node.uuid)) + _release_allocated_port(task) @METRICS.timer('IPMIShellinaboxConsole.get_console') def get_console(self, task): @@ -1407,6 +1434,9 @@ class IPMISocatConsole(IPMIConsole): :raises: ConsoleSubprocessFailed when invoking the subprocess failed """ driver_info = _parse_driver_info(task.node) + if not driver_info['port']: + driver_info['port'] = _allocate_port(task) + try: self._exec_stop_console(driver_info) except OSError: @@ -1430,6 +1460,7 @@ class IPMISocatConsole(IPMIConsole): ironic_utils.unlink_without_raise( _console_pwfile_path(task.node.uuid)) self._exec_stop_console(driver_info) + _release_allocated_port(task) def _exec_stop_console(self, driver_info): cmd = "sol deactivate" diff --git a/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index b2a1d832bc..dac661703d 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -8877,6 +8877,40 @@ 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') + @mock.patch('ironic.drivers.modules.fake.FakeConsole.start_console') + @mock.patch('ironic.drivers.modules.fake.FakeDeploy.take_over') + @mock.patch('ironic.drivers.modules.fake.FakeDeploy.prepare') + def test__do_takeover_with_console_port_cleaned(self, mock_prepare, + mock_take_over, + mock_start_console, + mock_notify): + self._start_service() + 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(mock.ANY) + mock_take_over.assert_called_once_with(mock.ANY) + mock_start_console.assert_called_once_with(mock.ANY) + 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_console_utils.py b/ironic/tests/unit/drivers/modules/test_console_utils.py index c010fdef50..a3cd142ade 100644 --- a/ironic/tests/unit/drivers/modules/test_console_utils.py +++ b/ironic/tests/unit/drivers/modules/test_console_utils.py @@ -652,3 +652,44 @@ class ConsoleUtilsTestCase(db_base.DbTestCase): mock_stop.assert_called_once_with(self.info['uuid']) # LOG.warning() is called when _stop_console() raises NoConsolePid self.assertTrue(mock_log_warning.called) + + def test_valid_console_port_range(self): + self.config(port_range='10000:20000', group='console') + start, stop = console_utils._get_port_range() + self.assertEqual((start, stop), (10000, 20000)) + + def test_invalid_console_port_range(self): + self.config(port_range='20000:10000', group='console') + self.assertRaises(exception.InvalidParameterValue, + console_utils._get_port_range) + + @mock.patch.object(console_utils, 'ALLOCATED_PORTS', autospec=True) + @mock.patch.object(console_utils, '_verify_port', autospec=True) + def test_allocate_port_success(self, mock_verify, mock_ports): + self.config(port_range='10000:10001', group='console') + port = console_utils.acquire_port() + mock_verify.assert_called_once_with(10000) + self.assertEqual(port, 10000) + mock_ports.add.assert_called_once_with(10000) + + @mock.patch.object(console_utils, 'ALLOCATED_PORTS', autospec=True) + @mock.patch.object(console_utils, '_verify_port', autospec=True) + def test_allocate_port_range_retry(self, mock_verify, mock_ports): + self.config(port_range='10000:10003', group='console') + mock_verify.side_effect = (exception.Conflict, exception.Conflict, + None) + port = console_utils.acquire_port() + verify_calls = [mock.call(10000), mock.call(10001), mock.call(10002)] + mock_verify.assert_has_calls(verify_calls) + self.assertEqual(port, 10002) + mock_ports.add.assert_called_once_with(10002) + + @mock.patch.object(console_utils, 'ALLOCATED_PORTS', autospec=True) + @mock.patch.object(console_utils, '_verify_port', autospec=True) + def test_allocate_port_no_free_ports(self, mock_verify, mock_ports): + self.config(port_range='10000:10005', group='console') + mock_verify.side_effect = exception.Conflict + self.assertRaises(exception.NoFreeIPMITerminalPorts, + console_utils.acquire_port) + verify_calls = [mock.call(p) for p in range(10000, 10005)] + mock_verify.assert_has_calls(verify_calls) diff --git a/ironic/tests/unit/drivers/modules/test_ipmitool.py b/ironic/tests/unit/drivers/modules/test_ipmitool.py index 7404f7a378..60b9afe436 100644 --- a/ironic/tests/unit/drivers/modules/test_ipmitool.py +++ b/ironic/tests/unit/drivers/modules/test_ipmitool.py @@ -830,6 +830,21 @@ class IPMIToolPrivateMethodTestCase( ipmi._parse_driver_info(node) self.assertFalse(mock_log.called) + def test__parse_driver_info_terminal_port_specified(self): + info = dict(INFO_DICT) + info['ipmi_terminal_port'] = 10000 + node = obj_utils.get_test_node(self.context, driver_info=info) + driver_info = ipmi._parse_driver_info(node) + self.assertEqual(driver_info['port'], 10000) + + def test__parse_driver_info_terminal_port_allocated(self): + info = dict(INFO_DICT) + internal_info = {'allocated_ipmi_terminal_port': 10001} + node = obj_utils.get_test_node(self.context, driver_info=info, + driver_internal_info=internal_info) + driver_info = ipmi._parse_driver_info(node) + self.assertEqual(driver_info['port'], 10001) + @mock.patch.object(ipmi, '_is_option_supported', autospec=True) @mock.patch.object(ipmi, '_make_password_file', _make_password_file_stub) @mock.patch.object(utils, 'execute', autospec=True) @@ -2524,6 +2539,31 @@ class IPMIToolDriverTestCase(Base): self.assertEqual(fake_ret, ret) + @mock.patch.object(console_utils, 'acquire_port', autospec=True) + def test__allocate_port(self, mock_acquire): + mock_acquire.return_value = 1234 + with task_manager.acquire(self.context, + self.node.uuid) as task: + port = ipmi._allocate_port(task) + mock_acquire.assert_called_once_with() + self.assertEqual(port, 1234) + info = task.node.driver_internal_info + self.assertEqual(info['allocated_ipmi_terminal_port'], 1234) + + @mock.patch.object(console_utils, 'release_port', autospec=True) + def test__release_allocated_port(self, mock_release): + info = self.node.driver_internal_info + info['allocated_ipmi_terminal_port'] = 1234 + self.node.driver_internal_info = info + self.node.save() + + with task_manager.acquire(self.context, + self.node.uuid) as task: + ipmi._release_allocated_port(task) + mock_release.assert_called_once_with(1234) + info = task.node.driver_internal_info + self.assertIsNone(info.get('allocated_ipmi_terminal_port')) + class IPMIToolShellinaboxTestCase(db_base.DbTestCase): console_interface = 'ipmitool-shellinabox' @@ -2553,6 +2593,13 @@ class IPMIToolShellinaboxTestCase(db_base.DbTestCase): self.assertRaises(exception.MissingParameterValue, task.driver.console.validate, task) + def test_console_validate_missing_port_auto_allocate(self): + self.config(port_range='10000:20000', group='console') + with task_manager.acquire( + self.context, self.node.uuid, shared=True) as task: + task.node.driver_info.pop('ipmi_terminal_port', None) + task.driver.console.validate(task) + def test_console_validate_invalid_port(self): with task_manager.acquire( self.context, self.node.uuid, shared=True) as task: @@ -2594,18 +2641,52 @@ class IPMIToolShellinaboxTestCase(db_base.DbTestCase): 'address': driver_info['address']}) self.assertEqual(expected_ipmi_cmd, ipmi_cmd) + @mock.patch.object(ipmi, '_allocate_port', autospec=True) @mock.patch.object(ipmi.IPMIConsole, '_start_console', autospec=True) - def test_start_console(self, mock_start): + def test_start_console(self, mock_start, mock_alloc): mock_start.return_value = None + mock_alloc.return_value = 10000 with task_manager.acquire(self.context, self.node.uuid) as task: self.console.start_console(task) driver_info = ipmi._parse_driver_info(task.node) + driver_info.update(port=10000) mock_start.assert_called_once_with( self.console, driver_info, console_utils.start_shellinabox_console) + @mock.patch.object(ipmi, '_allocate_port', autospec=True) + @mock.patch.object(ipmi, '_parse_driver_info', autospec=True) + @mock.patch.object(ipmi.IPMIConsole, '_start_console', autospec=True) + def test_start_console_with_port(self, mock_start, mock_info, mock_alloc): + mock_start.return_value = None + mock_info.return_value = {'port': 10000} + + with task_manager.acquire(self.context, + self.node.uuid) as task: + self.console.start_console(task) + mock_start.assert_called_once_with( + self.console, {'port': 10000}, + console_utils.start_shellinabox_console) + mock_alloc.assert_not_called() + + @mock.patch.object(ipmi, '_allocate_port', autospec=True) + @mock.patch.object(ipmi, '_parse_driver_info', autospec=True) + @mock.patch.object(ipmi.IPMIConsole, '_start_console', autospec=True) + def test_start_console_alloc_port(self, mock_start, mock_info, mock_alloc): + mock_start.return_value = None + mock_info.return_value = {'port': None} + mock_alloc.return_value = 1234 + + with task_manager.acquire(self.context, + self.node.uuid) as task: + self.console.start_console(task) + mock_start.assert_called_once_with( + self.console, {'port': 1234}, + console_utils.start_shellinabox_console) + mock_alloc.assert_called_once_with(mock.ANY) + @mock.patch.object(ipmi.IPMIConsole, '_get_ipmi_cmd', autospec=True) @mock.patch.object(console_utils, 'start_shellinabox_console', autospec=True) @@ -2673,9 +2754,10 @@ class IPMIToolShellinaboxTestCase(db_base.DbTestCase): self.info['port'], mock.ANY) + @mock.patch.object(ipmi, '_release_allocated_port', autospec=True) @mock.patch.object(console_utils, 'stop_shellinabox_console', autospec=True) - def test_stop_console(self, mock_stop): + def test_stop_console(self, mock_stop, mock_release): mock_stop.return_value = None with task_manager.acquire(self.context, @@ -2683,6 +2765,7 @@ class IPMIToolShellinaboxTestCase(db_base.DbTestCase): self.console.stop_console(task) mock_stop.assert_called_once_with(self.info['uuid']) + mock_release.assert_called_once_with(mock.ANY) @mock.patch.object(console_utils, 'stop_shellinabox_console', autospec=True) @@ -2740,22 +2823,71 @@ class IPMIToolSocatDriverTestCase(IPMIToolShellinaboxTestCase): {'address': driver_info['address']}) self.assertEqual(expected_ipmi_cmd, ipmi_cmd) + def test_console_validate_missing_port_auto_allocate(self): + self.config(port_range='10000:20000', group='console') + with task_manager.acquire( + self.context, self.node.uuid, shared=True) as task: + task.node.driver_info.pop('ipmi_terminal_port', None) + task.driver.console.validate(task) + + @mock.patch.object(ipmi, '_allocate_port', autospec=True) @mock.patch.object(ipmi.IPMIConsole, '_start_console', autospec=True) @mock.patch.object(ipmi.IPMISocatConsole, '_exec_stop_console', autospec=True) - def test_start_console(self, mock_stop, mock_start): + def test_start_console(self, mock_stop, mock_start, mock_alloc): mock_start.return_value = None mock_stop.return_value = None + mock_alloc.return_value = 10000 with task_manager.acquire(self.context, self.node.uuid) as task: self.console.start_console(task) driver_info = ipmi._parse_driver_info(task.node) + driver_info.update(port=10000) mock_stop.assert_called_once_with(self.console, driver_info) mock_start.assert_called_once_with( self.console, driver_info, console_utils.start_socat_console) + @mock.patch.object(ipmi, '_allocate_port', autospec=True) + @mock.patch.object(ipmi, '_parse_driver_info', autospec=True) + @mock.patch.object(ipmi.IPMIConsole, '_start_console', autospec=True) + @mock.patch.object(ipmi.IPMISocatConsole, '_exec_stop_console', + autospec=True) + def test_start_console_with_port(self, mock_stop, mock_start, mock_info, + mock_alloc): + mock_start.return_value = None + mock_info.return_value = {'port': 10000} + + with task_manager.acquire(self.context, + self.node.uuid) as task: + self.console.start_console(task) + mock_stop.assert_called_once_with(self.console, mock.ANY) + mock_start.assert_called_once_with( + self.console, {'port': 10000}, + console_utils.start_socat_console) + mock_alloc.assert_not_called() + + @mock.patch.object(ipmi, '_allocate_port', autospec=True) + @mock.patch.object(ipmi, '_parse_driver_info', autospec=True) + @mock.patch.object(ipmi.IPMIConsole, '_start_console', autospec=True) + @mock.patch.object(ipmi.IPMISocatConsole, '_exec_stop_console', + autospec=True) + def test_start_console_alloc_port(self, mock_stop, mock_start, mock_info, + mock_alloc): + mock_start.return_value = None + mock_info.return_value = {'port': None} + mock_alloc.return_value = 1234 + + with task_manager.acquire(self.context, + self.node.uuid) as task: + self.console.start_console(task) + mock_stop.assert_called_once_with(self.console, mock.ANY) + mock_start.assert_called_once_with( + self.console, {'port': 1234}, + console_utils.start_socat_console) + mock_alloc.assert_called_once_with(mock.ANY) + @mock.patch.object(ipmi.IPMISocatConsole, '_get_ipmi_cmd', autospec=True) @mock.patch.object(console_utils, 'start_socat_console', autospec=True) @@ -2827,11 +2959,12 @@ class IPMIToolSocatDriverTestCase(IPMIToolShellinaboxTestCase): self.info['port'], mock.ANY) + @mock.patch.object(ipmi, '_release_allocated_port', autospec=True) @mock.patch.object(ipmi.IPMISocatConsole, '_exec_stop_console', autospec=True) @mock.patch.object(console_utils, 'stop_socat_console', autospec=True) - def test_stop_console(self, mock_stop, mock_exec_stop): + def test_stop_console(self, mock_stop, mock_exec_stop, mock_release): mock_stop.return_value = None with task_manager.acquire(self.context, @@ -2842,6 +2975,7 @@ class IPMIToolSocatDriverTestCase(IPMIToolShellinaboxTestCase): mock_stop.assert_called_once_with(self.info['uuid']) mock_exec_stop.assert_called_once_with(self.console, driver_info) + mock_release.assert_called_once_with(mock.ANY) @mock.patch.object(ipmi.IPMISocatConsole, '_exec_stop_console', autospec=True) diff --git a/releasenotes/notes/console-port-allocation-bb07c43e3890c54c.yaml b/releasenotes/notes/console-port-allocation-bb07c43e3890c54c.yaml new file mode 100644 index 0000000000..dde3fc45c7 --- /dev/null +++ b/releasenotes/notes/console-port-allocation-bb07c43e3890c54c.yaml @@ -0,0 +1,9 @@ +--- +features: + - | + Adds a new configuration option ``[console]port_range``, which specifies + the range of ports can be consumed for the IPMI serial console. The + default value is ``None`` for backwards compatibility. If the + ``ipmi_terminal_port`` is not specified in the driver information for a + node, a free port will be allocated from the configured port range for + further use. \ No newline at end of file