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

2 files changed, 66 insertions, 43 deletions

diff --git a/ironic/drivers/modules/ipmitool.py b/ironic/drivers/modules/ipmitool.py
index 39b52a016..d759e8b75 100644
--- a/ironic/drivers/modules/ipmitool.py
+++ b/ironic/drivers/modules/ipmitool.py
@@ -404,30 +404,14 @@ def _exec_ipmitool_wait(timeout, driver_info, popen_obj):
                     {'node': driver_info['uuid'], 'cmd': popen_obj.cmd})
 
 
-def _exec_ipmitool(driver_info, command, check_exit_code=None,
-                   kill_on_timeout=False):
-    """Execute the ipmitool command.
-
-    :param driver_info: the ipmitool parameters for accessing a node.
-    :param command: the ipmitool command to be executed.
-    :param check_exit_code: Single bool, int, or list of allowed exit codes.
-    :param kill_on_timeout: if `True`, kill unresponsive ipmitool on
-        `min_command_interval` timeout. Default is `False`. Makes no
-        effect on Windows.
-    :returns: (stdout, stderr) from executing the command.
-    :raises: PasswordFileFailedToCreate from creating or writing to the
-             temporary file.
-    :raises: processutils.ProcessExecutionError from executing the command.
-
-    """
+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'],
+            '-I', ipmi_version,
+            '-H', driver_info['address'],
             '-L', driver_info['priv_level']
             ]
 
@@ -444,6 +428,37 @@ def _exec_ipmitool(driver_info, command, check_exit_code=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.
+
+    :param driver_info: the ipmitool parameters for accessing a node.
+    :param command: the ipmitool command to be executed.
+    :param check_exit_code: Single bool, int, or list of allowed exit codes.
+    :param kill_on_timeout: if `True`, kill unresponsive ipmitool on
+        `min_command_interval` timeout. Default is `False`. Makes no
+        effect on Windows.
+    :returns: (stdout, stderr) from executing the command.
+    :raises: PasswordFileFailedToCreate from creating or writing to the
+             temporary file.
+    :raises: processutils.ProcessExecutionError from executing the command.
+
+    """
+    args = _get_ipmitool_args(driver_info)
+
     timeout = CONF.ipmi.command_retry_timeout
 
     # specify retry timing more precisely, if supported
@@ -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')
-        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})
+        return ' '.join(_get_ipmitool_args(driver_info, pw_file=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):
diff --git a/ironic/tests/unit/drivers/modules/test_ipmitool.py b/ironic/tests/unit/drivers/modules/test_ipmitool.py
index 6712e7b7b..2ba10090d 100644
--- a/ironic/tests/unit/drivers/modules/test_ipmitool.py
+++ b/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 "
-                                 "-f pw_file" %
+                                 "-I lanplus -H %(address)s -L ADMINISTRATOR "
+                                 "-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 "
-                                 "-f pw_file" %
+                                 "-I lanplus -H %(address)s -L ADMINISTRATOR "
+                                 "-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 "
-                                 "-U %(user)s -f pw_file" %
+            expected_ipmi_cmd = ("ipmitool -I lanplus -H %(address)s "
+                                 "-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 "
-                                 "-f pw_file" %
+            expected_ipmi_cmd = ("ipmitool -I lanplus -H %(address)s "
+                                 "-L ADMINISTRATOR "
+                                 "-f pw_file -v" %
                                  {'address': driver_info['address']})
         self.assertEqual(expected_ipmi_cmd, ipmi_cmd)