From 46694187310fa19c5cf98b28de39e1a78dedcff8 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Wed, 28 Sep 2022 19:43:45 +0000 Subject: [PATCH] Volume prune: only prune anonymous volumes by default 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 Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 618f26ccbc3b5c314680dae4b09f5d13f9c02570) Signed-off-by: Sebastiaan van Stijn --- api/server/router/volume/volume_routes.go | 6 +++ api/swagger.yaml | 1 + docs/api/v1.42.yaml | 1 + docs/api/version-history.md | 1 + hack/make/test-docker-py | 3 +- integration/volume/volume_test.go | 53 +++++++++++++++++++ volume/service/convert.go | 22 ++++++++ volume/service/convert_test.go | 63 +++++++++++++++++++++++ volume/service/opts/opts.go | 10 ++++ volume/service/service.go | 15 +++++- volume/service/service_test.go | 12 ++--- 11 files changed, 178 insertions(+), 9 deletions(-) create mode 100644 volume/service/convert_test.go diff --git a/api/server/router/volume/volume_routes.go b/api/server/router/volume/volume_routes.go index 7f1adfaa78..6fd5097c8b 100644 --- a/api/server/router/volume/volume_routes.go +++ b/api/server/router/volume/volume_routes.go @@ -187,6 +187,12 @@ func (v *volumeRouter) postVolumesPrune(ctx context.Context, w http.ResponseWrit return err } + // API version 1.42 changes behavior where prune should only prune anonymous volumes. + // To keep older API behavior working, we need to add this filter option to consider all (local) volumes for pruning, not just anonymous ones. + if versions.LessThan(httputils.VersionFromContext(ctx), "1.42") { + pruneFilters.Add("all", "true") + } + pruneReport, err := v.backend.Prune(ctx, pruneFilters) if err != nil { return err diff --git a/api/swagger.yaml b/api/swagger.yaml index 4ae17c3251..cda2827619 100644 --- a/api/swagger.yaml +++ b/api/swagger.yaml @@ -9699,6 +9699,7 @@ paths: Available filters: - `label` (`label=`, `label==`, `label!=`, or `label!==`) 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. type: "string" responses: 200: diff --git a/docs/api/v1.42.yaml b/docs/api/v1.42.yaml index 4ae17c3251..cda2827619 100644 --- a/docs/api/v1.42.yaml +++ b/docs/api/v1.42.yaml @@ -9699,6 +9699,7 @@ paths: Available filters: - `label` (`label=`, `label==`, `label!=`, or `label!==`) 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. type: "string" responses: 200: diff --git a/docs/api/version-history.md b/docs/api/version-history.md index b6949b8e76..eeb52e809d 100644 --- a/docs/api/version-history.md +++ b/docs/api/version-history.md @@ -119,6 +119,7 @@ keywords: "API, Docker, rcli, REST, documentation" is set with a non-matching mount Type. * `POST /containers/{id}/exec` now accepts an optional `ConsoleSize` parameter. It allows to set the console size of the executed process immediately when it's created. +* `POST /volumes/prune` will now only prune "anonymous" volumes (volumes which were not given a name) by default. A new filter parameter `all` can be set to a truth-y value (`true`, `1`) to get the old behavior. ## v1.41 API changes diff --git a/hack/make/test-docker-py b/hack/make/test-docker-py index 3043a072d1..a3c09a29c2 100644 --- a/hack/make/test-docker-py +++ b/hack/make/test-docker-py @@ -16,7 +16,8 @@ source hack/make/.integration-test-helpers # --deselect=tests/integration/api_container_test.py::AttachContainerTest::test_attach_no_stream # 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 -: "${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_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}" ( bundle .integration-daemon-start diff --git a/integration/volume/volume_test.go b/integration/volume/volume_test.go index 278c2eb93b..c48d5caca2 100644 --- a/integration/volume/volume_test.go +++ b/integration/volume/volume_test.go @@ -11,10 +11,12 @@ import ( "github.com/docker/docker/api/types" "github.com/docker/docker/api/types/filters" "github.com/docker/docker/api/types/volume" + clientpkg "github.com/docker/docker/client" "github.com/docker/docker/integration/internal/container" "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" ) @@ -165,3 +167,54 @@ func getPrefixAndSlashFromDaemonPlatform() (prefix, slash string) { } return "", "/" } + +func TestVolumePruneAnonymous(t *testing.T) { + defer setupTest(t)() + + client := testEnv.APIClient() + ctx := context.Background() + + // Create an anonymous volume + v, err := client.VolumeCreate(ctx, volume.CreateOptions{}) + assert.NilError(t, err) + + // Create a named volume + vNamed, err := client.VolumeCreate(ctx, volume.CreateOptions{ + Name: "test", + }) + assert.NilError(t, err) + + // Prune anonymous volumes + pruneReport, err := client.VolumesPrune(ctx, filters.Args{}) + assert.NilError(t, err) + assert.Check(t, is.Equal(len(pruneReport.VolumesDeleted), 1)) + assert.Check(t, is.Equal(pruneReport.VolumesDeleted[0], v.Name)) + + _, err = client.VolumeInspect(ctx, vNamed.Name) + assert.NilError(t, err) + + // Prune all volumes + _, err = client.VolumeCreate(ctx, volume.CreateOptions{}) + assert.NilError(t, err) + + pruneReport, err = client.VolumesPrune(ctx, filters.NewArgs(filters.Arg("all", "1"))) + assert.NilError(t, err) + assert.Check(t, is.Equal(len(pruneReport.VolumesDeleted), 2)) + + // Validate that older API versions still have the old behavior of pruning all local volumes + clientOld, err := clientpkg.NewClientWithOpts(clientpkg.FromEnv, clientpkg.WithVersion("1.41")) + assert.NilError(t, err) + defer clientOld.Close() + assert.Equal(t, clientOld.ClientVersion(), "1.41") + + v, err = client.VolumeCreate(ctx, volume.CreateOptions{}) + assert.NilError(t, err) + vNamed, err = client.VolumeCreate(ctx, volume.CreateOptions{Name: "test-api141"}) + assert.NilError(t, err) + + pruneReport, err = clientOld.VolumesPrune(ctx, filters.Args{}) + assert.NilError(t, err) + assert.Check(t, is.Equal(len(pruneReport.VolumesDeleted), 2)) + assert.Check(t, cmp.Contains(pruneReport.VolumesDeleted, v.Name)) + assert.Check(t, cmp.Contains(pruneReport.VolumesDeleted, vNamed.Name)) +} diff --git a/volume/service/convert.go b/volume/service/convert.go index 01828280b4..00ea5d2792 100644 --- a/volume/service/convert.go +++ b/volume/service/convert.go @@ -2,10 +2,13 @@ package service import ( "context" + "fmt" + "strconv" "time" "github.com/docker/docker/api/types/filters" volumetypes "github.com/docker/docker/api/types/volume" + "github.com/docker/docker/errdefs" "github.com/docker/docker/pkg/directory" "github.com/docker/docker/volume" "github.com/sirupsen/logrus" @@ -130,3 +133,22 @@ func filtersToBy(filter filters.Args, acceptedFilters map[string]bool) (By, erro } return by, nil } + +func withPrune(filter filters.Args) error { + all := filter.Get("all") + switch { + case len(all) > 1: + return invalidFilter{"all", all} + case len(all) == 1: + ok, err := strconv.ParseBool(all[0]) + if err != nil { + return errdefs.InvalidParameter(fmt.Errorf("invalid filter 'all': %w", err)) + } + if ok { + return nil + } + } + + filter.Add("label", AnonymousLabel) + return nil +} diff --git a/volume/service/convert_test.go b/volume/service/convert_test.go new file mode 100644 index 0000000000..89c4620946 --- /dev/null +++ b/volume/service/convert_test.go @@ -0,0 +1,63 @@ +package service + +import ( + "testing" + + "github.com/docker/docker/api/types/filters" + "gotest.tools/v3/assert" + "gotest.tools/v3/assert/cmp" +) + +func TestFilterWithPrune(t *testing.T) { + f := filters.NewArgs() + assert.NilError(t, withPrune(f)) + assert.Check(t, cmp.Len(f.Get("label"), 1)) + assert.Check(t, f.Match("label", AnonymousLabel)) + + f = filters.NewArgs() + f.Add("label", "foo=bar") + f.Add("label", "bar=baz") + assert.NilError(t, withPrune(f)) + + assert.Check(t, cmp.Len(f.Get("label"), 3)) + assert.Check(t, f.Match("label", AnonymousLabel)) + assert.Check(t, f.Match("label", "foo=bar")) + assert.Check(t, f.Match("label", "bar=baz")) + + f = filters.NewArgs() + f.Add("label", "foo=bar") + f.Add("all", "1") + assert.NilError(t, withPrune(f)) + + assert.Check(t, cmp.Len(f.Get("label"), 1)) + assert.Check(t, f.Match("label", "foo=bar")) + + f = filters.NewArgs() + f.Add("label", "foo=bar") + f.Add("all", "true") + assert.NilError(t, withPrune(f)) + + assert.Check(t, cmp.Len(f.Get("label"), 1)) + assert.Check(t, f.Match("label", "foo=bar")) + + f = filters.NewArgs() + f.Add("all", "0") + assert.NilError(t, withPrune(f)) + assert.Check(t, cmp.Len(f.Get("label"), 1)) + assert.Check(t, f.Match("label", AnonymousLabel)) + + f = filters.NewArgs() + f.Add("all", "false") + assert.NilError(t, withPrune(f)) + assert.Check(t, cmp.Len(f.Get("label"), 1)) + assert.Check(t, f.Match("label", AnonymousLabel)) + + f = filters.NewArgs() + f.Add("all", "") + assert.ErrorContains(t, withPrune(f), "invalid filter 'all'") + + f = filters.NewArgs() + f.Add("all", "1") + f.Add("all", "0") + assert.ErrorContains(t, withPrune(f), "invalid filter 'all") +} diff --git a/volume/service/opts/opts.go b/volume/service/opts/opts.go index c190c3a70d..3b4f63196a 100644 --- a/volume/service/opts/opts.go +++ b/volume/service/opts/opts.go @@ -11,6 +11,16 @@ type CreateConfig struct { Reference string } +// WithCreateLabel creates a CreateOption which adds a label with the given key/value pair +func WithCreateLabel(key, value string) CreateOption { + return func(cfg *CreateConfig) { + if cfg.Labels == nil { + cfg.Labels = map[string]string{} + } + cfg.Labels[key] = value + } +} + // WithCreateLabels creates a CreateOption which sets the labels to the // passed in value func WithCreateLabels(labels map[string]string) CreateOption { diff --git a/volume/service/service.go b/volume/service/service.go index d96f6f785b..6432aff474 100644 --- a/volume/service/service.go +++ b/volume/service/service.go @@ -60,6 +60,10 @@ func (s *VolumesService) GetDriverList() []string { return s.ds.GetDriverList() } +// 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" + // Create creates a volume // If the caller is creating this volume to be consumed immediately, it is // expected that the caller specifies a reference ID. @@ -67,11 +71,12 @@ func (s *VolumesService) GetDriverList() []string { // // A good example for a reference ID is a container's ID. // When whatever is going to reference this volume is removed the caller should defeference the volume by calling `Release`. -func (s *VolumesService) Create(ctx context.Context, name, driverName string, opts ...opts.CreateOption) (*volumetypes.Volume, error) { +func (s *VolumesService) Create(ctx context.Context, name, driverName string, options ...opts.CreateOption) (*volumetypes.Volume, error) { if name == "" { name = stringid.GenerateRandomID() + options = append(options, opts.WithCreateLabel(AnonymousLabel, "")) } - v, err := s.vs.Create(ctx, name, driverName, opts...) + v, err := s.vs.Create(ctx, name, driverName, options...) if err != nil { return nil, err } @@ -171,6 +176,8 @@ func (s *VolumesService) Remove(ctx context.Context, name string, rmOpts ...opts var acceptedPruneFilters = map[string]bool{ "label": true, "label!": true, + // All tells the filter to consider all volumes not just anonymous ones. + "all": true, } var acceptedListFilters = map[string]bool{ @@ -215,6 +222,10 @@ func (s *VolumesService) Prune(ctx context.Context, filter filters.Args) (*types } defer atomic.StoreInt32(&s.pruneRunning, 0) + if err := withPrune(filter); err != nil { + return nil, err + } + by, err := filtersToBy(filter, acceptedPruneFilters) if err != nil { return nil, err diff --git a/volume/service/service_test.go b/volume/service/service_test.go index 431c3526e5..457b25d818 100644 --- a/volume/service/service_test.go +++ b/volume/service/service_test.go @@ -172,11 +172,11 @@ func TestServicePrune(t *testing.T) { _, err = service.Create(ctx, "test2", "other") assert.NilError(t, err) - pr, err := service.Prune(ctx, filters.NewArgs(filters.Arg("label", "banana"))) + pr, err := service.Prune(ctx, filters.NewArgs(filters.Arg("label", "banana"), filters.Arg("all", "true"))) assert.NilError(t, err) assert.Assert(t, is.Len(pr.VolumesDeleted, 0)) - pr, err = service.Prune(ctx, filters.NewArgs()) + pr, err = service.Prune(ctx, filters.NewArgs(filters.Arg("all", "true"))) assert.NilError(t, err) assert.Assert(t, is.Len(pr.VolumesDeleted, 1)) assert.Assert(t, is.Equal(pr.VolumesDeleted[0], "test")) @@ -191,7 +191,7 @@ func TestServicePrune(t *testing.T) { _, err = service.Create(ctx, "test", volume.DefaultDriverName) assert.NilError(t, err) - pr, err = service.Prune(ctx, filters.NewArgs(filters.Arg("label!", "banana"))) + pr, err = service.Prune(ctx, filters.NewArgs(filters.Arg("label!", "banana"), filters.Arg("all", "true"))) assert.NilError(t, err) assert.Assert(t, is.Len(pr.VolumesDeleted, 1)) assert.Assert(t, is.Equal(pr.VolumesDeleted[0], "test")) @@ -207,12 +207,12 @@ func TestServicePrune(t *testing.T) { _, err = service.Create(ctx, "test3", volume.DefaultDriverName, opts.WithCreateLabels(map[string]string{"banana": "split"})) assert.NilError(t, err) - pr, err = service.Prune(ctx, filters.NewArgs(filters.Arg("label!", "banana=split"))) + pr, err = service.Prune(ctx, filters.NewArgs(filters.Arg("label!", "banana=split"), filters.Arg("all", "true"))) assert.NilError(t, err) assert.Assert(t, is.Len(pr.VolumesDeleted, 1)) assert.Assert(t, is.Equal(pr.VolumesDeleted[0], "test")) - pr, err = service.Prune(ctx, filters.NewArgs(filters.Arg("label", "banana=split"))) + pr, err = service.Prune(ctx, filters.NewArgs(filters.Arg("label", "banana=split"), filters.Arg("all", "true"))) assert.NilError(t, err) assert.Assert(t, is.Len(pr.VolumesDeleted, 1)) assert.Assert(t, is.Equal(pr.VolumesDeleted[0], "test3")) @@ -225,7 +225,7 @@ func TestServicePrune(t *testing.T) { assert.Assert(t, is.Len(pr.VolumesDeleted, 0)) assert.Assert(t, service.Release(ctx, v.Name, t.Name())) - pr, err = service.Prune(ctx, filters.NewArgs()) + pr, err = service.Prune(ctx, filters.NewArgs(filters.Arg("all", "true"))) assert.NilError(t, err) assert.Assert(t, is.Len(pr.VolumesDeleted, 1)) assert.Assert(t, is.Equal(pr.VolumesDeleted[0], "test"))