Stop subprocesses from getting unexpectedly killed#44215
Stop subprocesses from getting unexpectedly killed#44215thaJeztah merged 2 commits intomoby:masterfrom
Conversation
Managed containerd processes are executed with SysProcAttr.Pdeathsig set to syscall.SIGKILL so that the managed containerd is automatically killed along with the daemon. At least, that is the intention. In practice, the signal is sent to the process when the creating _OS thread_ dies! If a goroutine exits while locked to an OS thread, the Go runtime will terminate the thread. If that thread happens to be the same thread which the subprocess was started from, the subprocess will be signaled. Prevent the journald driver from sometimes unintentionally killing child processes by ensuring that all runtime.LockOSThread() calls are paired with runtime.UnlockOSThread(). Signed-off-by: Cory Snider <csnider@mirantis.com>
|
Immediate thoughts -- this invariant/explanation needs to be in the doc-comment of |
65cdbad to
43d35bd
Compare
|
Oh wow... great find... I don't like relying on thread locking to handle this. Perhaps the way to go here is to have the systemd unit kill all processes on exit... we wouldn't be able to support live-restore when using managed containerd, but that may be a fare trade-off (or maybe we could move containerd to a separate cgroup in that case...). |
Right, the subprocesses are cleaned up on daemon exit. Thread locking does not break that intended behaviour; it only prevents subprocesses from being spuriously killed before the daemon exits. No threads outlive a daemon exit or SIGKILL or OOM kill. func CausePdeathsigChaos() {
for i := 0; i < 100; i++ {
go func() {
runtime.LockOSThread()
return // Runtime terminates the thread currently executing this goroutine.
}()
}
} |
neersighted
left a comment
There was a problem hiding this comment.
LGTM (including a manual test with @corhere's Russian roulette code -- I wonder if there's a way we could turn that into an integration test?)
|
I don't think we'll need an explicit integration test as #44210 will bring the chaos all on its own. The existing integration tests blew up spectacularly on that PR before I rebased it on top of this change. |
|
Or was that in the other PR? 🤔 |
|
@thaJeztah could you be thinking of this one? #44210 (comment) |
Hm.. no, I think there was one where, hm... what was it? Where go funcs would still be created? Perhaps @cpuguy83 recalls 🤔 |
On Linux, when (os/exec.Cmd).SysProcAttr.Pdeathsig is set, the signal will be sent to the process when the OS thread on which cmd.Start() was executed dies. The runtime terminates an OS thread when a goroutine exits after being wired to the thread with runtime.LockOSThread(). If other goroutines are allowed to be scheduled onto a thread which called cmd.Start(), an unrelated goroutine could cause the thread to be terminated and prematurely signal the command. See golang/go#27505 for more information. Prevent started subprocesses with Pdeathsig from getting signaled prematurely by wiring the starting goroutine to the OS thread until the subprocess has exited. No other goroutines can be scheduled onto a locked thread so it will remain alive until unlocked or the daemon process exits. Signed-off-by: Cory Snider <csnider@mirantis.com>
43d35bd to
1f22b15
Compare
The behaviour of
(syscall.SysProcAttr).Pdeathsigon Linux has some surprising behaviour, as detailed in golang/go#27505. The parent death signal is sent to the child process when the creating thread dies, which in a Go program is going to be an arbitrary thread taken from the goroutine thread pool and recycled for other goroutines to execute on. A goroutine can cause the thread it is running on to exit by wiring itself to the thread withruntime.LockOSThread()and returning. Letting the creating thread become available for other goroutines risks having that thread terminated arbitrarily, and the child process with it.While the problem can in theory be worked around by ensuring that no goroutine returns while wired to a thread, this is not practical. Unpaired
runtime.LockOSThread()calls could be tucked away in third-party modules, and mistakes happen in first-party code. And there are legitimate use-cases for terminating threads early, which heavily overlap with the use-cases forpkg/reexec, which makes it impractical to ban. The only viable solution is to arrange for the threads which are parents of subprocesses started withPdeathsigto not be eligible for other goroutines to execute on them while the subprocess is alive.- What I did
runtime.LockOSThread()in non-vendor code in the daemon process to ensure they had matchingruntime.UnlockOSThread()calls. Unmatched calls in reexec functions were left alone.Pdeathsigcould not be terminated before the subprocess or the daemon process exited, whichever comes first.- How I did it
runtime.LockOSThread()is both the problem and solution- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)