From 6c5c34d50d377d1c5318a255240fb2dc9c23cf92 Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Fri, 10 Jun 2016 07:40:09 -0700 Subject: [PATCH] Add `--force` in `docker volume rm` to fix out-of-band volume driver deletion This fix tries to address the issue in raised #23367 where an out-of-band volume driver deletion leaves some data in docker. This prevent the reuse of deleted volume names (by out-of-band volume driver like flocker). This fix adds a `--force` field in `docker volume rm` to forcefully purge the data of the volume that has already been deleted. Related documentations have been updated. This fix is tested manually with flocker, as is specified in #23367. An integration test has also been added for the scenario described. This fix fixes #23367. Signed-off-by: Yong Tang --- api/client/volume/remove.go | 26 ++++++++++---- api/server/router/volume/backend.go | 2 +- api/server/router/volume/volume_routes.go | 3 +- daemon/delete.go | 11 +++++- docs/reference/api/docker_remote_api.md | 1 + docs/reference/api/docker_remote_api_v1.25.md | 5 +++ docs/reference/commandline/volume_rm.md | 3 +- integration-cli/docker_cli_volume_test.go | 35 +++++++++++++++++++ volume/store/store.go | 6 ++-- 9 files changed, 80 insertions(+), 12 deletions(-) diff --git a/api/client/volume/remove.go b/api/client/volume/remove.go index b18fc991e6..9dcb19c397 100644 --- a/api/client/volume/remove.go +++ b/api/client/volume/remove.go @@ -10,27 +10,41 @@ import ( "github.com/spf13/cobra" ) +type removeOptions struct { + force bool + + volumes []string +} + func newRemoveCommand(dockerCli *client.DockerCli) *cobra.Command { - return &cobra.Command{ - Use: "rm VOLUME [VOLUME...]", + var opts removeOptions + + cmd := &cobra.Command{ + Use: "rm [OPTIONS] VOLUME [VOLUME]...", Aliases: []string{"remove"}, Short: "Remove one or more volumes", Long: removeDescription, Example: removeExample, Args: cli.RequiresMinArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - return runRemove(dockerCli, args) + opts.volumes = args + return runRemove(dockerCli, &opts) }, } + + flags := cmd.Flags() + flags.BoolVarP(&opts.force, "force", "f", false, "Force the removal of one or more volumes") + + return cmd } -func runRemove(dockerCli *client.DockerCli, volumes []string) error { +func runRemove(dockerCli *client.DockerCli, opts *removeOptions) error { client := dockerCli.Client() ctx := context.Background() status := 0 - for _, name := range volumes { - if err := client.VolumeRemove(ctx, name, false); err != nil { + for _, name := range opts.volumes { + if err := client.VolumeRemove(ctx, name, opts.force); err != nil { fmt.Fprintf(dockerCli.Err(), "%s\n", err) status = 1 continue diff --git a/api/server/router/volume/backend.go b/api/server/router/volume/backend.go index fbf5ed27f6..f9d1c64f17 100644 --- a/api/server/router/volume/backend.go +++ b/api/server/router/volume/backend.go @@ -11,5 +11,5 @@ type Backend interface { Volumes(filter string) ([]*types.Volume, []string, error) VolumeInspect(name string) (*types.Volume, error) VolumeCreate(name, driverName string, opts, labels map[string]string) (*types.Volume, error) - VolumeRm(name string) error + VolumeRm(name string, force bool) error } diff --git a/api/server/router/volume/volume_routes.go b/api/server/router/volume/volume_routes.go index 5aa0d4a7a7..737554c802 100644 --- a/api/server/router/volume/volume_routes.go +++ b/api/server/router/volume/volume_routes.go @@ -58,7 +58,8 @@ func (v *volumeRouter) deleteVolumes(ctx context.Context, w http.ResponseWriter, if err := httputils.ParseForm(r); err != nil { return err } - if err := v.backend.VolumeRm(vars["name"]); err != nil { + force := httputils.BoolValue(r, "force") + if err := v.backend.VolumeRm(vars["name"], force); err != nil { return err } w.WriteHeader(http.StatusNoContent) diff --git a/daemon/delete.go b/daemon/delete.go index a143cd0dc7..4aa1e2cc08 100644 --- a/daemon/delete.go +++ b/daemon/delete.go @@ -135,7 +135,16 @@ func (daemon *Daemon) cleanupContainer(container *container.Container, forceRemo // VolumeRm removes the volume with the given name. // If the volume is referenced by a container it is not removed // This is called directly from the remote API -func (daemon *Daemon) VolumeRm(name string) error { +func (daemon *Daemon) VolumeRm(name string, force bool) error { + err := daemon.volumeRm(name) + if err == nil || force { + daemon.volumes.Purge(name) + return nil + } + return err +} + +func (daemon *Daemon) volumeRm(name string) error { v, err := daemon.volumes.Get(name) if err != nil { return err diff --git a/docs/reference/api/docker_remote_api.md b/docs/reference/api/docker_remote_api.md index 42fc22546e..0647cfd789 100644 --- a/docs/reference/api/docker_remote_api.md +++ b/docs/reference/api/docker_remote_api.md @@ -119,6 +119,7 @@ This section lists each version from latest to oldest. Each listing includes a * `POST /containers/create` now takes `AutoRemove` in HostConfig, to enable auto-removal of the container on daemon side when the container's process exits. * `GET /containers/json` and `GET /containers/(id or name)/json` now return `"removing"` as a value for the `State.Status` field if the container is being removed. Previously, "exited" was returned as status. * `GET /containers/json` now accepts `removing` as a valid value for the `status` filter. +* `DELETE /volumes/(name)` now accepts a `force` query parameter to force removal of volumes that were already removed out of band by the volume driver plugin. ### v1.24 API changes diff --git a/docs/reference/api/docker_remote_api_v1.25.md b/docs/reference/api/docker_remote_api_v1.25.md index f63c447fbc..2f8c5b9b02 100644 --- a/docs/reference/api/docker_remote_api_v1.25.md +++ b/docs/reference/api/docker_remote_api_v1.25.md @@ -3062,6 +3062,11 @@ Instruct the driver to remove the volume (`name`). HTTP/1.1 204 No Content +**Query Parameters**: + +- **force** - 1/True/true or 0/False/false, Force the removal of the volume. + Default `false`. + **Status codes**: - **204** - no error diff --git a/docs/reference/commandline/volume_rm.md b/docs/reference/commandline/volume_rm.md index 52d7f067bf..aa66684259 100644 --- a/docs/reference/commandline/volume_rm.md +++ b/docs/reference/commandline/volume_rm.md @@ -11,7 +11,7 @@ parent = "smn_cli" # volume rm ```markdown -Usage: docker volume rm VOLUME [VOLUME...] +Usage: docker volume rm [OPTIONS] VOLUME [VOLUME...] Remove one or more volumes @@ -19,6 +19,7 @@ Aliases: rm, remove Options: + -f, --force Force the removal of one or more volumes --help Print usage ``` diff --git a/integration-cli/docker_cli_volume_test.go b/integration-cli/docker_cli_volume_test.go index bdbabb0644..dc081763da 100644 --- a/integration-cli/docker_cli_volume_test.go +++ b/integration-cli/docker_cli_volume_test.go @@ -374,3 +374,38 @@ func (s *DockerSuite) TestVolumeCliLsFilterLabels(c *check.C) { outArr = strings.Split(strings.TrimSpace(out), "\n") c.Assert(len(outArr), check.Equals, 1, check.Commentf("\n%s", out)) } + +func (s *DockerSuite) TestVolumeCliRmForceUsage(c *check.C) { + out, _ := dockerCmd(c, "volume", "create") + id := strings.TrimSpace(out) + + dockerCmd(c, "volume", "rm", "-f", id) + dockerCmd(c, "volume", "rm", "--force", "nonexist") + + out, _ = dockerCmd(c, "volume", "ls") + outArr := strings.Split(strings.TrimSpace(out), "\n") + c.Assert(len(outArr), check.Equals, 1, check.Commentf("%s\n", out)) +} + +func (s *DockerSuite) TestVolumeCliRmForce(c *check.C) { + testRequires(c, SameHostDaemon, DaemonIsLinux) + + name := "test" + out, _ := dockerCmd(c, "volume", "create", "--name", name) + id := strings.TrimSpace(out) + c.Assert(id, checker.Equals, name) + + out, _ = dockerCmd(c, "volume", "inspect", "--format", "{{.Mountpoint}}", name) + c.Assert(strings.TrimSpace(out), checker.Not(checker.Equals), "") + // Mountpoint is in the form of "/var/lib/docker/volumes/.../_data", removing `/_data` + path := strings.TrimSuffix(strings.TrimSpace(out), "/_data") + out, _, err := runCommandWithOutput(exec.Command("rm", "-rf", path)) + c.Assert(err, check.IsNil) + + dockerCmd(c, "volume", "rm", "-f", "test") + out, _ = dockerCmd(c, "volume", "ls") + c.Assert(out, checker.Not(checker.Contains), name) + dockerCmd(c, "volume", "create", "--name", "test") + out, _ = dockerCmd(c, "volume", "ls") + c.Assert(out, checker.Contains, name) +} diff --git a/volume/store/store.go b/volume/store/store.go index 9e2bd93d10..55fa7cf209 100644 --- a/volume/store/store.go +++ b/volume/store/store.go @@ -104,7 +104,9 @@ func (s *VolumeStore) setNamed(v volume.Volume, ref string) { s.globalLock.Unlock() } -func (s *VolumeStore) purge(name string) { +// Purge allows the cleanup of internal data on docker in case +// the internal data is out of sync with volumes driver plugins. +func (s *VolumeStore) Purge(name string) { s.globalLock.Lock() delete(s.names, name) delete(s.refs, name) @@ -425,7 +427,7 @@ func (s *VolumeStore) Remove(v volume.Volume) error { return &OpErr{Err: err, Name: name, Op: "remove"} } - s.purge(name) + s.Purge(name) return nil }