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>
... | ... |
@@ -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 |
... | ... |
@@ -423,14 +423,54 @@ func (s *DockerSuite) TestVolumeCLIRmForce(c *check.C) { |
423 | 423 |
path := strings.TrimSuffix(strings.TrimSpace(out), "/_data") |
424 | 424 |
icmd.RunCommand("rm", "-rf", path).Assert(c, icmd.Success) |
425 | 425 |
|
426 |
- dockerCmd(c, "volume", "rm", "-f", "test") |
|
426 |
+ dockerCmd(c, "volume", "rm", "-f", name) |
|
427 | 427 |
out, _ = dockerCmd(c, "volume", "ls") |
428 | 428 |
c.Assert(out, checker.Not(checker.Contains), name) |
429 |
- dockerCmd(c, "volume", "create", "test") |
|
429 |
+ dockerCmd(c, "volume", "create", name) |
|
430 | 430 |
out, _ = dockerCmd(c, "volume", "ls") |
431 | 431 |
c.Assert(out, checker.Contains, name) |
432 | 432 |
} |
433 | 433 |
|
434 |
+// TestVolumeCLIRmForceInUse verifies that repeated `docker volume rm -f` calls does not remove a volume |
|
435 |
+// if it is in use. Test case for https://github.com/docker/docker/issues/31446 |
|
436 |
+func (s *DockerSuite) TestVolumeCLIRmForceInUse(c *check.C) { |
|
437 |
+ name := "testvolume" |
|
438 |
+ out, _ := dockerCmd(c, "volume", "create", name) |
|
439 |
+ id := strings.TrimSpace(out) |
|
440 |
+ c.Assert(id, checker.Equals, name) |
|
441 |
+ |
|
442 |
+ prefix, slash := getPrefixAndSlashFromDaemonPlatform() |
|
443 |
+ out, e := dockerCmd(c, "create", "-v", "testvolume:"+prefix+slash+"foo", "busybox") |
|
444 |
+ cid := strings.TrimSpace(out) |
|
445 |
+ |
|
446 |
+ _, _, err := dockerCmdWithError("volume", "rm", "-f", name) |
|
447 |
+ c.Assert(err, check.NotNil) |
|
448 |
+ c.Assert(err.Error(), checker.Contains, "volume is in use") |
|
449 |
+ out, _ = dockerCmd(c, "volume", "ls") |
|
450 |
+ c.Assert(out, checker.Contains, name) |
|
451 |
+ |
|
452 |
+ // The original issue did not _remove_ the volume from the list |
|
453 |
+ // the first time. But a second call to `volume rm` removed it. |
|
454 |
+ // Calling `volume rm` a second time to confirm it's not removed |
|
455 |
+ // when calling twice. |
|
456 |
+ _, _, err = dockerCmdWithError("volume", "rm", "-f", name) |
|
457 |
+ c.Assert(err, check.NotNil) |
|
458 |
+ c.Assert(err.Error(), checker.Contains, "volume is in use") |
|
459 |
+ out, _ = dockerCmd(c, "volume", "ls") |
|
460 |
+ c.Assert(out, checker.Contains, name) |
|
461 |
+ |
|
462 |
+ // Verify removing the volume after the container is removed works |
|
463 |
+ _, e = dockerCmd(c, "rm", cid) |
|
464 |
+ c.Assert(e, check.Equals, 0) |
|
465 |
+ |
|
466 |
+ _, e = dockerCmd(c, "volume", "rm", "-f", name) |
|
467 |
+ c.Assert(e, check.Equals, 0) |
|
468 |
+ |
|
469 |
+ out, e = dockerCmd(c, "volume", "ls") |
|
470 |
+ c.Assert(e, check.Equals, 0) |
|
471 |
+ c.Assert(out, checker.Not(checker.Contains), name) |
|
472 |
+} |
|
473 |
+ |
|
434 | 474 |
func (s *DockerSuite) TestVolumeCliInspectWithVolumeOpts(c *check.C) { |
435 | 475 |
testRequires(c, DaemonIsLinux) |
436 | 476 |
|