Fix duplicate mount point for --volumes-from in docker run

This fix tries to fix the issue raised in 21845. The issue with 21845
is that if multiple `--volumes-from` with the same destination has been
specified, then one volume will be overridden by the other. This will mess
up with volumes reference and prevent the overridden volume from
being removed at the end.

Issue 21845 was observed with `docker-compose` though it is possible to
emulate the same behavior with `docker` alone:
```
$ cat Dockerfile
FROM busybox
VOLUME ["/tmp/data"]
$ docker build -t vimage .
$ docker run --name=data1 vimage true
$ docker run --name=data2 vimage true
$ docker run --name=app --volumes-from=data1 --volumes-from=data2 -d busybox top
$ docker rm -f -v $(docker ps -aq)
$ docker volume ls
$ docker volume rm ...
```
NOTE: Second case:
```
$ cat Dockerfile
FROM busybox
VOLUME ["/tmp/data"]
$ docker build -t vimage .
$ docker run --name=data1 vimage true
$ docker run --name=data2 vimage true
$ docker run --name=app --volumes-from=data1 --volumes-from=data2 -v /tmp/data:/tmp/data -d busybox top
$ docker rm -f -v $(docker ps -aq)
$ docker volume ls
$ docker volume rm ...
```
NOTE: Third case: Combination of --volumes-from and `HostConfig.Mounts` (API only)

This fix tries to address the issue by return an error if duplicate
mount points was used with `--volumes-from`.

An integration test has been added.

This fix fixes 21845.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
(cherry picked from commit 9526e5c6ae)
Signed-off-by: Victor Vieux <victorvieux@gmail.com>

daemon/volumes.go

@@ -85,6 +85,15 @@ func (daemon *Daemon) registerMountPoints(container *container.Container, hostCo
 		}
 	}()
 
+	dereferenceIfExists := func(destination string) {
+		if v, ok := mountPoints[destination]; ok {
+			logrus.Debugf("Duplicate mount point '%s'", destination)
+			if v.Volume != nil {
+				daemon.volumes.Dereference(v.Volume, container.ID)
+			}
+		}
+	}
+
 	// 1. Read already configured mount points.
 	for destination, point := range container.MountPoints {
 		mountPoints[destination] = point
@@ -121,7 +130,7 @@ func (daemon *Daemon) registerMountPoints(container *container.Container, hostCo
 				}
 				cp.Volume = v
 			}
-
+			dereferenceIfExists(cp.Destination)
 			mountPoints[cp.Destination] = cp
 		}
 	}
@@ -155,6 +164,7 @@ func (daemon *Daemon) registerMountPoints(container *container.Container, hostCo
 		}
 
 		binds[bind.Destination] = true
+		dereferenceIfExists(bind.Destination)
 		mountPoints[bind.Destination] = bind
 	}
 
@@ -199,6 +209,7 @@ func (daemon *Daemon) registerMountPoints(container *container.Container, hostCo
 		}
 
 		binds[mp.Destination] = true
+		dereferenceIfExists(mp.Destination)
 		mountPoints[mp.Destination] = mp
 	}
 

integration-cli/docker_cli_volume_test.go

@@ -3,6 +3,7 @@ package main
 import (
 	"fmt"
 	"io/ioutil"
+	"net/http"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -425,3 +426,162 @@ func (s *DockerSuite) TestVolumeCliInspectWithVolumeOpts(c *check.C) {
 	c.Assert(strings.TrimSpace(out), checker.Contains, fmt.Sprintf("%s:%s", k2, v2))
 	c.Assert(strings.TrimSpace(out), checker.Contains, fmt.Sprintf("%s:%s", k3, v3))
 }
+
+// Test case (1) for 21845: duplicate targets for --volumes-from
+func (s *DockerSuite) TestDuplicateMountpointsForVolumesFrom(c *check.C) {
+	testRequires(c, DaemonIsLinux)
+
+	image := "vimage"
+	_, err := buildImage(
+		image,
+		`
+    FROM busybox
+    VOLUME ["/tmp/data"]
+    `,
+		true)
+	c.Assert(err, check.IsNil)
+
+	dockerCmd(c, "run", "--name=data1", image, "true")
+	dockerCmd(c, "run", "--name=data2", image, "true")
+
+	out, _ := dockerCmd(c, "inspect", "--format", "{{(index .Mounts 0).Name}}", "data1")
+	data1 := strings.TrimSpace(out)
+	c.Assert(data1, checker.Not(checker.Equals), "")
+
+	out, _ = dockerCmd(c, "inspect", "--format", "{{(index .Mounts 0).Name}}", "data2")
+	data2 := strings.TrimSpace(out)
+	c.Assert(data2, checker.Not(checker.Equals), "")
+
+	// Both volume should exist
+	out, _ = dockerCmd(c, "volume", "ls", "-q")
+	c.Assert(strings.TrimSpace(out), checker.Contains, data1)
+	c.Assert(strings.TrimSpace(out), checker.Contains, data2)
+
+	out, _, err = dockerCmdWithError("run", "--name=app", "--volumes-from=data1", "--volumes-from=data2", "-d", "busybox", "top")
+	c.Assert(err, checker.IsNil, check.Commentf("Out: %s", out))
+
+	// Only the second volume will be referenced, this is backward compatible
+	out, _ = dockerCmd(c, "inspect", "--format", "{{(index .Mounts 0).Name}}", "app")
+	c.Assert(strings.TrimSpace(out), checker.Equals, data2)
+
+	dockerCmd(c, "rm", "-f", "-v", "app")
+	dockerCmd(c, "rm", "-f", "-v", "data1")
+	dockerCmd(c, "rm", "-f", "-v", "data2")
+
+	// Both volume should not exist
+	out, _ = dockerCmd(c, "volume", "ls", "-q")
+	c.Assert(strings.TrimSpace(out), checker.Not(checker.Contains), data1)
+	c.Assert(strings.TrimSpace(out), checker.Not(checker.Contains), data2)
+}
+
+// Test case (2) for 21845: duplicate targets for --volumes-from and -v (bind)
+func (s *DockerSuite) TestDuplicateMountpointsForVolumesFromAndBind(c *check.C) {
+	testRequires(c, DaemonIsLinux)
+
+	image := "vimage"
+	_, err := buildImage(image,
+		`
+    FROM busybox
+    VOLUME ["/tmp/data"]
+    `,
+		true)
+	c.Assert(err, check.IsNil)
+
+	dockerCmd(c, "run", "--name=data1", image, "true")
+	dockerCmd(c, "run", "--name=data2", image, "true")
+
+	out, _ := dockerCmd(c, "inspect", "--format", "{{(index .Mounts 0).Name}}", "data1")
+	data1 := strings.TrimSpace(out)
+	c.Assert(data1, checker.Not(checker.Equals), "")
+
+	out, _ = dockerCmd(c, "inspect", "--format", "{{(index .Mounts 0).Name}}", "data2")
+	data2 := strings.TrimSpace(out)
+	c.Assert(data2, checker.Not(checker.Equals), "")
+
+	// Both volume should exist
+	out, _ = dockerCmd(c, "volume", "ls", "-q")
+	c.Assert(strings.TrimSpace(out), checker.Contains, data1)
+	c.Assert(strings.TrimSpace(out), checker.Contains, data2)
+
+	out, _, err = dockerCmdWithError("run", "--name=app", "--volumes-from=data1", "--volumes-from=data2", "-v", "/tmp/data:/tmp/data", "-d", "busybox", "top")
+	c.Assert(err, checker.IsNil, check.Commentf("Out: %s", out))
+
+	// No volume will be referenced (mount is /tmp/data), this is backward compatible
+	out, _ = dockerCmd(c, "inspect", "--format", "{{(index .Mounts 0).Name}}", "app")
+	c.Assert(strings.TrimSpace(out), checker.Not(checker.Contains), data1)
+	c.Assert(strings.TrimSpace(out), checker.Not(checker.Contains), data2)
+
+	dockerCmd(c, "rm", "-f", "-v", "app")
+	dockerCmd(c, "rm", "-f", "-v", "data1")
+	dockerCmd(c, "rm", "-f", "-v", "data2")
+
+	// Both volume should not exist
+	out, _ = dockerCmd(c, "volume", "ls", "-q")
+	c.Assert(strings.TrimSpace(out), checker.Not(checker.Contains), data1)
+	c.Assert(strings.TrimSpace(out), checker.Not(checker.Contains), data2)
+}
+
+// Test case (3) for 21845: duplicate targets for --volumes-from and `Mounts` (API only)
+func (s *DockerSuite) TestDuplicateMountpointsForVolumesFromAndMounts(c *check.C) {
+	testRequires(c, DaemonIsLinux)
+
+	image := "vimage"
+	_, err := buildImage(image,
+		`
+    FROM busybox
+    VOLUME ["/tmp/data"]
+    `,
+		true)
+	c.Assert(err, check.IsNil)
+
+	dockerCmd(c, "run", "--name=data1", image, "true")
+	dockerCmd(c, "run", "--name=data2", image, "true")
+
+	out, _ := dockerCmd(c, "inspect", "--format", "{{(index .Mounts 0).Name}}", "data1")
+	data1 := strings.TrimSpace(out)
+	c.Assert(data1, checker.Not(checker.Equals), "")
+
+	out, _ = dockerCmd(c, "inspect", "--format", "{{(index .Mounts 0).Name}}", "data2")
+	data2 := strings.TrimSpace(out)
+	c.Assert(data2, checker.Not(checker.Equals), "")
+
+	// Both volume should exist
+	out, _ = dockerCmd(c, "volume", "ls", "-q")
+	c.Assert(strings.TrimSpace(out), checker.Contains, data1)
+	c.Assert(strings.TrimSpace(out), checker.Contains, data2)
+
+	// Mounts is available in API
+	status, body, err := sockRequest("POST", "/containers/create?name=app", map[string]interface{}{
+		"Image": "busybox",
+		"Cmd":   []string{"top"},
+		"HostConfig": map[string]interface{}{
+			"VolumesFrom": []string{
+				"data1",
+				"data2",
+			},
+			"Mounts": []map[string]interface{}{
+				{
+					"Type":   "bind",
+					"Source": "/tmp/data",
+					"Target": "/tmp/data",
+				},
+			}},
+	})
+
+	c.Assert(err, checker.IsNil, check.Commentf(string(body)))
+	c.Assert(status, checker.Equals, http.StatusCreated, check.Commentf(string(body)))
+
+	// No volume will be referenced (mount is /tmp/data), this is backward compatible
+	out, _ = dockerCmd(c, "inspect", "--format", "{{(index .Mounts 0).Name}}", "app")
+	c.Assert(strings.TrimSpace(out), checker.Not(checker.Contains), data1)
+	c.Assert(strings.TrimSpace(out), checker.Not(checker.Contains), data2)
+
+	dockerCmd(c, "rm", "-f", "-v", "app")
+	dockerCmd(c, "rm", "-f", "-v", "data1")
+	dockerCmd(c, "rm", "-f", "-v", "data2")
+
+	// Both volume should not exist
+	out, _ = dockerCmd(c, "volume", "ls", "-q")
+	c.Assert(strings.TrimSpace(out), checker.Not(checker.Contains), data1)
+	c.Assert(strings.TrimSpace(out), checker.Not(checker.Contains), data2)
+}