hyperhq / runv

Hypervisor-based Runtime for OCI
Apache License 2.0
828 stars 129 forks source link

add Wait() for nslistener process to avoid zombies #433

Closed pmorjan closed 7 years ago

pmorjan commented 7 years ago

This patch adds a missing cmd.Wait() to wait for process completion of containerd-nslistener. Without the wait every container leaves a defunct process. This is required since #430 removed the global SIGCHILD handler to avoid runtime panics.

Signed-off-by: Peter Morjan peter.morjan@de.ibm.com

laijs commented 7 years ago

I like something like this

                hp.nslistener.cmd.Process.Signal(syscall.SIGTERM)
                timer := time.AfterFunc(time.Second*1, func() { hp.nslistener.cmd.Process.Kill() })
                hp.nslistener.cmd.Wait()
                timer.Stop()

or just kill&wait, no timer&signal

pmorjan commented 7 years ago

i don't get the difference. Please fix it as you like. Thanks.

laijs commented 7 years ago

there is no Signal() in your code, so it will always take 1 seconds to stop the nslistener

laijs commented 7 years ago

@pmorjan runv cli is being improved continuously and had just been refactored #537 . As a result, it seams that the changes in this pr is outdated.

In the new code, this problem is fixed by this code: https://github.com/hyperhq/runv/blob/master/cli/network.go#L320-L325

    defer func() {
        if err != nil {
            cmd.Process.Kill()
        }
        cmd.Wait()
    }()

Thank you for your contribution.