#26346: docker volume inspect doesn't do partial ID matching by bbayani · Pull Request #27317 · moby/moby

bbayani · 2016-10-12T13:03:00Z

- What I did
Implemented code for performing partial ID matching for docker volumes (ID is name for volumes)

- How I did it
Added method GetVolumesByName. It iterates over all volumes in daemon volume store and returns the list of volumes whose names match with passed prefix

- How to verify it

Build a developer container with latest docker/master and this PR
Create binaries in container with hack/make.sh binary
Copy binaries in /usr/bin
Start docker daemon
Create docker volume with "docker volume create test1"
Try docker volume inspect with complete name
docker volume inspect test1
docker inspect test1
Try docker volume inspect with first part of volume name
docker volume inspect te
docker inspect te
Try volume inspect with non existing volume name and ensure you get error
docker volume inspect abc -> no such volume: abc
docker inspect abc -> no such container/network/volume/service
If more and one volumes with same first initial characters exists, inspect will be performed for first one.

- Description for the changelog

Removed a VolumeInspect method from daemon/inspect.go
Added this method in daemon/volumes.go
Added new method getVolumesByName in daemon/volumes.go which does partial name matching and returns list of matching volumes
Replaced method call in VolumeInspect that looks-up volume with complete name, with new method getVolumesByName.

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah · 2016-10-14T01:18:12Z

Thanks @bbayani - looks like there's some linting issues;

06:19:16 ---> Making bundle: validate-lint (in bundles/1.13.0-dev/validate-lint)
06:19:17 Errors from golint:
06:19:17 daemon/volumes.go:29:6: exported type VolumeWalker should have comment or be unexported
06:19:17 daemon/volumes.go:352:1: comment on exported method Daemon.WalkVolumes should be of the form "WalkVolumes ..."
06:19:17 
06:19:17 Please fix the above errors. You can test via "golint" and commit the result.

thaJeztah · 2016-10-25T07:32:29Z

ping @bbayani can you look at the failing CI?

cpuguy83 · 2016-10-25T16:13:30Z

If we really need to support prefix matching, we should use go-patricia like we do for containers.
I don't think walking the entire volume list is desirable here.

bbayani · 2016-10-26T05:49:50Z

@cpuguy83 : you commented on issue that you would not prefer go-patricia stuff as you are planning on removing it. Thus implemented the walking for entire volume list. Let me know if you have changed mind. I should implement similar prefix matching to containers.

cpuguy83 · 2016-10-26T13:16:15Z

We can use it while it's there, but I'm also hoping to get a PR in soon that does indeed remove it, so it may be for naught.

lowenna · 2016-11-09T04:24:17Z

How can I help move this forward?

bbayani · 2016-11-09T08:46:23Z

I am not clear. Should I use go-patria approach or not?

bbayani · 2016-11-09T09:31:14Z

Also, i am not able to understand the failing unit test in docker_api_containers_test.go..
FAIL: docker_api_containers_test.go:1776 DockerSuite.TestContainersAPICreateMountsCreate

Can someone guide?

thaJeztah · 2016-11-09T12:10:44Z

@bbayani the test checks if an anonymous volume is automatically removed when the container is removed. Possibly due to the prefix matching, a different volume with the same prefix (e.g. test11) is returned (so the test thinks the volume is not removed). See https://github.com/docker/docker/blob/master/integration-cli/docker_api_containers_test.go#L1898-L1900

Adressing golint failures Addressing unit test failures Addressing gofmt failure Signed-off-by: Bhumika Bayani <bhumikabayani@gmail.com>

bbayani · 2016-11-10T19:16:08Z

Thanks @thaJeztah! The unit tests are passing now.

bbayani · 2016-11-14T06:22:48Z

@thaJeztah All tests are passing now. Can you please let me know if there any more changes needed?

thaJeztah · 2016-11-14T15:02:30Z

Actually, looking at this change, I wonder if this is actually correct.
For other object types (containers, images, networks), we only do partial
matching on ID's, not on names. Volumes in docker don't have an ID, only
a name (for anonymous volumes, a name is generated; it may "look" like an ID,
but it's actually just a random name).

For example;

docker ps
CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS              PORTS               NAMES
04f5e223c6ff        busybox             "sh"                6 seconds ago       Up 5 seconds                            serene_lamport
01367cafcb32        busybox             "sh"                12 seconds ago      Up 11 seconds                           gloomy_murdock
cf079da3c9e3        busybox             "sh"                7 minutes ago       Up 7 minutes                            foobar
995a06a8dc68        busybox             "sh"                7 minutes ago       Up 7 minutes                            con2
e2748f8067d1        busybox             "sh"                7 minutes ago       Up 7 minutes                            con1

Trying to match partial names always returns an error;

root@921abf8761fe:/go/src/github.com/docker/docker# docker container inspect con
[]
Error: No such container: con

root@921abf8761fe:/go/src/github.com/docker/docker# docker container inspect foo
[]
Error: No such container: con

Only a partial ID works;

docker container inspect 99
[
    {
        "Id": "995a06a8dc68b323e227e78edb3afa3ef6e2bea2bd10807a867dbd2aa26f4679",
        "Created": "2016-11-14T13:22:07.952561089Z",
        "Path": "sh",
...

But produces an error if multiple results were found;

root@921abf8761fe:/go/src/github.com/docker/docker# docker container inspect 0
[]
Error response from daemon: Multiple IDs found with provided prefix: 04f5e223c6ff201e8a2f794a6c213f088aba6379ad93c43837880a4720973145

(Note that the error is incorrect, as it doesn't include the partial ID provided, which is something we should address)

Same for images:

docker images
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
wordpress           latest              51f253b66d9f        4 days ago          420 MB
httpd               latest              50f10ef90911        5 days ago          193 MB
busybox             latest              e02e811dd08f        5 weeks ago         1.09 MB

root@921abf8761fe:/go/src/github.com/docker/docker# docker image inspect busy
[]
Error: No such image: busy

docker image inspect 5
[]
Error: No such image: 5

(this error message is inconsistent, so may have to be updated)

And for networks:

docker network ls
NETWORK ID          NAME                DRIVER              SCOPE
35f057677352        bridge              bridge              local
d643531bb6ae        host                host                local
dbbf8b17d60b        net1                bridge              local
b21cd179c0b3        net2                bridge              local
e8f1b9e0ad31        none                null                local
root@921abf8761fe:/go/src/github.com/docker/docker# docker network inspect net
[]
Error: No such network: net
root@921abf8761fe:/go/src/github.com/docker/docker# docker network inspect foo
[]
Error: No such network: foo

docker network inspect d
[]
Error response from daemon: invalid id: d

(this error message is inconsistent, so may have to be updated)

thaJeztah · 2016-11-14T15:05:38Z

With the changes in this PR, partial lookup for volume names is implemented;

docker volume ls
DRIVER              VOLUME NAME
local               db1
local               db2

However, if multiple matches are possible, the first one is used;

docker volume inspect db
[
    {
        "Driver": "local",
        "Labels": {},
        "Mountpoint": "/var/lib/docker/volumes/db1/_data",
        "Name": "db1",
        "Options": {},
        "Scope": "local"
    }
]

I think that;

we should decide if we want partial matches to work for volumes (given that
volumes don't have an ID, that would be consistent with other objects)
if we do device we want it to work, we should produce an error if multiple
results are found
separate to this PR, we should review all the error messages, and make them
consistent (not sure which error message of we prefer)

ping @jhowardmsft @cpuguy83 WDYT?

thaJeztah · 2016-11-14T15:06:58Z

I'm moving this back to design review per the above (sorry @bbayani, not your fault, just now realized we would not be consistent, so this requires additional discussion)

bbayani · 2016-11-14T17:42:44Z

@thaJeztah no issues, once you guys decide on design, I can make required changes in this PR itself? Thanks!

bbayani · 2016-11-21T06:18:00Z

ping @cpuguy83 @jhowardmsft @thaJeztah

cpuguy83 · 2016-12-15T19:12:18Z

@bbayani Thanks for submitting this. In a maintainers meeting we decided that since we don't currently do partial matching on names of other objects it's best not to add them here, for now.

This is not to say this is forever a no, just not at this time.
Thanks again and sorry for the delay in responding to this!

lowenna · 2017-01-03T19:04:18Z

@thaJeztah Can this be re-visited to match by partial-ID?

thaJeztah · 2017-01-03T19:05:44Z

@jhowardmsft the problem is that volumes don't have an ID, only a name. Perhaps it's possible to add the option of having ID's to volumes at some point

lowenna · 2017-01-03T19:08:48Z

@thaJeztah Not quite true....

PS E:\go\src\github.com\docker\docker> docker volume create
547502f15897612c373486df3e8b3b49959ed4b6f56c504dc4ce078c4ae76f61
PS E:\go\src\github.com\docker\docker> docker volume ls
DRIVER              VOLUME NAME
local               547502f15897612c373486df3e8b3b49959ed4b6f56c504dc4ce078c4ae76f61
PS E:\go\src\github.com\docker\docker>

lowenna · 2017-01-03T19:09:30Z

Well half and half - the name is an ID. Which is horrible when it comes to attempting to delete it.

thaJeztah · 2017-01-03T19:12:12Z

Yes, it's a random name, and it "looks" like an ID. I'd personally be +1 if there was a way to give volumes an ID to be consistent with all other objects we have

GordonTheTurtle added status/0-triage dco/no Automatically set by a bot when one of the commits lacks proper signature labels Oct 12, 2016

bbayani force-pushed the volume_id_matching branch from 05d6dea to a089cc8 Compare October 12, 2016 13:10

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Oct 12, 2016

bbayani force-pushed the volume_id_matching branch from e14d06c to 2ab2b17 Compare October 13, 2016 06:18

thaJeztah added status/2-code-review and removed status/0-triage labels Oct 14, 2016

bbayani force-pushed the volume_id_matching branch from c6010e8 to d8374e4 Compare October 25, 2016 10:19

bbayani force-pushed the volume_id_matching branch from 70dc20c to db7030f Compare November 9, 2016 09:02

bbayani force-pushed the volume_id_matching branch from 2ea69cb to 35d787f Compare November 10, 2016 17:26

partial volume ID matching: return error if not found

a5644ef

Adressing golint failures Addressing unit test failures Addressing gofmt failure Signed-off-by: Bhumika Bayani <bhumikabayani@gmail.com>

bbayani force-pushed the volume_id_matching branch from de7ef52 to a5644ef Compare November 10, 2016 17:42

thaJeztah added status/1-design-review and removed status/2-code-review labels Nov 14, 2016

cpuguy83 closed this Dec 15, 2016

thaJeztah mentioned this pull request Dec 15, 2016

docker volume inspect doesn't do partial ID matching #26346

Closed

lowenna mentioned this pull request Jan 4, 2017

Correct comment in vol driver interface #29892

Merged

thaJeztah mentioned this pull request Feb 3, 2025

Flaky test (on Windows): TestContainersAPICreateMountsCreate #49382

Open

Conversation

bbayani commented Oct 12, 2016

Uh oh!

thaJeztah commented Oct 14, 2016

Uh oh!

thaJeztah commented Oct 25, 2016

Uh oh!

cpuguy83 commented Oct 25, 2016

Uh oh!

bbayani commented Oct 26, 2016

Uh oh!

cpuguy83 commented Oct 26, 2016

Uh oh!

lowenna commented Nov 9, 2016

Uh oh!

bbayani commented Nov 9, 2016

Uh oh!

bbayani commented Nov 9, 2016

Uh oh!

thaJeztah commented Nov 9, 2016

Uh oh!

bbayani commented Nov 10, 2016

Uh oh!

bbayani commented Nov 14, 2016

Uh oh!

thaJeztah commented Nov 14, 2016

Uh oh!

thaJeztah commented Nov 14, 2016

Uh oh!

thaJeztah commented Nov 14, 2016

Uh oh!

bbayani commented Nov 14, 2016

Uh oh!

bbayani commented Nov 21, 2016

Uh oh!

cpuguy83 commented Dec 15, 2016

Uh oh!

lowenna commented Jan 3, 2017

Uh oh!

thaJeztah commented Jan 3, 2017

Uh oh!

lowenna commented Jan 3, 2017

Uh oh!

lowenna commented Jan 3, 2017

Uh oh!

thaJeztah commented Jan 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants