diff --git a/ironic/drivers/modules/ipmitool.py b/ironic/drivers/modules/ipmitool.py index 92b9500e6b..85beca183c 100644 --- a/ironic/drivers/modules/ipmitool.py +++ b/ironic/drivers/modules/ipmitool.py @@ -30,13 +30,13 @@ DRIVER. """ import contextlib -import functools import os import re import subprocess import tempfile import time +from eventlet.green import subprocess as green_subprocess from ironic_lib import metrics_utils from ironic_lib import utils as ironic_utils from oslo_concurrency import processutils @@ -389,31 +389,6 @@ def _parse_driver_info(node): } -def _exec_ipmitool_wait(timeout, driver_info, popen_obj): - wait_interval = min(timeout, 0.5) - - while timeout >= 0: - if not popen_obj.poll(): - return - - time.sleep(wait_interval) - timeout -= wait_interval - - LOG.warning('Killing timed out IPMI process "%(cmd)s" for node %(node)s.', - {'node': driver_info['uuid'], 'cmd': popen_obj.cmd}) - - popen_obj.terminate() - time.sleep(0.5) - if popen_obj.poll(): - popen_obj.kill() - - time.sleep(1) - - if popen_obj.poll(): - LOG.warning('Could not kill IPMI process "%(cmd)s" for node %(node)s.', - {'node': driver_info['uuid'], 'cmd': popen_obj.cmd}) - - def _get_ipmitool_args(driver_info, pw_file=None): ipmi_version = ('lanplus' if driver_info['protocol_version'] == '2.0' @@ -491,14 +466,7 @@ def _exec_ipmitool(driver_info, command, check_exit_code=None, extra_args = {} if kill_on_timeout: - # NOTE(etingof): We can't trust ipmitool to terminate in time. - # Therefore we have to kill it if it is running for longer than - # we asked it to. - # For that purpose we inject the time-capped `popen.wait` call - # before the uncapped `popen.communicate` is called internally. - # That gives us a chance to kill misbehaving `ipmitool` child. - extra_args['on_execute'] = functools.partial( - _exec_ipmitool_wait, timeout, driver_info) + extra_args['timeout'] = timeout if check_exit_code is not None: extra_args['check_exit_code'] = check_exit_code @@ -588,7 +556,10 @@ def _set_and_wait(task, power_action, driver_info, timeout=None): try: _exec_ipmitool(driver_info, cmd) except (exception.PasswordFileFailedToCreate, - processutils.ProcessExecutionError) as e: + processutils.ProcessExecutionError, + subprocess.TimeoutExpired, + # https://github.com/eventlet/eventlet/issues/624 + green_subprocess.TimeoutExpired) as e: LOG.warning("IPMI power action %(cmd)s failed for node %(node_id)s " "with error: %(error)s.", {'node_id': driver_info['uuid'], 'cmd': cmd, 'error': e}) diff --git a/ironic/tests/unit/drivers/modules/test_ipmitool.py b/ironic/tests/unit/drivers/modules/test_ipmitool.py index f01c2f4248..e45aee26f3 100644 --- a/ironic/tests/unit/drivers/modules/test_ipmitool.py +++ b/ironic/tests/unit/drivers/modules/test_ipmitool.py @@ -1072,6 +1072,33 @@ 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_timeout( + self, mock_exec, mock_support): + args = [ + 'ipmitool', + '-I', 'lanplus', + '-H', self.info['address'], + '-L', self.info['priv_level'], + '-U', self.info['username'], + '-v', + '-R', '12', + '-N', '5', + '-f', awesome_password_filename, + 'A', 'B', 'C', + ] + + mock_support.return_value = True + mock_exec.return_value = (None, None) + + self.config(use_ipmitool_retries=True, group='ipmi') + ipmi._exec_ipmitool(self.info, 'A B C', kill_on_timeout=True) + + mock_support.assert_called_once_with('timing') + mock_exec.assert_called_once_with(*args, timeout=60) + @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) @@ -1101,14 +1128,6 @@ class IPMIToolPrivateMethodTestCase( 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] - ipmi._exec_ipmitool_wait(1, {'uuid': ''}, mock_popen) - - self.assertTrue(mock_popen.terminate.called) - self.assertTrue(mock_popen.kill.called) - @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) diff --git a/lower-constraints.txt b/lower-constraints.txt index 82533a1b1f..c29d25e8db 100644 --- a/lower-constraints.txt +++ b/lower-constraints.txt @@ -61,7 +61,7 @@ os-client-config==2.1.0 os-service-types==1.7.0 os-traits==0.4.0 osc-lib==2.0.0 -oslo.concurrency==3.26.0 +oslo.concurrency==4.2.0 oslo.config==5.2.0 oslo.context==2.19.2 oslo.db==4.40.0 diff --git a/requirements.txt b/requirements.txt index aa80cd787e..bb37845eb7 100644 --- a/requirements.txt +++ b/requirements.txt @@ -16,7 +16,7 @@ python-swiftclient>=3.2.0 # Apache-2.0 pytz>=2013.6 # MIT stevedore>=1.20.0 # Apache-2.0 pysendfile>=2.0.0;sys_platform!='win32' # MIT -oslo.concurrency>=3.26.0 # Apache-2.0 +oslo.concurrency>=4.2.0 # Apache-2.0 oslo.config>=5.2.0 # Apache-2.0 oslo.context>=2.19.2 # Apache-2.0 oslo.db>=4.40.0 # Apache-2.0