Browse code

Fix uneccessary calls to `volume.Unmount()` Fixes #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 <cpuguy83@gmail.com>

Brian Goff authored on 2016/10/04 02:53:06
Showing 7 changed files
... ...
@@ -567,6 +567,32 @@ func (container *Container) AddMountPointWithVolume(destination string, vol volu
567 567
 	}
568 568
 }
569 569
 
570
+// UnmountVolumes unmounts all volumes
571
+func (container *Container) UnmountVolumes(volumeEventLog func(name, action string, attributes map[string]string)) error {
572
+	var errors []string
573
+	for _, volumeMount := range container.MountPoints {
574
+		// Check if the mounpoint has an ID, this is currently the best way to tell if it's actually mounted
575
+		// TODO(cpuguyh83): there should be a better way to handle this
576
+		if volumeMount.Volume != nil && volumeMount.ID != "" {
577
+			if err := volumeMount.Volume.Unmount(volumeMount.ID); err != nil {
578
+				errors = append(errors, err.Error())
579
+				continue
580
+			}
581
+			volumeMount.ID = ""
582
+
583
+			attributes := map[string]string{
584
+				"driver":    volumeMount.Volume.DriverName(),
585
+				"container": container.ID,
586
+			}
587
+			volumeEventLog(volumeMount.Volume.Name(), "unmount", attributes)
588
+		}
589
+	}
590
+	if len(errors) > 0 {
591
+		return fmt.Errorf("error while unmounting volumes for container %s: %s", container.ID, strings.Join(errors, "; "))
592
+	}
593
+	return nil
594
+}
595
+
570 596
 // IsDestinationMounted checks whether a path is mounted on the container or not.
571 597
 func (container *Container) IsDestinationMounted(destination string) bool {
572 598
 	return container.MountPoints[destination] != nil
... ...
@@ -102,18 +102,6 @@ func (container *Container) BuildHostnameFile() error {
102 102
 	return ioutil.WriteFile(container.HostnamePath, []byte(container.Config.Hostname+"\n"), 0644)
103 103
 }
104 104
 
105
-// appendNetworkMounts appends any network mounts to the array of mount points passed in
106
-func appendNetworkMounts(container *Container, volumeMounts []volume.MountPoint) ([]volume.MountPoint, error) {
107
-	for _, mnt := range container.NetworkMounts() {
108
-		dest, err := container.GetResourcePath(mnt.Destination)
109
-		if err != nil {
110
-			return nil, err
111
-		}
112
-		volumeMounts = append(volumeMounts, volume.MountPoint{Destination: dest})
113
-	}
114
-	return volumeMounts, nil
115
-}
116
-
117 105
 // NetworkMounts returns the list of network mounts.
118 106
 func (container *Container) NetworkMounts() []Mount {
119 107
 	var mounts []Mount
... ...
@@ -353,49 +341,37 @@ func (container *Container) UpdateContainer(hostConfig *containertypes.HostConfi
353 353
 	return nil
354 354
 }
355 355
 
356
-// UnmountVolumes unmounts all volumes
357
-func (container *Container) UnmountVolumes(forceSyscall bool, volumeEventLog func(name, action string, attributes map[string]string)) error {
358
-	var (
359
-		volumeMounts []volume.MountPoint
360
-		err          error
361
-	)
356
+// DetachAndUnmount uses a detached mount on all mount destinations, then
357
+// unmounts each volume normally.
358
+// This is used from daemon/archive for `docker cp`
359
+func (container *Container) DetachAndUnmount(volumeEventLog func(name, action string, attributes map[string]string)) error {
360
+	networkMounts := container.NetworkMounts()
361
+	mountPaths := make([]string, 0, len(container.MountPoints)+len(networkMounts))
362 362
 
363 363
 	for _, mntPoint := range container.MountPoints {
364 364
 		dest, err := container.GetResourcePath(mntPoint.Destination)
365 365
 		if err != nil {
366
-			return err
366
+			logrus.Warnf("Failed to get volume destination path for container '%s' at '%s' while lazily unmounting: %v", container.ID, mntPoint.Destination, err)
367
+			continue
367 368
 		}
368
-
369
-		volumeMounts = append(volumeMounts, volume.MountPoint{Destination: dest, Volume: mntPoint.Volume, ID: mntPoint.ID})
370
-	}
371
-
372
-	// Append any network mounts to the list (this is a no-op on Windows)
373
-	if volumeMounts, err = appendNetworkMounts(container, volumeMounts); err != nil {
374
-		return err
369
+		mountPaths = append(mountPaths, dest)
375 370
 	}
376 371
 
377
-	for _, volumeMount := range volumeMounts {
378
-		if forceSyscall {
379
-			if err := detachMounted(volumeMount.Destination); err != nil {
380
-				logrus.Warnf("%s unmountVolumes: Failed to do lazy umount %v", container.ID, err)
381
-			}
372
+	for _, m := range networkMounts {
373
+		dest, err := container.GetResourcePath(m.Destination)
374
+		if err != nil {
375
+			logrus.Warnf("Failed to get volume destination path for container '%s' at '%s' while lazily unmounting: %v", container.ID, m.Destination, err)
376
+			continue
382 377
 		}
378
+		mountPaths = append(mountPaths, dest)
379
+	}
383 380
 
384
-		if volumeMount.Volume != nil {
385
-			if err := volumeMount.Volume.Unmount(volumeMount.ID); err != nil {
386
-				return err
387
-			}
388
-			volumeMount.ID = ""
389
-
390
-			attributes := map[string]string{
391
-				"driver":    volumeMount.Volume.DriverName(),
392
-				"container": container.ID,
393
-			}
394
-			volumeEventLog(volumeMount.Volume.Name(), "unmount", attributes)
381
+	for _, mountPath := range mountPaths {
382
+		if err := detachMounted(mountPath); err != nil {
383
+			logrus.Warnf("%s unmountVolumes: Failed to do lazy umount fo volume '%s': %v", container.ID, mountPath, err)
395 384
 		}
396 385
 	}
397
-
398
-	return nil
386
+	return container.UnmountVolumes(volumeEventLog)
399 387
 }
400 388
 
401 389
 // copyExistingContents copies from the source to the destination and
... ...
@@ -9,7 +9,6 @@ import (
9 9
 
10 10
 	containertypes "github.com/docker/docker/api/types/container"
11 11
 	"github.com/docker/docker/utils"
12
-	"github.com/docker/docker/volume"
13 12
 )
14 13
 
15 14
 // Container holds fields specific to the Windows implementation. See
... ...
@@ -54,41 +53,11 @@ func (container *Container) UnmountSecrets() error {
54 54
 	return nil
55 55
 }
56 56
 
57
-// UnmountVolumes explicitly unmounts volumes from the container.
58
-func (container *Container) UnmountVolumes(forceSyscall bool, volumeEventLog func(name, action string, attributes map[string]string)) error {
59
-	var (
60
-		volumeMounts []volume.MountPoint
61
-		err          error
62
-	)
63
-
64
-	for _, mntPoint := range container.MountPoints {
65
-		// Do not attempt to get the mountpoint destination on the host as it
66
-		// is not accessible on Windows in the case that a container is running.
67
-		// (It's a special reparse point which doesn't have any contextual meaning).
68
-		volumeMounts = append(volumeMounts, volume.MountPoint{Volume: mntPoint.Volume, ID: mntPoint.ID})
69
-	}
70
-
71
-	// atm, this is a no-op.
72
-	if volumeMounts, err = appendNetworkMounts(container, volumeMounts); err != nil {
73
-		return err
74
-	}
75
-
76
-	for _, volumeMount := range volumeMounts {
77
-		if volumeMount.Volume != nil {
78
-			if err := volumeMount.Volume.Unmount(volumeMount.ID); err != nil {
79
-				return err
80
-			}
81
-			volumeMount.ID = ""
82
-
83
-			attributes := map[string]string{
84
-				"driver":    volumeMount.Volume.DriverName(),
85
-				"container": container.ID,
86
-			}
87
-			volumeEventLog(volumeMount.Volume.Name(), "unmount", attributes)
88
-		}
89
-	}
90
-
91
-	return nil
57
+// DetachAndUnmount unmounts all volumes.
58
+// On Windows it only delegates to `UnmountVolumes` since there is nothing to
59
+// force unmount.
60
+func (container *Container) DetachAndUnmount(volumeEventLog func(name, action string, attributes map[string]string)) error {
61
+	return container.UnmountVolumes(volumeEventLog)
92 62
 }
93 63
 
94 64
 // TmpfsMounts returns the list of tmpfs mounts
... ...
@@ -119,13 +88,6 @@ func (container *Container) UpdateContainer(hostConfig *containertypes.HostConfi
119 119
 	return nil
120 120
 }
121 121
 
122
-// appendNetworkMounts appends any network mounts to the array of mount points passed in.
123
-// Windows does not support network mounts (not to be confused with SMB network mounts), so
124
-// this is a no-op.
125
-func appendNetworkMounts(container *Container, volumeMounts []volume.MountPoint) ([]volume.MountPoint, error) {
126
-	return volumeMounts, nil
127
-}
128
-
129 122
 // cleanResourcePath cleans a resource path by removing C:\ syntax, and prepares
130 123
 // to combine with a volume path
131 124
 func cleanResourcePath(path string) string {
... ...
@@ -87,7 +87,7 @@ func (daemon *Daemon) containerStatPath(container *container.Container, path str
87 87
 	defer daemon.Unmount(container)
88 88
 
89 89
 	err = daemon.mountVolumes(container)
90
-	defer container.UnmountVolumes(true, daemon.LogVolumeEvent)
90
+	defer container.DetachAndUnmount(daemon.LogVolumeEvent)
91 91
 	if err != nil {
92 92
 		return nil, err
93 93
 	}
... ...
@@ -122,7 +122,7 @@ func (daemon *Daemon) containerArchivePath(container *container.Container, path
122 122
 	defer func() {
123 123
 		if err != nil {
124 124
 			// unmount any volumes
125
-			container.UnmountVolumes(true, daemon.LogVolumeEvent)
125
+			container.DetachAndUnmount(daemon.LogVolumeEvent)
126 126
 			// unmount the container's rootfs
127 127
 			daemon.Unmount(container)
128 128
 		}
... ...
@@ -157,7 +157,7 @@ func (daemon *Daemon) containerArchivePath(container *container.Container, path
157 157
 
158 158
 	content = ioutils.NewReadCloserWrapper(data, func() error {
159 159
 		err := data.Close()
160
-		container.UnmountVolumes(true, daemon.LogVolumeEvent)
160
+		container.DetachAndUnmount(daemon.LogVolumeEvent)
161 161
 		daemon.Unmount(container)
162 162
 		container.Unlock()
163 163
 		return err
... ...
@@ -184,7 +184,7 @@ func (daemon *Daemon) containerExtractToDir(container *container.Container, path
184 184
 	defer daemon.Unmount(container)
185 185
 
186 186
 	err = daemon.mountVolumes(container)
187
-	defer container.UnmountVolumes(true, daemon.LogVolumeEvent)
187
+	defer container.DetachAndUnmount(daemon.LogVolumeEvent)
188 188
 	if err != nil {
189 189
 		return err
190 190
 	}
... ...
@@ -292,7 +292,7 @@ func (daemon *Daemon) containerCopy(container *container.Container, resource str
292 292
 	defer func() {
293 293
 		if err != nil {
294 294
 			// unmount any volumes
295
-			container.UnmountVolumes(true, daemon.LogVolumeEvent)
295
+			container.DetachAndUnmount(daemon.LogVolumeEvent)
296 296
 			// unmount the container's rootfs
297 297
 			daemon.Unmount(container)
298 298
 		}
... ...
@@ -329,7 +329,7 @@ func (daemon *Daemon) containerCopy(container *container.Container, resource str
329 329
 
330 330
 	reader := ioutils.NewReadCloserWrapper(archive, func() error {
331 331
 		err := archive.Close()
332
-		container.UnmountVolumes(true, daemon.LogVolumeEvent)
332
+		container.DetachAndUnmount(daemon.LogVolumeEvent)
333 333
 		daemon.Unmount(container)
334 334
 		container.Unlock()
335 335
 		return err
... ...
@@ -221,7 +221,7 @@ func (daemon *Daemon) Cleanup(container *container.Container) {
221 221
 	}
222 222
 
223 223
 	if container.BaseFS != "" {
224
-		if err := container.UnmountVolumes(false, daemon.LogVolumeEvent); err != nil {
224
+		if err := container.UnmountVolumes(daemon.LogVolumeEvent); err != nil {
225 225
 			logrus.Warnf("%s cleanup: Failed to umount volumes: %v", container.ID, err)
226 226
 		}
227 227
 	}
... ...
@@ -73,6 +73,7 @@ type vol struct {
73 73
 	Mountpoint string
74 74
 	Ninja      bool // hack used to trigger a null volume return on `Get`
75 75
 	Status     map[string]interface{}
76
+	Options    map[string]string
76 77
 }
77 78
 
78 79
 func (p *volumePlugin) Close() {
... ...
@@ -130,7 +131,7 @@ func newVolumePlugin(c *check.C, name string) *volumePlugin {
130 130
 		}
131 131
 		_, isNinja := pr.Opts["ninja"]
132 132
 		status := map[string]interface{}{"Hello": "world"}
133
-		s.vols[pr.Name] = vol{Name: pr.Name, Ninja: isNinja, Status: status}
133
+		s.vols[pr.Name] = vol{Name: pr.Name, Ninja: isNinja, Status: status, Options: pr.Opts}
134 134
 		send(w, nil)
135 135
 	})
136 136
 
... ...
@@ -212,6 +213,14 @@ func newVolumePlugin(c *check.C, name string) *volumePlugin {
212 212
 			return
213 213
 		}
214 214
 
215
+		if v, exists := s.vols[pr.Name]; exists {
216
+			// Use this to simulate a mount failure
217
+			if _, exists := v.Options["invalidOption"]; exists {
218
+				send(w, fmt.Errorf("invalid argument"))
219
+				return
220
+			}
221
+		}
222
+
215 223
 		p := hostVolumePath(pr.Name)
216 224
 		if err := os.MkdirAll(p, 0755); err != nil {
217 225
 			send(w, &pluginResp{Err: err.Error()})
... ...
@@ -562,3 +571,13 @@ func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverOutOfBandDelete(c *c
562 562
 	out, err = s.d.Cmd("volume", "create", "-d", "local", "--name", "test")
563 563
 	c.Assert(err, checker.IsNil, check.Commentf(out))
564 564
 }
565
+
566
+func (s *DockerExternalVolumeSuite) TestExternalVolumeDriverUnmountOnMountFail(c *check.C) {
567
+	c.Assert(s.d.StartWithBusybox(), checker.IsNil)
568
+	s.d.Cmd("volume", "create", "-d", "test-external-volume-driver", "--opt=invalidOption=1", "--name=testumount")
569
+
570
+	out, _ := s.d.Cmd("run", "-v", "testumount:/foo", "busybox", "true")
571
+	c.Assert(s.ec.unmounts, checker.Equals, 0, check.Commentf(out))
572
+	out, _ = s.d.Cmd("run", "-w", "/foo", "-v", "testumount:/foo", "busybox", "true")
573
+	c.Assert(s.ec.unmounts, checker.Equals, 0, check.Commentf(out))
574
+}
... ...
@@ -80,17 +80,33 @@ type DetailedVolume interface {
80 80
 // specifies which volume is to be used and where inside a container it should
81 81
 // be mounted.
82 82
 type MountPoint struct {
83
-	Source      string          // Container host directory
84
-	Destination string          // Inside the container
85
-	RW          bool            // True if writable
86
-	Name        string          // Name set by user
87
-	Driver      string          // Volume driver to use
88
-	Type        mounttypes.Type `json:",omitempty"` // Type of mount to use, see `Type<foo>` definitions
89
-	Volume      Volume          `json:"-"`
83
+	// Source is the source path of the mount.
84
+	// E.g. `mount --bind /foo /bar`, `/foo` is the `Source`.
85
+	Source string
86
+	// Destination is the path relative to the container root (`/`) to the mount point
87
+	// It is where the `Source` is mounted to
88
+	Destination string
89
+	// RW is set to true when the mountpoint should be mounted as read-write
90
+	RW bool
91
+	// Name is the name reference to the underlying data defined by `Source`
92
+	// e.g., the volume name
93
+	Name string
94
+	// Driver is the volume driver used to create the volume (if it is a volume)
95
+	Driver string
96
+	// Type of mount to use, see `Type<foo>` definitions in github.com/docker/docker/api/types/mount
97
+	Type mounttypes.Type `json:",omitempty"`
98
+	// Volume is the volume providing data to this mountpoint.
99
+	// This is nil unless `Type` is set to `TypeVolume`
100
+	Volume Volume `json:"-"`
90 101
 
102
+	// Mode is the comma separated list of options supplied by the user when creating
103
+	// the bind/volume mount.
91 104
 	// Note Mode is not used on Windows
92 105
 	Mode string `json:"Relabel,omitempty"` // Originally field was `Relabel`"
93 106
 
107
+	// Propagation describes how the mounts are propagated from the host into the
108
+	// mount point, and vice-versa.
109
+	// See https://www.kernel.org/doc/Documentation/filesystems/sharedsubtree.txt
94 110
 	// Note Propagation is not used on Windows
95 111
 	Propagation mounttypes.Propagation `json:",omitempty"` // Mount propagation string
96 112
 
... ...
@@ -100,7 +116,9 @@ type MountPoint struct {
100 100
 	CopyData bool `json:"-"`
101 101
 	// ID is the opaque ID used to pass to the volume driver.
102 102
 	// This should be set by calls to `Mount` and unset by calls to `Unmount`
103
-	ID   string `json:",omitempty"`
103
+	ID string `json:",omitempty"`
104
+
105
+	// Sepc is a copy of the API request that created this mount.
104 106
 	Spec mounttypes.Mount
105 107
 }
106 108
 
... ...
@@ -108,11 +126,16 @@ type MountPoint struct {
108 108
 // configured, or creating the source directory if supplied.
109 109
 func (m *MountPoint) Setup(mountLabel string, rootUID, rootGID int) (string, error) {
110 110
 	if m.Volume != nil {
111
-		if m.ID == "" {
112
-			m.ID = stringid.GenerateNonCryptoID()
111
+		id := m.ID
112
+		if id == "" {
113
+			id = stringid.GenerateNonCryptoID()
114
+		}
115
+		path, err := m.Volume.Mount(id)
116
+		if err != nil {
117
+			return "", errors.Wrapf(err, "error while mounting volume '%s'", m.Source)
113 118
 		}
114
-		path, err := m.Volume.Mount(m.ID)
115
-		return path, errors.Wrapf(err, "error while mounting volume '%s'", m.Source)
119
+		m.ID = id
120
+		return path, nil
116 121
 	}
117 122
 	if len(m.Source) == 0 {
118 123
 		return "", fmt.Errorf("Unable to setup mount point, neither source nor volume defined")