diff --git a/components/engine/container/container.go b/components/engine/container/container.go index 74d080d46c..2e033dffaa 100644 --- a/components/engine/container/container.go +++ b/components/engine/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 diff --git a/components/engine/container/container_unix.go b/components/engine/container/container_unix.go index f02be89e78..6e9c251400 100644 --- a/components/engine/container/container_unix.go +++ b/components/engine/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}) + mountPaths = append(mountPaths, dest) } - // Append any network mounts to the list (this is a no-op on Windows) - if volumeMounts, err = appendNetworkMounts(container, volumeMounts); err != nil { - return 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) } - 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) - } - } - - 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 diff --git a/components/engine/container/container_windows.go b/components/engine/container/container_windows.go index 4dbd02b4a7..b24aa3d845 100644 --- a/components/engine/container/container_windows.go +++ b/components/engine/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 { diff --git a/components/engine/daemon/archive.go b/components/engine/daemon/archive.go index 955e7b9b36..1999f1243b 100644 --- a/components/engine/daemon/archive.go +++ b/components/engine/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 diff --git a/components/engine/daemon/start.go b/components/engine/daemon/start.go index af08ccdf39..807fe569db 100644 --- a/components/engine/daemon/start.go +++ b/components/engine/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) } } diff --git a/components/engine/integration-cli/docker_cli_external_volume_driver_unix_test.go b/components/engine/integration-cli/docker_cli_external_volume_driver_unix_test.go index 30645bfaa6..2abd36389d 100644 --- a/components/engine/integration-cli/docker_cli_external_volume_driver_unix_test.go +++ b/components/engine/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)) +} diff --git a/components/engine/volume/volume.go b/components/engine/volume/volume.go index 39e6b08034..f3227fe485 100644 --- a/components/engine/volume/volume.go +++ b/components/engine/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` 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` 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(m.ID) - return path, errors.Wrapf(err, "error while mounting volume '%s'", m.Source) + path, err := m.Volume.Mount(id) + if err != nil { + return "", 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")