Merge pull request #1460 from mlaventure/pid-host-kill-init

Ensure all init children are dead when it exits

container_linux_test.go

@@ -896,3 +896,79 @@ func TestContainerRuntimeOptions(t *testing.T) {
 		t.Errorf("task creation should have failed because of lack of executable. Instead failed with: %v", err.Error())
 	}
 }
+
+func TestContainerKillInitPidHost(t *testing.T) {
+	client, err := newClient(t, address)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer client.Close()
+
+	var (
+		image       Image
+		ctx, cancel = testContext()
+		id          = t.Name()
+	)
+	defer cancel()
+
+	image, err = client.GetImage(ctx, testImage)
+	if err != nil {
+		t.Error(err)
+		return
+	}
+
+	container, err := client.NewContainer(ctx, id,
+		withNewSnapshot(id, image),
+		WithNewSpec(withImageConfig(image),
+			withProcessArgs("sh", "-c", "sleep 42; echo hi"),
+			WithHostNamespace(specs.PIDNamespace),
+		),
+	)
+	if err != nil {
+		t.Error(err)
+		return
+	}
+	defer container.Delete(ctx, WithSnapshotCleanup)
+
+	stdout := bytes.NewBuffer(nil)
+	task, err := container.NewTask(ctx, NewIO(bytes.NewBuffer(nil), stdout, bytes.NewBuffer(nil)))
+	if err != nil {
+		t.Error(err)
+		return
+	}
+	defer task.Delete(ctx)
+
+	statusC, err := task.Wait(ctx)
+	if err != nil {
+		t.Error(err)
+		return
+	}
+
+	if err := task.Start(ctx); err != nil {
+		t.Error(err)
+		return
+	}
+
+	if err := task.Kill(ctx, syscall.SIGKILL); err != nil {
+		t.Error(err)
+	}
+
+	// Give the shim time to reap the init process and kill the orphans
+	select {
+	case <-statusC:
+	case <-time.After(100 * time.Millisecond):
+	}
+
+	b, err := exec.Command("ps", "ax").CombinedOutput()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if strings.Contains(string(b), "sleep 42") {
+		t.Fatalf("killing init didn't kill all its children:\n%v", string(b))
+	}
+
+	if _, err := task.Delete(ctx, WithProcessKill); err != nil {
+		t.Error(err)
+	}
+}

linux/runtime.go

@@ -367,7 +367,10 @@ func (r *Runtime) loadTasks(ctx context.Context, ns string) ([]*Task, error) {
 func (r *Runtime) cleanupAfterDeadShim(ctx context.Context, bundle *bundle, ns, id string, pid int, ec chan runc.Exit) error {
 	ctx = namespaces.WithNamespace(ctx, ns)
 	if err := r.terminate(ctx, bundle, ns, id); err != nil {
-		return errors.New("failed to terminate task, leaving bundle for debugging")
+		if r.config.ShimDebug {
+			return errors.Wrap(err, "failed to terminate task, leaving bundle for debugging")
+		}
+		log.G(ctx).WithError(err).Warn("failed to terminate task")
 	}
 
 	if ec != nil {

linux/shim/service.go

@@ -393,6 +393,14 @@ func (s *Service) checkProcesses(e runc.Exit) {
 	defer s.mu.Unlock()
 	for _, p := range s.processes {
 		if p.Pid() == e.Pid {
+			if ip, ok := p.(*initProcess); ok {
+				// Ensure all children are killed
+				if err := ip.killAll(s.context); err != nil {
+					log.G(s.context).WithError(err).WithField("id", ip.ID()).
+						Error("failed to kill init's children")
+				}
+			}
+
 			p.SetExited(e.Status)
 			s.events <- &eventsapi.TaskExit{
 				ContainerID: s.id,

reaper/reaper.go

@@ -3,7 +3,6 @@
 package reaper
 
 import (
-	"bytes"
 	"os/exec"
 	"sync"
 	"time"
@@ -47,23 +46,6 @@ type Monitor struct {
 	subscribers map[chan runc.Exit]struct{}
 }
 
-func (m *Monitor) Output(c *exec.Cmd) ([]byte, error) {
-	var b bytes.Buffer
-	c.Stdout = &b
-	if err := m.Run(c); err != nil {
-		return nil, err
-	}
-	return b.Bytes(), nil
-}
-
-func (m *Monitor) CombinedOutput(c *exec.Cmd) ([]byte, error) {
-	var b bytes.Buffer
-	c.Stdout = &b
-	c.Stderr = &b
-	err := m.Run(c)
-	return b.Bytes(), err
-}
-
 // Start starts the command a registers the process with the reaper
 func (m *Monitor) Start(c *exec.Cmd) (chan runc.Exit, error) {
 	ec := m.Subscribe()
@@ -74,16 +56,9 @@ func (m *Monitor) Start(c *exec.Cmd) (chan runc.Exit, error) {
 	return ec, nil
 }
 
-// Run runs and waits for the command to finish
+// Wait blocks until a process is signal as dead.
-func (m *Monitor) Run(c *exec.Cmd) error {
+// User should rely on the value of the exit status to determine if the
-	ec, err := m.Start(c)
+// command was successful or not.
-	if err != nil {
-		return err
-	}
-	_, err = m.Wait(c, ec)
-	return err
-}
-
 func (m *Monitor) Wait(c *exec.Cmd, ec chan runc.Exit) (int, error) {
 	for e := range ec {
 		if e.Pid == c.Process.Pid {

vendor.conf

@@ -1,5 +1,5 @@
 github.com/coreos/go-systemd 48702e0da86bd25e76cfef347e2adeb434a0d0a6
-github.com/containerd/go-runc e103f453ff3db23ec69d31371cadc1ea0ce87ec0
+github.com/containerd/go-runc ba22f6a82e52be3be4eb4a00000fe816f4b41c2e
 github.com/containerd/console 76d18fd1d66972718ab2284449591db0b3cdb4de
 github.com/containerd/cgroups e6d1aa8c71c6103624b2c6e6f4be0863b67027f1
 github.com/docker/go-metrics 8fd5772bf1584597834c6f7961a530f06cbfbb87

vendor/github.com/containerd/go-runc/monitor.go

@@ -22,9 +22,6 @@ type Exit struct {
 // These methods should match the methods exposed by exec.Cmd to provide
 // a consistent experience for the caller
 type ProcessMonitor interface {
-	Output(*exec.Cmd) ([]byte, error)
-	CombinedOutput(*exec.Cmd) ([]byte, error)
-	Run(*exec.Cmd) error
 	Start(*exec.Cmd) (chan Exit, error)
 	Wait(*exec.Cmd, chan Exit) (int, error)
 }
@@ -32,18 +29,6 @@ type ProcessMonitor interface {
 type defaultMonitor struct {
 }
 
-func (m *defaultMonitor) Output(c *exec.Cmd) ([]byte, error) {
-	return c.Output()
-}
-
-func (m *defaultMonitor) CombinedOutput(c *exec.Cmd) ([]byte, error) {
-	return c.CombinedOutput()
-}
-
-func (m *defaultMonitor) Run(c *exec.Cmd) error {
-	return c.Run()
-}
-
 func (m *defaultMonitor) Start(c *exec.Cmd) (chan Exit, error) {
 	if err := c.Start(); err != nil {
 		return nil, err

vendor/github.com/containerd/go-runc/runc.go

@@ -46,7 +46,7 @@ type Runc struct {
 
 // List returns all containers created inside the provided runc root directory
 func (r *Runc) List(context context.Context) ([]*Container, error) {
-	data, err := Monitor.Output(r.command(context, "list", "--format=json"))
+	data, err := cmdOutput(r.command(context, "list", "--format=json"), false)
 	if err != nil {
 		return nil, err
 	}
@@ -59,7 +59,7 @@ func (r *Runc) List(context context.Context) ([]*Container, error) {
 
 // State returns the state for the container provided by id
 func (r *Runc) State(context context.Context, id string) (*Container, error) {
-	data, err := Monitor.CombinedOutput(r.command(context, "state", id))
+	data, err := cmdOutput(r.command(context, "state", id), true)
 	if err != nil {
 		return nil, fmt.Errorf("%s: %s", err, data)
 	}
@@ -128,7 +128,7 @@ func (r *Runc) Create(context context.Context, id, bundle string, opts *CreateOp
 	cmd.ExtraFiles = opts.ExtraFiles
 
 	if cmd.Stdout == nil && cmd.Stderr == nil {
-		data, err := Monitor.CombinedOutput(cmd)
+		data, err := cmdOutput(cmd, true)
 		if err != nil {
 			return fmt.Errorf("%s: %s", err, data)
 		}
@@ -145,7 +145,10 @@ func (r *Runc) Create(context context.Context, id, bundle string, opts *CreateOp
 			}
 		}
 	}
-	_, err = Monitor.Wait(cmd, ec)
+	status, err := Monitor.Wait(cmd, ec)
+	if err == nil && status != 0 {
+		err = fmt.Errorf("%s did not terminate sucessfully", cmd.Args[0])
+	}
 	return err
 }
 
@@ -204,7 +207,7 @@ func (r *Runc) Exec(context context.Context, id string, spec specs.Process, opts
 		opts.Set(cmd)
 	}
 	if cmd.Stdout == nil && cmd.Stderr == nil {
-		data, err := Monitor.CombinedOutput(cmd)
+		data, err := cmdOutput(cmd, true)
 		if err != nil {
 			return fmt.Errorf("%s: %s", err, data)
 		}
@@ -363,7 +366,7 @@ func (r *Runc) Resume(context context.Context, id string) error {
 
 // Ps lists all the processes inside the container returning their pids
 func (r *Runc) Ps(context context.Context, id string) ([]int, error) {
-	data, err := Monitor.CombinedOutput(r.command(context, "ps", "--format", "json", id))
+	data, err := cmdOutput(r.command(context, "ps", "--format", "json", id), true)
 	if err != nil {
 		return nil, fmt.Errorf("%s: %s", err, data)
 	}
@@ -546,7 +549,7 @@ type Version struct {
 
 // Version returns the runc and runtime-spec versions
 func (r *Runc) Version(context context.Context) (Version, error) {
-	data, err := Monitor.Output(r.command(context, "--version"))
+	data, err := cmdOutput(r.command(context, "--version"), false)
 	if err != nil {
 		return Version{}, err
 	}
@@ -614,11 +617,39 @@ func (r *Runc) args() (out []string) {
 // <stderr>
 func (r *Runc) runOrError(cmd *exec.Cmd) error {
 	if cmd.Stdout != nil || cmd.Stderr != nil {
-		return Monitor.Run(cmd)
+		ec, err := Monitor.Start(cmd)
+		if err != nil {
+			return err
 		}
-	data, err := Monitor.CombinedOutput(cmd)
+		status, err := Monitor.Wait(cmd, ec)
+		if err == nil && status != 0 {
+			err = fmt.Errorf("%s did not terminate sucessfully", cmd.Args[0])
+		}
+		return err
+	}
+	data, err := cmdOutput(cmd, true)
 	if err != nil {
 		return fmt.Errorf("%s: %s", err, data)
 	}
 	return nil
 }
+
+func cmdOutput(cmd *exec.Cmd, combined bool) ([]byte, error) {
+	var b bytes.Buffer
+
+	cmd.Stdout = &b
+	if combined {
+		cmd.Stderr = &b
+	}
+	ec, err := Monitor.Start(cmd)
+	if err != nil {
+		return nil, err
+	}
+
+	status, err := Monitor.Wait(cmd, ec)
+	if err == nil && status != 0 {
+		err = fmt.Errorf("%s did not terminate sucessfully", cmd.Args[0])
+	}
+
+	return b.Bytes(), err
+}