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 <yong.tang.github@outlook.com>

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

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
 }

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)

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

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
 

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

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
 ```
 

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)
+}

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
 }