Move responsibility of ls/inspect to volume driver#16534

cpuguy83 · 2015-09-23T20:43:29Z

Makes docker volume ls and docker volume inspect ask the volume
drivers rather than only using what is cached locally.

Previously in order to use a volume from an external driver, one would
either have to use docker volume create or have a container that is
already using that volume for it to be visible to the other volume
API's.

For keeping uniqueness of volume names in the daemon, names are bound to
a driver on a first come first serve basis. If two drivers have a volume
with the same name, the first one is chosen, and a warning is logged
about the second one.

Adds 2 new methods to the plugin API, List and Get.
If a plugin does not implement these endpoints, a user will not be able
to find the specified volumes as well requests go through the drivers.

Fixes #15997
Closes #16513
Closes #16473

ping @calavera @srust @thaJeztah @lukemarsden

srust · 2015-09-23T21:53:48Z

volume/store/store.go

 		return nil, ErrNoSuchVolume
 	}
-	return vc.Volume, nil
+
+	logrus.Debugf("Propbing all volume drivers for volume with name: %s", name)


"Probing all volume drivers"...

x1022as · 2015-09-24T01:36:15Z

volume/drivers/extpoint.go

+			ext = NewVolumeDriver(p.Name, p.Client)
+			drivers.extensions[p.Name] = ext
+		}
+		ds = append(ds, ext)


check plugin status with Activate() here?

This is done in plugins.GetAll(), or am I missing something?

sorry, I missed that :(

x1022as · 2015-09-24T02:09:09Z

volume/store/store.go

 		s.mu.Unlock()
-		return v, nil
+		<-vc.waitCreate
+		if vc.Volume != nil && (vc.Volume.DriverName() == driverName || driverName == volume.DefaultDriverName) {


driverName="local" vc.Volume.DriverName() = "test" will return true
driverName="" vc.Volume.DriverName() = "local" will return false

@cpuguy83 Don't understand the logic driverName == volume.DefaultDriverName. Does it mean if the
vc.Volume.DriverName() is convoy, but the driverName is local and then return the volume?

And one question, from what I understand VolumeStore only store the volumes which has been reference by
containers, so there is the situation, a volume exists in external volume driver but not stored in VolumeStore, so the Create will create a volume has the same name with these volumes. So we can't ensure the volume has a unique name. I think it's better to check if the volume is exist in external volume driver, but anyway, this may take long time to create a volume. Am I missing something?

This is correct, but also doesn't get passed in that way... but I will update to ensure it's never a problem.

@coolljt0725 This means that if the passed in driver name is the default driver name, but the one that we already have is for some other driver (like convoy), then we will assume that the user wanted convoy and not something from the default driver.
This is really a problem due to --volume-driver on docker run.

x1022as · 2015-09-24T03:04:28Z

volume/store/store.go

 	if err != nil {
 		return nil, err
 	}

-	v, err := vd.Create(name, opts)
+	v, err = vd.Create(name, opts)
 	if err != nil {
 		return nil, err


if err != nil {
vc.Volume = v
return v, nil
}
create() succeessfully, should return v?

if the err is not nil, then we need to return the error.

cpuguy83 · 2015-09-24T23:21:37Z

I'm going to close this for now.
The current VolumeStore implementation has just become too complex to suppor the finer grained locking that's needed as well as these enhancements.
I've got some other stuff in mind, stay tuned!

thaJeztah · 2015-09-24T23:30:36Z

Doh! Just got back from London and started to read up on this, LOL. I'll watch for the replacement PR 👍

cpuguy83 · 2015-09-28T18:22:37Z

This is now updated.

As a side note, I've found the current implementation using singletons for plugins and volume extpoints makes unit tests rather difficult
Also slow for unit tests testing drivers that are non-existant/missing.
This is not fixed but would make a good follow-up.

Also found that the integration tests for the external volume drivers, while passing, were passing due to a bug in the implementation of the plugin driver in that test file. This is fixed.

x1022as · 2015-09-29T03:08:59Z

volume/store/store.go

+		s.nameLocker.Unlock(v.Name())
+	}
+	s.mu.Unlock()
+	return vols, warnings, nil


return out?

doh! will fix.

erikh · 2015-12-15T19:10:42Z

Just allow me to turn it off and I'll be happy.

erikh · 2015-12-15T19:14:40Z

More accurately, I'd really like to avoid introducing a THIRD distributed database to manage my ceph volumes. I would really not like this behavior to cloud what I have that's already working reliably, so I can fight docker's etcd implementation too.

This is also really easy to get wrong (I've been doing it for the last 3-4 months now) and I would strongly caution you against doing it if you're not comfortable implementing crash recovery.

cpuguy83 · 2015-12-15T19:22:23Z

@thaJeztah The user also gets a warning in this case.
@erikh Definitely not implementing distributed K/V store in this PR at the very very least :)

erikh · 2015-12-15T19:36:34Z

Ah ok! :)

On 15 Dec 2015, at 11:26, Brian Goff wrote:

@thaJeztah The user also gets a warning in this case.
@erikh Definitely not implementing distributed K/V store in this PR at
the very very least :)

Reply to this email directly or view it on GitHub:
#16534 (comment)

thaJeztah · 2015-12-18T20:00:52Z

PR review session; moving to code review. We'll have a separate discussion about storing information about volumes or not.

thaJeztah · 2015-12-21T11:03:11Z

@cpuguy83 tests fail; could it be because no such volume now is No such volume (with a capital)?

docker_cli_start_volume_driver_unix_test.go:433:
    c.Assert(out, checker.Contains, "no such volume")
... obtained string = "" +
...     "[]\n" +
...     "Error: No such volume: dummy\n"
... substring string = "no such volume"

cpuguy83 · 2015-12-28T20:05:05Z

@thaJeztah Thanks.
Fixed and rebased.

thaJeztah · 2015-12-29T13:50:28Z

ping @calavera @runcom could you review this? Let's see if we can get this moving 👍

calavera · 2015-12-29T22:38:19Z

LGTM

thaJeztah · 2016-01-05T16:52:37Z

ping @cpuguy83, needs another rebase, sorry 😢

runcom · 2016-01-05T17:27:20Z

pkg/plugins/plugins.go

+
+// GetAll returns all the plugins for the specified implementation
+func GetAll(imp string) ([]*Plugin, error) {
+	pluginNames, err := Scan()


the only nit I have is this scan and activation of all plugins prior to checking if they implement what we're interested in (plugins may be many?).. I guess there's no solution right now.. (maybe using subdirs for specific plugins - /volumes, /network to narrow down the list? 😄 not related to this PR only)

runcom · 2016-01-05T17:27:55Z

LGTM, before the rebase :) I'll make sure to have another look after :)

Makes `docker volume ls` and `docker volume inspect` ask the volume drivers rather than only using what is cached locally. Previously in order to use a volume from an external driver, one would either have to use `docker volume create` or have a container that is already using that volume for it to be visible to the other volume API's. For keeping uniqueness of volume names in the daemon, names are bound to a driver on a first come first serve basis. If two drivers have a volume with the same name, the first one is chosen, and a warning is logged about the second one. Adds 2 new methods to the plugin API, `List` and `Get`. If a plugin does not implement these endpoints, a user will not be able to find the specified volumes as well requests go through the drivers. Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 · 2016-01-05T21:29:16Z

Rebased

estesp · 2016-01-05T22:21:08Z

LGTM - also note that Windows passed CI but failed on a technicality related to cleanup scripts 😿

thaJeztah · 2016-01-05T22:29:13Z

as discussed on IRC, there will be some docs changes in docs/extend/plugins_volume.md, but those will be in a separate PR. We want this merged because it's long overdue and it's better to have it merged before the API is moved to docker/engine-api :)

Move responsibility of ls/inspect to volume driver

cpuguy83 · 2016-01-06T02:08:38Z

🎉

GordonTheTurtle added the status/0-triage label Sep 23, 2015

cpuguy83 added status/1-design-review and removed status/0-triage labels Sep 23, 2015

srust reviewed Sep 23, 2015
View reviewed changes

cpuguy83 force-pushed the make_volume_drivers_responsible branch 2 times, most recently from fe76583 to d7b0e5e Compare September 24, 2015 01:11

x1022as reviewed Sep 24, 2015
View reviewed changes

cpuguy83 force-pushed the make_volume_drivers_responsible branch from d7b0e5e to c960a90 Compare September 24, 2015 02:05

x1022as reviewed Sep 24, 2015
View reviewed changes

cpuguy83 force-pushed the make_volume_drivers_responsible branch from c960a90 to 007a166 Compare September 24, 2015 02:10

x1022as reviewed Sep 24, 2015
View reviewed changes

cpuguy83 closed this Sep 24, 2015

cpuguy83 reopened this Sep 28, 2015

cpuguy83 force-pushed the make_volume_drivers_responsible branch 5 times, most recently from a80d41b to 5587a9a Compare September 28, 2015 18:11

cpuguy83 force-pushed the make_volume_drivers_responsible branch from 5587a9a to ff2572f Compare September 28, 2015 18:52

x1022as reviewed Sep 29, 2015
View reviewed changes

cpuguy83 force-pushed the make_volume_drivers_responsible branch 5 times, most recently from b0ec3ae to ba81fa1 Compare September 30, 2015 01:42

thaJeztah added status/2-code-review and removed status/1-design-review labels Dec 18, 2015

cpuguy83 force-pushed the make_volume_drivers_responsible branch from f769e3f to 8136da9 Compare December 19, 2015 15:02

cpuguy83 force-pushed the make_volume_drivers_responsible branch from 8136da9 to b61d658 Compare December 28, 2015 19:19

runcom reviewed Jan 5, 2016
View reviewed changes

cpuguy83 force-pushed the make_volume_drivers_responsible branch from b61d658 to 6a3a7ba Compare January 5, 2016 21:27

cpuguy83 force-pushed the make_volume_drivers_responsible branch from 6a3a7ba to d3eca44 Compare January 5, 2016 21:28

cpuguy83 added the impact/changelog label Jan 5, 2016

estesp added a commit that referenced this pull request Jan 5, 2016

Merge pull request #16534 from cpuguy83/make_volume_drivers_responsible

55137c1

Move responsibility of ls/inspect to volume driver

estesp merged commit 55137c1 into moby:master Jan 5, 2016

calavera mentioned this pull request Jan 5, 2016

Add volume list response warnings. docker/engine-api#3

Merged

cpuguy83 deleted the make_volume_drivers_responsible branch January 6, 2016 02:08

cpuguy83 mentioned this pull request Jan 21, 2016

Bump plugin API version #19549

Merged

msterin mentioned this pull request Feb 4, 2016

list of items - A Road to MVP (minimum viable product) vmware-archive/vsphere-storage-for-docker#3

Closed

29 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move responsibility of ls/inspect to volume driver#16534

Move responsibility of ls/inspect to volume driver #16534

cpuguy83 commented Sep 23, 2015

srust Sep 23, 2015

x1022as Sep 24, 2015

cpuguy83 Sep 24, 2015

x1022as Sep 24, 2015

x1022as Sep 24, 2015

coolljt0725 Sep 24, 2015

cpuguy83 Sep 24, 2015

cpuguy83 Sep 24, 2015

x1022as Sep 24, 2015

cpuguy83 Sep 24, 2015

cpuguy83 commented Sep 24, 2015

thaJeztah commented Sep 24, 2015

cpuguy83 commented Sep 28, 2015

x1022as Sep 29, 2015

cpuguy83 Sep 29, 2015

erikh commented Dec 15, 2015

erikh commented Dec 15, 2015

cpuguy83 commented Dec 15, 2015

erikh commented Dec 15, 2015

thaJeztah commented Dec 18, 2015

thaJeztah commented Dec 21, 2015

cpuguy83 commented Dec 28, 2015

thaJeztah commented Dec 29, 2015

calavera commented Dec 29, 2015

thaJeztah commented Jan 5, 2016

runcom Jan 5, 2016

runcom commented Jan 5, 2016

cpuguy83 commented Jan 5, 2016

estesp commented Jan 5, 2016

thaJeztah commented Jan 5, 2016

cpuguy83 commented Jan 6, 2016

Move responsibility of ls/inspect to volume driver#16534

Move responsibility of ls/inspect to volume driver #16534

Conversation

cpuguy83 commented Sep 23, 2015

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

Choose a reason for hiding this comment

cpuguy83 commented Sep 24, 2015

thaJeztah commented Sep 24, 2015

cpuguy83 commented Sep 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikh commented Dec 15, 2015

erikh commented Dec 15, 2015

cpuguy83 commented Dec 15, 2015

erikh commented Dec 15, 2015

thaJeztah commented Dec 18, 2015

thaJeztah commented Dec 21, 2015

cpuguy83 commented Dec 28, 2015

thaJeztah commented Dec 29, 2015

calavera commented Dec 29, 2015

thaJeztah commented Jan 5, 2016

Choose a reason for hiding this comment

runcom commented Jan 5, 2016

cpuguy83 commented Jan 5, 2016

estesp commented Jan 5, 2016

thaJeztah commented Jan 5, 2016

cpuguy83 commented Jan 6, 2016