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

Use a process supervisor on all functions by default #1101

Closed
zootalures opened this Issue Jun 29, 2018 · 2 comments

Comments

Projects
None yet
2 participants
@zootalures
Copy link
Member

commented Jun 29, 2018

Description

Currently we invoke functions directly via the equivalent of docker run, this invokes the entrypoint as PID 1 within the container.

This can cause problems for languages/code that do not set up the appropriate signal handling for a pid1 process which in turn can cause zombie processes to leak, eventually exhausting the resources of the sever.

https://hackernoon.com/my-process-became-pid-1-and-now-signals-behave-strangely-b05c52cc551c?gi=9eddcdf48d16

Describe the results you received:
Not tested directly but certain that this could catch people out when using many FDKs (python, node) or using arbitrary binaries.

Spawning child processes via exec will not be reaped, resulting in zombies

Describe the results you expected:

Child processes are reaped.

What to do ?

I think a simple solution here is to always run functions with the default docker-init wrapper in all environments, this provides consistency of experience and provides the most protection against accidental process leaks.

As of recent versions of docker this binary is installed as part of the docker distribution

within the docker agent we'd need to add InitBinary calls to the container construction call, sadly this also means reving our docker API dependency.

@rdallman

This comment has been minimized.

Copy link
Member

commented Jun 30, 2018

docs link: https://docs.docker.com/engine/reference/run/#specify-an-init-process

we may want to cross test this on OSX and Windows systems as well, it may be something arch-specific we have to do on linux and we could end up in a situation where a user may be running their container slightly differently with fn run on their mac vs. fn call on some linux-hosted fn server. this seems like something we can try and if it causes issues we may be able to remove it -- though maybe that cuts both ways, we could also potentially add an option to disable it if the user knows their process handles signals fine and has issues with tini for whatever reason.

upgrading the fsouza client should be relatively easy.

@rdallman rdallman self-assigned this Sep 4, 2018

@rdallman

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

we've updated the fsouza client lately... testing. not cleaning out my flagged mail has paid dividends

rdallman added a commit that referenced this issue Sep 4, 2018

use tini to run every container
fixes #1101

additional context:

* this was introduced in docker 1.13 (1/2017), we require docker 17.10
(10/2017), this should not have any issues dependency-wise, as `docker-init`
is in the docker install from that point in time. unless explicitly removed,
it should be in the dind container we use as well...
* the PR that introduced this to docker is
moby/moby#26061 for additional context
* it may be wise to put this through some paces, if anybody has any...
interesting... function containers. the tests seem to work fine, however, and
this shouldn't be something users have to think about (?) at all, just
something that we are doing. this isn't the default in docker for
compatibility reasons, which is maybe a yellow flag but I am not sure tbh

rdallman added a commit that referenced this issue Sep 4, 2018

use tini to run every container (#1195)
fixes #1101

additional context:

* this was introduced in docker 1.13 (1/2017), we require docker 17.10
(10/2017), this should not have any issues dependency-wise, as `docker-init`
is in the docker install from that point in time. unless explicitly removed,
it should be in the dind container we use as well...
* the PR that introduced this to docker is
moby/moby#26061 for additional context
* it may be wise to put this through some paces, if anybody has any...
interesting... function containers. the tests seem to work fine, however, and
this shouldn't be something users have to think about (?) at all, just
something that we are doing. this isn't the default in docker for
compatibility reasons, which is maybe a yellow flag but I am not sure tbh
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.