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

Idea: How to enable Buildpack built images to have a valid PID1 in unix terms #210

Open
BenjaminSchubert opened this issue Mar 9, 2022 · 4 comments

Comments

@BenjaminSchubert
Copy link

Problem

Background

PID1 on Unix has special responsibilities. Mainly, it needs to take parentship of orphan children, and reap zombie processes.

One important thing to note is that the number of PIDs on a machine is limited, and using all PIDs will prevent new processes from getting created. Reaping zombies helps ensuring that no PIDs are used for nothing.

When working with containers and/or pods, since they usually have their own namespace, the pod/container's PID1 is responsible of this handling.

However, how this is done depends on the platform to which the container is deployed and how it is deployed.

There are mainly two ways to do this:

  1. Using the pause container as part of the pod, and enabled PID sharing for the whole pod. There, the pause container will be PID1 and handle those responsibilities
  2. Having the entrypoint of the container that will be PID1 handling those responsibilities.

What happens if this is not done?

Failing to do so can lead a misbehaving application to own all the PIDs on a node and make this node unsuable/crash.

There are ways of mitigating this, e.g. ensuring each application is limited on the number of PIDs it can create. However this is not ideal.

Our needs

We currently need to deploy to platforms where 1. is not possible for us, and we therefore need to ensure 2. works.

Currently, ensuring this with Buildpacks is tricky:

  • Buildpack's processes cannot be overriden by another buildpack, so we cannot "just add" a new buildpack that would install a valid init as PID1.
  • We do not want users of our buildpacks to have to know about this and do special handling, and would like only the 'builders' owners to have to care about this.

Question

Would CNB want to, and if so, how could we enable owners of buildpack builders to ensure that buildpack built images have a valid PID1 so they can safely be deployed to any platform regardless of how it is configured?

Potential solutions

  1. Having the launch in the lifecycle install itself as PID1 and reaping processes automatically
@sambhav
Copy link
Member

sambhav commented Mar 9, 2022

cc: @buildpacks/implementation-maintainers @buildpacks/buildpack-authors-tooling-contributors

@dmikusa
Copy link
Contributor

dmikusa commented Mar 9, 2022

On Paketo, this has impacted us in different ways. For the language families, I help to support, it's been less about reaping processes and more about proper handling of signals so that your application can gracefully shut down.

For Paketo Java, we've chosen to not do anything. The JVM runs as PID1. It handles signals, so graceful shutdown works and we've not had any complaints about reaping subprocesses. Either users aren't complaining or there's not a lot of forking of subprocesses in Java apps.

For the Paketo Rust buildpack, it was a little more of a problem. Rust apps don't handle signals out-of-the-box so graceful shutdown didn't work. What we did for Rust was to add the tini binary by default. Process types are generated with tini starting the Rust binary, so that signal handling and, while it wasn't a concern for users, process reaping also works.

If we got complaints from Java users, I suspect that we'd probably add tini in there too, but probably opt-in unless there was a stronger case for making it the default.

I know that we also have a tini buildpack, which I think some of our other language families are using. @ForestEckhardt @sophiewigmore @fg-j Is there anything you could share about how we handle PID1 in other Paketo language families?


In regards to your question, I think my $0.02 would be that it's a legit concern. Probably one that's being overlooked by at least some portion of buildpack authors. I would like to see some sort of shared solution that buildpack authors can use, but not one that's required (or not able to be opted out of). As a buildpack author, I'd like to decide when I start something as PID1 or when I use a wrapper to handle the PID1 responsibilities. That would also leave the option to delegate that decision to a user, if for example they prefer using something like pause or docker run --init to handle PID1.

I also don't think that we should invent our own PID1 binary. If we're going to do this, we should use something that exists already and is vetted, like tini.


Possibly off-topic, but a challenge that we have with a buildpack like the Paketo tini buildpack is that it cannot be self-contained. It can be used to install the tini binary, but what you really want is for some other buildpack to emit the process types and for the tini buildpack to run and wrap those process types with tini + <whatever the other buildpack set>. I'm under the impression that is not possible (correct me if I'm wrong), which I think makes having an elegant solution at the buildpack level difficult.

@fg-j
Copy link

fg-j commented Mar 16, 2022

AFAIK, we don't actually use tini anywhere for graceful shutdown anymore. We ran into some issues with it – my recollection is thin; maybe other Paketo maintainers remember better.

In some buildpacks/for some cases, bash is PID1. For instance, in the Nodejs buildpack when a user has their source code in a nested subdirectory. bash obviously isn't ideal from a signal handling perspective.

Otherwise, in Go and for some .NET apps, the compiled binary of the app is PID1.

For some Node.js apps, node is PID1. Similarly, for some .NET apps, dotnet is PID1. As you said, @dmikusa-pivotal, I have spent more time thinking about the signal handling aspect of the PID1 than the zombie reaping. Feedback about the PID1 set by any given Paketo buildpack would be welcomed.

I don't have strong opinions about whether CNB should handle this somehow upstream.

@ekcasey
Copy link
Member

ekcasey commented Apr 7, 2022

Some thoughts:

I agree with @dmikusa-pivotal that

  1. This should be optional
  2. If possible we should enable use of an existing solution rather than reimplement an init process in the launcher

Instead of finding a way to allow a buildpack to modify or reference other buildpacks I would like to design a way that buildpacks could modify the container entrypoint and inject it upstream of the launcher. Therefore the entrypoint could be something like tini - v -- /cnb/process/web.

Maybe we could add a mechanism to launch.toml?

entrypoint = ["/path/to/tini", "-v", "--", "$(ENTRYPOINT)"]

The problem here becomes if you want to set environment variables to affect the behavior of the entrypoint. The launcher typically applies buildpack provided modifications to the environment. Maybe we could:

  1. provide a way for buildpack to contribute env modifications directly to the container env?
  2. apply all static and non-process-specific env modifications to the container config, rather than at launch?

@natalieparellano natalieparellano changed the title How to enable Buildpack built images to have a valid PID1 in unix terms Idea: How to enable Buildpack built images to have a valid PID1 in unix terms Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants