Volume prune: only prune anonymous volumes by default by cpuguy83 · Pull Request #44216 · moby/moby

cpuguy83 · 2022-09-28T19:55:26Z

fixes Consider not to delete named volumes by default when running system prune #31757

This adds a new filter argument to the volume prune endpoint "all". When this is not set, or it is a false-y value, then only anonymous volumes are considered for pruning.

When all is set to a truth-y value, you get the old behavior.

This is an API change, but I think one that is what most people would want.

tianon

This finally makes volume pruning consistent with all our other prune experiences! LGTM 😍

api/swagger.yaml

cpuguy83 · 2022-09-29T20:19:02Z

Updated this as discussed to make sure older API versions keep the old behavior.

thaJeztah · 2022-10-03T21:10:40Z

Looks like we need to skip one of the docker-py tests and/or fix in upstream. Haven't checked yet what the test does, and if it's incorrect.

That said; if it's using API v1.41, it probably should've passed 🤔

=================================== FAILURES ===================================
________________________ TestVolumes.test_prune_volumes ________________________
tests/integration/api_volume_test.py:64: in test_prune_volumes
    assert name in result['VolumesDeleted']
E   AssertionError: assert 'hopelessmasquerade' in []
------- generated xml file: /src/bundles/test-docker-py/junit-report.xml -------
=========================== short test summary info ============================

Test is https://github.com/docker/docker-py/blob/5.0.3/tests/integration/api_volume_test.py#L58-L64

~~(we also need to update docker-py to a newer version, to un-skip some tests that were fixed)~~

edit: LOL, and of course I already had a PR in draft; #43998

neersighted · 2022-10-03T21:27:12Z

The test looks quite straightforward in the current main, but I'm not sure what version is in use here.

https://github.com/docker/docker-py/blob/923e067dddc3d4b86e4e620a99fcdcdafbd17a98/tests/integration/api_volume_test.py#L58-L64

If this is actually what is failing here, I think the bug is in this PR; however I haven't dug in any further.

thaJeztah · 2022-10-03T21:36:53Z

I see brian is already testing a change in that code. Perhaps the docker-py tests don't specify an API version, in which case they could be using "latest" (not a fixed version).

I also dusted off #43998, which shows how tests can be skipped temporarily (we probably can do so if we know why it's failing)

cpuguy83 · 2022-10-03T21:38:12Z

It seems like it should be using 1.41 (it's what's in the docker-py makefile).
Looking into it.

cpuguy83 · 2022-10-03T22:02:02Z

The daemon logs from the docker-py run shows it is using API v1.43

https://github.com/moby/moby/actions/runs/3154324955/jobs/5131882252#step:8:17308

cpuguy83 · 2022-10-03T22:04:49Z

Ah, that's because we are running pytest directly.

thaJeztah · 2022-10-03T22:07:18Z

I see it's calling /version before various tests; is it using that to find the version to use? (and not negotiation based on the version it supports?)

thaJeztah · 2022-10-04T16:54:24Z

As isn't merged yet, and it looks like needs some work, perhaps you can update PY_TEST_OPTIONS

moby/hack/make/test-docker-py

Line 19 in 3c06ebd

    
           : "${PY_TEST_OPTIONS:=--junitxml=${DEST}/junit-report.xml --deselect=tests/integration/api_container_test.py::AttachContainerTest::test_attach_no_stream --deselect=tests/integration/api_container_test.py::CreateContainerTest::test_create_with_device_cgroup_rules}"

# TODO re-enable test_attach_no_stream after https://github.com/docker/docker-py/issues/2513 is resolved
# TODO re-enable test_create_with_device_cgroup_rules after https://github.com/docker/docker-py/issues/2939 is resolved
# TODO re-enable test_prune_volumes after https://github.com/docker/docker-py/pull/3051 is resolved
: "${PY_TEST_OPTIONS:=--junitxml=${DEST}/junit-report.xml --deselect=tests/integration/api_container_test.py::AttachContainerTest::test_attach_no_stream --deselect=tests/integration/api_container_test.py::CreateContainerTest::test_create_with_device_cgroup_rules --deselect=tests/integration/api_volume_test.py::TestVolumes::test_prune_volumes}"

thaJeztah

Left two comments; I have the changes locally, so let me open a quick PR against your branch

thaJeztah · 2022-10-04T16:51:22Z

api/swagger.yaml


            Available filters:
            - `label` (`label=<key>`, `label=<key>=<value>`, `label!=<key>`, or `label!=<key>=<value>`) Prune volumes with (or without, in case `label!=...` is used) the specified labels.
+            - `all` (`all=true`) - Consider all (local) volumes for pruning and not just anonymous volumes.


Looks like this PR was implemented to be included in API v1.42 (22.06); could you apply this change to the docs/api/v1.42.yaml file as well?

thaJeztah · 2022-10-04T16:57:01Z

opened cpuguy83#5 <-- PTAL @cpuguy83

This adds a new filter argument to the volume prune endpoint "all". When this is not set, or it is a false-y value, then only anonymous volumes are considered for pruning. When `all` is set to a truth-y value, you get the old behavior. This is an API change, but I think one that is what most people would want. Signed-off-by: Brian Goff <cpuguy83@gmail.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

cpuguy83 · 2022-10-05T15:35:56Z

All green

thaJeztah · 2022-10-05T15:54:47Z

volume/service/service.go


+// AnonymousLabel is the label used to indicate that a volume is anonymous
+// This is set automatically on a volume when a volume is created without a name specified, and as such an id is generated for it.
+const AnonymousLabel = "com.docker.volume.anonymous"


One thing I wonder; will this have an effect on volumes that "started as anonymous", but later on became "named"?

Thinking of situations like

docker run -d --name container1 some-image-with-volumes docker run --volumes-from=container2 some-image

Yes, the volume is still "anonymous", but no longer really "ephemeral"

The volume is still anonymous, but if a container is holding a reference to it, it won't be pruned anyway.

Yeah, makes sense

thaJeztah

LGTM

This is related to moby/moby#44216 Prunes will, by default, no longer prune named volumes, only anonymous ones. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

tianon approved these changes Sep 28, 2022

View reviewed changes

cpuguy83 force-pushed the volume_unnamed_label branch 2 times, most recently from 3a6e782 to 2312731 Compare September 28, 2022 20:24

neersighted requested changes Sep 29, 2022

View reviewed changes

api/swagger.yaml Outdated Show resolved Hide resolved

neersighted added area/api API kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. impact/changelog area/volumes Volumes labels Sep 29, 2022

neersighted added this to the v-next milestone Sep 29, 2022

neersighted added the status/2-code-review label Sep 29, 2022

cpuguy83 force-pushed the volume_unnamed_label branch 2 times, most recently from 1ddbd45 to 7fdf167 Compare September 29, 2022 18:30

neersighted approved these changes Sep 29, 2022

View reviewed changes

cpuguy83 force-pushed the volume_unnamed_label branch from 7fdf167 to 5ed9600 Compare September 29, 2022 20:18

cpuguy83 mentioned this pull request Oct 3, 2022

Change prune test to use anonymous volumes docker/docker-py#3051

Merged

thaJeztah requested changes Oct 4, 2022

View reviewed changes

thaJeztah mentioned this pull request Oct 4, 2022

review suggestions cpuguy83/docker#5

Merged

cpuguy83 force-pushed the volume_unnamed_label branch from 97c4744 to 07f46f6 Compare October 4, 2022 18:35

cpuguy83 force-pushed the volume_unnamed_label branch from 07f46f6 to 618f26c Compare October 4, 2022 20:55

thaJeztah reviewed Oct 5, 2022

View reviewed changes

thaJeztah approved these changes Oct 5, 2022

View reviewed changes

thaJeztah added the process/cherry-pick label Oct 5, 2022

thaJeztah merged commit 49940ab into moby:master Oct 5, 2022

cpuguy83 deleted the volume_unnamed_label branch October 5, 2022 18:23

thaJeztah mentioned this pull request Oct 5, 2022

[22.06 backport] Volume prune: only prune anonymous volumes by default #44259

Merged

thaJeztah added process/cherry-picked and removed process/cherry-pick labels Oct 5, 2022

cpuguy83 mentioned this pull request Oct 18, 2022

Proposal: Add "lock" or "no delete" flag to container #9878

Closed

DeliciousHouse mentioned this pull request Oct 20, 2022

test: use anonymous volume for prune (#3051) docker/docker-py#3055

Closed

kdrag0n mentioned this pull request Mar 20, 2023

docker system prune --all --volumes --force does not delete volumes orbstack/orbstack#40

Closed

This was referenced Apr 20, 2023

volumes: prune: add --all / -a option docker/cli#4218

Merged

[23.0 backport] volumes: prune: add --all / -a option docker/cli#4229

Merged

oliv3r mentioned this pull request Jun 10, 2023

feature: Protect volumes against deletion #45725

Closed

neersighted mentioned this pull request Jul 20, 2023

[BUG] Compose-created anonymous volumes are not marked as anonymous docker/compose#10833

Closed

This was referenced Aug 11, 2023

Support Docker 23.0 compatibility containerd/nerdctl#2427

Merged

Support create anonymous volume containerd/nerdctl#2433

Merged

thaJeztah mentioned this pull request Sep 25, 2023

Consider not to delete named volumes by default when running system prune #31757

Closed

thaJeztah mentioned this pull request Oct 19, 2023

Prune should always list items to be deleted before asking to confirm. #46678

Closed

thaJeztah mentioned this pull request May 13, 2024

Change the -v flag on docker compose down docker/roadmap#665

Open

This was referenced Oct 25, 2024

volume/mounts: fix anonymous volume not being labeled #48754

Merged

[27.x backport] volume/mounts: fix anonymous volume not being labeled #48755

Merged

Conversation

cpuguy83 commented Sep 28, 2022 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cpuguy83 commented Sep 29, 2022

Uh oh!

thaJeztah commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neersighted commented Oct 3, 2022

Uh oh!

thaJeztah commented Oct 3, 2022

Uh oh!

cpuguy83 commented Oct 3, 2022

Uh oh!

cpuguy83 commented Oct 3, 2022

Uh oh!

cpuguy83 commented Oct 3, 2022

Uh oh!

thaJeztah commented Oct 3, 2022

Uh oh!

thaJeztah commented Oct 4, 2022

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 4, 2022

Uh oh!

cpuguy83 commented Oct 5, 2022

Uh oh!

thaJeztah Oct 5, 2022

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Oct 5, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 5, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cpuguy83 commented Sep 28, 2022 •

edited by thaJeztah

Loading

thaJeztah commented Oct 3, 2022 •

edited

Loading