Adding "driver" and "dangling" filter options to "docker volume ls" #2831

dani-docker · 2018-01-08T15:33:00Z

volume ls in swarm classic only handles filters with options "node", "name" and "label".
This PR adds support for "driver" and "dangling"

Adding "driver" to the filter option is pretty straightforward.
The "dangling" filter is a bit more involved as the "volume" struct has no "dangling" property.

2 options to support "dangling":
1- add an extra call to the engine to get the dangling volumes
or
2- Iterate through cached "containers" and build a map of volumes that are still referenced by the container, and finally use the delta to delta to determine dangling volumes.

After chatting with @nishanttotla , 1) seems a bit taxing so I decided to go with option 2)
Note: I am not caching the map for referenced volumes, so we don't have to maintain it, besides, the calls to filter based on "dangling" should be fast and should not be that frequent.

Repro Steps for driver:

1- Have at least 2 volumes using 2 separate drivers.
for ex:
docker volume ls

DRIVER	VOLUME NAME
vieux/sshfs:latest	sshvolume
local	testVolume

2- Run
docker volume ls --filter=driver=local

3-
Expect Results

DRIVER	VOLUME NAME
local	testVolume

Actual Results (all volumes returned)

DRIVER	VOLUME NAME
vieux/sshfs:latest	sshvolume
local	testVolume

Repro Steps for dangling:

1- Have at least 2 volumes with one volume that is dangling (not used).
(sshvolume in this example is dangling)
docker volume ls

DRIVER	VOLUME NAME
vieux/sshfs:latest	sshvolume
local	testVolume

2- Run
docker volume ls --filter=dangling=true

3-
Expect Results

DRIVER	VOLUME NAME
vieux/sshfs:latest	sshvolume

Actual Results (all volumes returned)

DRIVER	VOLUME NAME
vieux/sshfs:latest	sshvolume
local	testVolume

Signed-off-by: Dani Louca dani.louca@docker.com

wsong · 2018-01-08T17:04:10Z

api/handlers.go

@@ -359,6 +383,18 @@ func getVolumes(c *context, w http.ResponseWriter, r *http.Request) {
 			continue
 		}

+		found = false
+		for _, driver := range drivers {
+			if strings.Contains(volume.Driver, driver) {


The daemon code uses filter.ExactMatch instead of a substring match here:

https://github.com/moby/moby/blob/a3efe9722f34af5cf4443fe3a5c4e4e3e0457b54/daemon/list.go#L636

makes sense, I will correct that

wsong · 2018-01-08T17:04:47Z

api/handlers.go

+			for _, container := range c.cluster.Containers() {
+				for _, mount := range container.Mounts {
+					if mount.Type == "volume" {
+						if mount.Driver == "local" {


Does this mean that the dangling filter will not work for non-local volumes?

no, it still works for all, local and non-local.
The condition on line 353 is there to honor the "existing" naming convention used around line 417 for local volumes

https://github.com/dani-docker/swarm/blob/9ae1c6e16226bceae46afb551a3ed3287c8d6176/api/handlers.go#L405-L418

wsong · 2018-01-08T19:29:13Z

api/handlers.go

@@ -359,6 +383,18 @@ func getVolumes(c *context, w http.ResponseWriter, r *http.Request) {
 			continue
 		}

+		found = false
+		for _, driver := range drivers {
+			if volume.Driver == driver {


Is there any reason you can't use filters.ExactMatch, just like the daemon code?

no reason, I was just following the existing pattern in name and node matching, but yes, using ExactMatch is much cleaner.
shall I change the name/node filter matching to mirror the daemon? ie using filters.Match?
or "if it ain't broke don't fix it"

Yeah, let's leave the name/node filtering alone. Let's keep this PR focused.

sounds good!
PR is updated with ExactMatch

wsong

LGTM, but ping @nishanttotla as well

nishanttotla

@dani-docker the CI is failing due to go fmt. Can you format your code using fmt? Also, could you add a unit test to this PR

wsong

For what it's worth, I'd recommend breaking up the getVolumes() function into helper functions to make it easier to unit test; that way your unit test doesn't have to worry about instantiating http.ResponseWriter objects, http.Request objects, etc.

allencloud · 2018-01-09T03:33:09Z

api/handlers.go

+	danglingOnly := false
+	refVolumes := make(map[string]bool)
+
+	if filters.Include("dangling") {


Actually I do not like so much ident. Could rewrite this to use return fast to make it much more readable?

allencloud · 2018-01-09T03:36:50Z

api/handlers.go

@@ -359,7 +382,13 @@ func getVolumes(c *context, w http.ResponseWriter, r *http.Request) {
 			continue
 		}

-		if filters.Include("label") {
+                if filters.Include("driver") {


please use gofmt to format these code.

thaJeztah · 2018-01-09T10:38:33Z

api/handlers.go

@@ -359,6 +382,12 @@ func getVolumes(c *context, w http.ResponseWriter, r *http.Request) {
 			continue
 		}

+		if filters.Include("driver") {


Instead of filtering in swarm, wouldn't it be possible to just have the engines do this filtering? The engine API already provides these filters, so should be able to return just what we're looking for?

(I'm not to familiar with the Swarm code-base, but always assumed that's what it did, but I now see all engine's functionality is replicated here)

The filtering is done on locally cached volumes, making calls to all engines in the cluster to filter out accordingly defeats the swarm classic design. Note, this is not specific to Volumes, same applies to Images, Containers, Networks etc...

Swarm classic is designed to always read from the cache (and do its own filtering) on read-only calls. For performance reasons, we cannot do engine calls during read-only operations.

thaJeztah · 2018-01-09T10:42:15Z

api/handlers.go

+
+	if filters.Include("dangling") {
+		if filters.ExactMatch("dangling", "true") {
+			danglingOnly = true


Note that the dangling filter also supports dangling=false, which returns all volumes that are not dangling (that case could be handled by passing the filter to the engine, and collect all non-dangling volumes)

see the original change in the engine; https://github.com/moby/moby/pull/21361/files#diff-9320049615863c9b6ab00c749bbb2f09R526

Right, calling the engine directly was one simple option (see PR description), but api calls to all engines in the cluster can be taxing, hence we decided to build the list of used volumes from locally cached containers.

Won't that easily lead to incorrect data (i.e. container may now use the volume, or vice-versa); or not a problem?

good catch @thaJeztah! I will address the above

dani-docker · 2018-01-09T16:16:36Z

had a chat with @thaJeztah and 2 valid use cases were identified:

dangling=false has not been handled in my PR, I was under the impression that false means don't do any filtering, but it actually means to return used volumes only . Verified and confirmed the behavior directly on the engine
Existing bug that was surfaced with the dangling fix. Currently, we only append the engine name to local volumes, this name uniqueness should be applied to all type of drivers otherwise we can see false/positive results.

~~I will update the PR shortly.~~
I will wait to see If we can get #2832 in, as the code change here will be much cleaner.

dani-docker · 2018-01-12T17:32:53Z

While waiting on #2832

I changed the code to address the below internally without changing the output

Existing bug that was surfaced with the dangling fix. Currently, we only append the engine name to local volumes, this name uniqueness should be applied to all type of drivers otherwise we can see false/positive results.

added 2 helper methods along with their unit tests

if #2832 is approved, I will update this PR with cleaner/simpler code
ping @wsong @nishanttotla

wsong · 2018-01-16T19:41:01Z

api/handlers.go

@@ -330,6 +330,39 @@ func getVolume(c *context, w http.ResponseWriter, r *http.Request) {
 	httpError(w, fmt.Sprintf("No such volume: %s", name), http.StatusNotFound)
 }

+func GetUsedVolumes(containers []*cluster.Container) map[string]bool {


This function doesn't need to be public (i.e. it can be named getUsedVolumes instead of GetUsedVolumes.

right, the unit test is in the same package, no need for public.
I will fix that

wsong · 2018-01-16T19:41:15Z

api/handlers.go

+	return usedVolumes
+}
+
+func GetFilteredVolumes(containers []*cluster.Container, volumes []*apitypes.Volume, dangling bool) []*apitypes.Volume {


As above, make this function private.

right, the unit test is in the same package, no need for public.
I will fix that

wsong · 2018-01-16T19:42:13Z

api/handlers.go

+	// volume name allowed char [a-zA-Z0-9][a-zA-Z0-9_.-]
+	// splitting on / is safe to keep the original output intact
+	stripEngine := func(vol *apitypes.Volume) *apitypes.Volume {
+		if vol.Driver != "local" {


Instead of special-casing local volumes, can you just do parts = strings.Split(vol.Name) and use parts[1] if len(parts) > 1 and vol.Name otherwise?

I have to special case local volumes so I don't change its engine_name/volume_name format.

In that case, add a comment.

dani-docker · 2018-01-16T21:51:23Z

PR updated with @wsong suggestions

wsong · 2018-01-16T21:59:42Z

api/handlers.go

+	// volume name allowed char [a-zA-Z0-9][a-zA-Z0-9_.-]
+	// splitting on / is safe to keep the original output intact
+	stripEngine := func(vol *apitypes.Volume) *apitypes.Volume {
+		//don't change local volume names "engine_name/volume_name"


I meant that you should add a comment explaining why you're doing this.

dani-docker · 2018-01-20T04:18:02Z

@wsong
PR updated with the detailed comment

wsong · 2018-01-22T17:42:49Z

api/handlers.go

+		//this naming format is temporarily extended to all volumes to provide uniqueness
+		// while identifying dangling volumes. Restoring the original naming convention back.
+		if vol.Driver != "local" {
+			vol.Name = strings.Split(vol.Name, "/")[1]


Would it make sense to check that strings.Split(vol.Name, "/") here has len() > 1 so that we don't panic if there's a problem here?

Possibly SplitN as well (I think with custom drivers, a slash may be allowed in names)

here https://github.com/dani-docker/swarm/blob/117d215985d6d06fb66115dd781e32253fe3aa79/api/handlers.go#L426 guarantees that "/" is appended when dangling filter is in use.
But if you think this helper method will be re-used and to be on the safe, I will make the change.

@thaJeztah
you are correct! when I looked at the code, I only checked local volume, as you stated, custom drivers do allow slash

changes done for both concerns

Signed-off-by: Dani Louca <dani.louca@docker.com>

dani-docker · 2018-01-22T20:50:27Z

PR updated to address the / in plugin volume names and safeguard check for len of split.
Also updated the unit test to cover the above cases.

wsong reviewed Jan 8, 2018

View reviewed changes

dani-docker force-pushed the esc-318 branch from 9ae1c6e to 0204616 Compare January 8, 2018 19:21

wsong reviewed Jan 8, 2018

View reviewed changes

dani-docker force-pushed the esc-318 branch from 0204616 to 65c3cf1 Compare January 8, 2018 21:44

wsong approved these changes Jan 8, 2018

View reviewed changes

nishanttotla added the area/API label Jan 8, 2018

nishanttotla suggested changes Jan 8, 2018

View reviewed changes

wsong reviewed Jan 8, 2018

View reviewed changes

allencloud reviewed Jan 9, 2018

View reviewed changes

dani-docker force-pushed the esc-318 branch from 65c3cf1 to f5d0140 Compare January 9, 2018 03:59

thaJeztah reviewed Jan 9, 2018

View reviewed changes

dani-docker mentioned this pull request Jan 10, 2018

Change 'volume ls' name format to enginename/volumename for non-local drivers #2832

Closed

dani-docker force-pushed the esc-318 branch from f5d0140 to 3807a88 Compare January 12, 2018 17:27

wsong reviewed Jan 16, 2018

View reviewed changes

dani-docker force-pushed the esc-318 branch from 3807a88 to 38d7f6c Compare January 16, 2018 21:47

wsong reviewed Jan 16, 2018

View reviewed changes

dani-docker force-pushed the esc-318 branch from 38d7f6c to 117d215 Compare January 20, 2018 04:14

wsong approved these changes Jan 22, 2018

View reviewed changes

Adding driver and dangling options to the list volumes filter call

70e7abf

Signed-off-by: Dani Louca <dani.louca@docker.com>

dani-docker force-pushed the esc-318 branch from 117d215 to 70e7abf Compare January 22, 2018 20:48

wsong approved these changes Jan 22, 2018

View reviewed changes

nishanttotla added this to the 1.2.9 milestone May 29, 2018

trapier mentioned this pull request Sep 15, 2018

docker volume ls -f dangling=true returns all volumes #1849

Closed

justincormack closed this Jun 4, 2020

Adding "driver" and "dangling" filter options to "docker volume ls" #2831

Adding "driver" and "dangling" filter options to "docker volume ls" #2831

Conversation

dani-docker commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wsong left a comment

Choose a reason for hiding this comment

nishanttotla left a comment

Choose a reason for hiding this comment

wsong left a comment

Choose a reason for hiding this comment

allencloud Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dani-docker commented Jan 9, 2018 • edited Loading

dani-docker commented Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dani-docker commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dani-docker commented Jan 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dani-docker Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dani-docker commented Jan 22, 2018

allencloud Jan 9, 2018 •

edited

Loading

thaJeztah Jan 9, 2018 •

edited

Loading

dani-docker commented Jan 9, 2018 •

edited

Loading

dani-docker commented Jan 12, 2018 •

edited

Loading

dani-docker Jan 22, 2018 •

edited

Loading