volumes: fix error-handling when removing volumes with swarm enabled by thaJeztah · Pull Request #45148 · moby/moby

thaJeztah · 2023-03-12T14:06:10Z

integration/volumes: TestVolumesRemove: add coverage for force/no-force

Add additional test-cases for deleting non-existing volumes (with/without force).

With this patch:

make TEST_FILTER=TestVolumesRemove DOCKER_GRAPHDRIVER=vfs test-integration

Running /go/src/github.com/docker/docker/integration/volume (arm64.integration.volume) flags=-test.v -test.timeout=10m  -test.run TestVolumesRemove
...
=== RUN   TestVolumesRemove
=== RUN   TestVolumesRemove/volume_in_use
=== RUN   TestVolumesRemove/volume_not_in_use
=== RUN   TestVolumesRemove/non-existing_volume
=== RUN   TestVolumesRemove/non-existing_volume_force
--- PASS: TestVolumesRemove (0.04s)
    --- PASS: TestVolumesRemove/volume_in_use (0.00s)
    --- PASS: TestVolumesRemove/volume_not_in_use (0.01s)
    --- PASS: TestVolumesRemove/non-existing_volume (0.00s)
    --- PASS: TestVolumesRemove/non-existing_volume_force (0.00s)
PASS

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

Commit 3246db3 (#44224) added handling for removing
cluster volumes, but in some conditions, this resulted in errors not being
returned if the volume was in use;

docker swarm init
docker volume create foo
docker create -v foo:/foo busybox top
docker volume rm foo

This patch changes the logic for ignoring "local" volume errors if swarm
is enabled (and cluster volumes supported).

While working on this fix, I also discovered that Cluster.RemoveVolume()
did not handle the "force" option correctly; while swarm correctly handled
these, the cluster backend performs a lookup of the volume first (to obtain
its ID), which would fail if the volume didn't exist.

Before this patch:

make TEST_FILTER=TestVolumesRemoveSwarmEnabled DOCKER_GRAPHDRIVER=vfs test-integration
...
Running /go/src/github.com/docker/docker/integration/volume (arm64.integration.volume) flags=-test.v -test.timeout=10m  -test.run TestVolumesRemoveSwarmEnabled
...
=== RUN   TestVolumesRemoveSwarmEnabled
=== PAUSE TestVolumesRemoveSwarmEnabled
=== CONT  TestVolumesRemoveSwarmEnabled
=== RUN   TestVolumesRemoveSwarmEnabled/volume_in_use
    volume_test.go:122: assertion failed: error is nil, not errdefs.IsConflict
    volume_test.go:123: assertion failed: expected an error, got nil
=== RUN   TestVolumesRemoveSwarmEnabled/volume_not_in_use
=== RUN   TestVolumesRemoveSwarmEnabled/non-existing_volume
=== RUN   TestVolumesRemoveSwarmEnabled/non-existing_volume_force
    volume_test.go:143: assertion failed: error is not nil: Error response from daemon: volume no_such_volume not found
--- FAIL: TestVolumesRemoveSwarmEnabled (1.57s)
    --- FAIL: TestVolumesRemoveSwarmEnabled/volume_in_use (0.00s)
    --- PASS: TestVolumesRemoveSwarmEnabled/volume_not_in_use (0.01s)
    --- PASS: TestVolumesRemoveSwarmEnabled/non-existing_volume (0.00s)
    --- FAIL: TestVolumesRemoveSwarmEnabled/non-existing_volume_force (0.00s)
FAIL

With this patch:

make TEST_FILTER=TestVolumesRemoveSwarmEnabled DOCKER_GRAPHDRIVER=vfs test-integration
...
Running /go/src/github.com/docker/docker/integration/volume (arm64.integration.volume) flags=-test.v -test.timeout=10m  -test.run TestVolumesRemoveSwarmEnabled
...
=== RUN   TestVolumesRemoveSwarmEnabled
=== PAUSE TestVolumesRemoveSwarmEnabled
=== CONT  TestVolumesRemoveSwarmEnabled
=== RUN   TestVolumesRemoveSwarmEnabled/volume_in_use
=== RUN   TestVolumesRemoveSwarmEnabled/volume_not_in_use
=== RUN   TestVolumesRemoveSwarmEnabled/non-existing_volume
=== RUN   TestVolumesRemoveSwarmEnabled/non-existing_volume_force
--- PASS: TestVolumesRemoveSwarmEnabled (1.53s)
    --- PASS: TestVolumesRemoveSwarmEnabled/volume_in_use (0.00s)
    --- PASS: TestVolumesRemoveSwarmEnabled/volume_not_in_use (0.01s)
    --- PASS: TestVolumesRemoveSwarmEnabled/non-existing_volume (0.00s)
    --- PASS: TestVolumesRemoveSwarmEnabled/non-existing_volume_force (0.00s)
PASS

- Description for the changelog

Fix removing volumes not returning an error if swarm is enabled

- A picture of a cute animal (not mandatory but encouraged)

Add additional test-cases for deleting non-existing volumes (with/without force). With this patch: make TEST_FILTER=TestVolumesRemove DOCKER_GRAPHDRIVER=vfs test-integration Running /go/src/github.com/docker/docker/integration/volume (arm64.integration.volume) flags=-test.v -test.timeout=10m -test.run TestVolumesRemove ... === RUN TestVolumesRemove === RUN TestVolumesRemove/volume_in_use === RUN TestVolumesRemove/volume_not_in_use === RUN TestVolumesRemove/non-existing_volume === RUN TestVolumesRemove/non-existing_volume_force --- PASS: TestVolumesRemove (0.04s) --- PASS: TestVolumesRemove/volume_in_use (0.00s) --- PASS: TestVolumesRemove/volume_not_in_use (0.01s) --- PASS: TestVolumesRemove/non-existing_volume (0.00s) --- PASS: TestVolumesRemove/non-existing_volume_force (0.00s) PASS Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2023-03-12T14:44:25Z

api/server/router/volume/volume_routes.go

+	// is enabled, in which case the remove won't return an error if no local
+	// volume existed, so we don't know if removal succeeded. In that case
+	// we always try to delete cluster volumes as well.
+	if errdefs.IsNotFound(err) || force {


Note that checking for errdefs.IsNotFound() is actually redundant here; I thought it made the code slightly more descriptive, but happy to revert that change.

thaJeztah · 2023-03-12T14:46:52Z

api/server/router/volume/volume_routes.go

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


I'm also looking if we can remove the force from the backends, and instead handle "not-exist" errors on the caller site, but looks like that may need more work ("purge" does more than only ignoring "not-exist" errors, as it also updates state in the backend to remove volumes from the store if the (remote ?) volume store (no longer) has a volume.

thaJeztah · 2023-03-12T18:29:35Z

@cpuguy83 @dperny @neersighted ptal

dperny · 2023-03-13T15:34:54Z

LGTM!

neersighted · 2023-03-13T16:10:58Z

api/server/router/volume/volume_routes.go

+	if err != nil && !errdefs.IsNotFound(err) {
+		return err
+	}


The conditional flow here (and through all of these blocks) feels very clunky. Is there a better way to structure this that makes the flow more obvious, even if we have mode code movement?

Yes, I was hoping to remove the "force" from the backend entirely, and just handle notfound errors (which are the ones we want to ignore) to the caller site; #45148 (comment)

Unfortunately, there is code in the backend that still needs it (if the plugin doesn't have the volume, then our local storage still has to remove it, and not fail)

Perhaps still possible, if we would have the backend ignore the error, for what it needs to do, but make it still return it, but that still requires more changes (probably out of scope for this PR)

cpuguy83 · 2023-03-13T16:11:10Z

api/server/router/volume/volume_routes.go

-				if err != nil {
-					return err
-				}
+			if cvErr := v.cluster.RemoveVolume(vars["name"], force); cvErr != nil {


Can we unify the branches here?
I think we can do err = v.cluster.RemoveVolume and then do nothing else in the branch.
This of course assumes cluster.RemoveVolume will return a not found error if force=false

Oh! You're right; I guess that would work; just overwrite the original err value if we reach this code.

cpuguy83 · 2023-03-13T16:12:27Z

integration/volume/volume_test.go

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


I'm not sure how this error check works?
errdefs.IsNotFound is a function

Ah, yes, so is.ErrorType supports either a type or an assertion function, which errdefs.IsXXX satisfies

(also didn't know this until not too long ago)

Commit 3246db3 added handling for removing cluster volumes, but in some conditions, this resulted in errors not being returned if the volume was in use; docker swarm init docker volume create foo docker create -v foo:/foo busybox top docker volume rm foo This patch changes the logic for ignoring "local" volume errors if swarm is enabled (and cluster volumes supported). While working on this fix, I also discovered that Cluster.RemoveVolume() did not handle the "force" option correctly; while swarm correctly handled these, the cluster backend performs a lookup of the volume first (to obtain its ID), which would fail if the volume didn't exist. Before this patch: make TEST_FILTER=TestVolumesRemoveSwarmEnabled DOCKER_GRAPHDRIVER=vfs test-integration ... Running /go/src/github.com/docker/docker/integration/volume (arm64.integration.volume) flags=-test.v -test.timeout=10m -test.run TestVolumesRemoveSwarmEnabled ... === RUN TestVolumesRemoveSwarmEnabled === PAUSE TestVolumesRemoveSwarmEnabled === CONT TestVolumesRemoveSwarmEnabled === RUN TestVolumesRemoveSwarmEnabled/volume_in_use volume_test.go:122: assertion failed: error is nil, not errdefs.IsConflict volume_test.go:123: assertion failed: expected an error, got nil === RUN TestVolumesRemoveSwarmEnabled/volume_not_in_use === RUN TestVolumesRemoveSwarmEnabled/non-existing_volume === RUN TestVolumesRemoveSwarmEnabled/non-existing_volume_force volume_test.go:143: assertion failed: error is not nil: Error response from daemon: volume no_such_volume not found --- FAIL: TestVolumesRemoveSwarmEnabled (1.57s) --- FAIL: TestVolumesRemoveSwarmEnabled/volume_in_use (0.00s) --- PASS: TestVolumesRemoveSwarmEnabled/volume_not_in_use (0.01s) --- PASS: TestVolumesRemoveSwarmEnabled/non-existing_volume (0.00s) --- FAIL: TestVolumesRemoveSwarmEnabled/non-existing_volume_force (0.00s) FAIL With this patch: make TEST_FILTER=TestVolumesRemoveSwarmEnabled DOCKER_GRAPHDRIVER=vfs test-integration ... Running /go/src/github.com/docker/docker/integration/volume (arm64.integration.volume) flags=-test.v -test.timeout=10m -test.run TestVolumesRemoveSwarmEnabled ... make TEST_FILTER=TestVolumesRemoveSwarmEnabled DOCKER_GRAPHDRIVER=vfs test-integration ... Running /go/src/github.com/docker/docker/integration/volume (arm64.integration.volume) flags=-test.v -test.timeout=10m -test.run TestVolumesRemoveSwarmEnabled ... === RUN TestVolumesRemoveSwarmEnabled === PAUSE TestVolumesRemoveSwarmEnabled === CONT TestVolumesRemoveSwarmEnabled === RUN TestVolumesRemoveSwarmEnabled/volume_in_use === RUN TestVolumesRemoveSwarmEnabled/volume_not_in_use === RUN TestVolumesRemoveSwarmEnabled/non-existing_volume === RUN TestVolumesRemoveSwarmEnabled/non-existing_volume_force --- PASS: TestVolumesRemoveSwarmEnabled (1.53s) --- PASS: TestVolumesRemoveSwarmEnabled/volume_in_use (0.00s) --- PASS: TestVolumesRemoveSwarmEnabled/volume_not_in_use (0.01s) --- PASS: TestVolumesRemoveSwarmEnabled/non-existing_volume (0.00s) --- PASS: TestVolumesRemoveSwarmEnabled/non-existing_volume_force (0.00s) PASS Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2023-03-14T09:58:33Z

Let me bring this one in, and do a cherry-pick later

thaJeztah · 2023-03-14T10:44:01Z

cherry-pick in #45155

thaJeztah added status/2-code-review process/cherry-pick area/swarm area/volumes Volumes kind/bugfix PR's that fix bugs labels Mar 12, 2023

thaJeztah added this to the v-next milestone Mar 12, 2023

This comment was marked as resolved.

Sign in to view

thaJeztah force-pushed the fix_volume_error_handling branch from 4f2fee7 to 53297e5 Compare March 12, 2023 14:38

thaJeztah commented Mar 12, 2023

View reviewed changes

thaJeztah force-pushed the fix_volume_error_handling branch from 53297e5 to b51a682 Compare March 12, 2023 15:02

thaJeztah requested review from cpuguy83 and neersighted March 12, 2023 18:29

neersighted reviewed Mar 13, 2023

View reviewed changes

cpuguy83 requested changes Mar 13, 2023

View reviewed changes

thaJeztah force-pushed the fix_volume_error_handling branch from b51a682 to 2d554cd Compare March 13, 2023 18:15

thaJeztah force-pushed the fix_volume_error_handling branch from 2d554cd to 058a31e Compare March 13, 2023 18:17

thaJeztah requested review from cpuguy83 and neersighted March 13, 2023 20:24

cpuguy83 approved these changes Mar 13, 2023

View reviewed changes

vvoland approved these changes Mar 14, 2023

View reviewed changes

thaJeztah merged commit 8224c74 into moby:master Mar 14, 2023

thaJeztah deleted the fix_volume_error_handling branch March 14, 2023 09:58

thaJeztah mentioned this pull request Mar 14, 2023

[23.0 backport] volumes: fix error-handling when removing volumes with swarm enabled #45155

Merged

thaJeztah removed the process/cherry-pick label Mar 14, 2023

thaJeztah added the process/cherry-picked label Mar 14, 2023

sam-thibault mentioned this pull request Mar 14, 2023

Docker volume rm #45037

Closed

neersighted added the impact/changelog label Mar 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

volumes: fix error-handling when removing volumes with swarm enabled#45148
thaJeztah merged 2 commits intomoby:masterfrom
thaJeztah:fix_volume_error_handling

thaJeztah commented Mar 12, 2023 •

edited

Loading

Uh oh!

This comment was marked as resolved.

thaJeztah Mar 12, 2023

Uh oh!

thaJeztah Mar 12, 2023

Uh oh!

thaJeztah commented Mar 12, 2023

Uh oh!

dperny commented Mar 13, 2023

Uh oh!

neersighted Mar 13, 2023

Uh oh!

thaJeztah Mar 13, 2023

Uh oh!

cpuguy83 Mar 13, 2023

Uh oh!

thaJeztah Mar 13, 2023

Uh oh!

cpuguy83 Mar 13, 2023

Uh oh!

thaJeztah Mar 13, 2023

Uh oh!

thaJeztah commented Mar 14, 2023

Uh oh!

thaJeztah commented Mar 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

thaJeztah commented Mar 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

integration/volumes: TestVolumesRemove: add coverage for force/no-force

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

Uh oh!

This comment was marked as resolved.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 12, 2023

Uh oh!

dperny commented Mar 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 14, 2023

Uh oh!

thaJeztah commented Mar 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thaJeztah commented Mar 12, 2023 •

edited

Loading