New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move responsibility of ls/inspect to volume driver #16534

Merged
merged 1 commit into from Jan 5, 2016

Conversation

Projects
None yet
@cpuguy83
Contributor

cpuguy83 commented Sep 23, 2015

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

Show outdated Hide outdated volume/store/store.go
Show outdated Hide outdated volume/store/store.go
Show outdated Hide outdated volume/store/store.go
Show outdated Hide outdated volume/store/store.go
@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Sep 24, 2015

Contributor

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!

Contributor

cpuguy83 commented Sep 24, 2015

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!

@cpuguy83 cpuguy83 closed this Sep 24, 2015

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Sep 24, 2015

Member

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

Member

thaJeztah commented Sep 24, 2015

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

@cpuguy83 cpuguy83 reopened this Sep 28, 2015

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Sep 28, 2015

Contributor

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.

Contributor

cpuguy83 commented Sep 28, 2015

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.

Show outdated Hide outdated volume/store/store.go
Show outdated Hide outdated volume/store/store.go
@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Sep 30, 2015

Contributor

Moving this to code review, because it's a very necessary change.

Contributor

calavera commented Sep 30, 2015

Moving this to code review, because it's a very necessary change.

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Sep 30, 2015

Contributor

were passing due to a bug in the implementation of the plugin driver in that test file

😔

Contributor

calavera commented Sep 30, 2015

were passing due to a bug in the implementation of the plugin driver in that test file

😔

vol := strings.Fields(ls[len(ls)-1])
c.Assert(len(vol), check.Equals, 2, check.Commentf("%v", vol))
c.Assert(vol[0], check.Equals, "test-external-volume-driver")

This comment has been minimized.

@calavera

calavera Sep 30, 2015

Contributor

is the order here consistent? this is not going to turn into a flaky test, right?? 😬

@calavera

calavera Sep 30, 2015

Contributor

is the order here consistent? this is not going to turn into a flaky test, right?? 😬

This comment has been minimized.

@cpuguy83

cpuguy83 Sep 30, 2015

Contributor

This is not 2 volumes, it is one volume and we're checking the driver name and name are correct.

@cpuguy83

cpuguy83 Sep 30, 2015

Contributor

This is not 2 volumes, it is one volume and we're checking the driver name and name are correct.

This comment has been minimized.

@calavera

calavera Sep 30, 2015

Contributor

oh, sorry, you're right.

@calavera

calavera Sep 30, 2015

Contributor

oh, sorry, you're right.

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Sep 30, 2015

Contributor

I have a couple of minor comments, but the implementation looks good so far.
I'm pretty sure we can use that locker package somewhere else.

Contributor

calavera commented Sep 30, 2015

I have a couple of minor comments, but the implementation looks good so far.
I'm pretty sure we can use that locker package somewhere else.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Sep 30, 2015

Contributor

@calavera Updated to use an IsInUse function.

Contributor

cpuguy83 commented Sep 30, 2015

@calavera Updated to use an IsInUse function.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Oct 1, 2015

Contributor

Holy 💩, it's all green, praise be unto the CI gods.

Contributor

cpuguy83 commented Oct 1, 2015

Holy 💩, it's all green, praise be unto the CI gods.

@cpuguy83 cpuguy83 added this to the 1.9.0 milestone Oct 2, 2015

Volume *proxyVolume
Err string
}

This comment has been minimized.

@erikh

erikh Oct 7, 2015

Contributor

@cpuguy83 would it be possible to have exported versions of these + other structs used in the volume plugins?

@erikh

erikh Oct 7, 2015

Contributor

@cpuguy83 would it be possible to have exported versions of these + other structs used in the volume plugins?

This comment has been minimized.

@cpuguy83

cpuguy83 Oct 7, 2015

Contributor

I'd rather not, especially since this is generated code.

@cpuguy83

cpuguy83 Oct 7, 2015

Contributor

I'd rather not, especially since this is generated code.

@@ -1143,8 +1138,5 @@ func configureVolumes(config *Config) (*store.VolumeStore, error) {
}
volumedrivers.Register(volumesDriver, volumesDriver.Name())
s := store.New()
s.AddAll(volumesDriver.List())

This comment has been minimized.

@tiborvass

tiborvass Oct 12, 2015

Collaborator

@cpuguy83 can you explain why this was removed?

@tiborvass

tiborvass Oct 12, 2015

Collaborator

@cpuguy83 can you explain why this was removed?

This comment has been minimized.

@cpuguy83

cpuguy83 Oct 13, 2015

Contributor

Because it's handled by the "List" call now.

@cpuguy83

cpuguy83 Oct 13, 2015

Contributor

Because it's handled by the "List" call now.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Oct 13, 2015

Contributor

Rebased to fix a some issues with code changes from another merged PR.

Contributor

cpuguy83 commented Oct 13, 2015

Rebased to fix a some issues with code changes from another merged PR.

@erikh

This comment has been minimized.

Show comment
Hide comment
@erikh

erikh Dec 14, 2015

Contributor

I see. Just so I'm clear, I think you're talking about two plugins
reporting the same volume name on a given host?

On 14 Dec 2015, at 12:09, Sebastiaan van Stijn wrote:

@erikh what's your proposal on handling "enabling a new plugin that
has a conflicting name"? Because you don't want a new plugin to be
"hijacking" an existing volume.


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

Contributor

erikh commented Dec 14, 2015

I see. Just so I'm clear, I think you're talking about two plugins
reporting the same volume name on a given host?

On 14 Dec 2015, at 12:09, Sebastiaan van Stijn wrote:

@erikh what's your proposal on handling "enabling a new plugin that
has a conflicting name"? Because you don't want a new plugin to be
"hijacking" an existing volume.


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

@erikh

This comment has been minimized.

Show comment
Hide comment
@erikh

erikh Dec 14, 2015

Contributor

OK, I think it should be listable, but not modifiable or usable. That
is, the daemon could list on each mount/create/remove and determine
whether or not it was dealing with a duplicate volume, and reject that
operation.

This way the administrator still has diagnostic vision into the
situation, but the daemon is prevented from making any critical
mistakes.

Resolving it would be to disable one of the plugins and resolve the
conflict, either by renaming or removing the volume for one of the
plugins.

On 14 Dec 2015, at 12:09, Sebastiaan van Stijn wrote:

@erikh what's your proposal on handling "enabling a new plugin that
has a conflicting name"? Because you don't want a new plugin to be
"hijacking" an existing volume.


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

Contributor

erikh commented Dec 14, 2015

OK, I think it should be listable, but not modifiable or usable. That
is, the daemon could list on each mount/create/remove and determine
whether or not it was dealing with a duplicate volume, and reject that
operation.

This way the administrator still has diagnostic vision into the
situation, but the daemon is prevented from making any critical
mistakes.

Resolving it would be to disable one of the plugins and resolve the
conflict, either by renaming or removing the volume for one of the
plugins.

On 14 Dec 2015, at 12:09, Sebastiaan van Stijn wrote:

@erikh what's your proposal on handling "enabling a new plugin that
has a conflicting name"? Because you don't want a new plugin to be
"hijacking" an existing volume.


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

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 14, 2015

Member

@erikh yes, e.g.

docker volume create foo --driver=zzz
docker volume ls

DRIVER              VOLUME NAME
zzz                 foo

# install driver aaa. driver already has a volume
# named "foo" registered.
# restart daemon

DRIVER              VOLUME NAME
aaa                 foo
Member

thaJeztah commented Dec 14, 2015

@erikh yes, e.g.

docker volume create foo --driver=zzz
docker volume ls

DRIVER              VOLUME NAME
zzz                 foo

# install driver aaa. driver already has a volume
# named "foo" registered.
# restart daemon

DRIVER              VOLUME NAME
aaa                 foo
@srust

This comment has been minimized.

Show comment
Hide comment
@srust

srust Dec 15, 2015

Contributor

Hi,

In the context of third-party plugins, this PR is pretty important. It allows volumes to be managed out of band. It enables more predictable/reliable multi-host access to volumes. It gets docker out of the storage management business (which is a good thing!).

As @cpuguy83 has eluded to, one docker host does not "own" a volume, in the third-party plugin case. In fact, the local volume plugin on each docker host does not even contain the data or the volume necessarily (think: iscsi, etc.). The volume plugin enables access to the data. It configures access for the data. The data and actual volume can exist anywhere.

Metadata on a per-daemon basis as @thaJeztah describes is what we have today in 1.9. I think the limitations of that approach have been clearly described in at least #15997. Would be happy to go into it again. :)

To work around the volume name conflict issue, @calavera has suggested here and elsewhere that a good "best practice" can be to name your volumes well. You can easily include the volume plugin name in the volume name itself (Use volume@plugin, volume2@plugin2 for the volume names). Indeed, if you are using multiple volume plugins, you probably should be naming your volumes well.

If additional bumpers are really required/desired, I think they should be handled outside of this PR, where we can debate fully the merits of any approach. It should not hold back this PR, in my opinion. The functionality implemented here, does not introduce this behavior. In a multi-host environment, this same problem exists today. It would be good to get this in.

Thanks.

Contributor

srust commented Dec 15, 2015

Hi,

In the context of third-party plugins, this PR is pretty important. It allows volumes to be managed out of band. It enables more predictable/reliable multi-host access to volumes. It gets docker out of the storage management business (which is a good thing!).

As @cpuguy83 has eluded to, one docker host does not "own" a volume, in the third-party plugin case. In fact, the local volume plugin on each docker host does not even contain the data or the volume necessarily (think: iscsi, etc.). The volume plugin enables access to the data. It configures access for the data. The data and actual volume can exist anywhere.

Metadata on a per-daemon basis as @thaJeztah describes is what we have today in 1.9. I think the limitations of that approach have been clearly described in at least #15997. Would be happy to go into it again. :)

To work around the volume name conflict issue, @calavera has suggested here and elsewhere that a good "best practice" can be to name your volumes well. You can easily include the volume plugin name in the volume name itself (Use volume@plugin, volume2@plugin2 for the volume names). Indeed, if you are using multiple volume plugins, you probably should be naming your volumes well.

If additional bumpers are really required/desired, I think they should be handled outside of this PR, where we can debate fully the merits of any approach. It should not hold back this PR, in my opinion. The functionality implemented here, does not introduce this behavior. In a multi-host environment, this same problem exists today. It would be good to get this in.

Thanks.

@erikh

This comment has been minimized.

Show comment
Hide comment
@erikh

erikh Dec 15, 2015

Contributor

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

Contributor

erikh commented Dec 15, 2015

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

@erikh

This comment has been minimized.

Show comment
Hide comment
@erikh

erikh Dec 15, 2015

Contributor

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.

Contributor

erikh commented Dec 15, 2015

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

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Dec 15, 2015

Contributor

@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 :)

Contributor

cpuguy83 commented Dec 15, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@erikh

erikh Dec 15, 2015

Contributor

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)

Contributor

erikh commented Dec 15, 2015

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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 18, 2015

Member

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

Member

thaJeztah commented Dec 18, 2015

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

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 21, 2015

Member

@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"
Member

thaJeztah commented Dec 21, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Dec 28, 2015

Contributor

@thaJeztah Thanks.
Fixed and rebased.

Contributor

cpuguy83 commented Dec 28, 2015

@thaJeztah Thanks.
Fixed and rebased.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 29, 2015

Member

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

Member

thaJeztah commented Dec 29, 2015

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

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Dec 29, 2015

Contributor

LGTM

Contributor

calavera commented Dec 29, 2015

LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 5, 2016

Member

ping @cpuguy83, needs another rebase, sorry 😢

Member

thaJeztah commented Jan 5, 2016

ping @cpuguy83, needs another rebase, sorry 😢

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

This comment has been minimized.

@runcom

runcom Jan 5, 2016

Member

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

runcom Jan 5, 2016

Member

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

This comment has been minimized.

Show comment
Hide comment
@runcom

runcom Jan 5, 2016

Member

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

Member

runcom commented Jan 5, 2016

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

Move responsibility of ls/inspect to volume driver
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

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jan 5, 2016

Contributor

Rebased

Contributor

cpuguy83 commented Jan 5, 2016

Rebased

@estesp

This comment has been minimized.

Show comment
Hide comment
@estesp

estesp Jan 5, 2016

Contributor

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

Contributor

estesp commented Jan 5, 2016

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

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 5, 2016

Member

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 :)

Member

thaJeztah commented Jan 5, 2016

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 :)

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

Merge pull request #16534 from cpuguy83/make_volume_drivers_responsible
Move responsibility of ls/inspect to volume driver

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

5 of 6 checks passed

windows Jenkins build Windows-PRs 19721 has failed
Details
docker/dco-signed All commits signed
Details
documentation success 2 tests run, 0 skipped, 0 failed.
Details
experimental Jenkins build Docker-PRs-experimental 13072 has succeeded
Details
janky Jenkins build Docker-PRs 21865 has succeeded
Details
userns Jenkins build Docker-PRs-userns 4313 has succeeded
Details

@cpuguy83 cpuguy83 deleted the cpuguy83:make_volume_drivers_responsible branch Jan 6, 2016

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jan 6, 2016

Contributor

🎉

Contributor

cpuguy83 commented Jan 6, 2016

🎉

@cpuguy83 cpuguy83 referenced this pull request Jan 21, 2016

Merged

Bump plugin API version #19549

@msterin msterin referenced this pull request Feb 4, 2016

Closed

list of items - A Road to MVP (minimum viable product) #3

21 of 29 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment