From 1e514b64404ee668ff0651ffb2ad30217f5b1b81 Mon Sep 17 00:00:00 2001
From: Julia Kreger <juliaashleykreger@gmail.com>
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.

Change-Id: I0fae7bad5613865dfd4d1c663be08d40debe157a
---
 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 ec4d76e99f..9545bde179 100644
--- a/ironic/conf/ipmi.py
+++ b/ironic/conf/ipmi.py
@@ -53,6 +53,12 @@ opts = [
                     default=[],
                     help=_('Additional errors ipmitool may encounter, '
                            'specific to the environment it is run in.')),
+    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 666e48b45d..5b7e8915ad 100644
--- a/ironic/drivers/modules/ipmitool.py
+++ b/ironic/drivers/modules/ipmitool.py
@@ -446,7 +446,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 60b9afe436..6f7b2c5138 100644
--- a/ironic/tests/unit/drivers/modules/test_ipmitool.py
+++ b/ironic/tests/unit/drivers/modules/test_ipmitool.py
@@ -459,6 +459,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',
@@ -2622,7 +2623,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']})
@@ -2636,7 +2637,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)
@@ -2806,7 +2807,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)
@@ -2819,7 +2820,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.