docker-archive / classicswarm

Swarm Classic: a container clustering system. Not to be confused with Docker Swarm which is at https://github.com/docker/swarmkit
Apache License 2.0
5.75k stars 1.08k forks source link

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

Closed dani-docker closed 4 years ago

dani-docker commented 6 years ago

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

dani-docker commented 6 years ago

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

  1. 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

  2. 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 commented 6 years ago

While waiting on #2832

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.

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

dani-docker commented 6 years ago

PR updated with @wsong suggestions

dani-docker commented 6 years ago

@wsong PR updated with the detailed comment

dani-docker commented 6 years ago

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.