From 70d7bb369a151455eed198a7d66ffeff21ec8d22 Mon Sep 17 00:00:00 2001 From: Lars Kellogg-Stedman Date: Mon, 4 Mar 2019 21:54:46 -0500 Subject: [PATCH] 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 | 78 ++++++++++--------- .../unit/drivers/modules/test_ipmitool.py | 37 +++++++-- .../ipmi-console-port-ec6348df4eee6746.yaml | 5 ++ 3 files changed, 74 insertions(+), 46 deletions(-) create mode 100644 releasenotes/notes/ipmi-console-port-ec6348df4eee6746.yaml diff --git a/ironic/drivers/modules/ipmitool.py b/ironic/drivers/modules/ipmitool.py index 39b52a016e..d759e8b75a 100644 --- a/ironic/drivers/modules/ipmitool.py +++ b/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' - 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]) + args = _get_ipmitool_args(driver_info) 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') - 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 6712e7b7bb..2ba10090da 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) diff --git a/releasenotes/notes/ipmi-console-port-ec6348df4eee6746.yaml b/releasenotes/notes/ipmi-console-port-ec6348df4eee6746.yaml new file mode 100644 index 0000000000..969b261963 --- /dev/null +++ b/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``.