Merge pull request #16995 from jfrazelle/lxc-i-loathe-you

Fix lxc

daemon/execdriver/lxc/driver.go

@@ -324,24 +324,20 @@ 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, oomKillNotification)
-
+		chOOM := make(chan struct{})
+		close(chOOM)
+		hooks.Start(&c.ProcessConfig, pid, chOOM)
 	}
 
+	oomKillNotification := notifyChannelOOM(cgroupPaths)
+
 	<-waitLock
 	exitCode := getExitCode(c)
 
-	if err == nil {
-		_, oomKill = <-oomKillNotification
-		logrus.Debugf("oomKill error: %v, waitErr: %v", oomKill, waitErr)
-	} else {
-		logrus.Warnf("Your kernel does not support OOM notifications: %s", err)
-	}
+	_, oomKill := <-oomKillNotification
+	logrus.Debugf("oomKill error: %v, waitErr: %v", oomKill, waitErr)
 
 	// check oom error
 	if oomKill {
@@ -351,6 +347,17 @@ func (d *Driver) Run(c *execdriver.Command, pipes *execdriver.Pipes, hooks execd
 	return execdriver.ExitStatus{ExitCode: exitCode, OOMKilled: oomKill}, waitErr
 }
 
+func notifyChannelOOM(paths map[string]string) <-chan struct{} {
+	oom, err := notifyOnOOM(paths)
+	if err != nil {
+		logrus.Warnf("Your kernel does not support OOM notifications: %s", err)
+		c := make(chan struct{})
+		close(c)
+		return c
+	}
+	return oom
+}
+
 // copy from libcontainer
 func notifyOnOOM(paths map[string]string) (<-chan struct{}, error) {
 	dir := paths["memory"]
@@ -386,11 +393,13 @@ func notifyOnOOM(paths map[string]string) (<-chan struct{}, error) {
 		buf := make([]byte, 8)
 		for {
 			if _, err := eventfd.Read(buf); err != nil {
+				logrus.Warn(err)
 				return
 			}
 			// When a cgroup is destroyed, an event is sent to eventfd.
 			// So if the control path is gone, return instead of notifying.
 			if _, err := os.Lstat(eventControlPath); os.IsNotExist(err) {
+				logrus.Warn(err)
 				return
 			}
 			ch <- struct{}{}
@@ -424,6 +433,11 @@ func cgroupPaths(containerID string) (map[string]string, error) {
 			//unsupported subystem
 			continue
 		}
+		// if we are running dind
+		dockerPathIdx := strings.LastIndex(cgroupDir, "docker")
+		if dockerPathIdx != -1 {
+			cgroupDir = cgroupDir[:dockerPathIdx-1]
+		}
 		path := filepath.Join(cgroupRoot, cgroupDir, "lxc", containerID)
 		paths[subsystem] = path
 	}

daemon/execdriver/lxc/lxc_template.go

@@ -91,9 +91,9 @@ lxc.mount.entry = {{$value.Source}} {{escapeFstabSpaces $ROOTFS}}/{{escapeFstabS
 {{if .Resources}}
 {{if .Resources.Memory}}
 lxc.cgroup.memory.limit_in_bytes = {{.Resources.Memory}}
-{{with $memSwap := getMemorySwap .Resources}}
-lxc.cgroup.memory.memsw.limit_in_bytes = {{$memSwap}}
 {{end}}
+{{if gt .Resources.MemorySwap 0}}
+lxc.cgroup.memory.memsw.limit_in_bytes = {{.Resources.MemorySwap}}
 {{end}}
 {{if gt .Resources.MemoryReservation 0}}
 lxc.cgroup.memory.soft_limit_in_bytes = {{.Resources.MemoryReservation}}
@@ -209,15 +209,6 @@ func isDirectory(source string) string {
 	return "file"
 }
 
-func getMemorySwap(v *execdriver.Resources) int64 {
-	// By default, MemorySwap is set to twice the size of RAM.
-	// If you want to omit MemorySwap, set it to `-1'.
-	if v.MemorySwap < 0 {
-		return 0
-	}
-	return v.Memory * 2
-}
-
 func getLabel(c map[string][]string, name string) string {
 	label := c["label"]
 	for _, l := range label {
@@ -242,7 +233,6 @@ func getHostname(env []string) string {
 func init() {
 	var err error
 	funcMap := template.FuncMap{
-		"getMemorySwap":     getMemorySwap,
 		"escapeFstabSpaces": escapeFstabSpaces,
 		"formatMountLabel":  label.FormatMountLabel,
 		"isDirectory":       isDirectory,

daemon/execdriver/lxc/lxc_template_unit_test.go

@@ -34,6 +34,7 @@ func TestLXCConfig(t *testing.T) {
 		memMin = 33554432
 		memMax = 536870912
 		mem    = memMin + r.Intn(memMax-memMin)
+		swap   = memMax
 		cpuMin = 100
 		cpuMax = 10000
 		cpu    = cpuMin + r.Intn(cpuMax-cpuMin)
@@ -46,8 +47,9 @@ func TestLXCConfig(t *testing.T) {
 	command := &execdriver.Command{
 		ID: "1",
 		Resources: &execdriver.Resources{
-			Memory:    int64(mem),
-			CPUShares: int64(cpu),
+			Memory:     int64(mem),
+			MemorySwap: int64(swap),
+			CPUShares:  int64(cpu),
 		},
 		Network: &execdriver.Network{
 			Mtu: 1500,
@@ -63,7 +65,7 @@ func TestLXCConfig(t *testing.T) {
 		fmt.Sprintf("lxc.cgroup.memory.limit_in_bytes = %d", mem))
 
 	grepFile(t, p,
-		fmt.Sprintf("lxc.cgroup.memory.memsw.limit_in_bytes = %d", mem*2))
+		fmt.Sprintf("lxc.cgroup.memory.memsw.limit_in_bytes = %d", swap))
 }
 
 func TestCustomLxcConfig(t *testing.T) {

daemon/execdriver/native/driver.go

@@ -167,7 +167,6 @@ func (d *Driver) Run(c *execdriver.Command, pipes *execdriver.Pipes, hooks execd
 
 	oom := notifyOnOOM(cont)
 	if hooks.Start != nil {
-
 		pid, err := p.Pid()
 		if err != nil {
 			p.Signal(os.Kill)

integration-cli/check_test.go

@@ -31,6 +31,7 @@ func (s *DockerSuite) TearDownTest(c *check.C) {
 	deleteAllContainers()
 	deleteAllImages()
 	deleteAllVolumes()
+	deleteAllNetworks()
 }
 
 func init() {

integration-cli/docker_cli_diff_test.go

@@ -61,6 +61,7 @@ func (s *DockerSuite) TestDiffEnsureOnlyKmsgAndPtmx(c *check.C) {
 		"C /dev":         true,
 		"A /dev/full":    true, // busybox
 		"C /dev/ptmx":    true, // libcontainer
+		"A /dev/mqueue":  true, // lxc
 		"A /dev/kmsg":    true, // lxc
 		"A /dev/fd":      true,
 		"A /dev/fuse":    true,

integration-cli/docker_cli_events_unix_test.go

@@ -56,6 +56,7 @@ func (s *DockerSuite) TestEventsRedirectStdout(c *check.C) {
 
 func (s *DockerSuite) TestEventsOOMDisableFalse(c *check.C) {
 	testRequires(c, DaemonIsLinux)
+	testRequires(c, NativeExecDriver)
 	testRequires(c, oomControl)
 
 	errChan := make(chan error)
@@ -103,6 +104,7 @@ func (s *DockerSuite) TestEventsOOMDisableFalse(c *check.C) {
 
 func (s *DockerSuite) TestEventsOOMDisableTrue(c *check.C) {
 	testRequires(c, DaemonIsLinux)
+	testRequires(c, NativeExecDriver)
 	testRequires(c, oomControl)
 
 	errChan := make(chan error)

integration-cli/docker_cli_run_test.go

@@ -1197,7 +1197,7 @@ func (s *DockerSuite) TestRunNonRootUserResolvName(c *check.C) {
 // uses the host's /etc/resolv.conf and does not have any dns options provided.
 func (s *DockerSuite) TestRunResolvconfUpdate(c *check.C) {
 	// Not applicable on Windows as testing unix specific functionality
-	testRequires(c, SameHostDaemon, DaemonIsLinux)
+	testRequires(c, SameHostDaemon, DaemonIsLinux, NativeExecDriver)
 
 	tmpResolvConf := []byte("search pommesfrites.fr\nnameserver 12.34.56.78\n")
 	tmpLocalhostResolvConf := []byte("nameserver 127.0.0.1")
@@ -3425,13 +3425,10 @@ func (s *DockerSuite) TestContainersInUserDefinedNetwork(c *check.C) {
 	dockerCmd(c, "run", "-d", "--net=testnetwork", "--name=first", "busybox", "top")
 	c.Assert(waitRun("first"), check.IsNil)
 	dockerCmd(c, "run", "-t", "--net=testnetwork", "--name=second", "busybox", "ping", "-c", "1", "first")
-	dockerCmd(c, "stop", "first")
-	dockerCmd(c, "stop", "second")
-	dockerCmd(c, "network", "rm", "testnetwork")
 }
 
 func (s *DockerSuite) TestContainersInMultipleNetworks(c *check.C) {
-	testRequires(c, DaemonIsLinux, NotUserNamespace)
+	testRequires(c, DaemonIsLinux, NotUserNamespace, NativeExecDriver)
 	// Create 2 networks using bridge driver
 	dockerCmd(c, "network", "create", "-d", "bridge", "testnetwork1")
 	dockerCmd(c, "network", "create", "-d", "bridge", "testnetwork2")
@@ -3447,14 +3444,10 @@ func (s *DockerSuite) TestContainersInMultipleNetworks(c *check.C) {
 	dockerCmd(c, "network", "connect", "testnetwork2", "second")
 	// Check connectivity between containers
 	dockerCmd(c, "exec", "second", "ping", "-c", "1", "first.testnetwork2")
-	dockerCmd(c, "stop", "first")
-	dockerCmd(c, "stop", "second")
-	dockerCmd(c, "network", "rm", "testnetwork1")
-	dockerCmd(c, "network", "rm", "testnetwork2")
 }
 
 func (s *DockerSuite) TestContainersNetworkIsolation(c *check.C) {
-	testRequires(c, DaemonIsLinux, NotUserNamespace)
+	testRequires(c, DaemonIsLinux, NotUserNamespace, NativeExecDriver)
 	// Create 2 networks using bridge driver
 	dockerCmd(c, "network", "create", "-d", "bridge", "testnetwork1")
 	dockerCmd(c, "network", "create", "-d", "bridge", "testnetwork2")
@@ -3478,11 +3471,6 @@ func (s *DockerSuite) TestContainersNetworkIsolation(c *check.C) {
 	// ping must fail again
 	_, _, err = dockerCmdWithError("exec", "first", "ping", "-c", "1", "second")
 	c.Assert(err, check.NotNil)
-
-	dockerCmd(c, "stop", "first")
-	dockerCmd(c, "stop", "second")
-	dockerCmd(c, "network", "rm", "testnetwork1")
-	dockerCmd(c, "network", "rm", "testnetwork2")
 }
 
 func (s *DockerSuite) TestNetworkRmWithActiveContainers(c *check.C) {
@@ -3501,17 +3489,14 @@ func (s *DockerSuite) TestNetworkRmWithActiveContainers(c *check.C) {
 	dockerCmd(c, "stop", "first")
 	_, _, err = dockerCmdWithError("network", "rm", "testnetwork1")
 	c.Assert(err, check.NotNil)
-
-	dockerCmd(c, "stop", "second")
-	// Network delete must succeed after all the connected containers are inactive
-	dockerCmd(c, "network", "rm", "testnetwork1")
 }
 
 func (s *DockerSuite) TestContainerRestartInMultipleNetworks(c *check.C) {
-	testRequires(c, DaemonIsLinux, NotUserNamespace)
+	testRequires(c, DaemonIsLinux, NotUserNamespace, NativeExecDriver)
 	// Create 2 networks using bridge driver
 	dockerCmd(c, "network", "create", "-d", "bridge", "testnetwork1")
 	dockerCmd(c, "network", "create", "-d", "bridge", "testnetwork2")
+
 	// Run and connect containers to testnetwork1
 	dockerCmd(c, "run", "-d", "--net=testnetwork1", "--name=first", "busybox", "top")
 	c.Assert(waitRun("first"), check.IsNil)
@@ -3536,11 +3521,6 @@ func (s *DockerSuite) TestContainerRestartInMultipleNetworks(c *check.C) {
 	dockerCmd(c, "start", "second")
 	dockerCmd(c, "exec", "first", "ping", "-c", "1", "second.testnetwork1")
 	dockerCmd(c, "exec", "second", "ping", "-c", "1", "first.testnetwork2")
-
-	dockerCmd(c, "stop", "first")
-	dockerCmd(c, "stop", "second")
-	dockerCmd(c, "network", "rm", "testnetwork1")
-	dockerCmd(c, "network", "rm", "testnetwork2")
 }
 
 func (s *DockerSuite) TestContainerWithConflictingHostNetworks(c *check.C) {
@@ -3555,8 +3535,6 @@ func (s *DockerSuite) TestContainerWithConflictingHostNetworks(c *check.C) {
 	// Connecting to the user defined network must fail
 	_, _, err := dockerCmdWithError("network", "connect", "testnetwork1", "first")
 	c.Assert(err, check.NotNil)
-	dockerCmd(c, "stop", "first")
-	dockerCmd(c, "network", "rm", "testnetwork1")
 }
 
 func (s *DockerSuite) TestContainerWithConflictingSharedNetwork(c *check.C) {
@@ -3574,10 +3552,6 @@ func (s *DockerSuite) TestContainerWithConflictingSharedNetwork(c *check.C) {
 	out, _, err := dockerCmdWithError("network", "connect", "testnetwork1", "second")
 	c.Assert(err, check.NotNil)
 	c.Assert(out, checker.Contains, runconfig.ErrConflictSharedNetwork.Error())
-
-	dockerCmd(c, "stop", "first")
-	dockerCmd(c, "stop", "second")
-	dockerCmd(c, "network", "rm", "testnetwork1")
 }
 
 func (s *DockerSuite) TestContainerWithConflictingNoneNetwork(c *check.C) {
@@ -3600,10 +3574,6 @@ func (s *DockerSuite) TestContainerWithConflictingNoneNetwork(c *check.C) {
 	// Connect second container to none network. it must fail as well
 	_, _, err = dockerCmdWithError("network", "connect", "none", "second")
 	c.Assert(err, check.NotNil)
-
-	dockerCmd(c, "stop", "first")
-	dockerCmd(c, "stop", "second")
-	dockerCmd(c, "network", "rm", "testnetwork1")
 }
 
 // #11957 - stdin with no tty does not exit if stdin is not closed even though container exited

integration-cli/docker_cli_run_unix_test.go

@@ -17,6 +17,7 @@ import (
 	"github.com/docker/docker/pkg/mount"
 	"github.com/docker/docker/pkg/parsers"
 	"github.com/docker/docker/pkg/sysinfo"
+	"github.com/docker/docker/pkg/units"
 	"github.com/go-check/check"
 	"github.com/kr/pty"
 )
@@ -419,7 +420,7 @@ func (s *DockerSuite) TestRunInvalidCpusetMemsFlagValue(c *check.C) {
 }
 
 func (s *DockerSuite) TestRunInvalidCPUShares(c *check.C) {
-	testRequires(c, cpuShare)
+	testRequires(c, cpuShare, NativeExecDriver)
 	out, _, err := dockerCmdWithError("run", "--cpu-shares", "1", "busybox", "echo", "test")
 	c.Assert(err, check.NotNil, check.Commentf(out))
 	expected := "The minimum allowed cpu-shares is 2"
@@ -435,3 +436,22 @@ func (s *DockerSuite) TestRunInvalidCPUShares(c *check.C) {
 	expected = "The maximum allowed cpu-shares is"
 	c.Assert(out, checker.Contains, expected)
 }
+
+func (s *DockerSuite) TestRunWithCorrectMemorySwapOnLXC(c *check.C) {
+	testRequires(c, memoryLimitSupport)
+	testRequires(c, swapMemorySupport)
+	testRequires(c, SameHostDaemon)
+
+	out, _ := dockerCmd(c, "run", "-d", "-m", "16m", "--memory-swap", "64m", "busybox", "top")
+	if _, err := os.Stat("/sys/fs/cgroup/memory/lxc"); err != nil {
+		c.Skip("Excecution driver must be LXC for this test")
+	}
+	id := strings.TrimSpace(out)
+	memorySwap, err := ioutil.ReadFile(fmt.Sprintf("/sys/fs/cgroup/memory/lxc/%s/memory.memsw.limit_in_bytes", id))
+	c.Assert(err, check.IsNil)
+	cgSwap, err := strconv.ParseInt(strings.TrimSpace(string(memorySwap)), 10, 64)
+	c.Assert(err, check.IsNil)
+	swap, err := units.RAMInBytes("64m")
+	c.Assert(err, check.IsNil)
+	c.Assert(cgSwap, check.Equals, swap)
+}

integration-cli/docker_utils.go

@@ -506,6 +506,42 @@ func deleteAllContainers() error {
 	return nil
 }
 
+func deleteAllNetworks() error {
+	networks, err := getAllNetworks()
+	if err != nil {
+		return err
+	}
+	var errors []string
+	for _, n := range networks {
+		if n.Name != "bridge" {
+			status, b, err := sockRequest("DELETE", "/networks/"+n.Name, nil)
+			if err != nil {
+				errors = append(errors, err.Error())
+				continue
+			}
+			if status != http.StatusNoContent {
+				errors = append(errors, fmt.Sprintf("error deleting network %s: %s", n.Name, string(b)))
+			}
+		}
+	}
+	if len(errors) > 0 {
+		return fmt.Errorf(strings.Join(errors, "\n"))
+	}
+	return nil
+}
+
+func getAllNetworks() ([]types.NetworkResource, error) {
+	var networks []types.NetworkResource
+	_, b, err := sockRequest("GET", "/networks", nil)
+	if err != nil {
+		return nil, err
+	}
+	if err := json.Unmarshal(b, &networks); err != nil {
+		return nil, err
+	}
+	return networks, nil
+}
+
 func deleteAllVolumes() error {
 	volumes, err := getAllVolumes()
 	if err != nil {