Add refcount for MountPoint

This makes sure that multiple users of MountPoint pointer can
mount/unmount without affecting each other.

Before this PR, if you run a container (stay running), then do `docker
cp`, when the `docker cp` is done the MountPoint is mutated such that
when the container stops the volume driver will not get an Unmount
request. Effectively there would be two mounts with only one unmount.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

container/container.go

@@ -400,21 +400,20 @@ func (container *Container) AddMountPointWithVolume(destination string, vol volu
 func (container *Container) UnmountVolumes(volumeEventLog func(name, action string, attributes map[string]string)) error {
 	var errors []string
 	for _, volumeMount := range container.MountPoints {
-		// Check if the mountpoint has an ID, this is currently the best way to tell if it's actually mounted
+		if volumeMount.Volume == nil {
-		// TODO(cpuguyh83): there should be a better way to handle this
+			continue
-		if volumeMount.Volume != nil && volumeMount.ID != "" {
-			if err := volumeMount.Volume.Unmount(volumeMount.ID); err != nil {
-				errors = append(errors, err.Error())
-				continue
-			}
-			volumeMount.ID = ""
-
-			attributes := map[string]string{
-				"driver":    volumeMount.Volume.DriverName(),
-				"container": container.ID,
-			}
-			volumeEventLog(volumeMount.Volume.Name(), "unmount", attributes)
 		}
+
+		if err := volumeMount.Cleanup(); err != nil {
+			errors = append(errors, err.Error())
+			continue
+		}
+
+		attributes := map[string]string{
+			"driver":    volumeMount.Volume.DriverName(),
+			"container": container.ID,
+		}
+		volumeEventLog(volumeMount.Volume.Name(), "unmount", attributes)
 	}
 	if len(errors) > 0 {
 		return fmt.Errorf("error while unmounting volumes for container %s: %s", container.ID, strings.Join(errors, "; "))

integration-cli/docker_cli_external_volume_driver_unix_test.go

@@ -616,3 +616,18 @@ func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverUnmountOnMountFail(c
 	out, _ = s.d.Cmd("run", "-w", "/foo", "-v", "testumount:/foo", "busybox", "true")
 	c.Assert(s.ec.unmounts, checker.Equals, 0, check.Commentf(out))
 }
+
+func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverUnmountOnCp(c *check.C) {
+	s.d.StartWithBusybox(c)
+	s.d.Cmd("volume", "create", "-d", "test-external-volume-driver", "--name=test")
+
+	out, _ := s.d.Cmd("run", "-d", "--name=test", "-v", "test:/foo", "busybox", "/bin/sh", "-c", "touch /test && top")
+	c.Assert(s.ec.mounts, checker.Equals, 1, check.Commentf(out))
+
+	out, _ = s.d.Cmd("cp", "test:/test", "/tmp/test")
+	c.Assert(s.ec.mounts, checker.Equals, 2, check.Commentf(out))
+	c.Assert(s.ec.unmounts, checker.Equals, 1, check.Commentf(out))
+
+	out, _ = s.d.Cmd("kill", "test")
+	c.Assert(s.ec.unmounts, checker.Equals, 2, check.Commentf(out))
+}

volume/volume.go

@@ -120,6 +120,28 @@ type MountPoint struct {
 
 	// Sepc is a copy of the API request that created this mount.
 	Spec mounttypes.Mount
+
+	// Track usage of this mountpoint
+	// Specicially needed for containers which are running and calls to `docker cp`
+	// because both these actions require mounting the volumes.
+	active int
+}
+
+// Cleanup frees resources used by the mountpoint
+func (m *MountPoint) Cleanup() error {
+	if m.Volume == nil || m.ID == "" {
+		return nil
+	}
+
+	if err := m.Volume.Unmount(m.ID); err != nil {
+		return errors.Wrapf(err, "error unmounting volume %s", m.Volume.Name())
+	}
+
+	m.active--
+	if m.active == 0 {
+		m.ID = ""
+	}
+	return nil
 }
 
 // Setup sets up a mount point by either mounting the volume if it is
@@ -147,12 +169,16 @@ func (m *MountPoint) Setup(mountLabel string, rootUID, rootGID int) (path string
 		if err != nil {
 			return "", errors.Wrapf(err, "error while mounting volume '%s'", m.Source)
 		}
+
 		m.ID = id
+		m.active++
 		return path, nil
 	}
+
 	if len(m.Source) == 0 {
 		return "", fmt.Errorf("Unable to setup mount point, neither source nor volume defined")
 	}
+
 	// system.MkdirAll() produces an error if m.Source exists and is a file (not a directory),
 	if m.Type == mounttypes.TypeBind {
 		// idtools.MkdirAllNewAs() produces an error if m.Source exists and is a file (not a directory)