Automatic port allocation for the serial console

Introduces [console]port_range configuration option and implements
the feature of automatic port allocation for IPMI based serial console.

The ipmi_terminal_port in driver_info takes precedance if specified,
otherwise ironic will allocate free port from configured port range
for underlying serial proxy tools.

The implementation deviation with the original proposal is this patch
doesn't validate whether user specified ipmi_terminal_port falls in the
range, based on following considerations:
a. ipmi_terminal_port is considered a resort for backwards compatibility,
we will remove this eventually.
b. different conductors may have different port range configured (rare,
but could happen).
c. force ipmi_terminal_port in the port range could raise the
possibility of conflicts with ports in the configured range, this is not
a desired result, so leave the choice to the end users.

Change-Id: If8722d09dc74878f4da2e4a7f059d9b079c3e472
Story: 2007099
Task: 38135

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.")

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:

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 <start>:<stop>. This option '
+                      'is used by both Shellinabox and Socat console')),
 ]
 
 

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 <start>:<stop> 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.
 

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"

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

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)

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)

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.