Merge pull request #45148 from thaJeztah/fix_volume_error_handling

volumes: fix error-handling when removing volumes with swarm enabled

api/server/router/volume/volume_routes.go

@@ -159,25 +159,29 @@ func (v *volumeRouter) deleteVolumes(ctx context.Context, w http.ResponseWriter,
 	}
 	force := httputils.BoolValue(r, "force")
 
-	version := httputils.VersionFromContext(ctx)
-
+	// First we try deleting local volume. The volume may not be found as a
+	// local volume, but could be a cluster volume, so we ignore "not found"
+	// errors at this stage. Note that no "not found" error is produced if
+	// "force" is enabled.
 	err := v.backend.Remove(ctx, vars["name"], opts.WithPurgeOnError(force))
-	// when a removal is forced, if the volume does not exist, no error will be
-	// returned. this means that to ensure forcing works on swarm volumes as
-	// well, we should always also force remove against the cluster.
-	if err != nil || force {
+	if err != nil && !errdefs.IsNotFound(err) {
+		return err
+	}
+
+	// If no volume was found, the volume may be a cluster volume. If force
+	// is enabled, the volume backend won't return an error for non-existing
+	// volumes, so we don't know if removal succeeded (or not volume existed).
+	// In that case we always try to delete cluster volumes as well.
+	if errdefs.IsNotFound(err) || force {
+		version := httputils.VersionFromContext(ctx)
 		if versions.GreaterThanOrEqualTo(version, clusterVolumesVersion) && v.cluster.IsManager() {
-			if errdefs.IsNotFound(err) || force {
-				err := v.cluster.RemoveVolume(vars["name"], force)
-				if err != nil {
-					return err
-				}
-			}
-		} else {
-			return err
+			err = v.cluster.RemoveVolume(vars["name"], force)
 		}
 	}
 
+	if err != nil {
+		return err
+	}
 	w.WriteHeader(http.StatusNoContent)
 	return nil
 }

daemon/cluster/volumes.go

@@ -90,6 +90,9 @@ func (c *Cluster) RemoveVolume(nameOrID string, force bool) error {
 	return c.lockedManagerAction(func(ctx context.Context, state nodeState) error {
 		volume, err := getVolume(ctx, state.controlClient, nameOrID)
 		if err != nil {
+			if force && errdefs.IsNotFound(err) {
+				return nil
+			}
 			return err
 		}
 

integration/volume/volume_test.go

@@ -13,12 +13,15 @@ import (
 	"github.com/docker/docker/api/types/filters"
 	"github.com/docker/docker/api/types/volume"
 	clientpkg "github.com/docker/docker/client"
+	"github.com/docker/docker/errdefs"
 	"github.com/docker/docker/integration/internal/container"
+	"github.com/docker/docker/testutil/daemon"
 	"github.com/docker/docker/testutil/request"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"gotest.tools/v3/assert"
 	"gotest.tools/v3/assert/cmp"
 	is "gotest.tools/v3/assert/cmp"
+	"gotest.tools/v3/skip"
 )
 
 func TestVolumesCreateAndList(t *testing.T) {
@@ -75,16 +78,83 @@ func TestVolumesRemove(t *testing.T) {
 	assert.NilError(t, err)
 	vname := c.Mounts[0].Name
 
-	err = client.VolumeRemove(ctx, vname, false)
-	assert.Check(t, is.ErrorContains(err, "volume is in use"))
+	t.Run("volume in use", func(t *testing.T) {
+		err = client.VolumeRemove(ctx, vname, false)
+		assert.Check(t, is.ErrorType(err, errdefs.IsConflict))
+		assert.Check(t, is.ErrorContains(err, "volume is in use"))
+	})
+
+	t.Run("volume not in use", func(t *testing.T) {
+		err = client.ContainerRemove(ctx, id, types.ContainerRemoveOptions{
+			Force: true,
+		})
+		assert.NilError(t, err)
 
-	err = client.ContainerRemove(ctx, id, types.ContainerRemoveOptions{
-		Force: true,
+		err = client.VolumeRemove(ctx, vname, false)
+		assert.NilError(t, err)
 	})
-	assert.NilError(t, err)
 
-	err = client.VolumeRemove(ctx, vname, false)
+	t.Run("non-existing volume", func(t *testing.T) {
+		err = client.VolumeRemove(ctx, "no_such_volume", false)
+		assert.Check(t, is.ErrorType(err, errdefs.IsNotFound))
+	})
+
+	t.Run("non-existing volume force", func(t *testing.T) {
+		err = client.VolumeRemove(ctx, "no_such_volume", true)
+		assert.NilError(t, err)
+	})
+}
+
+// TestVolumesRemoveSwarmEnabled tests that an error is returned if a volume
+// is in use, also if swarm is enabled (and cluster volumes are supported).
+//
+// Regression test for https://github.com/docker/cli/issues/4082
+func TestVolumesRemoveSwarmEnabled(t *testing.T) {
+	skip.If(t, testEnv.IsRemoteDaemon, "cannot run daemon when remote daemon")
+	skip.If(t, testEnv.OSType == "windows", "TODO enable on windows")
+	t.Parallel()
+	defer setupTest(t)()
+
+	// Spin up a new daemon, so that we can run this test in parallel (it's a slow test)
+	d := daemon.New(t)
+	d.StartAndSwarmInit(t)
+	defer d.Stop(t)
+
+	client := d.NewClientT(t)
+
+	ctx := context.Background()
+	prefix, slash := getPrefixAndSlashFromDaemonPlatform()
+	id := container.Create(ctx, t, client, container.WithVolume(prefix+slash+"foo"))
+
+	c, err := client.ContainerInspect(ctx, id)
 	assert.NilError(t, err)
+	vname := c.Mounts[0].Name
+
+	t.Run("volume in use", func(t *testing.T) {
+		err = client.VolumeRemove(ctx, vname, false)
+		assert.Check(t, is.ErrorType(err, errdefs.IsConflict))
+		assert.Check(t, is.ErrorContains(err, "volume is in use"))
+	})
+
+	t.Run("volume not in use", func(t *testing.T) {
+		err = client.ContainerRemove(ctx, id, types.ContainerRemoveOptions{
+			Force: true,
+		})
+		assert.NilError(t, err)
+
+		err = client.VolumeRemove(ctx, vname, false)
+		assert.NilError(t, err)
+	})
+
+	t.Run("non-existing volume", func(t *testing.T) {
+		err = client.VolumeRemove(ctx, "no_such_volume", false)
+		assert.Check(t, is.ErrorType(err, errdefs.IsNotFound))
+	})
+
+	t.Run("non-existing volume force", func(t *testing.T) {
+		err = client.VolumeRemove(ctx, "no_such_volume", true)
+		assert.NilError(t, err)
+	})
 }
 
 func TestVolumesInspect(t *testing.T) {