From d4c49f2f2177ab60279e2b537a99fdeaee90dcaf Mon Sep 17 00:00:00 2001 From: Rick Boyd Date: Wed, 27 Jul 2022 11:10:44 -0400 Subject: [PATCH] Surface ES startup errors to the Rally CLI console (#1476) This change uses the subprocess.run function introduced in Python 3.5 to check the subprocess and print its output to the console on failure. There is some background to why we do not do this currently in #879, but I believe the output is worthwhile, and at the time I think 3.5 was very recently our minimum Python version so this option may have been overlooked... or there may be a more technical reason I am missing. --- esrally/mechanic/launcher.py | 23 ++++++++++------------- tests/driver/runner_test.py | 2 +- tests/mechanic/launcher_test.py | 1 + 3 files changed, 12 insertions(+), 14 deletions(-) diff --git a/esrally/mechanic/launcher.py b/esrally/mechanic/launcher.py index 2c6266f6d..29c3209bb 100644 --- a/esrally/mechanic/launcher.py +++ b/esrally/mechanic/launcher.py @@ -18,12 +18,13 @@ import os import shlex import subprocess +from textwrap import indent import psutil from esrally import exceptions, telemetry, time from esrally.mechanic import cluster, java_resolver -from esrally.utils import io, opts, process +from esrally.utils import console, io, opts, process class DockerLauncher: @@ -199,13 +200,9 @@ def _set_env(self, env, k, v, separator=" ", prepend=False): @staticmethod def _run_subprocess(command_line, env): command_line_args = shlex.split(command_line) - - with subprocess.Popen( - command_line_args, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, env=env, start_new_session=True - ) as command_line_process: - # wait for it to finish - command_line_process.wait() - return command_line_process.returncode + subprocess.run( + command_line_args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env, start_new_session=True, check=True, text=True + ) @staticmethod def _start_process(binary_path, env): @@ -215,11 +212,11 @@ def _start_process(binary_path, env): cmd = [io.escape_path(os.path.join(".", "bin", "elasticsearch"))] pid_path = io.escape_path(os.path.join(".", "pid")) cmd.extend(["-d", "-p", pid_path]) - ret = ProcessLauncher._run_subprocess(command_line=" ".join(cmd), env=env) - if ret != 0: - msg = "Daemon startup failed with exit code [{}]".format(ret) - logging.error(msg) - raise exceptions.LaunchError(msg) + try: + ProcessLauncher._run_subprocess(command_line=" ".join(cmd), env=env) + except subprocess.CalledProcessError as e: + console.error("Daemon startup failed with exit code [%s]. STDERR:\n\n%s\n" % (e.returncode, indent(e.stderr, "\t"))) + raise e return wait_for_pidfile(pid_path) diff --git a/tests/driver/runner_test.py b/tests/driver/runner_test.py index 87db65446..476a3dc35 100644 --- a/tests/driver/runner_test.py +++ b/tests/driver/runner_test.py @@ -6215,7 +6215,7 @@ async def test_adds_request_timings(self): assert len(timings) == 3 assert timings[0]["operation"] == "initial-call" - assert timings[0]["service_time"] == pytest.approx(0.1, abs=0.1) + assert timings[0]["service_time"] == pytest.approx(0.1, abs=0.15) assert timings[1]["operation"] == "stream-a" assert timings[1]["service_time"] == pytest.approx(0.2, abs=0.1) diff --git a/tests/mechanic/launcher_test.py b/tests/mechanic/launcher_test.py index 49969177e..8718c56e7 100644 --- a/tests/mechanic/launcher_test.py +++ b/tests/mechanic/launcher_test.py @@ -97,6 +97,7 @@ def __init__(self, *args, **kwargs): # mocking them as their optional functionality is disabled. self.returncode = 1 self.stdout = io.StringIO() + self.args = None def communicate(self, input=None, timeout=None): return [b"", b""]