Browse code

Ensure unmount before removing local volume.

When there is an error unmounting a local volume, it is still possible
to call `Remove()` on the volume causing removal of the mounted
resources which is generally not desirable.

This ensures that resources are unmounted before attempting removal.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

Brian Goff authored on 2017/04/04 02:10:48
Showing 2 changed files
... ...
@@ -88,8 +88,8 @@ func (s *DockerDaemonSuite) TestDaemonRestartWithVolumesRefs(c *check.C) {
88 88
 
89 89
 	s.d.Restart(c)
90 90
 
91
-	if _, err := s.d.Cmd("run", "-d", "--volumes-from", "volrestarttest1", "--name", "volrestarttest2", "busybox", "top"); err != nil {
92
-		c.Fatal(err)
91
+	if out, err := s.d.Cmd("run", "-d", "--volumes-from", "volrestarttest1", "--name", "volrestarttest2", "busybox", "top"); err != nil {
92
+		c.Fatal(err, out)
93 93
 	}
94 94
 
95 95
 	if out, err := s.d.Cmd("rm", "-fv", "volrestarttest2"); err != nil {
... ...
@@ -218,6 +218,14 @@ func (r *Root) Remove(v volume.Volume) error {
218 218
 		return fmt.Errorf("unknown volume type %T", v)
219 219
 	}
220 220
 
221
+	if lv.active.count > 0 {
222
+		return fmt.Errorf("volume has active mounts")
223
+	}
224
+
225
+	if err := lv.unmount(); err != nil {
226
+		return err
227
+	}
228
+
221 229
 	realPath, err := filepath.EvalSymlinks(lv.path)
222 230
 	if err != nil {
223 231
 		if !os.IsNotExist(err) {
... ...
@@ -306,6 +314,7 @@ func (v *localVolume) Path() string {
306 306
 }
307 307
 
308 308
 // Mount implements the localVolume interface, returning the data location.
309
+// If there are any provided mount options, the resources will be mounted at this point
309 310
 func (v *localVolume) Mount(id string) (string, error) {
310 311
 	v.m.Lock()
311 312
 	defer v.m.Unlock()
... ...
@@ -321,19 +330,35 @@ func (v *localVolume) Mount(id string) (string, error) {
321 321
 	return v.path, nil
322 322
 }
323 323
 
324
-// Umount is for satisfying the localVolume interface and does not do anything in this driver.
324
+// Unmount dereferences the id, and if it is the last reference will unmount any resources
325
+// that were previously mounted.
325 326
 func (v *localVolume) Unmount(id string) error {
326 327
 	v.m.Lock()
327 328
 	defer v.m.Unlock()
329
+
330
+	// Always decrement the count, even if the unmount fails
331
+	// Essentially docker doesn't care if this fails, it will send an error, but
332
+	// ultimately there's nothing that can be done. If we don't decrement the count
333
+	// this volume can never be removed until a daemon restart occurs.
328 334
 	if v.opts != nil {
329 335
 		v.active.count--
330
-		if v.active.count == 0 {
331
-			if err := mount.Unmount(v.path); err != nil {
332
-				v.active.count++
336
+	}
337
+
338
+	if v.active.count > 0 {
339
+		return nil
340
+	}
341
+
342
+	return v.unmount()
343
+}
344
+
345
+func (v *localVolume) unmount() error {
346
+	if v.opts != nil {
347
+		if err := mount.Unmount(v.path); err != nil {
348
+			if mounted, mErr := mount.Mounted(v.path); mounted || mErr != nil {
333 349
 				return errors.Wrapf(err, "error while unmounting volume path '%s'", v.path)
334 350
 			}
335
-			v.active.mounted = false
336 351
 		}
352
+		v.active.mounted = false
337 353
 	}
338 354
 	return nil
339 355
 }