Merge pull request #16235 from HuKeping/oom-event

Events for OOM needs to be shift to an earlier time

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

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)

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

daemon/execdriver/lxc/driver.go

@@ -324,13 +324,14 @@ func (d *Driver) Run(c *execdriver.Command, pipes *execdriver.Pipes, hooks execd
 
 	c.ContainerPid = pid
 
+	oomKill := false
+	oomKillNotification, err := notifyOnOOM(cgroupPaths)
+
 	if hooks.Start != nil {
 		logrus.Debugf("Invoking startCallback")
-		hooks.Start(&c.ProcessConfig, pid)
-	}
+		hooks.Start(&c.ProcessConfig, pid, oomKillNotification)
 
-	oomKill := false
-	oomKillNotification, err := notifyOnOOM(cgroupPaths)
+	}
 
 	<-waitLock
 	exitCode := getExitCode(c)

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
 						}
 					}

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,

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()

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 {

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

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

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)
+		}
+	}
+}