diff --git a/api/server/router/volume/volume_routes.go b/api/server/router/volume/volume_routes.go index 04c6b0d654..9bbe13687f 100644 --- a/api/server/router/volume/volume_routes.go +++ b/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 } diff --git a/daemon/cluster/volumes.go b/daemon/cluster/volumes.go index 6f7b085697..7b53dd4691 100644 --- a/daemon/cluster/volumes.go +++ b/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 } diff --git a/integration/volume/volume_test.go b/integration/volume/volume_test.go index 87392620a4..ecdac18daa 100644 --- a/integration/volume/volume_test.go +++ b/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")) - - err = client.ContainerRemove(ctx, id, types.ContainerRemoveOptions{ - Force: true, + 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")) }) - assert.NilError(t, err) - err = client.VolumeRemove(ctx, vname, false) + 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) + }) +} + +// 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) {