[23.0 backport] volumes: fix error-handling when removing volumes with swarm enabled #45155
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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>
(cherry picked from commit 7531f05)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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> (cherry picked from commit 058a31e) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
added
status/2-code-review
area/volumes
kind/bugfix
This was referenced Mar 14, 2023
neersighted
approved these changes
Mar 14, 2023
vvoland
approved these changes
Mar 14, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
integration/volumes: TestVolumesRemove: add coverage for force/no-force
Add additional test-cases for deleting non-existing volumes (with/without force).
With this patch:
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;
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:
With this patch:
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)