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

Reap Children when run as PID1 #62

Closed
justenwalker opened this issue Jan 4, 2016 · 8 comments
Closed

Reap Children when run as PID1 #62

justenwalker opened this issue Jan 4, 2016 · 8 comments

Comments

@justenwalker
Copy link
Contributor

Containerbuddy should be performing the duties of the init process if it is run as PID1: Reaping child processes.

One possible way to do this is by using this go library:
https://github.com/ramr/go-reaper

Although since Containerbuddy is already handling signals, we can just run

func reapChildren() {
    var wstatus syscall.WaitStatus
    pid, err := syscall.Wait4(-1, &wstatus, 0, nil)
    for syscall.EINTR == err {
        pid, err = syscall.Wait4(-1, &wstatus, 0, nil)
    }
    if syscall.ECHILD == err {
        return
    }
    log.Printf("Reaped: pid=%d, wstatus=%+v", pid, wstatus)
}

// ... in handleSignals

if 1 == os.Getpid() {
    signal.Notify(sig, syscall.SIGCHLD)
}

//...

case syscall.SIGCHLD:
    reapChildren()
@tgross
Copy link
Contributor

tgross commented Jan 4, 2016

I want to be very careful about this subject -- we're very specifically not supporting the whole pile of things that modern supervisors do. Even the idea of just taking on reaping children is dubious; if you're in a spot where this is a problem then the application probably isn't being configured correctly (ex. running Nginx in daemon mode) or you should be using a "real" supervisor with all the features that entails.

@tgross tgross mentioned this issue Jan 4, 2016
Closed
3 tasks
@justenwalker
Copy link
Contributor Author

Containerbuddy shouldn't be an init system a la systemd or upstart, but reaping children is the only required responsibility of PID1.

Since the configuration lets you run arbitrary commands for check health and onStart - they may spawn background processes which would require reaping. I'm not saying thats a good idea or not, but it's possible.

@tgross
Copy link
Contributor

tgross commented Jan 4, 2016

Ok, that's fair.

Adding the go-reaper library doesn't sound like the best approach though, given that:

  • we already have a signal handler loop in play
  • it's another third-party dependency for a very minimal set of code

In addition to the signal handler, don't init systems usually hit Wait4 periodically so that they can catch zombies they did not receive SIGCHLD for?

@justenwalker
Copy link
Contributor Author

In addition to the signal handler, don't init systems usually hit Wait4 periodically so that they can catch zombies they did not receive SIGCHLD for?

From what I've read, I think we can use either or. Doing both might be overkill? I'd have to survey more init systems to see which is used most often.

See:

  • Demystifying the init system (PID 1) - A kernel dev's intro to init systems - only uses SIGCHLD, although might be just to keep the example small
  • Yelp/dumb-init Uses loop method - no SIGCHLD
  • simple_init.c - Uses SIGCHLD, but keeps running wait until there are no more children. Seems like a good idea since several exits could collapse into just 1 signal

@tgross
Copy link
Contributor

tgross commented Jan 7, 2016

The dumb-init approach will work for our main application because it's the one that forks that process. But it's the ones in the "side loops" like the onChange and health handlers that I want to make sure we hit too. So I think we're going to want to do both.

I have a minor tweak I want to do to keep Containerbuddy from using up a lot of threads in an environment like Triton where naive M:N with cause resource contention (i.e we actually don't want to set GOMAXPROCS to the default "use all cores I can see"). So when I'm looking at that, probably tomorrow or Monday, I'll make an attempt at this work as well.

@tgross tgross self-assigned this Jan 7, 2016
@tgross
Copy link
Contributor

tgross commented Jan 8, 2016

I spent a little more time reading the reference material last night and I think the go-reaper implementation will actually do everything we need. We just need to lift their implementation into our main signal processing loop rather than have it as an external dependency. I'll do so and write up some tests for it.

@tgross
Copy link
Contributor

tgross commented Jan 22, 2016

#71 has been merged and this is awaiting release.

@tgross
Copy link
Contributor

tgross commented Jan 22, 2016

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

2 participants