-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
Conversation
return nil, ErrNoSuchVolume | ||
} | ||
return vc.Volume, nil | ||
|
||
logrus.Debugf("Propbing all volume drivers for volume with name: %s", name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Probing all volume drivers"...
fe76583
to
d7b0e5e
Compare
ext = NewVolumeDriver(p.Name, p.Client) | ||
drivers.extensions[p.Name] = ext | ||
} | ||
ds = append(ds, ext) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check plugin status with Activate() here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done in plugins.GetAll()
, or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I missed that :(
d7b0e5e
to
c960a90
Compare
s.mu.Unlock() | ||
return v, nil | ||
<-vc.waitCreate | ||
if vc.Volume != nil && (vc.Volume.DriverName() == driverName || driverName == volume.DefaultDriverName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
driverName="local" vc.Volume.DriverName() = "test" will return true
driverName="" vc.Volume.DriverName() = "local" will return false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is correct, but also doesn't get passed in that way... but I will update to ensure it's never a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
.
c960a90
to
007a166
Compare
if err != nil { | ||
return nil, err | ||
} | ||
|
||
v, err := vd.Create(name, opts) | ||
v, err = vd.Create(name, opts) | ||
if err != nil { | ||
return nil, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if err != nil {
vc.Volume = v
return v, nil
}
create() succeessfully, should return v?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the err is not nil, then we need to return the error.
I'm going to close this for now. |
Doh! Just got back from London and started to read up on this, LOL. I'll watch for the replacement PR 👍 |
a80d41b
to
5587a9a
Compare
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 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. |
5587a9a
to
ff2572f
Compare
s.nameLocker.Unlock(v.Name()) | ||
} | ||
s.mu.Unlock() | ||
return vols, warnings, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doh! will fix.
b0ec3ae
to
ba81fa1
Compare
Just allow me to turn it off and I'll be happy. |
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. |
@thaJeztah The user also gets a warning in this case. |
Ah ok! :) On 15 Dec 2015, at 11:26, Brian Goff wrote:
|
PR review session; moving to code review. We'll have a separate discussion about storing information about volumes or not. |
f769e3f
to
8136da9
Compare
@cpuguy83 tests fail; could it be because
|
8136da9
to
b61d658
Compare
@thaJeztah Thanks. |
LGTM |
ping @cpuguy83, needs another rebase, sorry 😢 |
|
||
// GetAll returns all the plugins for the specified implementation | ||
func GetAll(imp string) ([]*Plugin, error) { | ||
pluginNames, err := Scan() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
LGTM, before the rebase :) I'll make sure to have another look after :) |
b61d658
to
6a3a7ba
Compare
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>
6a3a7ba
to
d3eca44
Compare
Rebased |
LGTM - also note that Windows passed CI but failed on a technicality related to cleanup scripts 😿 |
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
🎉 |
Makes
docker volume ls
anddocker volume inspect
ask the volumedrivers 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 isalready 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
andGet
.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