From 1de3db3b16f3e0475e506e540ca5d5ed6edb4cbf Mon Sep 17 00:00:00 2001 From: Steve Baker Date: Tue, 6 Oct 2020 13:31:47 +1300 Subject: [PATCH] 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 --- ironic/drivers/modules/ipmitool.py | 42 +++++++++++++------ .../unit/drivers/modules/test_ipmitool.py | 38 ++++++++++++++++- ...ommand_retry_timeout-889a49b402e82b97.yaml | 9 ++++ 3 files changed, 75 insertions(+), 14 deletions(-) create mode 100644 releasenotes/notes/ipmi_command_retry_timeout-889a49b402e82b97.yaml diff --git a/ironic/drivers/modules/ipmitool.py b/ironic/drivers/modules/ipmitool.py index 0f1037eebb..3a7c1cab12 100644 --- a/ironic/drivers/modules/ipmitool.py +++ b/ironic/drivers/modules/ipmitool.py @@ -443,6 +443,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. @@ -463,18 +491,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 = {} @@ -486,6 +503,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 3c7ad07a7c..08c40bcc69 100644 --- a/ironic/tests/unit/drivers/modules/test_ipmitool.py +++ b/ironic/tests/unit/drivers/modules/test_ipmitool.py @@ -1050,7 +1050,7 @@ def test__exec_ipmitool_with_timing( '-L', self.info['priv_level'], '-U', self.info['username'], '-v', - '-R', '12', + '-R', '7', '-N', '5', '-f', awesome_password_filename, 'A', 'B', 'C', @@ -1106,7 +1106,7 @@ def test__exec_ipmitool_with_timeout( '-L', self.info['priv_level'], '-U', self.info['username'], '-v', - '-R', '12', + '-R', '7', '-N', '5', '-f', awesome_password_filename, 'A', 'B', 'C', @@ -1572,6 +1572,40 @@ def test___set_and_wait_no_needless_status_polling( 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