From a738df0354cc615c8d0fa3254621b3db811fe0b9 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Thu, 18 Dec 2014 09:57:36 -0500 Subject: [PATCH] Fix volumes-from re-applying on each start Fixes #9709 In cases where the volumes-from container is removed and the consuming container is restarted, docker was trying to re-apply volumes from that now missing container, which is uneccessary since the volumes are already applied. Also cleaned up the volumes-from parsing function, which was doing way more than it should have been. Signed-off-by: Brian Goff --- daemon/container.go | 7 ++- daemon/volumes.go | 80 +++++++++++++++----------- integration-cli/docker_cli_run_test.go | 39 ++++++++++--- 3 files changed, 82 insertions(+), 44 deletions(-) diff --git a/daemon/container.go b/daemon/container.go index becc69fcec..79ae5f08e5 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -92,9 +92,10 @@ type Container struct { VolumesRW map[string]bool hostConfig *runconfig.HostConfig - activeLinks map[string]*links.Link - monitor *containerMonitor - execCommands *execStore + activeLinks map[string]*links.Link + monitor *containerMonitor + execCommands *execStore + AppliedVolumesFrom map[string]struct{} } func (container *Container) FromDisk() error { diff --git a/daemon/volumes.go b/daemon/volumes.go index ad2dd3a6ad..fa38b253fa 100644 --- a/daemon/volumes.go +++ b/daemon/volumes.go @@ -214,20 +214,61 @@ func parseBindMountSpec(spec string) (string, string, bool, error) { return path, mountToPath, writable, nil } +func parseVolumesFromSpec(spec string) (string, string, error) { + specParts := strings.SplitN(spec, ":", 2) + if len(specParts) == 0 { + return "", "", fmt.Errorf("malformed volumes-from specification: %s", spec) + } + + var ( + id = specParts[0] + mode = "rw" + ) + if len(specParts) == 2 { + mode = specParts[1] + if !validMountMode(mode) { + return "", "", fmt.Errorf("invalid mode for volumes-from: %s", mode) + } + } + return id, mode, nil +} + func (container *Container) applyVolumesFrom() error { volumesFrom := container.hostConfig.VolumesFrom + if len(volumesFrom) > 0 && container.AppliedVolumesFrom == nil { + container.AppliedVolumesFrom = make(map[string]struct{}) + } - mountGroups := make([]map[string]*Mount, 0, len(volumesFrom)) + mountGroups := make(map[string][]*Mount) for _, spec := range volumesFrom { - mountGroup, err := parseVolumesFromSpec(container.daemon, spec) + id, mode, err := parseVolumesFromSpec(spec) if err != nil { return err } - mountGroups = append(mountGroups, mountGroup) + if _, exists := container.AppliedVolumesFrom[id]; exists { + // Don't try to apply these since they've already been applied + continue + } + + c := container.daemon.Get(id) + if c == nil { + return fmt.Errorf("container %s not found, impossible to mount its volumes", id) + } + + var ( + fromMounts = c.VolumeMounts() + mounts []*Mount + ) + + for _, mnt := range fromMounts { + mnt.Writable = mnt.Writable && (mode == "rw") + mounts = append(mounts, mnt) + } + mountGroups[id] = mounts } - for _, mounts := range mountGroups { + for id, mounts := range mountGroups { for _, mnt := range mounts { mnt.from = mnt.container mnt.container = container @@ -235,6 +276,7 @@ func (container *Container) applyVolumesFrom() error { return err } } + container.AppliedVolumesFrom[id] = struct{}{} } return nil } @@ -284,36 +326,6 @@ func (container *Container) setupMounts() error { return nil } -func parseVolumesFromSpec(daemon *Daemon, spec string) (map[string]*Mount, error) { - specParts := strings.SplitN(spec, ":", 2) - if len(specParts) == 0 { - return nil, fmt.Errorf("Malformed volumes-from specification: %s", spec) - } - - c := daemon.Get(specParts[0]) - if c == nil { - return nil, fmt.Errorf("Container %s not found. Impossible to mount its volumes", specParts[0]) - } - - mounts := c.VolumeMounts() - - if len(specParts) == 2 { - mode := specParts[1] - if !validMountMode(mode) { - return nil, fmt.Errorf("Invalid mode for volumes-from: %s", mode) - } - - // Set the mode for the inheritted volume - for _, mnt := range mounts { - // Ensure that if the inherited volume is not writable, that we don't make - // it writable here - mnt.Writable = mnt.Writable && (mode == "rw") - } - } - - return mounts, nil -} - func (container *Container) VolumeMounts() map[string]*Mount { mounts := make(map[string]*Mount) diff --git a/integration-cli/docker_cli_run_test.go b/integration-cli/docker_cli_run_test.go index 30acd9e1c4..e23aea648d 100644 --- a/integration-cli/docker_cli_run_test.go +++ b/integration-cli/docker_cli_run_test.go @@ -428,6 +428,7 @@ func TestRunVolumesMountedAsReadonly(t *testing.T) { } func TestRunVolumesFromInReadonlyMode(t *testing.T) { + defer deleteAllContainers() cmd := exec.Command(dockerBinary, "run", "--name", "parent", "-v", "/test", "busybox", "true") if _, err := runCommand(cmd); err != nil { t.Fatal(err) @@ -438,13 +439,12 @@ func TestRunVolumesFromInReadonlyMode(t *testing.T) { t.Fatalf("run should fail because volume is ro: exit code %d", code) } - deleteAllContainers() - logDone("run - volumes from as readonly mount") } // Regression test for #1201 func TestRunVolumesFromInReadWriteMode(t *testing.T) { + defer deleteAllContainers() cmd := exec.Command(dockerBinary, "run", "--name", "parent", "-v", "/test", "busybox", "true") if _, err := runCommand(cmd); err != nil { t.Fatal(err) @@ -456,7 +456,7 @@ func TestRunVolumesFromInReadWriteMode(t *testing.T) { } cmd = exec.Command(dockerBinary, "run", "--volumes-from", "parent:bar", "busybox", "touch", "/test/file") - if out, _, err := runCommandWithOutput(cmd); err == nil || !strings.Contains(out, "Invalid mode for volumes-from: bar") { + if out, _, err := runCommandWithOutput(cmd); err == nil || !strings.Contains(out, "invalid mode for volumes-from: bar") { t.Fatalf("running --volumes-from foo:bar should have failed with invalid mount mode: %q", out) } @@ -465,12 +465,11 @@ func TestRunVolumesFromInReadWriteMode(t *testing.T) { t.Fatalf("running --volumes-from parent failed with output: %q\nerror: %v", out, err) } - deleteAllContainers() - logDone("run - volumes from as read write mount") } func TestVolumesFromGetsProperMode(t *testing.T) { + defer deleteAllContainers() cmd := exec.Command(dockerBinary, "run", "--name", "parent", "-v", "/test:/test:ro", "busybox", "true") if _, err := runCommand(cmd); err != nil { t.Fatal(err) @@ -491,8 +490,6 @@ func TestVolumesFromGetsProperMode(t *testing.T) { t.Fatal("Expected volumes-from to inherit read-only volume even when passing in `ro`") } - deleteAllContainers() - logDone("run - volumes from ignores `rw` if inherrited volume is `ro`") } @@ -3058,3 +3055,31 @@ func TestRunContainerWithReadonlyRootfs(t *testing.T) { } logDone("run - read only rootfs") } + +func TestRunVolumesFromRestartAfterRemoved(t *testing.T) { + defer deleteAllContainers() + + out, _, err := runCommandWithOutput(exec.Command(dockerBinary, "run", "-d", "--name", "voltest", "-v", "/foo", "busybox")) + if err != nil { + t.Fatal(out, err) + } + + out, _, err = runCommandWithOutput(exec.Command(dockerBinary, "run", "-d", "--name", "restarter", "--volumes-from", "voltest", "busybox", "top")) + if err != nil { + t.Fatal(out, err) + } + + // Remove the main volume container and restart the consuming container + out, _, err = runCommandWithOutput(exec.Command(dockerBinary, "rm", "-f", "voltest")) + if err != nil { + t.Fatal(out, err) + } + + // This should not fail since the volumes-from were already applied + out, _, err = runCommandWithOutput(exec.Command(dockerBinary, "restart", "restarter")) + if err != nil { + t.Fatalf("expected container to restart successfully: %v\n%s", err, out) + } + + logDone("run - can restart a volumes-from container after producer is removed") +}