From 1d8da80dbf8212f177a4cc4f44bf8dc43e810afc Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Mon, 3 Feb 2020 12:28:19 -0800 Subject: [PATCH] Check tmpfs mounts before create anon volume This makes sure that things like `--tmpfs` mounts over an anonymous volume don't create volumes uneccessarily. One method only checks mountpoints, the other checks both mountpoints and tmpfs... the usage of these should likely be consolidated. Ideally, processing for `--tmpfs` mounts would get merged in with the rest of the mount parsing. I opted not to do that for this change so the fix is minimal and can potentially be backported with fewer changes of breaking things. Merging the mount processing for tmpfs can be handled in a followup. Signed-off-by: Brian Goff (cherry picked from commit f464c31668db6fe781b3195f23dc786ee09e91c0) Signed-off-by: Brian Goff --- daemon/create_unix.go | 4 ++- integration/container/create_test.go | 47 +++++++++++++++++++++++++++ integration/internal/container/ops.go | 21 ++++++++++-- 3 files changed, 69 insertions(+), 3 deletions(-) diff --git a/daemon/create_unix.go b/daemon/create_unix.go index e78415aaef..a1ad3570c1 100644 --- a/daemon/create_unix.go +++ b/daemon/create_unix.go @@ -46,7 +46,9 @@ func (daemon *Daemon) createContainerOSSpecificSettings(container *container.Con // Skip volumes for which we already have something mounted on that // destination because of a --volume-from. - if container.IsDestinationMounted(destination) { + if container.HasMountFor(destination) { + logrus.WithField("container", container.ID).WithField("destination", spec).Debug("mountpoint already exists, skipping anonymous volume") + // Not an error, this could easily have come from the image config. continue } path, err := container.GetResourcePath(destination) diff --git a/integration/container/create_test.go b/integration/container/create_test.go index b4bd23bce8..c6e148b35c 100644 --- a/integration/container/create_test.go +++ b/integration/container/create_test.go @@ -535,3 +535,50 @@ func TestCreateWithInvalidHealthcheckParams(t *testing.T) { }) } } + +// Make sure that anonymous volumes can be overritten by tmpfs +// https://github.com/moby/moby/issues/40446 +func TestCreateTmpfsOverrideAnonymousVolume(t *testing.T) { + skip.If(t, testEnv.DaemonInfo.OSType == "windows", "windows does not support tmpfs") + defer setupTest(t)() + client := testEnv.APIClient() + ctx := context.Background() + + id := ctr.Create(ctx, t, client, + ctr.WithVolume("/foo"), + ctr.WithTmpfs("/foo"), + ctr.WithVolume("/bar"), + ctr.WithTmpfs("/bar:size=999"), + ctr.WithCmd("/bin/sh", "-c", "mount | grep '/foo' | grep tmpfs && mount | grep '/bar' | grep tmpfs"), + ) + + defer func() { + err := client.ContainerRemove(ctx, id, types.ContainerRemoveOptions{Force: true}) + assert.NilError(t, err) + }() + + inspect, err := client.ContainerInspect(ctx, id) + assert.NilError(t, err) + // tmpfs do not currently get added to inspect.Mounts + // Normally an anoynmous volume would, except now tmpfs should prevent that. + assert.Assert(t, is.Len(inspect.Mounts, 0)) + + chWait, chErr := client.ContainerWait(ctx, id, container.WaitConditionNextExit) + assert.NilError(t, client.ContainerStart(ctx, id, types.ContainerStartOptions{})) + + timeout := time.NewTimer(30 * time.Second) + defer timeout.Stop() + + select { + case <-timeout.C: + t.Fatal("timeout waiting for container to exit") + case status := <-chWait: + var errMsg string + if status.Error != nil { + errMsg = status.Error.Message + } + assert.Equal(t, int(status.StatusCode), 0, errMsg) + case err := <-chErr: + assert.NilError(t, err) + } +} diff --git a/integration/internal/container/ops.go b/integration/internal/container/ops.go index 10fc064702..62e6d638a8 100644 --- a/integration/internal/container/ops.go +++ b/integration/internal/container/ops.go @@ -2,6 +2,7 @@ package container import ( "fmt" + "strings" containertypes "github.com/docker/docker/api/types/container" mounttypes "github.com/docker/docker/api/types/mount" @@ -77,12 +78,12 @@ func WithMount(m mounttypes.Mount) func(*TestContainerConfig) { } // WithVolume sets the volume of the container -func WithVolume(name string) func(*TestContainerConfig) { +func WithVolume(target string) func(*TestContainerConfig) { return func(c *TestContainerConfig) { if c.Config.Volumes == nil { c.Config.Volumes = map[string]struct{}{} } - c.Config.Volumes[name] = struct{}{} + c.Config.Volumes[target] = struct{}{} } } @@ -93,6 +94,22 @@ func WithBind(src, target string) func(*TestContainerConfig) { } } +// WithTmpfs sets a target path in the container to a tmpfs +func WithTmpfs(target string) func(config *TestContainerConfig) { + return func(c *TestContainerConfig) { + if c.HostConfig.Tmpfs == nil { + c.HostConfig.Tmpfs = make(map[string]string) + } + + spec := strings.SplitN(target, ":", 2) + var opts string + if len(spec) > 1 { + opts = spec[1] + } + c.HostConfig.Tmpfs[spec[0]] = opts + } +} + // WithIPv4 sets the specified ip for the specified network of the container func WithIPv4(network, ip string) func(*TestContainerConfig) { return func(c *TestContainerConfig) {