Fix uneccessary calls to volume.Unmount()

Fixes moby#22564

When an error occurs on mount, there should not be any call later to
unmount. This can throw off refcounting in the underlying driver
unexpectedly.

Consider these two cases:

```
$ docker run -v foo:/bar busybox true
```

```
$ docker run -v foo:/bar -w /foo busybox true
```

In the first case, if mounting `foo` fails, the volume driver will not
get a call to unmount (this is the incorrect behavior).

In the second case, the volume driver will not get a call to unmount
(correct behavior).

This occurs because in the first case, `/bar` does not exist in the
container, and as such there is no call to `volume.Mount()` during the
`create` phase. It will error out during the `start` phase.

In the second case `/bar` is created before dealing with the volume
because of the `-w`. Because of this, when the volume is being setup
docker will try to copy the image path contents in the volume, in which
case it will attempt to mount the volume and fail. This happens during
the `create` phase. This makes it so the container will not be created
(or at least fully created) and the user gets the error on `create`
instead of `start`. The error handling is different in these two phases.

Changed to only send `unmount` if the volume is mounted.

While investigating the cause of the reported issue I found some odd
behavior in unmount calls so I've cleaned those up a bit here as well.

Signed-off-by: Brian Goff <[email protected]>

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,19 +116,26 @@ 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
 }
 
 // Setup sets up a mount point by either mounting the volume if it is
 // 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")