Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't read from stdout/stderr on ES startup #879

Merged
merged 2 commits into from
Jan 27, 2020
Merged

Don't read from stdout/stderr on ES startup #879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dc16f8a
Don't read from stdout/stderr on ES startup
danielmitterdorfer Jan 27, 2020
ee68ff8
No output, no newlines
danielmitterdorfer Jan 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion esrally/mechanic/launcher.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
# under the License.
import logging
import os
import shlex
import subprocess

import psutil

Expand Down Expand Up @@ -186,14 +188,28 @@ def _set_env(self, env, k, v, separator=' ', prepend=False):
else:
env[k] = env[k] + separator + v

@staticmethod
def _run_subprocess(command_line, env):
command_line_args = shlex.split(command_line)

# pylint: disable=subprocess-popen-preexec-fn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we didn't comment this last time when this was added, but when going back over this code I completely forgot about why we have a preexec_fn. Seems an uncommonly used function that is prone to trouble as in the Pylint warning. Perhaps we can briefly comment why it is necessary for detached processes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we can "upgrade" to using the start_new_session argument described in https://docs.python.org/3.2/library/subprocess.html

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback! As this is currently blocking our nightly benchmarks I'll merge this PR as is but this is definitely a good idea for a for a future PR. I'll keep this on my radar! :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed #884.

with subprocess.Popen(command_line_args,
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
env=env,
preexec_fn=os.setpgrp) as command_line_process:
# wait for it to finish
command_line_process.poll()
return command_line_process.returncode

@staticmethod
def _start_process(binary_path, env):
if os.name == "posix" and os.geteuid() == 0:
raise exceptions.LaunchError("Cannot launch Elasticsearch as root. Please run Rally as a non-root user.")
os.chdir(binary_path)
cmd = [io.escape_path(os.path.join(".", "bin", "elasticsearch"))]
cmd.extend(["-d", "-p", "pid"])
ret = process.run_subprocess_with_logging(command_line=" ".join(cmd), env=env, detach=True)
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)
Expand Down
3 changes: 3 additions & 0 deletions tests/mechanic/launcher_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,9 @@ def __init__(self, *args, **kwargs):
def communicate(self, input=None, timeout=None):
return [b"", b""]

def poll(self):
pass

def __enter__(self):
return self

Expand Down