From 69bca6d97b23b23e17e6ede70911c19b15aaaddb Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 10 Nov 2017 15:41:02 -0800 Subject: [PATCH 1/2] container.NetworkMounts(): don't lookup mounts twice The code in question looks up mounts two times: first by using HasMountFor(), and then directly by looking in container.MountPoints. There is no need to do it twice. Signed-off-by: Kir Kolyshkin Upstream-commit: eab3ac3e70a510b97f9399efd13e3dc01a07c413 Component: engine --- components/engine/container/container_unix.go | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/components/engine/container/container_unix.go b/components/engine/container/container_unix.go index 77851946f3..2e1b95f404 100644 --- a/components/engine/container/container_unix.go +++ b/components/engine/container/container_unix.go @@ -65,12 +65,11 @@ func (container *Container) NetworkMounts() []Mount { if _, err := os.Stat(container.ResolvConfPath); err != nil { logrus.Warnf("ResolvConfPath set to %q, but can't stat this filename (err = %v); skipping", container.ResolvConfPath, err) } else { - if !container.HasMountFor("/etc/resolv.conf") { - label.Relabel(container.ResolvConfPath, container.MountLabel, shared) - } writable := !container.HostConfig.ReadonlyRootfs if m, exists := container.MountPoints["/etc/resolv.conf"]; exists { writable = m.RW + } else { + label.Relabel(container.ResolvConfPath, container.MountLabel, shared) } mounts = append(mounts, Mount{ Source: container.ResolvConfPath, @@ -84,12 +83,11 @@ func (container *Container) NetworkMounts() []Mount { if _, err := os.Stat(container.HostnamePath); err != nil { logrus.Warnf("HostnamePath set to %q, but can't stat this filename (err = %v); skipping", container.HostnamePath, err) } else { - if !container.HasMountFor("/etc/hostname") { - label.Relabel(container.HostnamePath, container.MountLabel, shared) - } writable := !container.HostConfig.ReadonlyRootfs if m, exists := container.MountPoints["/etc/hostname"]; exists { writable = m.RW + } else { + label.Relabel(container.HostnamePath, container.MountLabel, shared) } mounts = append(mounts, Mount{ Source: container.HostnamePath, @@ -103,12 +101,11 @@ func (container *Container) NetworkMounts() []Mount { if _, err := os.Stat(container.HostsPath); err != nil { logrus.Warnf("HostsPath set to %q, but can't stat this filename (err = %v); skipping", container.HostsPath, err) } else { - if !container.HasMountFor("/etc/hosts") { - label.Relabel(container.HostsPath, container.MountLabel, shared) - } writable := !container.HostConfig.ReadonlyRootfs if m, exists := container.MountPoints["/etc/hosts"]; exists { writable = m.RW + } else { + label.Relabel(container.HostsPath, container.MountLabel, shared) } mounts = append(mounts, Mount{ Source: container.HostsPath, From 6e7cb1931d8fc3b1fe2bcb41e9572394c41accbc Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 10 Nov 2017 15:43:57 -0800 Subject: [PATCH 2/2] Fix "duplicate mount point" when --tmpfs /dev/shm is used This is a fix to the following issue: $ docker run --tmpfs /dev/shm busybox sh docker: Error response from daemon: linux mounts: Duplicate mount point '/dev/shm'. In current code (daemon.createSpec()), tmpfs mount from --tmpfs is added to list of mounts (`ms`), when the mount from IpcMounts() is added. While IpcMounts() is checking for existing mounts first, it does that by using container.HasMountFor() function which only checks container.Mounts but not container.Tmpfs. Ultimately, the solution is to get rid of container.Tmpfs (moving its data to container.Mounts). Current workaround is to add checking of container.Tmpfs into container.HasMountFor(). A unit test case is included. Unfortunately we can't call daemon.createSpec() from a unit test, as the code relies a lot on various daemon structures to be initialized properly, and it is hard to achieve. Therefore, we minimally mimick the code flow of daemon.createSpec() -- barely enough to reproduce the issue. https://github.com/moby/moby/issues/35455 Signed-off-by: Kir Kolyshkin Upstream-commit: 1861abdc4a31efad202a5c3d89a895bb7a62799a Component: engine --- components/engine/container/container_unix.go | 13 ++++- components/engine/daemon/oci_linux_test.go | 50 +++++++++++++++++++ 2 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 components/engine/daemon/oci_linux_test.go diff --git a/components/engine/container/container_unix.go b/components/engine/container/container_unix.go index 2e1b95f404..28a3722c79 100644 --- a/components/engine/container/container_unix.go +++ b/components/engine/container/container_unix.go @@ -157,7 +157,18 @@ func (container *Container) ShmResourcePath() (string, error) { // HasMountFor checks if path is a mountpoint func (container *Container) HasMountFor(path string) bool { _, exists := container.MountPoints[path] - return exists + if exists { + return true + } + + // Also search among the tmpfs mounts + for dest := range container.HostConfig.Tmpfs { + if dest == path { + return true + } + } + + return false } // UnmountIpcMount uses the provided unmount function to unmount shm if it was mounted diff --git a/components/engine/daemon/oci_linux_test.go b/components/engine/daemon/oci_linux_test.go new file mode 100644 index 0000000000..f2f455f9c6 --- /dev/null +++ b/components/engine/daemon/oci_linux_test.go @@ -0,0 +1,50 @@ +package daemon + +import ( + "testing" + + containertypes "github.com/docker/docker/api/types/container" + "github.com/docker/docker/container" + "github.com/docker/docker/daemon/config" + "github.com/docker/docker/oci" + "github.com/docker/docker/pkg/idtools" + + "github.com/stretchr/testify/assert" +) + +// TestTmpfsDevShmNoDupMount checks that a user-specified /dev/shm tmpfs +// mount (as in "docker run --tmpfs /dev/shm:rw,size=NNN") does not result +// in "Duplicate mount point" error from the engine. +// https://github.com/moby/moby/issues/35455 +func TestTmpfsDevShmNoDupMount(t *testing.T) { + d := Daemon{ + // some empty structs to avoid getting a panic + // caused by a null pointer dereference + idMappings: &idtools.IDMappings{}, + configStore: &config.Config{}, + } + c := &container.Container{ + ShmPath: "foobar", // non-empty, for c.IpcMounts() to work + HostConfig: &containertypes.HostConfig{ + IpcMode: containertypes.IpcMode("shareable"), // default mode + // --tmpfs /dev/shm:rw,exec,size=NNN + Tmpfs: map[string]string{ + "/dev/shm": "rw,exec,size=1g", + }, + }, + } + + // Mimick the code flow of daemon.createSpec(), enough to reproduce the issue + ms, err := d.setupMounts(c) + assert.NoError(t, err) + + ms = append(ms, c.IpcMounts()...) + + tmpfsMounts, err := c.TmpfsMounts() + assert.NoError(t, err) + ms = append(ms, tmpfsMounts...) + + s := oci.DefaultSpec() + err = setMounts(&d, &s, c, ms) + assert.NoError(t, err) +}