Merge "Automatic port allocation for the serial console"

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

@@ -1767,6 +1767,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

@@ -6847,6 +6847,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.