diff --git a/daemon/container.go b/daemon/container.go index 023b4327a6..b66ae5b1a8 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -783,7 +783,7 @@ func (container *Container) exec(ExecConfig *ExecConfig) error { container.Lock() defer container.Unlock() - callback := func(processConfig *execdriver.ProcessConfig, pid int) error { + callback := func(processConfig *execdriver.ProcessConfig, pid int, chOOM <-chan struct{}) error { if processConfig.Tty { // The callback is called after the process Start() // so we are in the parent process. In TTY mode, stdin/out/err is the PtySlave diff --git a/daemon/daemon.go b/daemon/daemon.go index b5451e1e71..97da87d688 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -882,7 +882,7 @@ func (daemon *Daemon) run(c *Container, pipes *execdriver.Pipes, startCallback e hooks := execdriver.Hooks{ Start: startCallback, } - hooks.PreStart = append(hooks.PreStart, func(processConfig *execdriver.ProcessConfig, pid int) error { + hooks.PreStart = append(hooks.PreStart, func(processConfig *execdriver.ProcessConfig, pid int, chOOM <-chan struct{}) error { return c.setNetworkNamespaceKey(pid) }) return daemon.execDriver.Run(c.command, pipes, hooks) diff --git a/daemon/execdriver/driver.go b/daemon/execdriver/driver.go index 142003fdd0..7e8998d272 100644 --- a/daemon/execdriver/driver.go +++ b/daemon/execdriver/driver.go @@ -27,8 +27,9 @@ var ( // DriverCallback defines a callback function which is used in "Run" and "Exec". // This allows work to be done in the parent process when the child is passing // through PreStart, Start and PostStop events. -// Callbacks are provided a processConfig pointer and the pid of the child -type DriverCallback func(processConfig *ProcessConfig, pid int) error +// Callbacks are provided a processConfig pointer and the pid of the child. +// The channel will be used to notify the OOM events. +type DriverCallback func(processConfig *ProcessConfig, pid int, chOOM <-chan struct{}) error // Hooks is a struct containing function pointers to callbacks // used by any execdriver implementation exploiting hooks capabilities diff --git a/daemon/execdriver/lxc/driver.go b/daemon/execdriver/lxc/driver.go index 5a6274d8f2..30b36e775d 100644 --- a/daemon/execdriver/lxc/driver.go +++ b/daemon/execdriver/lxc/driver.go @@ -324,14 +324,15 @@ func (d *Driver) Run(c *execdriver.Command, pipes *execdriver.Pipes, hooks execd c.ContainerPid = pid - if hooks.Start != nil { - logrus.Debugf("Invoking startCallback") - hooks.Start(&c.ProcessConfig, pid) - } - oomKill := false oomKillNotification, err := notifyOnOOM(cgroupPaths) + if hooks.Start != nil { + logrus.Debugf("Invoking startCallback") + hooks.Start(&c.ProcessConfig, pid, oomKillNotification) + + } + <-waitLock exitCode := getExitCode(c) diff --git a/daemon/execdriver/native/create.go b/daemon/execdriver/native/create.go index 1fbc3cfde0..c480534585 100644 --- a/daemon/execdriver/native/create.go +++ b/daemon/execdriver/native/create.go @@ -146,7 +146,11 @@ func (d *Driver) createNetwork(container *configs.Config, c *execdriver.Command, configs.NewFunctionHook(func(s configs.HookState) error { if len(hooks.PreStart) > 0 { for _, fnHook := range hooks.PreStart { - if err := fnHook(&c.ProcessConfig, s.Pid); err != nil { + // A closed channel for OOM is returned here as it will be + // non-blocking and return the correct result when read. + chOOM := make(chan struct{}) + close(chOOM) + if err := fnHook(&c.ProcessConfig, s.Pid, chOOM); err != nil { return err } } diff --git a/daemon/execdriver/native/driver.go b/daemon/execdriver/native/driver.go index 49d06ad9a1..5a54d3130f 100644 --- a/daemon/execdriver/native/driver.go +++ b/daemon/execdriver/native/driver.go @@ -165,17 +165,18 @@ func (d *Driver) Run(c *execdriver.Command, pipes *execdriver.Pipes, hooks execd return execdriver.ExitStatus{ExitCode: -1}, err } + oom := notifyOnOOM(cont) if hooks.Start != nil { + pid, err := p.Pid() if err != nil { p.Signal(os.Kill) p.Wait() return execdriver.ExitStatus{ExitCode: -1}, err } - hooks.Start(&c.ProcessConfig, pid) + hooks.Start(&c.ProcessConfig, pid, oom) } - oom := notifyOnOOM(cont) waitF := p.Wait if nss := cont.Config().Namespaces; !nss.Contains(configs.NEWPID) { // we need such hack for tracking processes with inherited fds, diff --git a/daemon/execdriver/native/exec.go b/daemon/execdriver/native/exec.go index 6cd1a92872..b15e0cbd71 100644 --- a/daemon/execdriver/native/exec.go +++ b/daemon/execdriver/native/exec.go @@ -52,7 +52,12 @@ func (d *Driver) Exec(c *execdriver.Command, processConfig *execdriver.ProcessCo p.Wait() return -1, err } - hooks.Start(&c.ProcessConfig, pid) + + // A closed channel for OOM is returned here as it will be + // non-blocking and return the correct result when read. + chOOM := make(chan struct{}) + close(chOOM) + hooks.Start(&c.ProcessConfig, pid, chOOM) } ps, err := p.Wait() diff --git a/daemon/execdriver/windows/exec.go b/daemon/execdriver/windows/exec.go index e0896c0e86..5ea12a8cbb 100644 --- a/daemon/execdriver/windows/exec.go +++ b/daemon/execdriver/windows/exec.go @@ -70,7 +70,11 @@ func (d *Driver) Exec(c *execdriver.Command, processConfig *execdriver.ProcessCo // Invoke the start callback if hooks.Start != nil { - hooks.Start(&c.ProcessConfig, int(pid)) + // A closed channel for OOM is returned here as it will be + // non-blocking and return the correct result when read. + chOOM := make(chan struct{}) + close(chOOM) + hooks.Start(&c.ProcessConfig, int(pid), chOOM) } if exitCode, err = hcsshim.WaitForProcessInComputeSystem(c.ID, pid); err != nil { diff --git a/daemon/execdriver/windows/run.go b/daemon/execdriver/windows/run.go index cc3617c491..09d31aa29e 100644 --- a/daemon/execdriver/windows/run.go +++ b/daemon/execdriver/windows/run.go @@ -291,7 +291,11 @@ func (d *Driver) Run(c *execdriver.Command, pipes *execdriver.Pipes, hooks execd d.Unlock() if hooks.Start != nil { - hooks.Start(&c.ProcessConfig, int(pid)) + // A closed channel for OOM is returned here as it will be + // non-blocking and return the correct result when read. + chOOM := make(chan struct{}) + close(chOOM) + hooks.Start(&c.ProcessConfig, int(pid), chOOM) } var exitCode int32 diff --git a/daemon/monitor.go b/daemon/monitor.go index 4d34a83bef..4af0d2a2fd 100644 --- a/daemon/monitor.go +++ b/daemon/monitor.go @@ -162,9 +162,6 @@ func (m *containerMonitor) Start() error { if m.shouldRestart(exitStatus.ExitCode) { m.container.setRestarting(&exitStatus) - if exitStatus.OOMKilled { - m.container.logEvent("oom") - } m.container.logEvent("die") m.resetContainer(true) @@ -179,9 +176,7 @@ func (m *containerMonitor) Start() error { } continue } - if exitStatus.OOMKilled { - m.container.logEvent("oom") - } + m.container.logEvent("die") m.resetContainer(true) return err @@ -250,7 +245,14 @@ func (m *containerMonitor) shouldRestart(exitCode int) bool { // callback ensures that the container's state is properly updated after we // received ack from the execution drivers -func (m *containerMonitor) callback(processConfig *execdriver.ProcessConfig, pid int) error { +func (m *containerMonitor) callback(processConfig *execdriver.ProcessConfig, pid int, chOOM <-chan struct{}) error { + go func() { + _, ok := <-chOOM + if ok { + m.container.logEvent("oom") + } + }() + if processConfig.Tty { // The callback is called after the process Start() // so we are in the parent process. In TTY mode, stdin/out/err is the PtySlave diff --git a/integration-cli/docker_cli_events_unix_test.go b/integration-cli/docker_cli_events_unix_test.go index 1a08f2b3c0..ca7c092214 100644 --- a/integration-cli/docker_cli_events_unix_test.go +++ b/integration-cli/docker_cli_events_unix_test.go @@ -8,6 +8,8 @@ import ( "io/ioutil" "os" "os/exec" + "strings" + "time" "unicode" "github.com/go-check/check" @@ -51,3 +53,99 @@ func (s *DockerSuite) TestEventsRedirectStdout(c *check.C) { } } + +func (s *DockerSuite) TestEventsOOMDisableFalse(c *check.C) { + testRequires(c, DaemonIsLinux) + testRequires(c, oomControl) + + errChan := make(chan error) + go func() { + defer close(errChan) + out, exitCode, _ := dockerCmdWithError("run", "--name", "oomFalse", "-m", "10MB", "busybox", "sh", "-c", "x=a; while true; do x=$x$x$x$x; done") + if expected := 137; exitCode != expected { + errChan <- fmt.Errorf("wrong exit code for OOM container: expected %d, got %d (output: %q)", expected, exitCode, out) + } + }() + select { + case err := <-errChan: + c.Assert(err, check.IsNil) + case <-time.After(30 * time.Second): + c.Fatal("Timeout waiting for container to die on OOM") + } + + out, _ := dockerCmd(c, "events", "--since=0", "-f", "container=oomFalse", fmt.Sprintf("--until=%d", daemonTime(c).Unix())) + events := strings.Split(strings.TrimSuffix(out, "\n"), "\n") + if len(events) < 5 { + c.Fatalf("Missing expected event") + } + + createEvent := strings.Fields(events[len(events)-5]) + attachEvent := strings.Fields(events[len(events)-4]) + startEvent := strings.Fields(events[len(events)-3]) + oomEvent := strings.Fields(events[len(events)-2]) + dieEvent := strings.Fields(events[len(events)-1]) + if createEvent[len(createEvent)-1] != "create" { + c.Fatalf("event should be create, not %#v", createEvent) + } + if attachEvent[len(attachEvent)-1] != "attach" { + c.Fatalf("event should be attach, not %#v", attachEvent) + } + if startEvent[len(startEvent)-1] != "start" { + c.Fatalf("event should be start, not %#v", startEvent) + } + if oomEvent[len(oomEvent)-1] != "oom" { + c.Fatalf("event should be oom, not %#v", oomEvent) + } + if dieEvent[len(dieEvent)-1] != "die" { + c.Fatalf("event should be die, not %#v", dieEvent) + } +} + +func (s *DockerSuite) TestEventsOOMDisableTrue(c *check.C) { + testRequires(c, DaemonIsLinux) + testRequires(c, oomControl) + + errChan := make(chan error) + go func() { + defer close(errChan) + out, exitCode, _ := dockerCmdWithError("run", "--oom-kill-disable=true", "--name", "oomTrue", "-m", "10MB", "busybox", "sh", "-c", "x=a; while true; do x=$x$x$x$x; done") + if expected := 137; exitCode != expected { + errChan <- fmt.Errorf("wrong exit code for OOM container: expected %d, got %d (output: %q)", expected, exitCode, out) + } + }() + select { + case err := <-errChan: + c.Assert(err, check.IsNil) + case <-time.After(20 * time.Second): + defer dockerCmd(c, "kill", "oomTrue") + + out, _ := dockerCmd(c, "events", "--since=0", "-f", "container=oomTrue", fmt.Sprintf("--until=%d", daemonTime(c).Unix())) + events := strings.Split(strings.TrimSuffix(out, "\n"), "\n") + if len(events) < 4 { + c.Fatalf("Missing expected event") + } + + createEvent := strings.Fields(events[len(events)-4]) + attachEvent := strings.Fields(events[len(events)-3]) + startEvent := strings.Fields(events[len(events)-2]) + oomEvent := strings.Fields(events[len(events)-1]) + + if createEvent[len(createEvent)-1] != "create" { + c.Fatalf("event should be create, not %#v", createEvent) + } + if attachEvent[len(attachEvent)-1] != "attach" { + c.Fatalf("event should be attach, not %#v", attachEvent) + } + if startEvent[len(startEvent)-1] != "start" { + c.Fatalf("event should be start, not %#v", startEvent) + } + if oomEvent[len(oomEvent)-1] != "oom" { + c.Fatalf("event should be oom, not %#v", oomEvent) + } + + out, _ = dockerCmd(c, "inspect", "-f", "{{.State.Status}}", "oomTrue") + if strings.TrimSpace(out) != "running" { + c.Fatalf("container should be still running, not %v", out) + } + } +}