From 9d251cbd1b0876d346fb377a99e268f5d9d77c5e Mon Sep 17 00:00:00 2001 From: Kenfe-Mickael Laventure Date: Fri, 1 Sep 2017 09:38:03 -0700 Subject: [PATCH 1/4] Delete bundle dir on restore if we're not debugging the shim Signed-off-by: Kenfe-Mickael Laventure --- linux/runtime.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/linux/runtime.go b/linux/runtime.go index 6f6567aaa..7b7d2b6a7 100644 --- a/linux/runtime.go +++ b/linux/runtime.go @@ -364,7 +364,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 { From 92772bd471ff3ee24486fa0cc2f497d17d35590a Mon Sep 17 00:00:00 2001 From: Kenfe-Mickael Laventure Date: Fri, 1 Sep 2017 09:41:37 -0700 Subject: [PATCH 2/4] linux: Ensure all init children are dead when it exits This ensure that when using the host pid, we don't let process alive, preventing Wait() to return until they all die. Signed-off-by: Kenfe-Mickael Laventure --- container_linux_test.go | 76 +++++++++++++++++++++++++++++++++++++++++ linux/shim/service.go | 8 +++++ 2 files changed, 84 insertions(+) diff --git a/container_linux_test.go b/container_linux_test.go index f70fb5bb3..093225884 100644 --- a/container_linux_test.go +++ b/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) + } +} diff --git a/linux/shim/service.go b/linux/shim/service.go index 874babb9e..fc4d0993e 100644 --- a/linux/shim/service.go +++ b/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, From a6fb9bc11171bd2a0494a2aad1e764eef5acf82d Mon Sep 17 00:00:00 2001 From: Kenfe-Mickael Laventure Date: Fri, 1 Sep 2017 09:43:04 -0700 Subject: [PATCH 3/4] reaper: Return an error if exit status is not 0 Signed-off-by: Kenfe-Mickael Laventure --- reaper/reaper.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/reaper/reaper.go b/reaper/reaper.go index 97df901dc..042ba83da 100644 --- a/reaper/reaper.go +++ b/reaper/reaper.go @@ -90,6 +90,9 @@ func (m *Monitor) Wait(c *exec.Cmd, ec chan runc.Exit) (int, error) { // make sure we flush all IO c.Wait() m.Unsubscribe(ec) + if e.Status != 0 { + return e.Status, errors.New("unsucessful command") + } return e.Status, nil } } From 939ad32117abe037cda88247d109e4a9f32b7e56 Mon Sep 17 00:00:00 2001 From: Kenfe-Mickael Laventure Date: Fri, 1 Sep 2017 11:07:31 -0700 Subject: [PATCH 4/4] Update go-runc to ba22f6a82e52be3be4eb4a00000fe816f4b41c2e Signed-off-by: Kenfe-Mickael Laventure --- reaper/reaper.go | 34 ++----------- vendor.conf | 2 +- .../github.com/containerd/go-runc/monitor.go | 15 ------ vendor/github.com/containerd/go-runc/runc.go | 49 +++++++++++++++---- 4 files changed, 44 insertions(+), 56 deletions(-) diff --git a/reaper/reaper.go b/reaper/reaper.go index 042ba83da..76efc6199 100644 --- a/reaper/reaper.go +++ b/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,25 +56,15 @@ func (m *Monitor) Start(c *exec.Cmd) (chan runc.Exit, error) { return ec, nil } -// Run runs and waits for the command to finish -func (m *Monitor) Run(c *exec.Cmd) error { - ec, err := m.Start(c) - if err != nil { - return err - } - _, err = m.Wait(c, ec) - return err -} - +// Wait blocks until a process is signal as dead. +// User should rely on the value of the exit status to determine if the +// command was successful or not. func (m *Monitor) Wait(c *exec.Cmd, ec chan runc.Exit) (int, error) { for e := range ec { if e.Pid == c.Process.Pid { // make sure we flush all IO c.Wait() m.Unsubscribe(ec) - if e.Status != 0 { - return e.Status, errors.New("unsucessful command") - } return e.Status, nil } } diff --git a/vendor.conf b/vendor.conf index 7c58b0334..e2c64d0c7 100644 --- a/vendor.conf +++ b/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 diff --git a/vendor/github.com/containerd/go-runc/monitor.go b/vendor/github.com/containerd/go-runc/monitor.go index 4ad539927..2d62c5a41 100644 --- a/vendor/github.com/containerd/go-runc/monitor.go +++ b/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 diff --git a/vendor/github.com/containerd/go-runc/runc.go b/vendor/github.com/containerd/go-runc/runc.go index 92218db42..d8ae4eec0 100644 --- a/vendor/github.com/containerd/go-runc/runc.go +++ b/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) { // 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 + } + 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 := Monitor.CombinedOutput(cmd) + 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 +}