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

Kill the container ourselves when done #1019

Merged
merged 1 commit into from
Dec 11, 2023
Merged

Kill the container ourselves when done #1019

merged 1 commit into from
Dec 11, 2023

Conversation

shepmaster
Copy link
Member

We stream messages from the coordinator to the worker via stdin/stdout piped to docker run. However, sometimes we close the pipe corresponding to docker run's stdout before it has had a chance to fully write everything. If that happens, docker run exits without deleting the container 1.

This has likely always been a problem with the coordinator / worker architecture, but it's exacerbated by the recent addition of the memory / CPU status messages that occur every second, which greatly increase the amount of data crossing the boundary.

This reorganizes the code to pass in a known reasonably unique name to the container and then attempts to kill that container at cleanup time.

I tested by:

  • Setting the worker status interval to 16ms (from 1s)
  • Setting the webserver idle interval to 7s (from 60s)
  • Setting the webserver session timeout to 14s (from 45m)
  • Running code that prints infinitely (loop { println!("moo"); })

This reliably recreated the write /dev/stdout: broken pipe error from the docker run invocation, but after this commit there were no vestigial running containers.

We stream messages from the coordinator to the worker via stdin/stdout
piped to `docker run`. However, sometimes we close the pipe
corresponding to `docker run`'s stdout before it has had a chance to
fully write everything. If that happens, `docker run` exits without
deleting the container [1].

This has likely always been a problem with the coordinator / worker
architecture, but it's exacerbated by the recent addition of the
memory / CPU status messages that occur every second, which greatly
increase the amount of data crossing the boundary.

This reorganizes the code to pass in a known reasonably unique name to
the container and then attempts to kill that container at cleanup
time.

I tested by:

- Setting the worker status interval to 16ms (from 1s)
- Setting the webserver idle interval to 7s (from 60s)
- Setting the webserver session timeout to 14s (from 45m)
- Running code that prints infinitely (`loop { println!("moo"); }`)

This reliably recreated the `write /dev/stdout: broken pipe` error
from the `docker run` invocation, but after this commit there were no
vestigial running containers.

[1]: moby/moby#38769
@shepmaster shepmaster added bug The playground isn't doing what it was intended to maintenance Keeping the wheels turning labels Dec 11, 2023
@shepmaster shepmaster merged commit d544d19 into main Dec 11, 2023
17 checks passed
@shepmaster shepmaster deleted the kill-more branch December 11, 2023 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The playground isn't doing what it was intended to maintenance Keeping the wheels turning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant