From d1aa76d0c18c4af6cbf78bd68ff41d23b368a8a0 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Thu, 12 Mar 2020 07:19:44 -0700 Subject: [PATCH] Move ipmi logging to a separate option The IPMI verbose output being turned on by the debug option is confusing and misleading, and since many operators run ironic in debug mode anyway, it doesn't make much sense to spam logs with errors and information that can be misleading to a less experienced operator. Also... less logging output. Back-porting per discussion[0] in IRC where we believe this is the best action possible and the verbose ipmitool output tends not to be extremely helpful for operators. [0]: http://eavesdrop.openstack.org/irclogs/%23openstack-ironic/%23openstack-ironic.2020-04-01.log.html#t2020-04-01T16:02:43 Change-Id: I0fae7bad5613865dfd4d1c663be08d40debe157a (cherry picked from commit 1e514b64404ee668ff0651ffb2ad30217f5b1b81) (cherry picked from commit 3e5816e10a18d3d001aeb73ecb3595e9488e764d) --- ironic/conf/ipmi.py | 6 ++++++ ironic/drivers/modules/ipmitool.py | 2 +- ironic/tests/unit/drivers/modules/test_ipmitool.py | 9 +++++---- releasenotes/notes/ipmi-debug-1c7e090c6cc71903.yaml | 13 +++++++++++++ 4 files changed, 25 insertions(+), 5 deletions(-) create mode 100644 releasenotes/notes/ipmi-debug-1c7e090c6cc71903.yaml diff --git a/ironic/conf/ipmi.py b/ironic/conf/ipmi.py index 466888e353..49dfda91c4 100644 --- a/ironic/conf/ipmi.py +++ b/ironic/conf/ipmi.py @@ -49,6 +49,12 @@ opts = [ 'that command, the default value is True. It may be ' 'overridden by per-node \'ipmi_disable_boot_timeout\' ' 'option in node\'s \'driver_info\' field.')), + cfg.BoolOpt('debug', + default=False, + help=_('Enables all ipmi commands to be executed with an ' + 'additional debugging output. This is a separate ' + 'option as ipmitool can log a substantial amount ' + 'of misleading text when in this mode.')), ] diff --git a/ironic/drivers/modules/ipmitool.py b/ironic/drivers/modules/ipmitool.py index bb37b44c85..1d285e18b3 100644 --- a/ironic/drivers/modules/ipmitool.py +++ b/ironic/drivers/modules/ipmitool.py @@ -445,7 +445,7 @@ def _get_ipmitool_args(driver_info, pw_file=None): args.append('-f') args.append(pw_file) - if CONF.debug: + if CONF.ipmi.debug: args.append('-v') # ensure all arguments are strings diff --git a/ironic/tests/unit/drivers/modules/test_ipmitool.py b/ironic/tests/unit/drivers/modules/test_ipmitool.py index 8aa3c3a2c1..efea67721e 100644 --- a/ironic/tests/unit/drivers/modules/test_ipmitool.py +++ b/ironic/tests/unit/drivers/modules/test_ipmitool.py @@ -454,6 +454,7 @@ class Base(db_base.DbTestCase): enabled_console_interfaces=['fake', 'ipmitool-socat', 'ipmitool-shellinabox', 'no-console']) + self.config(debug=True, group="ipmi") self.node = obj_utils.create_test_node( self.context, console_interface='ipmitool-socat', @@ -2569,7 +2570,7 @@ class IPMIToolShellinaboxTestCase(db_base.DbTestCase): ipmi_cmd = self.console._get_ipmi_cmd(driver_info, 'pw_file') expected_ipmi_cmd = ("/:%(uid)s:%(gid)s:HOME:ipmitool " "-I lanplus -H %(address)s -L ADMINISTRATOR " - "-U %(user)s -f pw_file -v" % + "-U %(user)s -f pw_file" % {'uid': os.getuid(), 'gid': os.getgid(), 'address': driver_info['address'], 'user': driver_info['username']}) @@ -2583,7 +2584,7 @@ class IPMIToolShellinaboxTestCase(db_base.DbTestCase): ipmi_cmd = self.console._get_ipmi_cmd(driver_info, 'pw_file') expected_ipmi_cmd = ("/:%(uid)s:%(gid)s:HOME:ipmitool " "-I lanplus -H %(address)s -L ADMINISTRATOR " - "-f pw_file -v" % + "-f pw_file" % {'uid': os.getuid(), 'gid': os.getgid(), 'address': driver_info['address']}) self.assertEqual(expected_ipmi_cmd, ipmi_cmd) @@ -2717,7 +2718,7 @@ class IPMIToolSocatDriverTestCase(IPMIToolShellinaboxTestCase): ipmi_cmd = self.console._get_ipmi_cmd(driver_info, 'pw_file') expected_ipmi_cmd = ("ipmitool -I lanplus -H %(address)s " "-L ADMINISTRATOR -U %(user)s " - "-f pw_file -v" % + "-f pw_file" % {'address': driver_info['address'], 'user': driver_info['username']}) self.assertEqual(expected_ipmi_cmd, ipmi_cmd) @@ -2730,7 +2731,7 @@ class IPMIToolSocatDriverTestCase(IPMIToolShellinaboxTestCase): ipmi_cmd = self.console._get_ipmi_cmd(driver_info, 'pw_file') expected_ipmi_cmd = ("ipmitool -I lanplus -H %(address)s " "-L ADMINISTRATOR " - "-f pw_file -v" % + "-f pw_file" % {'address': driver_info['address']}) self.assertEqual(expected_ipmi_cmd, ipmi_cmd) diff --git a/releasenotes/notes/ipmi-debug-1c7e090c6cc71903.yaml b/releasenotes/notes/ipmi-debug-1c7e090c6cc71903.yaml new file mode 100644 index 0000000000..d0feb47f40 --- /dev/null +++ b/releasenotes/notes/ipmi-debug-1c7e090c6cc71903.yaml @@ -0,0 +1,13 @@ +--- +features: + - | + Adds a new ``[ipmi]debug`` option that allows users to explicitly turn + IPMI command debugging on, as opposed to relying upon the system debug + setting ``[DEFAULT]debug``. Users wishing to continue to log this output + should set ``[ipmi]debug`` to ``True`` in their ironic.conf. +upgrade: + - Debug logging control has been moved to the ``[ipmi]debug`` configuration + setting as opposed to the "conductor" ``[DEFAULT]debug`` setting as + the existing ``ipmitool`` output can be extremely misleading for users. + Operators who wish to continue to log ``ipmitool`` verbose output in their + logs should explicitly set the ``[ipmi]debug`` command to True.