sourcegraph / lsp-adapter

lsp-adapter provides a proxy which adapts Sourcegraph LSP requests to vanilla LSP requests
MIT License
29 stars 8 forks source link

Act like tini in a docker container #17

Closed keegancsmith closed 6 years ago

keegancsmith commented 6 years ago

To make docker containers act nicely we would also need them to use tini. But we could just copy that functionality.

From https://github.com/krallin/tini

  • It protects you from software that accidentally creates zombie processes, which can (over time!) starve your entire system for PIDs (and make it unusable).
  • It ensures that the default signal handlers work for the software you run in your Docker image. For example, with Tini, SIGTERM properly terminates your process even if you didn't explicitly install a signal handler for it.
  • It does so completely transparently! Docker images that work without Tini will work with Tini without any changes

cc @ggilmore

keegancsmith commented 6 years ago

I see we had some fun discussion in https://github.com/sourcegraph/javascript-typescript-buildserver/pull/2 about this. It seems the magic to make us reap a child will be as simple as (untested):

for {
  var wstatus syscall.WaitStatus
  var rusage syscall.Rusage
  currentPID, err := syscall.Wait4(-1, &wstatus, syscall.WNOHANG, &rusage)
  if err != nil {
    if err == syscall.ECHILD {
      break // no child to wait
    }
    return err
  }
  if currentPID == 0 {
    break // no child to reap
  }
  // currentPID is a child that has exited
}

I'm unsure if we have the above running somewhere if it would badly interact with running exec.Cmd. So may be better to just use tini :)

https://github.com/krallin/tini/blob/master/src/tini.c#L546 https://github.com/phusion/baseimage-docker/blob/rel-0.9.16/image/bin/my_init#L105

nicksnyder commented 6 years ago

It would be pretty neat if we could solve this in Go without needing tini. For testing we should create a simple program that spams zombie processes.

nicksnyder commented 6 years ago

this actually may be trickier that it looks like. Please read: https://github.com/krallin/tini/issues/8#issuecomment-146135930

keegancsmith commented 6 years ago

First, if Jenkins runs as PID 1, then it's difficult to differentiate between process that were re-parented to Jenkins (which should be reaped), and processes that were spawned by Jenkins (which shouldn't, because there's other code that's already expecting to wait them).

yeah this is exactly what I was alluding to above. And there is the other issues they mention. In theory we could solve all of this via forking at startup. but that wouldn't even be a generic solution thanks to our gratuitous use of init functions, so we can't control what happens in them.

I'm gonna close this out. We should be using tini in all of our containers going forward. Docker has support for --init, but thats an extra flag for the user to remember + I don't think k8s supports it yet.