Let ipmitool natively retry commands

Instead of calling ipmitool multiple times on failure via
  utils.execute(*args, attempts=3)
allow ipmitool to use its own native retry behavior with -N.. -R..
if those options are supported by the installed version of ipmitool.
This will fall back to a single run of ipmitool on older versions,
which should be fine -- it defaults to retry several times anyway.

This patch adds a configurable min time between retries, which is used,
in conjunction with the ipmi retry time, to determine these option's
values. It will be further leveraged in a subsequent patch as well.

It also adds a note in the deployer docs about known issues with
the openipmi project.

Change-Id: I7a4ff941144a03bd441459561efb68760391da1a
Partial-bug: #1320513

6 files changed, 179 insertions, 15 deletions

diff --git a/doc/source/deploy/install-guide.rst b/doc/source/deploy/install-guide.rst
index 916ee440a..7f5724e3d 100644
--- a/doc/source/deploy/install-guide.rst
+++ b/doc/source/deploy/install-guide.rst
@@ -340,3 +340,14 @@ PXE needs to be set up.
      ubuntu: /usr/lib/syslinux/pxelinux.0
      fedora/RHEL: /usr/share/syslinux/pxelinux.0
 
+IPMI support
+------------
+
+If using the IPMITool driver, the ``ipmitool`` command must be present on the
+service node(s) where ``ironic-conductor`` is running. On most distros, this
+is provided as part of the ``ipmitool`` package. Source code is available at
+http://ipmitool.sourceforge.net/
+
+Note that certain distros, notably Mac OS X and SLES, install ``openipmi``
+instead of ``ipmitool`` by default. THIS DRIVER IS NOT COMPATIBLE WITH
+``openipmi`` AS IT RELIES ON ERROR HANDLING OPTIONS NOT PROVIDED BY THIS TOOL.
diff --git a/etc/ironic/ironic.conf.sample b/etc/ironic/ironic.conf.sample
index dd8396fec..7affe8c75 100644
--- a/etc/ironic/ironic.conf.sample
+++ b/etc/ironic/ironic.conf.sample
@@ -706,6 +706,12 @@
 # value)
 #retry_timeout=60
 
+# Minimum time, in seconds, between IPMI operations sent to a
+# server. There is a risk with some hardware that setting this
+# too low may cause the BMC to crash. Recommended setting is 5
+# seconds. (integer value)
+#min_command_interval=5
+
 
 [keystone_authtoken]
 
diff --git a/ironic/drivers/modules/ipminative.py b/ironic/drivers/modules/ipminative.py
index 508b5025e..7a2adb9fb 100644
--- a/ironic/drivers/modules/ipminative.py
+++ b/ironic/drivers/modules/ipminative.py
@@ -33,6 +33,12 @@ opts = [
     cfg.IntOpt('retry_timeout',
                default=60,
                help='Maximum time in seconds to retry IPMI operations.'),
+    cfg.IntOpt('min_command_interval',
+               default=5,
+               help='Minimum time, in seconds, between IPMI operations '
+                    'sent to a server. There is a risk with some hardware '
+                    'that setting this too low may cause the BMC to crash. '
+                    'Recommended setting is 5 seconds.'),
     ]
 
 CONF = cfg.CONF
diff --git a/ironic/drivers/modules/ipmitool.py b/ironic/drivers/modules/ipmitool.py
index b48d85853..818e855ac 100644
--- a/ironic/drivers/modules/ipmitool.py
+++ b/ironic/drivers/modules/ipmitool.py
@@ -18,7 +18,15 @@
 #    under the License.
 
 """
-Ironic IPMI power manager.
+IPMI power manager driver.
+
+Uses the 'ipmitool' command (http://ipmitool.sourceforge.net/) to remotely
+manage hardware.  This includes setting the boot device, getting a
+serial-over-LAN console, and controlling the power state of the machine.
+
+NOTE THAT CERTAIN DISTROS MAY INSTALL openipmi BY DEFAULT, INSTEAD OF ipmitool,
+WHICH PROVIDES DIFFERENT COMMAND-LINE OPTIONS AND *IS NOT SUPPORTED* BY THIS
+DRIVER.
 """
 
 import contextlib
@@ -37,14 +45,64 @@ from ironic.drivers.modules import console_utils
 from ironic.openstack.common import excutils
 from ironic.openstack.common import log as logging
 from ironic.openstack.common import loopingcall
+from ironic.openstack.common import processutils
+
 
 CONF = cfg.CONF
+CONF.import_opt('retry_timeout',
+                'ironic.drivers.modules.ipminative',
+                group='ipmi')
+CONF.import_opt('min_command_interval',
+                'ironic.drivers.modules.ipminative',
+                group='ipmi')
 
 LOG = logging.getLogger(__name__)
 
 VALID_BOOT_DEVICES = ['pxe', 'disk', 'safe', 'cdrom', 'bios']
 VALID_PRIV_LEVELS = ['ADMINISTRATOR', 'CALLBACK', 'OPERATOR', 'USER']
 
+TIMING_SUPPORT = None
+
+
+def _is_timing_supported(is_supported=None):
+    # shim to allow module variable to be mocked in unit tests
+    global TIMING_SUPPORT
+
+    if (TIMING_SUPPORT is None) and (is_supported is not None):
+        TIMING_SUPPORT = is_supported
+    return TIMING_SUPPORT
+
+
+def check_timing_support():
+    """Check the installed version of ipmitool for -N -R option support.
+
+    Support was added in 1.8.12 for the -N -R options, which enable
+    more precise control over timing of ipmi packets. Prior to this,
+    the default behavior was to retry each command up to 18 times at
+    1 to 5 second intervals.
+    http://ipmitool.cvs.sourceforge.net/viewvc/ipmitool/ipmitool/ChangeLog?revision=1.37  # noqa
+
+    This method updates the module-level TIMING_SUPPORT variable so that
+    it is accessible by any driver interface class in this module. It is
+    intended to be called from the __init__ method of such classes only.
+
+    :returns: boolean indicating whether support for -N -R is present
+    :raises: OSError
+    """
+    if _is_timing_supported() is None:
+        # Directly check ipmitool for support of -N and -R options. Because
+        # of the way ipmitool processes' command line options, if the local
+        # ipmitool does not support setting the timing options, the command
+        # below will fail.
+        try:
+            out, err = utils.execute(*['ipmitool', '-N', '0', '-R', '0', '-h'])
+        except processutils.ProcessExecutionError:
+            # the local ipmitool does not support the -N and -R options.
+            _is_timing_supported(False)
+        else:
+            # looks like ipmitool supports timing options.
+            _is_timing_supported(True)
+
 
 def _console_pwfile_path(uuid):
     """Return the file path for storing the ipmi password for a console."""
@@ -140,6 +198,16 @@ def _exec_ipmitool(driver_info, command):
         args.append('-U')
         args.append(driver_info['username'])
 
+    # specify retry timing more precisely, if supported
+    if _is_timing_supported():
+        num_tries = max(
+            (CONF.ipmi.retry_timeout // CONF.ipmi.min_command_interval), 1)
+        args.append('-R')
+        args.append(str(num_tries))
+
+        args.append('-N')
+        args.append(str(CONF.ipmi.min_command_interval))
+
     # 'ipmitool' command will prompt password if there is no '-f' option,
     # we set it to '\0' to write a password file to support empty password
 
@@ -147,10 +215,9 @@ def _exec_ipmitool(driver_info, command):
         args.append('-f')
         args.append(pw_file)
         args.extend(command.split(" "))
-        out, err = utils.execute(*args, attempts=3)
-        LOG.debug("ipmitool stdout: '%(out)s', stderr: '%(err)s', from node "
-                  "%(node_id)s",
-                  {'out': out, 'err': err, 'node_id': driver_info['uuid']})
+        out, err = utils.execute(*args)
+        LOG.debug("ipmitool stdout: '%(out)s', stderr: '%(err)s'",
+                  {'out': out, 'err': err})
         return out, err
 
 
@@ -278,6 +345,14 @@ def _power_status(driver_info):
 
 class IPMIPower(base.PowerInterface):
 
+    def __init__(self):
+        try:
+            check_timing_support()
+        except OSError:
+            # TODO(deva): raise a DriverLoadError if ipmitool
+            #             is not present on the system.
+            pass
+
     def validate(self, task, node):
         """Validate driver_info for ipmitool driver.
 
@@ -404,6 +479,14 @@ class VendorPassthru(base.VendorInterface):
 class IPMIShellinaboxConsole(base.ConsoleInterface):
     """A ConsoleInterface that uses ipmitool and shellinabox."""
 
+    def __init__(self):
+        try:
+            check_timing_support()
+        except OSError:
+            # TODO(deva): raise DriverLoadError if ipmitool
+            # is not present on the system.
+            pass
+
     def validate(self, task, node):
         """Validate the Node console info.
 
diff --git a/ironic/tests/drivers/test_ipmitool.py b/ironic/tests/drivers/test_ipmitool.py
index de0fd8fa4..71017c78d 100644
--- a/ironic/tests/drivers/test_ipmitool.py
+++ b/ironic/tests/drivers/test_ipmitool.py
@@ -47,6 +47,42 @@ CONF = cfg.CONF
 INFO_DICT = db_utils.get_test_ipmi_info()
 
 
+class IPMIToolCheckTimingTestCase(base.TestCase):
+
+    @mock.patch.object(ipmi, '_is_timing_supported')
+    @mock.patch.object(utils, 'execute')
+    def test_check_timing_pass(self, mock_exc, mock_timing):
+        mock_exc.return_value = (None, None)
+        mock_timing.return_value = None
+        expected = [mock.call(), mock.call(True)]
+
+        ipmi.check_timing_support()
+        self.assertTrue(mock_exc.called)
+        self.assertEqual(expected, mock_timing.call_args_list)
+
+    @mock.patch.object(ipmi, '_is_timing_supported')
+    @mock.patch.object(utils, 'execute')
+    def test_check_timing_fail(self, mock_exc, mock_timing):
+        mock_exc.side_effect = processutils.ProcessExecutionError()
+        mock_timing.return_value = None
+        expected = [mock.call(), mock.call(False)]
+
+        ipmi.check_timing_support()
+        self.assertTrue(mock_exc.called)
+        self.assertEqual(expected, mock_timing.call_args_list)
+
+    @mock.patch.object(ipmi, '_is_timing_supported')
+    @mock.patch.object(utils, 'execute')
+    def test_check_timing_no_ipmitool(self, mock_exc, mock_timing):
+        mock_exc.side_effect = OSError()
+        mock_timing.return_value = None
+        expected = [mock.call()]
+
+        self.assertRaises(OSError, ipmi.check_timing_support)
+        self.assertTrue(mock_exc.called)
+        self.assertEqual(expected, mock_timing.call_args_list)
+
+
 class IPMIToolPrivateMethodTestCase(base.TestCase):
 
     def setUp(self):
@@ -104,9 +140,11 @@ class IPMIToolPrivateMethodTestCase(base.TestCase):
                           ipmi._parse_driver_info,
                           node)
 
+    @mock.patch.object(ipmi, '_is_timing_supported')
     @mock.patch.object(ipmi, '_make_password_file', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test__exec_ipmitool(self, mock_exec, mock_pwf):
+    def test__exec_ipmitool_without_timing(self, mock_exec, mock_pwf,
+            mock_timing_support):
         pw_file_handle = tempfile.NamedTemporaryFile()
         pw_file = pw_file_handle.name
         file_handle = open(pw_file, "w")
@@ -120,18 +158,20 @@ class IPMIToolPrivateMethodTestCase(base.TestCase):
             'A', 'B', 'C',
             ]
 
+        mock_timing_support.return_value = False
         mock_pwf.return_value = file_handle
         mock_exec.return_value = (None, None)
 
         ipmi._exec_ipmitool(self.info, 'A B C')
 
         mock_pwf.assert_called_once_with(self.info['password'])
-        mock_exec.assert_called_once_with(*args, attempts=3)
+        mock_exec.assert_called_once_with(*args)
 
+    @mock.patch.object(ipmi, '_is_timing_supported')
     @mock.patch.object(ipmi, '_make_password_file', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test__exec_ipmitool_without_password(self, mock_exec, mock_pwf):
-        self.info['password'] = None
+    def test__exec_ipmitool_with_timing(self, mock_exec, mock_pwf,
+            mock_timing_support):
         pw_file_handle = tempfile.NamedTemporaryFile()
         pw_file = pw_file_handle.name
         file_handle = open(pw_file, "w")
@@ -141,19 +181,26 @@ class IPMIToolPrivateMethodTestCase(base.TestCase):
             '-H', self.info['address'],
             '-L', self.info['priv_level'],
             '-U', self.info['username'],
+            '-R', '12',
+            '-N', '5',
             '-f', file_handle,
             'A', 'B', 'C',
             ]
 
+        mock_timing_support.return_value = True
         mock_pwf.return_value = file_handle
         mock_exec.return_value = (None, None)
+
         ipmi._exec_ipmitool(self.info, 'A B C')
-        self.assertTrue(mock_pwf.called)
-        mock_exec.assert_called_once_with(*args, attempts=3)
 
+        mock_pwf.assert_called_once_with(self.info['password'])
+        mock_exec.assert_called_once_with(*args)
+
+    @mock.patch.object(ipmi, '_is_timing_supported')
     @mock.patch.object(ipmi, '_make_password_file', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test__exec_ipmitool_without_username(self, mock_exec, mock_pwf):
+    def test__exec_ipmitool_without_username(self, mock_exec, mock_pwf,
+            mock_timing_support):
         self.info['username'] = None
         pw_file_handle = tempfile.NamedTemporaryFile()
         pw_file = pw_file_handle.name
@@ -167,15 +214,18 @@ class IPMIToolPrivateMethodTestCase(base.TestCase):
             'A', 'B', 'C',
             ]
 
+        mock_timing_support.return_value = False
         mock_pwf.return_value = file_handle
         mock_exec.return_value = (None, None)
         ipmi._exec_ipmitool(self.info, 'A B C')
         self.assertTrue(mock_pwf.called)
-        mock_exec.assert_called_once_with(*args, attempts=3)
+        mock_exec.assert_called_once_with(*args)
 
+    @mock.patch.object(ipmi, '_is_timing_supported')
     @mock.patch.object(ipmi, '_make_password_file', autospec=True)
     @mock.patch.object(utils, 'execute', autospec=True)
-    def test__exec_ipmitool_exception(self, mock_exec, mock_pwf):
+    def test__exec_ipmitool_exception(self, mock_exec, mock_pwf,
+            mock_timing_support):
         pw_file_handle = tempfile.NamedTemporaryFile()
         pw_file = pw_file_handle.name
         file_handle = open(pw_file, "w")
@@ -189,13 +239,14 @@ class IPMIToolPrivateMethodTestCase(base.TestCase):
             'A', 'B', 'C',
             ]
 
+        mock_timing_support.return_value = False
         mock_pwf.return_value = file_handle
         mock_exec.side_effect = processutils.ProcessExecutionError("x")
         self.assertRaises(processutils.ProcessExecutionError,
                           ipmi._exec_ipmitool,
                           self.info, 'A B C')
         mock_pwf.assert_called_once_with(self.info['password'])
-        mock_exec.assert_called_once_with(*args, attempts=3)
+        mock_exec.assert_called_once_with(*args)
 
     @mock.patch.object(ipmi, '_exec_ipmitool', autospec=True)
     def test__power_status_on(self, mock_exec):
diff --git a/ironic/tests/drivers/third_party_driver_mocks.py b/ironic/tests/drivers/third_party_driver_mocks.py
index a1b5d808e..8cf86e630 100644
--- a/ironic/tests/drivers/third_party_driver_mocks.py
+++ b/ironic/tests/drivers/third_party_driver_mocks.py
@@ -28,6 +28,7 @@ import sys
 
 import mock
 
+from ironic.drivers.modules import ipmitool
 from ironic.openstack.common import importutils
 
 
@@ -47,3 +48,9 @@ if not seamicroclient:
 # the external library has been mocked
 if 'ironic.drivers.modules.seamicro' in sys.modules:
     reload(sys.modules['ironic.drivers.modules.seamicro'])
+
+
+# IPMITool driver checks the system for presense of 'ipmitool' binary during
+# __init__. We bypass that check in order to run the unit tests, which do not
+# depend on 'ipmitool' being on the system.
+ipmitool.TIMING_SUPPORT = False