Merge pull request #32909 from cpuguy83/32907_volume_unmount_on_cp

Add refcount for MountPoint

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
-		// TODO(cpuguyh83): there should be a better way to handle this
-		if volumeMount.Volume != nil && volumeMount.ID != "" {
-			if err := volumeMount.Volume.Unmount(volumeMount.ID); err != nil {
-				errors = append(errors, err.Error())
-				continue
-			}
-			volumeMount.ID = ""
+		if volumeMount.Volume == nil {
+			continue
+		}
 
-			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)