Browse code

do not ignore "volume in use" errors when force-delete

When using `docker volume rm -f`, all errors were ignored,
and volumes where Purged, even if they were still in
use by a container.

As a result, repeated calls to `docker volume rm -f`
actually removed the volume.

The `-f` option was implemented to ignore errors
in case a volume was already removed out-of-band
by a volume driver plugin.

This patch changes the remove function to not
ignore "volume in use" errors if `-f` is used.
Other errors are still ignored as before.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 9d521a4d2fbbeec0f672b986e294fd5f1c4d2f32)
Signed-off-by: Victor Vieux <victorvieux@gmail.com>

Sebastiaan van Stijn authored on 2017/03/01 22:36:09
Showing 2 changed files
... ...
@@ -8,11 +8,12 @@ import (
8 8
 	"time"
9 9
 
10 10
 	"github.com/Sirupsen/logrus"
11
-	"github.com/docker/docker/api/errors"
11
+	apierrors "github.com/docker/docker/api/errors"
12 12
 	"github.com/docker/docker/api/types"
13 13
 	"github.com/docker/docker/container"
14 14
 	"github.com/docker/docker/layer"
15 15
 	volumestore "github.com/docker/docker/volume/store"
16
+	"github.com/pkg/errors"
16 17
 )
17 18
 
18 19
 // ContainerRm removes the container id from the filesystem. An error
... ...
@@ -29,7 +30,7 @@ func (daemon *Daemon) ContainerRm(name string, config *types.ContainerRmConfig)
29 29
 	// Container state RemovalInProgress should be used to avoid races.
30 30
 	if inProgress := container.SetRemovalInProgress(); inProgress {
31 31
 		err := fmt.Errorf("removal of container %s is already in progress", name)
32
-		return errors.NewBadRequestError(err)
32
+		return apierrors.NewBadRequestError(err)
33 33
 	}
34 34
 	defer container.ResetRemovalInProgress()
35 35
 
... ...
@@ -80,7 +81,7 @@ func (daemon *Daemon) cleanupContainer(container *container.Container, forceRemo
80 80
 	if container.IsRunning() {
81 81
 		if !forceRemove {
82 82
 			err := fmt.Errorf("You cannot remove a running container %s. Stop the container before attempting removal or use -f", container.ID)
83
-			return errors.NewRequestConflictError(err)
83
+			return apierrors.NewRequestConflictError(err)
84 84
 		}
85 85
 		if err := daemon.Kill(container); err != nil {
86 86
 			return fmt.Errorf("Could not kill running container %s, cannot remove - %v", container.ID, err)
... ...
@@ -143,6 +144,9 @@ func (daemon *Daemon) cleanupContainer(container *container.Container, forceRemo
143 143
 // This is called directly from the Engine API
144 144
 func (daemon *Daemon) VolumeRm(name string, force bool) error {
145 145
 	err := daemon.volumeRm(name)
146
+	if err != nil && volumestore.IsInUse(err) {
147
+		return apierrors.NewRequestConflictError(err)
148
+	}
146 149
 	if err == nil || force {
147 150
 		daemon.volumes.Purge(name)
148 151
 		return nil
... ...
@@ -157,11 +161,7 @@ func (daemon *Daemon) volumeRm(name string) error {
157 157
 	}
158 158
 
159 159
 	if err := daemon.volumes.Remove(v); err != nil {
160
-		if volumestore.IsInUse(err) {
161
-			err := fmt.Errorf("Unable to remove volume, volume still in use: %v", err)
162
-			return errors.NewRequestConflictError(err)
163
-		}
164
-		return fmt.Errorf("Error while removing volume %s: %v", name, err)
160
+		return errors.Wrap(err, "unable to remove volume")
165 161
 	}
166 162
 	daemon.LogVolumeEvent(v.Name(), "destroy", map[string]string{"driver": v.DriverName()})
167 163
 	return nil
... ...
@@ -398,14 +398,54 @@ func (s *DockerSuite) TestVolumeCLIRmForce(c *check.C) {
398 398
 	out, _, err := runCommandWithOutput(exec.Command("rm", "-rf", path))
399 399
 	c.Assert(err, check.IsNil)
400 400
 
401
-	dockerCmd(c, "volume", "rm", "-f", "test")
401
+	dockerCmd(c, "volume", "rm", "-f", name)
402 402
 	out, _ = dockerCmd(c, "volume", "ls")
403 403
 	c.Assert(out, checker.Not(checker.Contains), name)
404
-	dockerCmd(c, "volume", "create", "test")
404
+	dockerCmd(c, "volume", "create", name)
405 405
 	out, _ = dockerCmd(c, "volume", "ls")
406 406
 	c.Assert(out, checker.Contains, name)
407 407
 }
408 408
 
409
+// TestVolumeCLIRmForceInUse verifies that repeated `docker volume rm -f` calls does not remove a volume
410
+// if it is in use. Test case for https://github.com/docker/docker/issues/31446
411
+func (s *DockerSuite) TestVolumeCLIRmForceInUse(c *check.C) {
412
+	name := "testvolume"
413
+	out, _ := dockerCmd(c, "volume", "create", name)
414
+	id := strings.TrimSpace(out)
415
+	c.Assert(id, checker.Equals, name)
416
+
417
+	prefix, slash := getPrefixAndSlashFromDaemonPlatform()
418
+	out, e := dockerCmd(c, "create", "-v", "testvolume:"+prefix+slash+"foo", "busybox")
419
+	cid := strings.TrimSpace(out)
420
+
421
+	_, _, err := dockerCmdWithError("volume", "rm", "-f", name)
422
+	c.Assert(err, check.NotNil)
423
+	c.Assert(err.Error(), checker.Contains, "volume is in use")
424
+	out, _ = dockerCmd(c, "volume", "ls")
425
+	c.Assert(out, checker.Contains, name)
426
+
427
+	// The original issue did not _remove_ the volume from the list
428
+	// the first time. But a second call to `volume rm` removed it.
429
+	// Calling `volume rm` a second time to confirm it's not removed
430
+	// when calling twice.
431
+	_, _, err = dockerCmdWithError("volume", "rm", "-f", name)
432
+	c.Assert(err, check.NotNil)
433
+	c.Assert(err.Error(), checker.Contains, "volume is in use")
434
+	out, _ = dockerCmd(c, "volume", "ls")
435
+	c.Assert(out, checker.Contains, name)
436
+
437
+	// Verify removing the volume after the container is removed works
438
+	_, e = dockerCmd(c, "rm", cid)
439
+	c.Assert(e, check.Equals, 0)
440
+
441
+	_, e = dockerCmd(c, "volume", "rm", "-f", name)
442
+	c.Assert(e, check.Equals, 0)
443
+
444
+	out, e = dockerCmd(c, "volume", "ls")
445
+	c.Assert(e, check.Equals, 0)
446
+	c.Assert(out, checker.Not(checker.Contains), name)
447
+}
448
+
409 449
 func (s *DockerSuite) TestVolumeCliInspectWithVolumeOpts(c *check.C) {
410 450
 	testRequires(c, DaemonIsLinux)
411 451