Thanks for opening this PR @clintonskitson. I agree that those improvements should make the volume extensions much more useful. I don't think we should try to put everything into a single pull request, it will be much harder to review and merge.

I think you should check these three PRs out too:

#13524
#13951
#13835

Also I've got a working implementation of a top level volumes API (just the API atm, not CLI).
It does need some discussion on how certain semantics should be handled.

@cpuguy83 the commit clintkitson@26539cf may be interesting to review as it includes some semantics around extra settings and snapshot management building from your 1.4.1 work. It includes shoehorning in a Go volume manager at the time to get EC2 and other storage platform integration but represents some aspects that may be interesting to include in the API design. But yes, definitely a good conversation can be had around how someone would interact with the additional semantics available.

@calavera and @cpuguy83, would you support the idea that the three items left could represent a PR? I believe the three are inter-related to include the ability for a remote driver to function. Although the abstraction in the first item is critical to allow complete packages to be embedded similar to the graph driver implementations.

Any guidance on how you would want to see the three related items submitted would be appreciated.

Changed the scope of the PR to focus only on abstracting volume driver proxy.

This one just got re-based and all tests are passing.

ping @calavera @cpuguy83

I keep going back and forth with this PR and I have a couple concerns/unknowns:

1. It looks like the volume selection is driven now by what the plugin says that it implements. I kinda like that, but I wonder what can happen if a plugin says that it implements Local. It also makes me wonder if the local driver needs any init function at all.
2. Options in the init function are never used, there might be a use case there, but I'd rather add them when we really need them.

1. I would agree with that. I believe the Local could sit outside the initialization process since it is called in a static way today.
2. I will make this change.

Ping @icecrime and @calavera, the PR has been updated and rebased.

I just left a couple of minor comments, but it's looking great so far.

I made those small tweaks.

LGTM

Rebased for new merges.

Is there further work that needs to take place on this one? @calavera

As far as I know it only needs a review by another maintainer.

/cc @cpuguy83, @tiborvass

@calavera Rebased.

@cpuguy83 following up on a conversation we had around using the code generator a couple of plugin files.

1. The https://github.com/docker/docker/blob/master/volume/drivers/extpoint.go file had a header that says //go:generate pluginrpc-gen -i $GOFILE -o proxy.go -type VolumeDriver -name VolumeDriver but it looks to be misplaced. This file doesn't look to have been generated automatically?
2. Relating to the proxy.go file.. The pluginrpc-gen tools looks to need to be updated based on the abstraction of drivers. Key differences a) Package name is now different per driver b) Options are coming from separate package c) Will move newProxy to localvolumedriver.go and make it local d) VolumeDriver may not be a proper input field based on new abstraction

~/go/bin/pluginrpc-gen -i ~/go/src/github.com/docker/docker/volume/drivers/localvolumedriver/localvolumedriver.go -o proxy.go -type VolumeDriver -name VolumeDriver
error parsing requested type VolumeDriver: could not find object VolumeDriver in /Users/clintonkitson/go/src/github.com/docker/docker/volume/drivers/localvolumedriver/localvolumedriver.go

The abstraction of the drivers, or further implementation towards a driver pattern in certain ways that may overlap with the code generation tool. If leveraging the defined Docker Volume API then this it can be a valid generation tool. But the new driver abstraction can open the door for alternative APIs or vendoring to achieve the same functionality which wouldn't necessarily be code gen'd.

dicey1:docker clintonkitson$ diff proxy.go volume/drivers/localvolumedriver/proxy.go 
1,3c1
< // generated code - DO NOT EDIT
< 
< package volumedrivers
---
> package localvolumedriver
4a3
> import volumedrivers "github.com/docker/docker/volume/drivers"
17c16
<   Opts opts
---
>   Opts volumedrivers.Opts
24c23
< func (pp *volumeDriverProxy) Create(name string, opts opts) (err error) {
---
> func (pp *volumeDriverProxy) Create(name string, opts volumedrivers.Opts) (err error) {
42a42,45
> func NewProxy(c client) *volumeDriverProxy {
>   return &volumeDriverProxy{c}
> }
> 

@clintonskitson the purpose of the code generator is to read a defined interface and generate the relevant proxy code that can call out over the plugin API.
The file with the go:generate header generates proxy.go from the VolumeDriver interface defined in that file.
This makes sure the proxy code is always in sync with the actual driver interface without having to manually update.... not that we have to use it, but that syncing still needs to take place.
I think in this case we need a driver to adapt the real volume driver interface to what we model over the plugin interface, and then generate the proxy code from there. This is what extpoint.go and proxy.go are doing today.

There should only ever be 1 interface for dealing with drivers as far as the daemon can see.

@cpuguy83 Re the previous comment around There should only ever be 1 interface for dealing with drivers as far as the daemon can see.. Definitely true. The single driver interface<>multiple drivers is what we are doing here. The enabling part of this though is that certain drivers like localvolumedriver may choose to leverage the proxy implementation for comm. Something like remotevolumedriver may use a proxy but with additional methods (attach/detach/networkname) under the covers. Others may choose to leverage a vendored approach. And more specifically this enables the general volumedrivers.Mount method to work uniformly through all of these approaches.

It seems like the pluginrpc-gen needs to be updated here if is to be used to gen the proxy.go file. Once there is a stable proxy.go, I think popping open that and the template would then make sense.

Collective review @cpuguy83 @vbatts @duglin @tonistiigi @abronan

@clintonskitson

If we're taking a step back, what's the purpose of this refactoring?

If it is to allow multiple builtin volume drivers, then it's not clear that is desired at this time, because we already have volume drivers as plugins (although, out-of-process) and you can already have multiple of those for the same container.

What's wrong with docker run -v $(docker volume create --driver=flocker):/foo -v /bar ? That will have /foo be handled via flocker, whereas /bar will be handled by the default local volume driver.

Throwing back to design review since it seems like there's some discussion on if we want to make this change.

Yes very valid today as is. The opportunity however is that is the volume driver, if properly abstracted would allow for volume drivers that operate in slightly different ways. For example, today the volume driver represents local capabilities where the volume drivers always need to reside with the Docker engine.

For example, a Mount method gets called which can only be done by something local. With the enhancement, there could be a RemoteVolumeDriver which would interpret the general Driver interface Mount call to actually perform some local investigation, ie. who is the Docker's underlying host, and then pass this information off to a remote storage platform directly.

The trajectory of this functionality goes towards allowing this and a vendor'd approach for managing volumes and something that gets closer to something like libStorage in the future.

@clintonskitson then what you're asking for is for a volume driver to determine whether it is running on the same host as the engine that's making requests. That seems fair to me. /cc @cpuguy83

Thanks for the response @tiborvass. @cpuguy83, let me know what I can do to assist.

@clintonskitson I haven't been ignoring. I think it makes sense.. however before making changes I'd personally like to see how things with libnetwork/network-overlay shake out in the engine so that we can follow the same kind of model (and discovery mechanisms) for remote drivers (and potentially extract remote/local/other into a single interface).

@clintonskitson, @cpuguy83 I'm going to close this PR. I like how this was looking but I feel like we're not arriving to a good compromise that we all agree on. We're still changing the internals of the volume plugins too much and I don't think it's reasonable to have this PR waiting around until things settle down.

Please, feel free to reopen if you think we must move this forward, we could reconsider it. But, I think we should wait to land this change in at least until #16534 is merged.