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 <cpuguy83@gmail.com>

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 {

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)
 

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