diff --git a/ironic/drivers/modules/ipmitool.py b/ironic/drivers/modules/ipmitool.py index 5f9cb334d4..9020b8234f 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 7236ef399a..7a172ec2d4 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 0000000000..b599b0b141 --- /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