No zombie process reaping protection built in. about s2i-python-container HOT 56 CLOSED

If it isn't handled inside the container, then if people don't use OpenShift, but use S2I directly and then use the resulting image with Docker (without tini support enabled), or some other container execution engine, then the container will not behave as one would want. An application image should not really depend on the underlying infrastructure outside of the container, that it is executing in, and do what it needs to do to work standalone. From memory I think there is something in the 12 factor manifesto along these lines.

If I can get through some backlog of stuff, will try and put together a PR, as well as deal with issues around some other outstanding PRs.

from s2i-python-container.

Also not sure I like the name ADD_APP_SCRIPT as not clear what it is for. Better off using something like ENABLE_INIT_WRAPPER or similar. Initially could use the suggested script, but later on if something like tini became part of SCL, could use that.

from s2i-python-container.

@GrahamDumpleton @bparees this affects all other images as well, right?
@GrahamDumpleton by any chance, do you know what's the state of this for database containers?

Is there an open issue in Docker related to this?

from s2i-python-container.

@rhcarvalho it affects all docker images and yes there have been rich discussions on docker about it (along with some recent mailing list threads.....), but basically each image has to deal with it on its own right now and it's more relevant to some images than others depending on the kinds of things those images do in terms of spawning child processes.

from s2i-python-container.

@bparees @GrahamDumpleton if the process table fills with zombie processes, I would assume the docker will kill it as out of resources. It would be nice if we can set the limit for number of processes for docker containers and then let Kubernetes to restart containers with a lot of "zombies".

Another solution might be to use "supervisord" or "systemd", but I think it might be too heavy for S2I images (and will require some extra packaging for centos and rhel7).

from s2i-python-container.

@bparees @GrahamDumpleton if the process table fills with zombie processes

i believe the process table in question is the host process table, so no, docker won't save you.

from s2i-python-container.

@bparees there is --ulimit option for docker run (also API option) that allows you to set something sane. I guess if you have too much processes and the server fail to create new child process, it will crash and k8s will restart it.

from s2i-python-container.

@bparees maybe k8s should expose that option for Pod->Container.

from s2i-python-container.

The Docker container wouldn't necessarily crash as depends on how the application behaves when it tries to do something and runs out of processes.

from s2i-python-container.

@GrahamDumpleton if it does not crash it might stop responding to the liveness checks.

from s2i-python-container.

Might still respond to those fine. Think of a multi process web server where still had existing processes handling requests and was just one request handler in serving a request that tried to run some sub processes to calculate something. That sub processes exec would fail. Handler would return a HTTP 500 response and go onto the next request and handle it.

from s2i-python-container.

Ok, so let's try to nail down an attack plan for solving this.

First, the path that perhaps require the minimal amount of changes: would it suffice to document the problem and the score card above, and point to using supervisord (or tini or other) as a mitigation for the web servers that do not do proper process reaping?

This would make the problem clear, would note that the image works fine with the recommended servers (gunicorn, mod_wsgi), and wouldn't require extra packaging -- assuming users can pip install the required tooling.

WDYT?

If this is not enough, let's iterate on to the next solution. Thanks!

from s2i-python-container.

Documenting the use of supervisord (which is a very heavyweight solution and still isn't available for Python 3 that I know of), or tini doesn't help much for the default S2I Python builder. This is because it is hard to actually replace process ID 1 in the way its run script works.

If people are relying on gunicorn they are okay, but if it falls back to manage.py for Django they can't insert their own program for process ID 1. If using Tornado via app.py, it means they can't put their application in app.py and have to use app.py to os.exec() their replacement process ID 1 which handles process reaping and then it has to run their Tornado app.

So it can quite messy if people have to start trying to insert their own solution.

from s2i-python-container.

@GrahamDumpleton thank you! Do you have in mind what would be the next best solution?

from s2i-python-container.

I thought I outlined things to consider in the original issue if we can't rely on something getting into SCL. :-)

from s2i-python-container.

@GrahamDumpleton if we are going to fix it, I assume we want to fix it for all images, so the solution should be common (like supervisord or systemd-container). Otherwise we are going to maintain different solutions for different images. For example, we can install supervisord in sti-base and re-use it everywhere if it makes sense. I think packaging supervisord will not be that complex (it is no-dep).

from s2i-python-container.

Don't really like supervisord if all it is going to be used for is this zombie reaping. It has a base memory footprint of 10MB from memory, where as purpose built tini is in kilobytes and even purpose Python script is not so bad.

I suspect supervisord would also have to be dragged in against system Python as as I say above, I don't think it is available for Python 3. Is the system Python still present in other language images?

from s2i-python-container.

Hmm, from the issue description we have:

1. given shell script, with a note:

   This likely isn't going to be entirely reliable, but, still better than nothing.

2. learn from the Python script mini init and implement our own to suit our requirements

Did I miss something? :-)

These two solutions would require a good level of confidence in a custom made script, and unless we put a good amount of effort to get to some robust solution, I'd prefer not wrapping the servers that handle this themselves (assuming they can do it better than us).

I agree with @mfojtik that it would be better to have a solution that we can reuse. Is there anything we could do in this direction that is in a time frame shorter than 6 months or so?

I suspect supervisord would also have to be dragged in against system Python as as I say above, I don't think it is available for Python 3. Is the system Python still present in other language images?

It is.

from s2i-python-container.

@rhcarvalho for the start I would ping @tdawson (or send him an email), asking him to package http://supervisord.org for OpenShift. Next step is to open BZ ticket for SCL team (@hhorak, do you know which component that will be?) and ask them to include it in the next version of SCL (or in RHEL7.2 extras channel). Once we have package for RHEL7, we can start prototyping this. @bparees ?

from s2i-python-container.

How can it be part of SCL? Doesn't that imply it has to use the Python versions from SCL also?

If that is the case and supervisord only works for Python 2, what are you going to do for Python 3? Or even though under SCL, would it use the system Python 2 version?

from s2i-python-container.

@GrahamDumpleton no, supervisord should not be part of any collection, it should be independent package.

What do you mean supervisord is not available for Python 3? I thought it can manage arbitrary processes.

from s2i-python-container.

@GrahamDumpleton every system has python 2.4 in base os, right? so we can use system python to run supervisord.

from s2i-python-container.

Supervisord itself is implemented in Python. As far as I know its code base hasn't been ported to Python 3 such that it is regarded as stable under Python 3. I can't check PyPi classifiers right now for it as PyPi is down. If system Python is only 2.4 though, that may not be new enough either.

from s2i-python-container.

Documentation for supervisord says:

• http://supervisord.org/introduction.html#platform-requirements

Supervisor is known to work with Python 2.4 or later but will not work under any version of Python 3.

from s2i-python-container.

@GrahamDumpleton last time I tried supervisord on centos7 it worked (dunno what python is there).

from s2i-python-container.

The system Python in sti-base is 2.7.5.
@GrahamDumpleton you are right, released versions of supervisord require Python 2.x.
https://github.com/Supervisor/supervisor

Using supervisord is viable, the question is if it is the way to go, as @GrahamDumpleton pointed his concern with that.

from s2i-python-container.

@GrahamDumpleton but before we go further into this, I would really want to know how badly we are broken. You said that gunicorn works (which is our default for python, but not for Django). Are other images vulnerable as well? Is puma or PHP in Apache broken? What are our options? If supervisord is heavy and use ancient python, can we use systemd (which is already packaged for RHEL?) or we want to use something exotic like tiny?

from s2i-python-container.

If PHP image is simply running Apache it should be fine. I don't know about Ruby or Node. For Java if it is running one of its monoliths, it likely would be okay as it probably handles process management fine.

from s2i-python-container.

@GrahamDumpleton k, so it sounds like we are only broken for python. I agree with @rhcarvalho that in that case we should document what people can do to fix this problem (is it viable to use gunicorn in django?).

from s2i-python-container.

@mfojtik gunicorn is the default for Django as well, the difference there is that we have a fallback to the Django development server, that doesn't do any process management among other things (not production ready, and you get a warning if using that fallback).

from s2i-python-container.

If they install gunicorn then it will not use manage.py, but then they will themselves need to modify their Django application to use Whitenoise WSGI middleware around Django to handle serving static files.

FWIW. The latest version of my own S2I Python builder has an auto mode for Django which defaults to using mod_wsgi-express, but user can override it and say want to use gunicorn, uWSGI or Waitress. For mod_wsgi-express and uWSGI it will use inbuilt abilities of those to handle static files. For gunicorn and Waitress it will automatically apply Whitenoise WSGI middleware wrapper around Django WSGI application entry point. So all works automatically no matter which of the popular WSGI servers you prefer to use.

from s2i-python-container.

I think we should focus on systemd option here, in case it is possible. @lnykryn @praiskup: I guess you might have a better idea what is the current status of the systemd in containers (current RHEL 7) -- is it ready?

from s2i-python-container.

@hhorak we would need systemd in both centos and rhel.

from s2i-python-container.

In centos/rhel7 systemd is ready to be used in containers. Or at least it is in phase "try it and report bugs".

from s2i-python-container.

@rhcarvalho @GrahamDumpleton can we create trello card for systemd ?

from s2i-python-container.

@GrahamDumpleton k, so it sounds like we are only broken for python.

@mfojtik I wouldn't say it's just python. I couldn't find any evidence it's broken for ruby, but most (if not all) articles I've stumbled upon suggested using supervisord or similar solution to 'PID 1' problem. I'd assume that only apache-based images are good as is, all others should have that problem addressed. That includes: python, ruby, nodejs.

from s2i-python-container.

AFAIR, the issue with systemd & docker & openshift was that to use systemd
sanely in container it has to be run as root user.

from s2i-python-container.

I thought systemd in container required mounting /sys/fs/cgroup which made it basically require root.

from s2i-python-container.

In theory no. You could use user namespaces, map the root user in the container to more harmless one and from the outside change the owner of the cgroup subtree to that user.

from s2i-python-container.

There are some packages that can be installed into a container to handle the init zombie reaping problem, starting services, and other container initialization issues. Here is a utility (written in Go) that we use to start services, pre-run commands, start running with privilege etc.

https://github.com/markriggins/dockerfy

Key Features

• Overlays of alternative content at runtime
• Templates for configuration and content
• Environment Variable substitutions into templates and overlays
• Secrets injected into configuration files (without leaking them to the environment)
• Waiting for dependencies (any server and port) to become available before the primary command starts
• Tailing log files to the container's stdout and/or stderr
• Running commands before the primary command begins
• Starting Services -- and shutting down the container if they fail
• Propagating signals to child processes
• Reaping Zombie (defunct) processes
• Running services and commands under various user accounts

Hope this helps

Mark

from s2i-python-container.

I have still not found any issues with my 6 line shell script for handling this. Unfortunately I made the mistake of saying when I mentioned it: 'This likely isn't going to be entirely reliable, but, still better than nothing'.

What I was really wanting to say, was that we should have someone more knowledgeable review it (good old imposter syndrome kicked in though). Even so, there seemed to be some resistance to even having a home grown solution, even if it may be just 6 lines of shell script.

Anyway, the reason the 6 lines of shell script works is because we don't need a generic solution that tries to solve wider problems beyond what we need. We just need zombie reaping and signal propagation for key signals that are used in OpenShift and we care about. Even if there might be some corner case where signals don't get through if received during the initial moment of time when things are starting up, OpenShift would kill the container later anyway.

So the shell script solution appears to handle that all fine. We don't need the process ID 1 to restart processes as OpenShift/Kubernetes worries about container crashing. We don't need it to handle logs and all that other stuff.

It would be good if we could find someone amongst all the resources we have, who is knowledgable on shell programming and bash to look at it and comment. May save so much time and pain from trying to shoehorn in some much more complicated solution.

from s2i-python-container.

@GrahamDumpleton can you put that script in PR to this repo? that'd be the best way for us to pull in a shell script expert and debate the merits.

from s2i-python-container.

It is in the initial report above. If you integrate it in then harder for someone to evaluate it.

Also, the original blog post that based this script as being adequate on was:

• http://veithen.github.io/2014/11/16/sigterm-propagation.html

from s2i-python-container.

Recently released Docker version 1.13.0 added the following:

Add boolean flag --init on dockerd and on docker run to use tini a zombie-reaping init process as PID 1

I believe that should solve this issue.

from s2i-python-container.

Where does the tini executable come from? Does it need to be part of the image, or does it come from the Docker host?

If this is not a default option for Docker and is not always used, it would still be best practice for the image itself to deal with it.

FWIW, I still have not encountered any issues with that small shell script snippet I previously described. I really don't understand at this point why custom init process programs are being developed. It isn't like they need to handle re-spawning in the case of the managed process exiting and so don't need to be that complicated.

from s2i-python-container.

Where does the tini executable come from? Does it need to be part of the image, or does it come from the Docker host?

Does not need to be part of the image, is bind mounted from the host.

According to this comment, there might be a new Dockerfile instruction at some point... but it will be time until we can start using it.

And according to this other comment it is the image author's responsibility to add an init process. I agree with @GrahamDumpleton in that this should be handled at the image level, not depend on the features of the container runtime.

from s2i-python-container.

In that case is anyone against going with @GrahamDumpleton's original small shell snippet? It's been a year and if no issues were encountered, it should be safe to use.

from s2i-python-container.

I agree with @GrahamDumpleton in that this should be handled at the image level, not depend on the features of the container runtime.

Hm. I think that such things should be handled by container runtime (be that docker or OpenShift). The environment provides PID 1 abstraction and yes, reaping zombies is natural thing to do then; delegating this to container creator brings asymptotic maintenance burden.

from s2i-python-container.

container runtime (be that docker or OpenShift)

OpenShift is not a container runtime and never will be.

from s2i-python-container.

👍 for the solution proposed by Graham, here.

from s2i-python-container.

OpenShift is not a container runtime and never will be.

Accepted, but with maintainer's heart in me ... :) OpenShift is just an "environment" for container maintainers (in the same fashion as docker is) and dealing with PID 1 problem (and other similar issues) in thousands of OpenShift-related containers is much, much worse solution than having this solved once and forever in environment.

from s2i-python-container.

Here's my proposal for fixing this issue.

Observation:

ATM, it seems that there are two potential paths in {pyver}/s2i/bin/run scripts that this problem can be hit.

1. using APP_FILE
2. having Django project and using the development server to run it

The other paths are:

3. using gunicorn (which is safe base on the original comment)
4. using custom script with APP_SCRIPT (user-provided custom script)

How to solve this:

• For paths 1 and 2 that can trigger this problem, I propose adding env variable ADD_APP_SCRIPT, which would make s2i/bin/run to add the wrapper script and use it.
• Nothing needs to be done for path 3.
• For path 4 (user provided APP_SCRIPT), we should document that this script should be able to handle zombie process reaping.

Does this sound ok? If so, I'll go ahead with the implementation.

from s2i-python-container.

I am wary of not doing anything for 4 if you are going to try and handle 1 and 2 anyway. When using APP_SCRIPT the last thing that would normally be done is to exec the actual web application, thus replacing the script file. There is no guarantee that what was run would handle running as pid 1 and I think it is expecting too much for a user to understand from documentation what they would need to do and integrate something in their own script.

from s2i-python-container.

Yeah, ENABLE_INIT_WRAPPER sounds much better, thanks for the suggestion. As for path 4, do I understand it correctly that your suggestion is to take the same solution as for 1 and 2?

from s2i-python-container.

If you are going to only enable the behaviour with an environment variable, I see no reason why not.

from s2i-python-container.

I agree, if it's an opt-in there's no reason to not enable the init wrapper even for path 4. Overall I like the proposal, @bkabrda.

from s2i-python-container.