run_kernel() records the kernel it starts in a single self._process attribute, which the timeout thread, the cleanup at the end of the output and the SIGINT handler all read back. That only works while one kernel is running at a time, which rules out running suites in parallel guests from multiple threads.
Track running kernels in a list protected by a lock instead, with each run_kernel() invocation only adding and removing its own entry. The timeout thread and the cleanup code work on a local reference to their own process rather than going via self, so one kernel timing out can no longer terminate a different one. The SIGINT handler terminates every kernel that is still running. No functional change when a single kernel is run at a time. Signed-off-by: Mark Brown <[email protected]> --- tools/testing/kunit/kunit_kernel.py | 45 ++++++++++++++++++++-------------- tools/testing/kunit/kunit_tool_test.py | 27 ++++++++++++++++++++ 2 files changed, 53 insertions(+), 19 deletions(-) diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index 2641db9eddba..1d3226f5a136 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -277,7 +277,11 @@ class LinuxSourceTree: if kconfig_add: kconfig = kunit_config.parse_from_string('\n'.join(kconfig_add)) self._kconfig.merge_in_entries(kconfig) - self._process : Optional[subprocess.Popen[Any]] = None + # Kernels currently running. There can be several when + # run_kernel() is called concurrently from multiple threads. + self._processes : List[subprocess.Popen[Any]] = [] + # SIGINT can interrupt the main thread while it holds this lock. + self._processes_lock = threading.RLock() def arch(self) -> str: return self._arch @@ -368,7 +372,9 @@ class LinuxSourceTree: """Boots the kernel and yields its console output line by line. The output is also written to `outfile`, which defaults to - test.log in the build directory. + test.log in the build directory. This may be called from + several threads at once to run kernels concurrently, in which + case each caller must supply a distinct `outfile`. """ # Copy to avoid mutating the caller-supplied list. exec_tests() reuses # the same args across repeated run_kernel() calls (e.g. --run_isolated), @@ -385,37 +391,36 @@ class LinuxSourceTree: if outfile is None: outfile = get_outfile_path(build_dir) - self._process = self._ops.start(args, build_dir) - assert self._process is not None # tell mypy it's set - assert self._process.stdout is not None # tell mypy it's set + process = self._ops.start(args, build_dir) + assert process is not None # tell mypy it's set + assert process.stdout is not None # tell mypy it's set + with self._processes_lock: + self._processes.append(process) # Enforce the timeout in a background thread. def _wait_proc() -> None: try: - if self._process: - self._process.wait(timeout=timeout) + process.wait(timeout=timeout) except Exception as e: print(e) - if self._process: - self._process.terminate() - self._process.wait() + process.terminate() + process.wait() waiter = threading.Thread(target=_wait_proc) waiter.start() output = open(outfile, 'w') try: # Tee the output to the file and to our caller in real time. - for line in self._process.stdout: + for line in process.stdout: output.write(line) yield line # This runs even if our caller doesn't consume every line. finally: # Flush any leftover output to the file - if self._process: - if self._process.stdout: - output.write(self._process.stdout.read()) - self._process.stdout.close() - self._process = None + output.write(process.stdout.read()) + process.stdout.close() + with self._processes_lock: + self._processes.remove(process) output.close() waiter.join() @@ -423,7 +428,9 @@ class LinuxSourceTree: def signal_handler(self, unused_sig: int, unused_frame: Optional[FrameType]) -> None: logging.error('Build interruption occurred. Cleaning console.') - if self._process: - self._process.terminate() - self._process.wait() + with self._processes_lock: + processes = list(self._processes) + for process in processes: + process.terminate() + process.wait() self._restore_terminal_if_tty() diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py index 4e8068dce918..c4999273e491 100755 --- a/tools/testing/kunit/kunit_tool_test.py +++ b/tools/testing/kunit/kunit_tool_test.py @@ -18,6 +18,7 @@ import os import signal import subprocess import sys +import textwrap from typing import Iterable import xml.etree.ElementTree as ET @@ -538,6 +539,8 @@ class LinuxSourceTreeTest(unittest.TestCase): with open(outfile, 'rt') as f: self.assertEqual(f.read(), 'hi\n') self.assertFalse(os.path.exists(kunit_kernel.get_outfile_path(build_dir))) + # The kernel has exited so should no longer be tracked. + self.assertEqual(tree._processes, []) def test_run_kernel_args_not_mutated(self): """Verify run_kernel() copies args so callers can reuse them.""" @@ -607,6 +610,30 @@ class LinuxSourceTreeTest(unittest.TestCase): mock_error.assert_called_once() mock_call.assert_called_once_with(['stty', 'sane']) + def test_signal_handler_during_process_update(self): + # Run in a subprocess so a deadlock fails with a timeout rather + # than hanging the test runner. Deliver a real signal while the + # main thread holds the lock used to update the process list. + code = textwrap.dedent('''\ + import os + import signal + from unittest import mock + import kunit_kernel + + tree = kunit_kernel.LinuxSourceTree('', kunitconfig_paths=[os.devnull]) + process = mock.Mock() + with tree._processes_lock: + tree._processes.append(process) + os.kill(os.getpid(), signal.SIGINT) + process.terminate.assert_called_once_with() + process.wait.assert_called_once_with() + ''') + result = subprocess.run([sys.executable, '-c', code], + cwd=os.path.dirname(os.path.abspath(__file__)), + stdin=subprocess.DEVNULL, stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, text=True, timeout=5) + self.assertEqual(result.returncode, 0, msg=result.stdout) + def test_build_reconfig_no_config(self): with tempfile.TemporaryDirectory('') as build_dir: with open(kunit_kernel.get_kunitconfig_path(build_dir), 'w') as f: -- 2.47.3
