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.

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

Make it safe for workload executors to output large amounts of text #9

Closed
prashantmital opened this issue Feb 28, 2020 · 1 comment · Fixed by #63
Closed

Make it safe for workload executors to output large amounts of text #9

prashantmital opened this issue Feb 28, 2020 · 1 comment · Fixed by #63
Assignees
Labels
bug Something isn't working enhancement New feature or request high priority This is needed urgently
Milestone

Comments

@prashantmital
Copy link
Contributor

Currently, when running the workload executor, we PIPE the output from its stderr and stdout streams. To avoid blocking the child process when the pipe fills up, we need to continuously read the worker_subprocess output (via worker_subprocess.stdout.read(..)).

Thanks to @ShaneHarvey for the context:

See the note in subprocess.wait:

This will deadlock when using stdout=PIPE or stderr=PIPE and the child process generates enough output to a pipe such that it blocks waiting for the OS pipe buffer to accept more data. Use Popen.communicate() when using pipes to avoid that.

https://docs.python.org/3/library/subprocess.html#subprocess.Popen.wait

Just to be clear: there's no risk of deadlock in the current approach. The risk is that the child process can fill up the pipe with output and then it will block until we call worker_subprocess.communicate() down below.

A more elegant solution will be to redirect the worker_subprocess output to a file. Then we can open and read the file after the process exits.

@prashantmital prashantmital added bug Something isn't working enhancement New feature or request labels Feb 28, 2020
@prashantmital prashantmital changed the title Handle Make it safe for workload executors to output large amounts of text Feb 28, 2020
@prashantmital prashantmital added the high priority This is needed urgently label Mar 19, 2020
@prashantmital prashantmital added this to the MVP milestone Apr 3, 2020
@prashantmital prashantmital self-assigned this Apr 23, 2020
@prashantmital
Copy link
Contributor Author

Since we no longer require workload executors to send all output to STDOUT, we can simply not pipe the standard streams of the workload executor subprocess back to the parent astrolabe process. This means we will get a unified log on evergreen containing astrolabe logs, workload executor stdout, and workload executor stderr. That should suffice for the time being and also resolve this issue of the pipe getting saturated and blocking the execution of workload executor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request high priority This is needed urgently
Projects
None yet
1 participant