Fix ipmitool timing argument calculation

Calculating the ipmitool `-N` and `-R` arguments from ironic.conf
[ipmi] `command_retry_timeout` and `min_command_interval` now takes
into account the 1 second interval increment that ipmitool adds on
each retry event.

Failure-path ipmitool run duration will now be just less than
`command_retry_timeout` instead of much longer.

Change-Id: Ia3d8d85497651290c62341ac121e2aa438b4ac50
(cherry picked from commit 1de3db3b16f3e0475e506e540ca5d5ed6edb4cbf)

3 files changed, 75 insertions, 14 deletions

diff --git a/ironic/drivers/modules/ipmitool.py b/ironic/drivers/modules/ipmitool.py
index 5f9cb334d..9020b8234 100644
--- a/ironic/drivers/modules/ipmitool.py
+++ b/ironic/drivers/modules/ipmitool.py
@@ -487,6 +487,34 @@ def _get_ipmitool_args(driver_info, pw_file=None):
     return args
 
 
+def _ipmitool_timing_args():
+    if not _is_option_supported('timing'):
+        return []
+
+    if not CONF.ipmi.use_ipmitool_retries:
+        return [
+            '-R', '1',
+            '-N', str(CONF.ipmi.min_command_interval)
+        ]
+
+    timeout = CONF.ipmi.command_retry_timeout
+    interval = CONF.ipmi.min_command_interval
+    num_tries = 0
+    cmd_duration = 0
+
+    while cmd_duration + interval <= timeout:
+        cmd_duration += interval
+        # for each attempt, ipmitool adds one second to the previous
+        # interval value
+        interval += 1
+        num_tries += 1
+
+    return [
+        '-R', str(num_tries),
+        '-N', str(CONF.ipmi.min_command_interval)
+    ]
+
+
 def _exec_ipmitool(driver_info, command, check_exit_code=None,
                    kill_on_timeout=False):
     """Execute the ipmitool command.
@@ -507,18 +535,7 @@ def _exec_ipmitool(driver_info, command, check_exit_code=None,
 
     timeout = CONF.ipmi.command_retry_timeout
 
-    # specify retry timing more precisely, if supported
-    num_tries = max((timeout // CONF.ipmi.min_command_interval), 1)
-
-    if _is_option_supported('timing'):
-        args.append('-R')
-        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))
+    args.extend(_ipmitool_timing_args())
 
     extra_args = {}
 
@@ -530,6 +547,7 @@ def _exec_ipmitool(driver_info, command, check_exit_code=None,
 
     end_time = (time.time() + timeout)
 
+    num_tries = max((timeout // CONF.ipmi.min_command_interval), 1)
     while True:
         num_tries = num_tries - 1
         # NOTE(tenbrae): ensure that no communications are sent to a BMC more
diff --git a/ironic/tests/unit/drivers/modules/test_ipmitool.py b/ironic/tests/unit/drivers/modules/test_ipmitool.py
index 7236ef399..7a172ec2d 100644
--- a/ironic/tests/unit/drivers/modules/test_ipmitool.py
+++ b/ironic/tests/unit/drivers/modules/test_ipmitool.py
@@ -1075,7 +1075,7 @@ class IPMIToolPrivateMethodTestCase(
             '-L', self.info['priv_level'],
             '-U', self.info['username'],
             '-v',
-            '-R', '12',
+            '-R', '7',
             '-N', '5',
             '-f', awesome_password_filename,
             'A', 'B', 'C',
@@ -1131,7 +1131,7 @@ class IPMIToolPrivateMethodTestCase(
             '-L', self.info['priv_level'],
             '-U', self.info['username'],
             '-v',
-            '-R', '12',
+            '-R', '7',
             '-N', '5',
             '-f', awesome_password_filename,
             'A', 'B', 'C',
@@ -1598,6 +1598,40 @@ class IPMIToolPrivateMethodTestCase(
                               self.info)
         self.assertFalse(mock_status.called)
 
+    @mock.patch.object(ipmi, '_is_option_supported', autospec=True)
+    def test__ipmitool_timing_args(self, mock_support):
+        # timing arguments not supported
+        mock_support.return_value = False
+        self.assertEqual([], ipmi._ipmitool_timing_args())
+
+        # handle retries by calling ipmitool repeatedly
+        mock_support.return_value = True
+        self.config(use_ipmitool_retries=False, group='ipmi')
+        self.config(min_command_interval=5, group='ipmi')
+        self.assertEqual(['-R', '1', '-N', '5'], ipmi._ipmitool_timing_args())
+
+        # confirm that different combinations of min_command_interval and
+        # command_retry_timeout result in the command finishing before
+        # command_retry_timeout
+        self.config(use_ipmitool_retries=True, group='ipmi')
+
+        # 7 retries, first interval is 5s
+        # 5 + 6 + 7 + 8 + 9 + 10 + 11 = 56s
+        self.config(command_retry_timeout=60, group='ipmi')
+        self.assertEqual(['-R', '7', '-N', '5'], ipmi._ipmitool_timing_args())
+
+        # 11 retries, first interval is 5s
+        # 5 + 6 + 7 + 8 + 9 + 10 + 11 + 12 + 13 + 14 + 15 = 110s
+        self.config(command_retry_timeout=120, group='ipmi')
+        self.config(min_command_interval=5, group='ipmi')
+        self.assertEqual(['-R', '11', '-N', '5'], ipmi._ipmitool_timing_args())
+
+        # 7 retries, first interval is 1s
+        # 1 + 2 + 3 + 4 + 5 + 6 + 7 = 28s
+        self.config(command_retry_timeout=30, group='ipmi')
+        self.config(min_command_interval=1, group='ipmi')
+        self.assertEqual(['-R', '7', '-N', '1'], ipmi._ipmitool_timing_args())
+
 
 class IPMIToolDriverTestCase(Base):
 
diff --git a/releasenotes/notes/ipmi_command_retry_timeout-889a49b402e82b97.yaml b/releasenotes/notes/ipmi_command_retry_timeout-889a49b402e82b97.yaml
new file mode 100644
index 000000000..b599b0b14
--- /dev/null
+++ b/releasenotes/notes/ipmi_command_retry_timeout-889a49b402e82b97.yaml
@@ -0,0 +1,9 @@
+---
+fixes:
+  - |
+    Calculating the ipmitool `-N` and `-R` arguments from ironic.conf [ipmi]
+    `command_retry_timeout` and `min_command_interval` now takes into account the
+    1 second interval increment that ipmitool adds on each retry event.
+
+    Failure-path ipmitool run duration will now be just less than
+    `command_retry_timeout` instead of much longer.
\ No newline at end of file