-
Notifications
You must be signed in to change notification settings - Fork 184
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
SIGTERM isn't propegated or respected, making docker stop
slow and then forceful
#771
Comments
Nice, thanks. Do I follow correctly that this issue just impacts the binder-based images running the default command
Would you suggest just changing this entrypoint for the binder-based images? Or is this unrelated to the default CMD that is being used to launch the instance? Is tini being used as the entrypoint in, say, pangeo/jupyter stacks? You mention both As you probably saw, most of the other images in this stack already use a lightweight init manager, s6-init, by default. Thanks much. |
I think the issue is for all images in this repo, but I've not verified it. By declaring ENTRYPOINT next to CMD, they combine to the final command to execute where entrypoint comes first - if not specified, it just defaults to a shell running the command in CMD i think. The issue is probably the same for most container runtimes, at least those defaulting entrypoint to a shell. I didn't see that a6-init was used, and have not heard about it before. If something like that is already used, it should be declared in the entrypoint i think, otherwise a shell will be the entrypoint and fail to propegate signals to the thing provided in CMD i think. Got to run back to taking care of our little baby! |
Thanks @consideRatio . Is tini used in other projects you are familiar with, e.g. in the jupyterhub system? I think we should definitely consider revisiting this:
s6-init has always been a source of friction. We adopted maybe almost a decade ago when it was afaik the only init manager that played nicely with Docker (and there were many long blog posts about the problems of using either of the more standard init managers distributed in Ubuntu with docker). s6-overlay install has always been a bit more convoluted than a simple apt-get, and because it's not coming from official ubuntu repos it means we have to pin version with an env var, and it's always been a bit complex to configure. So I think there's good reasons for us to revisit this and maybe switching to tini. That said, this could easily be a breaking change, and I think the issue is specific to binder image, so for now it probably makes more sense to address it there. Note that s6-init is used in all containers in this image other than the base image,
The binder image overrides the default command, and thus bypasses S6-init.
Nevertheless, we've fallen behind the times here -- Scanning the docs, it looks like it now supports / recommends using ENTRYPOINT as well. So I do think it makes sense to migrate away from the s6-overlay mechanism to something simpler and more standard, but probably wait for the next major lift. Meanwhile maybe we can consider a simpler fix for binder image alone? @consideRatio 's solution looks reasonable to me. Maybe you can send a PR? |
Yes quite systematically across jupyterhub project's providing Dockerfiles, either
I'm not confident on how things get started from running ENTRYPOINT ["tini", "--"]
CMD ["jupyterhub", "--config", "/usr/local/etc/jupyterhub/jupyterhub_config.py"] Since only I opened #773, but I think I'd like to settle with that as I lack knowledge about a6-overlay and isn't motivated enough given time my contrainsts to learn about it at the moment. |
Thank you @cboettig for your quick feedback and discussion about this, and your work in this project in general!!! |
A good overview: https://ahmet.im/blog/minimal-init-process-for-containers/ |
Thanks @benz0li , that's a nice writeup. Sounds like s6-overlay is still the favorite option for handling more than one child process (which I know at least some users take advantage of, though I agree it's a bit of an edge case. yes the blog shows a hack around that with It's notable to me that docker already supports drop-in tini via
I think we should try to move to tini entrypoint in our next overhaul of the build logic. we can keep the s6 install script in scripts dir as an opt-in fallback. |
@cboettig Then, you need to adapt rocker-versioned2/scripts/init_set_env.sh Lines 4 to 10 in 08427f9
to something like ## Set our dynamic variables in Renviron.site to be reflected by RStudio Server or Shiny Server
exclude_vars="BATCH_USER_CREATION HOME OLDPWD PASSWORD PWD RSTUDIO_VERSION SHLVL"
for var in $(compgen -e); do
[[ ! $exclude_vars =~ $var ]] && echo "$var=${!var}" >>"$(R RHOME)/etc/Renviron.site" || echo "skipping ${var}" \
done IMHO Cross reference: jupyterhub/jupyter-rsession-proxy#145 |
@benz0li oh interesting. Yeah I was wondering how we ensure init scripts are run if we drop S6. I'm afraid this went a bit over my head though -- how do we have an init script that executes at runtime before bringing up RStudio? e.g. we'd no longer have these, right?: rocker-versioned2/scripts/install_rstudio.sh Lines 128 to 131 in 08427f9
|
@cboettig Similar to the Jupyter Docker Stacks:
|
Fixes #771, not by introducing `tini`, but by ensuring we don't let `sh` be the init process which in turn runs `jupyter lab` without propegating SIGTERM to it. There may be a point to introduce `tini` or similar, but from the SIGTERM perspective, this does the trick as well. This PR is modelled on for example https://github.com/rocker-org/rocker-versioned2/pull/740/files where I see that all Dockerfiles are updated along with a repsective .json file.
Issue
docker stop
andkubectl delete
etc will pass a SIGTERM signal to kindly ask that it self-terminates, and after a while (10 seconds for docker, 30 seconds for kubectl), it will pass SIGKILL forcefully terminating everything. What often can happen is that a SIGTERM signal is only passed to process id 1 in the container, but then not propegated onwards to other processes that otherwise would terminate as long as they received the signal.A resolution
By using an entrypoint called
tini
, we can resolve this.The text was updated successfully, but these errors were encountered: