Merge pull request #27116 from cpuguy83/22564_fix_volume_unmount

Fix uneccessary calls to `volume.Unmount()`

container/container.go

@@ -567,6 +567,32 @@ func (container *Container) AddMountPointWithVolume(destination string, vol volu
 	}
 }
 
+// UnmountVolumes unmounts all volumes
+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 mounpoint 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 = ""
+
+			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, "; "))
+	}
+	return nil
+}
+
 // IsDestinationMounted checks whether a path is mounted on the container or not.
 func (container *Container) IsDestinationMounted(destination string) bool {
 	return container.MountPoints[destination] != nil

container/container_unix.go

@@ -102,18 +102,6 @@ func (container *Container) BuildHostnameFile() error {
 	return ioutil.WriteFile(container.HostnamePath, []byte(container.Config.Hostname+"\n"), 0644)
 }
 
-// appendNetworkMounts appends any network mounts to the array of mount points passed in
-func appendNetworkMounts(container *Container, volumeMounts []volume.MountPoint) ([]volume.MountPoint, error) {
-	for _, mnt := range container.NetworkMounts() {
-		dest, err := container.GetResourcePath(mnt.Destination)
-		if err != nil {
-			return nil, err
-		}
-		volumeMounts = append(volumeMounts, volume.MountPoint{Destination: dest})
-	}
-	return volumeMounts, nil
-}
-
 // NetworkMounts returns the list of network mounts.
 func (container *Container) NetworkMounts() []Mount {
 	var mounts []Mount
@@ -353,49 +341,37 @@ func (container *Container) UpdateContainer(hostConfig *containertypes.HostConfi
 	return nil
 }
 
-// UnmountVolumes unmounts all volumes
-func (container *Container) UnmountVolumes(forceSyscall bool, volumeEventLog func(name, action string, attributes map[string]string)) error {
-	var (
-		volumeMounts []volume.MountPoint
-		err          error
-	)
+// DetachAndUnmount uses a detached mount on all mount destinations, then
+// unmounts each volume normally.
+// This is used from daemon/archive for `docker cp`
+func (container *Container) DetachAndUnmount(volumeEventLog func(name, action string, attributes map[string]string)) error {
+	networkMounts := container.NetworkMounts()
+	mountPaths := make([]string, 0, len(container.MountPoints)+len(networkMounts))
 
 	for _, mntPoint := range container.MountPoints {
 		dest, err := container.GetResourcePath(mntPoint.Destination)
 		if err != nil {
-			return err
+			logrus.Warnf("Failed to get volume destination path for container '%s' at '%s' while lazily unmounting: %v", container.ID, mntPoint.Destination, err)
+			continue
 		}
-
-		volumeMounts = append(volumeMounts, volume.MountPoint{Destination: dest, Volume: mntPoint.Volume, ID: mntPoint.ID})
-	}
-
-	// Append any network mounts to the list (this is a no-op on Windows)
-	if volumeMounts, err = appendNetworkMounts(container, volumeMounts); err != nil {
-		return err
+		mountPaths = append(mountPaths, dest)
 	}
 
-	for _, volumeMount := range volumeMounts {
-		if forceSyscall {
-			if err := detachMounted(volumeMount.Destination); err != nil {
-				logrus.Warnf("%s unmountVolumes: Failed to do lazy umount %v", container.ID, err)
-			}
+	for _, m := range networkMounts {
+		dest, err := container.GetResourcePath(m.Destination)
+		if err != nil {
+			logrus.Warnf("Failed to get volume destination path for container '%s' at '%s' while lazily unmounting: %v", container.ID, m.Destination, err)
+			continue
 		}
+		mountPaths = append(mountPaths, dest)
+	}
 
-		if volumeMount.Volume != nil {
-			if err := volumeMount.Volume.Unmount(volumeMount.ID); err != nil {
-				return err
-			}
-			volumeMount.ID = ""
-
-			attributes := map[string]string{
-				"driver":    volumeMount.Volume.DriverName(),
-				"container": container.ID,
-			}
-			volumeEventLog(volumeMount.Volume.Name(), "unmount", attributes)
+	for _, mountPath := range mountPaths {
+		if err := detachMounted(mountPath); err != nil {
+			logrus.Warnf("%s unmountVolumes: Failed to do lazy umount fo volume '%s': %v", container.ID, mountPath, err)
 		}
 	}
-
-	return nil
+	return container.UnmountVolumes(volumeEventLog)
 }
 
 // copyExistingContents copies from the source to the destination and

container/container_windows.go

@@ -9,7 +9,6 @@ import (
 
 	containertypes "github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/utils"
-	"github.com/docker/docker/volume"
 )
 
 // Container holds fields specific to the Windows implementation. See
@@ -54,41 +53,11 @@ func (container *Container) UnmountSecrets() error {
 	return nil
 }
 
-// UnmountVolumes explicitly unmounts volumes from the container.
-func (container *Container) UnmountVolumes(forceSyscall bool, volumeEventLog func(name, action string, attributes map[string]string)) error {
-	var (
-		volumeMounts []volume.MountPoint
-		err          error
-	)
-
-	for _, mntPoint := range container.MountPoints {
-		// Do not attempt to get the mountpoint destination on the host as it
-		// is not accessible on Windows in the case that a container is running.
-		// (It's a special reparse point which doesn't have any contextual meaning).
-		volumeMounts = append(volumeMounts, volume.MountPoint{Volume: mntPoint.Volume, ID: mntPoint.ID})
-	}
-
-	// atm, this is a no-op.
-	if volumeMounts, err = appendNetworkMounts(container, volumeMounts); err != nil {
-		return err
-	}
-
-	for _, volumeMount := range volumeMounts {
-		if volumeMount.Volume != nil {
-			if err := volumeMount.Volume.Unmount(volumeMount.ID); err != nil {
-				return err
-			}
-			volumeMount.ID = ""
-
-			attributes := map[string]string{
-				"driver":    volumeMount.Volume.DriverName(),
-				"container": container.ID,
-			}
-			volumeEventLog(volumeMount.Volume.Name(), "unmount", attributes)
-		}
-	}
-
-	return nil
+// DetachAndUnmount unmounts all volumes.
+// On Windows it only delegates to `UnmountVolumes` since there is nothing to
+// force unmount.
+func (container *Container) DetachAndUnmount(volumeEventLog func(name, action string, attributes map[string]string)) error {
+	return container.UnmountVolumes(volumeEventLog)
 }
 
 // TmpfsMounts returns the list of tmpfs mounts
@@ -119,13 +88,6 @@ func (container *Container) UpdateContainer(hostConfig *containertypes.HostConfi
 	return nil
 }
 
-// appendNetworkMounts appends any network mounts to the array of mount points passed in.
-// Windows does not support network mounts (not to be confused with SMB network mounts), so
-// this is a no-op.
-func appendNetworkMounts(container *Container, volumeMounts []volume.MountPoint) ([]volume.MountPoint, error) {
-	return volumeMounts, nil
-}
-
 // cleanResourcePath cleans a resource path by removing C:\ syntax, and prepares
 // to combine with a volume path
 func cleanResourcePath(path string) string {

daemon/archive.go

@@ -87,7 +87,7 @@ func (daemon *Daemon) containerStatPath(container *container.Container, path str
 	defer daemon.Unmount(container)
 
 	err = daemon.mountVolumes(container)
-	defer container.UnmountVolumes(true, daemon.LogVolumeEvent)
+	defer container.DetachAndUnmount(daemon.LogVolumeEvent)
 	if err != nil {
 		return nil, err
 	}
@@ -122,7 +122,7 @@ func (daemon *Daemon) containerArchivePath(container *container.Container, path
 	defer func() {
 		if err != nil {
 			// unmount any volumes
-			container.UnmountVolumes(true, daemon.LogVolumeEvent)
+			container.DetachAndUnmount(daemon.LogVolumeEvent)
 			// unmount the container's rootfs
 			daemon.Unmount(container)
 		}
@@ -157,7 +157,7 @@ func (daemon *Daemon) containerArchivePath(container *container.Container, path
 
 	content = ioutils.NewReadCloserWrapper(data, func() error {
 		err := data.Close()
-		container.UnmountVolumes(true, daemon.LogVolumeEvent)
+		container.DetachAndUnmount(daemon.LogVolumeEvent)
 		daemon.Unmount(container)
 		container.Unlock()
 		return err
@@ -184,7 +184,7 @@ func (daemon *Daemon) containerExtractToDir(container *container.Container, path
 	defer daemon.Unmount(container)
 
 	err = daemon.mountVolumes(container)
-	defer container.UnmountVolumes(true, daemon.LogVolumeEvent)
+	defer container.DetachAndUnmount(daemon.LogVolumeEvent)
 	if err != nil {
 		return err
 	}
@@ -292,7 +292,7 @@ func (daemon *Daemon) containerCopy(container *container.Container, resource str
 	defer func() {
 		if err != nil {
 			// unmount any volumes
-			container.UnmountVolumes(true, daemon.LogVolumeEvent)
+			container.DetachAndUnmount(daemon.LogVolumeEvent)
 			// unmount the container's rootfs
 			daemon.Unmount(container)
 		}
@@ -329,7 +329,7 @@ func (daemon *Daemon) containerCopy(container *container.Container, resource str
 
 	reader := ioutils.NewReadCloserWrapper(archive, func() error {
 		err := archive.Close()
-		container.UnmountVolumes(true, daemon.LogVolumeEvent)
+		container.DetachAndUnmount(daemon.LogVolumeEvent)
 		daemon.Unmount(container)
 		container.Unlock()
 		return err

daemon/start.go

@@ -221,7 +221,7 @@ func (daemon *Daemon) Cleanup(container *container.Container) {
 	}
 
 	if container.BaseFS != "" {
-		if err := container.UnmountVolumes(false, daemon.LogVolumeEvent); err != nil {
+		if err := container.UnmountVolumes(daemon.LogVolumeEvent); err != nil {
 			logrus.Warnf("%s cleanup: Failed to umount volumes: %v", container.ID, err)
 		}
 	}

integration-cli/docker_cli_external_volume_driver_unix_test.go

@@ -73,6 +73,7 @@ type vol struct {
 	Mountpoint string
 	Ninja      bool // hack used to trigger a null volume return on `Get`
 	Status     map[string]interface{}
+	Options    map[string]string
 }
 
 func (p *volumePlugin) Close() {
@@ -130,7 +131,7 @@ func newVolumePlugin(c *check.C, name string) *volumePlugin {
 		}
 		_, isNinja := pr.Opts["ninja"]
 		status := map[string]interface{}{"Hello": "world"}
-		s.vols[pr.Name] = vol{Name: pr.Name, Ninja: isNinja, Status: status}
+		s.vols[pr.Name] = vol{Name: pr.Name, Ninja: isNinja, Status: status, Options: pr.Opts}
 		send(w, nil)
 	})
 
@@ -212,6 +213,14 @@ func newVolumePlugin(c *check.C, name string) *volumePlugin {
 			return
 		}
 
+		if v, exists := s.vols[pr.Name]; exists {
+			// Use this to simulate a mount failure
+			if _, exists := v.Options["invalidOption"]; exists {
+				send(w, fmt.Errorf("invalid argument"))
+				return
+			}
+		}
+
 		p := hostVolumePath(pr.Name)
 		if err := os.MkdirAll(p, 0755); err != nil {
 			send(w, &pluginResp{Err: err.Error()})
@@ -562,3 +571,13 @@ func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverOutOfBandDelete(c *c
 	out, err = s.d.Cmd("volume", "create", "-d", "local", "--name", "test")
 	c.Assert(err, checker.IsNil, check.Commentf(out))
 }
+
+func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverUnmountOnMountFail(c *check.C) {
+	c.Assert(s.d.StartWithBusybox(), checker.IsNil)
+	s.d.Cmd("volume", "create", "-d", "test-external-volume-driver", "--opt=invalidOption=1", "--name=testumount")
+
+	out, _ := s.d.Cmd("run", "-v", "testumount:/foo", "busybox", "true")
+	c.Assert(s.ec.unmounts, checker.Equals, 0, check.Commentf(out))
+	out, _ = s.d.Cmd("run", "-w", "/foo", "-v", "testumount:/foo", "busybox", "true")
+	c.Assert(s.ec.unmounts, checker.Equals, 0, check.Commentf(out))
+}

volume/volume.go

@@ -80,17 +80,33 @@ type DetailedVolume interface {
 // specifies which volume is to be used and where inside a container it should
 // be mounted.
 type MountPoint struct {
-	Source      string          // Container host directory
-	Destination string          // Inside the container
-	RW          bool            // True if writable
-	Name        string          // Name set by user
-	Driver      string          // Volume driver to use
-	Type        mounttypes.Type `json:",omitempty"` // Type of mount to use, see `Type<foo>` definitions
-	Volume      Volume          `json:"-"`
+	// Source is the source path of the mount.
+	// E.g. `mount --bind /foo /bar`, `/foo` is the `Source`.
+	Source string
+	// Destination is the path relative to the container root (`/`) to the mount point
+	// It is where the `Source` is mounted to
+	Destination string
+	// RW is set to true when the mountpoint should be mounted as read-write
+	RW bool
+	// Name is the name reference to the underlying data defined by `Source`
+	// e.g., the volume name
+	Name string
+	// Driver is the volume driver used to create the volume (if it is a volume)
+	Driver string
+	// Type of mount to use, see `Type<foo>` definitions in github.com/docker/docker/api/types/mount
+	Type mounttypes.Type `json:",omitempty"`
+	// Volume is the volume providing data to this mountpoint.
+	// This is nil unless `Type` is set to `TypeVolume`
+	Volume Volume `json:"-"`
 
+	// Mode is the comma separated list of options supplied by the user when creating
+	// the bind/volume mount.
 	// Note Mode is not used on Windows
 	Mode string `json:"Relabel,omitempty"` // Originally field was `Relabel`"
 
+	// Propagation describes how the mounts are propagated from the host into the
+	// mount point, and vice-versa.
+	// See https://www.kernel.org/doc/Documentation/filesystems/sharedsubtree.txt
 	// Note Propagation is not used on Windows
 	Propagation mounttypes.Propagation `json:",omitempty"` // Mount propagation string
 
@@ -100,7 +116,9 @@ type MountPoint struct {
 	CopyData bool `json:"-"`
 	// ID is the opaque ID used to pass to the volume driver.
 	// This should be set by calls to `Mount` and unset by calls to `Unmount`
-	ID   string `json:",omitempty"`
+	ID string `json:",omitempty"`
+
+	// Sepc is a copy of the API request that created this mount.
 	Spec mounttypes.Mount
 }
 
@@ -108,11 +126,16 @@ type MountPoint struct {
 // configured, or creating the source directory if supplied.
 func (m *MountPoint) Setup(mountLabel string, rootUID, rootGID int) (string, error) {
 	if m.Volume != nil {
-		if m.ID == "" {
-			m.ID = stringid.GenerateNonCryptoID()
+		id := m.ID
+		if id == "" {
+			id = stringid.GenerateNonCryptoID()
+		}
+		path, err := m.Volume.Mount(id)
+		if err != nil {
+			return "", errors.Wrapf(err, "error while mounting volume '%s'", m.Source)
 		}
-		path, err := m.Volume.Mount(m.ID)
-		return path, errors.Wrapf(err, "error while mounting volume '%s'", m.Source)
+		m.ID = id
+		return path, nil
 	}
 	if len(m.Source) == 0 {
 		return "", fmt.Errorf("Unable to setup mount point, neither source nor volume defined")