honor ipmi_port in serial console drivers

teach the ipmitool driver about _get_ipmitool_args and use that in all
cases that we want to build an ipmitool command line.   this solves
the problem that the serial console drivers were failing to honor the
ipmi_port setting in driver_info, while it was being correctly used
for power state, etc.

Change-Id: Ifbf6a92c2305567985cfbc41dbf76a076ecb8a7b
Story: 2005138
Task: 29826

ironic/drivers/modules/ipmitool.py

@@ -404,6 +404,43 @@ def _exec_ipmitool_wait(timeout, driver_info, popen_obj):
                     {'node': driver_info['uuid'], 'cmd': popen_obj.cmd})
 
 
+def _get_ipmitool_args(driver_info, pw_file=None):
+    ipmi_version = ('lanplus'
+                    if driver_info['protocol_version'] == '2.0'
+                    else 'lan')
+
+    args = ['ipmitool',
+            '-I', ipmi_version,
+            '-H', driver_info['address'],
+            '-L', driver_info['priv_level']
+            ]
+
+    if driver_info['dest_port']:
+        args.append('-p')
+        args.append(driver_info['dest_port'])
+
+    if driver_info['username']:
+        args.append('-U')
+        args.append(driver_info['username'])
+
+    for name, option in BRIDGING_OPTIONS:
+        if driver_info[name] is not None:
+            args.append(option)
+            args.append(driver_info[name])
+
+    if pw_file:
+        args.append('-f')
+        args.append(pw_file)
+
+    if CONF.debug:
+        args.append('-v')
+
+    # ensure all arguments are strings
+    args = [str(arg) for arg in args]
+
+    return args
+
+
 def _exec_ipmitool(driver_info, command, check_exit_code=None,
                    kill_on_timeout=False):
     """Execute the ipmitool command.
@@ -420,29 +457,7 @@ def _exec_ipmitool(driver_info, command, check_exit_code=None,
     :raises: processutils.ProcessExecutionError from executing the command.
 
     """
-    ipmi_version = ('lanplus'
+    args = _get_ipmitool_args(driver_info)
-                    if driver_info['protocol_version'] == '2.0'
-                    else 'lan')
-    args = ['ipmitool',
-            '-I',
-            ipmi_version,
-            '-H',
-            driver_info['address'],
-            '-L', driver_info['priv_level']
-            ]
-
-    if driver_info['dest_port']:
-        args.append('-p')
-        args.append(driver_info['dest_port'])
-
-    if driver_info['username']:
-        args.append('-U')
-        args.append(driver_info['username'])
-
-    for name, option in BRIDGING_OPTIONS:
-        if driver_info[name] is not None:
-            args.append(option)
-            args.append(driver_info[name])
 
     timeout = CONF.ipmi.command_retry_timeout
 
@@ -1277,13 +1292,7 @@ class IPMIConsole(base.ConsoleInterface):
         :param pw_file: password file to be used in ipmitool command
         :returns: returns a command string for ipmitool
         """
-        user = driver_info.get('username')
+        return ' '.join(_get_ipmitool_args(driver_info, pw_file=pw_file))
-        user = ' -U {}'.format(user) if user else ''
-        return ("ipmitool -H %(address)s -I lanplus"
-                "%(user)s -f %(pwfile)s"
-                % {'address': driver_info['address'],
-                   'user': user,
-                   'pwfile': pw_file})
 
     def _start_console(self, driver_info, start_method):
         """Start a remote console for the node.
@@ -1301,15 +1310,8 @@ class IPMIConsole(base.ConsoleInterface):
         pw_file = console_utils.make_persistent_password_file(
             path, driver_info['password'] or '\0')
         ipmi_cmd = self._get_ipmi_cmd(driver_info, pw_file)
+        ipmi_cmd += ' sol activate'
 
-        for name, option in BRIDGING_OPTIONS:
-            if driver_info[name] is not None:
-                ipmi_cmd = " ".join([ipmi_cmd,
-                                     option, driver_info[name]])
-
-        if CONF.debug:
-            ipmi_cmd += " -v"
-        ipmi_cmd += " sol activate"
         try:
             start_method(driver_info['uuid'], driver_info['port'], ipmi_cmd)
         except (exception.ConsoleError, exception.ConsoleSubprocessFailed):

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

@@ -822,6 +822,7 @@ class IPMIToolPrivateMethodTestCase(Base):
             '-H', self.info['address'],
             '-L', self.info['priv_level'],
             '-U', self.info['username'],
+            '-v',
             '-f', awesome_password_filename,
             'A', 'B', 'C',
         ]
@@ -847,6 +848,7 @@ class IPMIToolPrivateMethodTestCase(Base):
             '-H', self.info['address'],
             '-L', self.info['priv_level'],
             '-U', self.info['username'],
+            '-v',
             '-f', awesome_password_filename,
             'A', 'B', 'C',
         ], [
@@ -855,6 +857,7 @@ class IPMIToolPrivateMethodTestCase(Base):
             '-H', self.info['address'],
             '-L', self.info['priv_level'],
             '-U', self.info['username'],
+            '-v',
             '-f', awesome_password_filename,
             'D', 'E', 'F',
         ]]
@@ -884,6 +887,7 @@ class IPMIToolPrivateMethodTestCase(Base):
             '-H', self.info['address'],
             '-L', self.info['priv_level'],
             '-U', self.info['username'],
+            '-v',
             '-f', awesome_password_filename,
             'A', 'B', 'C',
         ], [
@@ -892,6 +896,7 @@ class IPMIToolPrivateMethodTestCase(Base):
             '-H', self.info['address'],
             '-L', self.info['priv_level'],
             '-U', self.info['username'],
+            '-v',
             '-f', awesome_password_filename,
             'D', 'E', 'F',
         ]]
@@ -923,6 +928,7 @@ class IPMIToolPrivateMethodTestCase(Base):
             '-H', self.info['address'],
             '-L', self.info['priv_level'],
             '-U', self.info['username'],
+            '-v',
             '-f', awesome_password_filename,
             'A', 'B', 'C',
         ], [
@@ -931,6 +937,7 @@ class IPMIToolPrivateMethodTestCase(Base):
             '-H', '127.127.127.127',
             '-L', self.info['priv_level'],
             '-U', self.info['username'],
+            '-v',
             '-f', awesome_password_filename,
             'D', 'E', 'F',
         ]]
@@ -959,6 +966,7 @@ class IPMIToolPrivateMethodTestCase(Base):
             '-H', self.info['address'],
             '-L', self.info['priv_level'],
             '-U', self.info['username'],
+            '-v',
             '-f', awesome_password_filename,
             'A', 'B', 'C',
         ]
@@ -982,6 +990,7 @@ class IPMIToolPrivateMethodTestCase(Base):
             '-H', self.info['address'],
             '-L', self.info['priv_level'],
             '-U', self.info['username'],
+            '-v',
             '-R', '12',
             '-N', '5',
             '-f', awesome_password_filename,
@@ -1017,6 +1026,7 @@ class IPMIToolPrivateMethodTestCase(Base):
             '-I', 'lanplus',
             '-H', self.info['address'],
             '-L', self.info['priv_level'],
+            '-v',
             '-f', awesome_password_filename,
             'A', 'B', 'C',
         ]
@@ -1040,6 +1050,7 @@ class IPMIToolPrivateMethodTestCase(Base):
             '-I', 'lanplus',
             '-H', self.info['address'],
             '-L', self.info['priv_level'],
+            '-v',
             '-f', awesome_password_filename,
             'A', 'B', 'C',
         ]
@@ -1066,6 +1077,7 @@ class IPMIToolPrivateMethodTestCase(Base):
             '-H', self.info['address'],
             '-L', self.info['priv_level'],
             '-U', self.info['username'],
+            '-v',
             '-f', awesome_password_filename,
             'A', 'B', 'C',
         ]
@@ -1093,6 +1105,7 @@ class IPMIToolPrivateMethodTestCase(Base):
             '-H', self.info['address'],
             '-L', self.info['priv_level'],
             '-U', self.info['username'],
+            '-v',
             '-f', awesome_password_filename,
             'A', 'B', 'C',
         ]
@@ -1127,6 +1140,7 @@ class IPMIToolPrivateMethodTestCase(Base):
             '-T', info['transit_address'],
             '-b', info['target_channel'],
             '-t', info['target_address'],
+            '-v',
             '-f', awesome_password_filename,
             'A', 'B', 'C',
         ]
@@ -1164,6 +1178,7 @@ class IPMIToolPrivateMethodTestCase(Base):
             '-m', info['local_address'],
             '-b', info['target_channel'],
             '-t', info['target_address'],
+            '-v',
             '-f', awesome_password_filename,
             'A', 'B', 'C',
         ]
@@ -1187,6 +1202,7 @@ class IPMIToolPrivateMethodTestCase(Base):
             '-H', self.info['address'],
             '-L', self.info['priv_level'],
             '-U', self.info['username'],
+            '-v',
             '-f', awesome_password_filename,
             'A', 'B', 'C',
         ]
@@ -1213,6 +1229,7 @@ class IPMIToolPrivateMethodTestCase(Base):
             '-H', self.info['address'],
             '-L', self.info['priv_level'],
             '-U', self.info['username'],
+            '-v',
             '-f', awesome_password_filename,
             'A', 'B', 'C',
         ]
@@ -1236,6 +1253,7 @@ class IPMIToolPrivateMethodTestCase(Base):
             '-L', self.info['priv_level'],
             '-p', '1623',
             '-U', self.info['username'],
+            '-v',
             '-f', awesome_password_filename,
             'A', 'B', 'C',
         ]
@@ -1260,6 +1278,7 @@ class IPMIToolPrivateMethodTestCase(Base):
             '-H', self.info['address'],
             '-L', self.info['priv_level'],
             '-U', self.info['username'],
+            '-v',
             '-f', awesome_password_filename,
             'A', 'B', 'C',
         ]
@@ -2461,8 +2480,8 @@ class IPMIToolShellinaboxTestCase(db_base.DbTestCase):
             driver_info = ipmi._parse_driver_info(task.node)
             ipmi_cmd = self.console._get_ipmi_cmd(driver_info, 'pw_file')
             expected_ipmi_cmd = ("/:%(uid)s:%(gid)s:HOME:ipmitool "
-                                 "-H %(address)s -I lanplus -U %(user)s "
+                                 "-I lanplus -H %(address)s -L ADMINISTRATOR "
-                                 "-f pw_file" %
+                                 "-U %(user)s -f pw_file -v" %
                                  {'uid': os.getuid(), 'gid': os.getgid(),
                                   'address': driver_info['address'],
                                   'user': driver_info['username']})
@@ -2475,8 +2494,8 @@ class IPMIToolShellinaboxTestCase(db_base.DbTestCase):
             driver_info['username'] = None
             ipmi_cmd = self.console._get_ipmi_cmd(driver_info, 'pw_file')
             expected_ipmi_cmd = ("/:%(uid)s:%(gid)s:HOME:ipmitool "
-                                 "-H %(address)s -I lanplus "
+                                 "-I lanplus -H %(address)s -L ADMINISTRATOR "
-                                 "-f pw_file" %
+                                 "-f pw_file -v" %
                                  {'uid': os.getuid(), 'gid': os.getgid(),
                                   'address': driver_info['address']})
         self.assertEqual(expected_ipmi_cmd, ipmi_cmd)
@@ -2608,8 +2627,9 @@ class IPMIToolSocatDriverTestCase(IPMIToolShellinaboxTestCase):
                                   self.node.uuid) as task:
             driver_info = ipmi._parse_driver_info(task.node)
             ipmi_cmd = self.console._get_ipmi_cmd(driver_info, 'pw_file')
-            expected_ipmi_cmd = ("ipmitool -H %(address)s -I lanplus "
+            expected_ipmi_cmd = ("ipmitool -I lanplus -H %(address)s "
-                                 "-U %(user)s -f pw_file" %
+                                 "-L ADMINISTRATOR -U %(user)s "
+                                 "-f pw_file -v" %
                                  {'address': driver_info['address'],
                                   'user': driver_info['username']})
         self.assertEqual(expected_ipmi_cmd, ipmi_cmd)
@@ -2620,8 +2640,9 @@ class IPMIToolSocatDriverTestCase(IPMIToolShellinaboxTestCase):
             driver_info = ipmi._parse_driver_info(task.node)
             driver_info['username'] = None
             ipmi_cmd = self.console._get_ipmi_cmd(driver_info, 'pw_file')
-            expected_ipmi_cmd = ("ipmitool -H %(address)s -I lanplus "
+            expected_ipmi_cmd = ("ipmitool -I lanplus -H %(address)s "
-                                 "-f pw_file" %
+                                 "-L ADMINISTRATOR "
+                                 "-f pw_file -v" %
                                  {'address': driver_info['address']})
         self.assertEqual(expected_ipmi_cmd, ipmi_cmd)
 

releasenotes/notes/ipmi-console-port-ec6348df4eee6746.yaml

@@ -0,0 +1,5 @@
+---
+fixes:
+  - |
+    Fixes the IPMI console implementation to respect all supported IPMI
+    ``driver_info`` and configuration options, particularly ``ipmi_port``.