New configuration parameter to use ipmitool retries

Add a new ``[ipmi]use_ipmitool_retries`` option. When set to
``True`` and timing is supported by ipmitool, the number of
retries and command interval will be passed to ipmitool so
that ipmitool will do the retries.  When set to ``False``,
ironic will do the retries.

The default is ``True``, so this will not change the current
behaviour which is to have ipmitool do the retries when
timing is supported.

Setting to ``False`` will help with certain BMCs which do
not support the Cipher Suites command.  In this case ipmitool
can take up to 10 seconds for each retry which results in a
total time exceeding ``[ipmi]command_retry_timeout``.

Change-Id: I1d0194e7c7ae9fcdd4665e6115ee26d10b14e480
Story: 2007632
Task: 39676

ironic/conf/ipmi.py

@@ -35,6 +35,14 @@ opts = [
                       '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.')),
+    cfg.BoolOpt('use_ipmitool_retries',
+                default=True,
+                help=_('When set to True and the parameters are supported by '
+                       'ipmitool, the number of retries and the retry '
+                       'interval are passed to ipmitool as parameters, and '
+                       'ipmitool will do the retries.  When set to False, '
+                       'ironic will retry the ipmitool commands. '
+                       'Recommended setting is True')),
     cfg.BoolOpt('kill_on_timeout',
                 default=True,
                 help=_('Kill `ipmitool` process invoked by ironic to read '

ironic/drivers/modules/ipmitool.py

@@ -480,10 +480,16 @@ def _exec_ipmitool(driver_info, command, check_exit_code=None,
 
     if _is_option_supported('timing'):
         args.append('-R')
-        args.append(str(num_tries))
+        if CONF.ipmi.use_ipmitool_retries:
+            args.append(str(num_tries))
+        else:
+            args.append('1')
 
         args.append('-N')
-        args.append(str(CONF.ipmi.min_command_interval))
+        if CONF.ipmi.use_ipmitool_retries:
+            args.append(str(CONF.ipmi.min_command_interval))
+        else:
+            args.append('1')
 
     extra_args = {}
 
@@ -530,9 +536,12 @@ def _exec_ipmitool(driver_info, command, check_exit_code=None,
                             IPMITOOL_RETRYABLE_FAILURES
                             + CONF.ipmi.additional_retryable_ipmi_errors)
                         if x in str(e)]
+                    # If Ironic is doing retries then retry all errors
+                    retry_failures = (err_list
+                                      or not CONF.ipmi.use_ipmitool_retries)
                     if ((time.time() > end_time)
                         or (num_tries == 0)
-                        or not err_list):
+                        or not retry_failures):
                         LOG.error('IPMI Error while attempting "%(cmd)s" '
                                   'for node %(node)s. Error: %(error)s',
                                   {'node': driver_info['uuid'],

ironic/tests/unit/drivers/modules/test_ipmitool.py

@@ -1042,6 +1042,63 @@ class IPMIToolPrivateMethodTestCase(
         mock_support.assert_called_once_with('timing')
         mock_exec.assert_called_once_with(*args)
 
+    @mock.patch.object(ipmi, '_is_option_supported', autospec=True)
+    @mock.patch.object(ipmi, '_make_password_file', _make_password_file_stub)
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test__exec_ipmitool_with_ironic_retries(
+            self, mock_exec, mock_support):
+        args = [
+            'ipmitool',
+            '-I', 'lanplus',
+            '-H', self.info['address'],
+            '-L', self.info['priv_level'],
+            '-U', self.info['username'],
+            '-v',
+            '-R', '1',
+            '-N', '1',
+            '-f', awesome_password_filename,
+            'A', 'B', 'C',
+        ]
+
+        mock_support.return_value = True
+        mock_exec.return_value = (None, None)
+
+        self.config(use_ipmitool_retries=False, group='ipmi')
+
+        ipmi._exec_ipmitool(self.info, 'A B C')
+
+        mock_support.assert_called_once_with('timing')
+        mock_exec.assert_called_once_with(*args)
+
+    @mock.patch.object(ipmi, '_is_option_supported', autospec=True)
+    @mock.patch.object(ipmi, '_make_password_file', _make_password_file_stub)
+    @mock.patch.object(utils, 'execute', autospec=True)
+    def test__exec_ipmitool_with_ironic_retries_multiple(
+            self, mock_exec, mock_support):
+
+        mock_exec.side_effect = [
+            processutils.ProcessExecutionError(
+                stderr="Unknown"
+            ),
+            processutils.ProcessExecutionError(
+                stderr="Unknown"
+            ),
+            processutils.ProcessExecutionError(
+                stderr="Unknown"
+            ),
+        ]
+
+        self.config(min_command_interval=1, group='ipmi')
+        self.config(command_retry_timeout=3, group='ipmi')
+        self.config(use_ipmitool_retries=False, group='ipmi')
+
+        self.assertRaises(processutils.ProcessExecutionError,
+                          ipmi._exec_ipmitool,
+                          self.info, 'A B C')
+
+        mock_support.assert_called_once_with('timing')
+        self.assertEqual(3, mock_exec.call_count)
+
     def test__exec_ipmitool_wait(self):
         mock_popen = mock.MagicMock()
         mock_popen.poll.side_effect = [1, 1, 1, 1, 1]

releasenotes/notes/ipmitool-use_ipmitool_retries-b55b2b8ed5cab603.yaml

@@ -0,0 +1,16 @@
+---
+features:
+  - |
+    Adds a new ``[ipmi]use_ipmitool_retries`` option. When set to
+    ``True`` and timing is supported by ipmitool, the number of
+    retries and command interval will be passed to ipmitool so
+    that ipmitool will do the retries.  When set to ``False``,
+    ironic will do the retries.  Default is ``True``.
+issues:
+  - |
+    Some BMCs do not support the ``Channel Cipher Suites`` command
+    that newer versions of ipmitool use.  These versions of
+    ipmitool will resend this command for each ipmitool retry,
+    resulting in long response times.  Setting
+    ``[ipmi]use_ipmitool_retries`` to ``false`` will avoid this
+    situation by implementing retries on the ironic level.