From 9d521a4d2fbbeec0f672b986e294fd5f1c4d2f32 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 1 Mar 2017 14:36:09 +0100 Subject: [PATCH] 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 --- daemon/delete.go | 16 ++++----- integration-cli/docker_cli_volume_test.go | 44 +++++++++++++++++++++-- 2 files changed, 50 insertions(+), 10 deletions(-) diff --git a/daemon/delete.go b/daemon/delete.go index 9227290357..c27cef0d3c 100644 --- a/daemon/delete.go +++ b/daemon/delete.go @@ -8,11 +8,12 @@ import ( "time" "github.com/Sirupsen/logrus" - "github.com/docker/docker/api/errors" + apierrors "github.com/docker/docker/api/errors" "github.com/docker/docker/api/types" "github.com/docker/docker/container" "github.com/docker/docker/layer" volumestore "github.com/docker/docker/volume/store" + "github.com/pkg/errors" ) // ContainerRm removes the container id from the filesystem. An error @@ -29,7 +30,7 @@ func (daemon *Daemon) ContainerRm(name string, config *types.ContainerRmConfig) // Container state RemovalInProgress should be used to avoid races. if inProgress := container.SetRemovalInProgress(); inProgress { err := fmt.Errorf("removal of container %s is already in progress", name) - return errors.NewBadRequestError(err) + return apierrors.NewBadRequestError(err) } defer container.ResetRemovalInProgress() @@ -80,7 +81,7 @@ func (daemon *Daemon) cleanupContainer(container *container.Container, forceRemo if container.IsRunning() { if !forceRemove { err := fmt.Errorf("You cannot remove a running container %s. Stop the container before attempting removal or use -f", container.ID) - return errors.NewRequestConflictError(err) + return apierrors.NewRequestConflictError(err) } if err := daemon.Kill(container); err != nil { 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 // This is called directly from the Engine API func (daemon *Daemon) VolumeRm(name string, force bool) error { err := daemon.volumeRm(name) + if err != nil && volumestore.IsInUse(err) { + return apierrors.NewRequestConflictError(err) + } if err == nil || force { daemon.volumes.Purge(name) return nil @@ -157,11 +161,7 @@ func (daemon *Daemon) volumeRm(name string) error { } if err := daemon.volumes.Remove(v); err != nil { - if volumestore.IsInUse(err) { - err := fmt.Errorf("Unable to remove volume, volume still in use: %v", err) - return errors.NewRequestConflictError(err) - } - return fmt.Errorf("Error while removing volume %s: %v", name, err) + return errors.Wrap(err, "unable to remove volume") } daemon.LogVolumeEvent(v.Name(), "destroy", map[string]string{"driver": v.DriverName()}) return nil diff --git a/integration-cli/docker_cli_volume_test.go b/integration-cli/docker_cli_volume_test.go index 2bdb063030..a6284aa94f 100644 --- a/integration-cli/docker_cli_volume_test.go +++ b/integration-cli/docker_cli_volume_test.go @@ -423,14 +423,54 @@ func (s *DockerSuite) TestVolumeCLIRmForce(c *check.C) { path := strings.TrimSuffix(strings.TrimSpace(out), "/_data") icmd.RunCommand("rm", "-rf", path).Assert(c, icmd.Success) - dockerCmd(c, "volume", "rm", "-f", "test") + dockerCmd(c, "volume", "rm", "-f", name) out, _ = dockerCmd(c, "volume", "ls") c.Assert(out, checker.Not(checker.Contains), name) - dockerCmd(c, "volume", "create", "test") + dockerCmd(c, "volume", "create", name) out, _ = dockerCmd(c, "volume", "ls") c.Assert(out, checker.Contains, name) } +// TestVolumeCLIRmForceInUse verifies that repeated `docker volume rm -f` calls does not remove a volume +// if it is in use. Test case for https://github.com/docker/docker/issues/31446 +func (s *DockerSuite) TestVolumeCLIRmForceInUse(c *check.C) { + name := "testvolume" + out, _ := dockerCmd(c, "volume", "create", name) + id := strings.TrimSpace(out) + c.Assert(id, checker.Equals, name) + + prefix, slash := getPrefixAndSlashFromDaemonPlatform() + out, e := dockerCmd(c, "create", "-v", "testvolume:"+prefix+slash+"foo", "busybox") + cid := strings.TrimSpace(out) + + _, _, err := dockerCmdWithError("volume", "rm", "-f", name) + c.Assert(err, check.NotNil) + c.Assert(err.Error(), checker.Contains, "volume is in use") + out, _ = dockerCmd(c, "volume", "ls") + c.Assert(out, checker.Contains, name) + + // The original issue did not _remove_ the volume from the list + // the first time. But a second call to `volume rm` removed it. + // Calling `volume rm` a second time to confirm it's not removed + // when calling twice. + _, _, err = dockerCmdWithError("volume", "rm", "-f", name) + c.Assert(err, check.NotNil) + c.Assert(err.Error(), checker.Contains, "volume is in use") + out, _ = dockerCmd(c, "volume", "ls") + c.Assert(out, checker.Contains, name) + + // Verify removing the volume after the container is removed works + _, e = dockerCmd(c, "rm", cid) + c.Assert(e, check.Equals, 0) + + _, e = dockerCmd(c, "volume", "rm", "-f", name) + c.Assert(e, check.Equals, 0) + + out, e = dockerCmd(c, "volume", "ls") + c.Assert(e, check.Equals, 0) + c.Assert(out, checker.Not(checker.Contains), name) +} + func (s *DockerSuite) TestVolumeCliInspectWithVolumeOpts(c *check.C) { testRequires(c, DaemonIsLinux)