Use native oslo.concurrency execution timeout in ipmitool

This change replaces custom Popen-based code with the new argument
(backed by the corresponding stdlib argument).

Story: #2004449
Task: #40283
Change-Id: I6840b1caffd272ef12ab2b259a02376ec185bc3f

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'
@@ -494,14 +469,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.
+        extra_args['timeout'] = timeout
-        # 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)
 
     if check_exit_code is not None:
         extra_args['check_exit_code'] = check_exit_code
@@ -591,7 +559,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})

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)

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

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