-
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
Add support for volume scopes #22077
Conversation
4f8374b
to
9316d8f
Compare
Global networks are stored in the KV store, but for volumes we already rely on the volume plugins to tell us everything about the volumes so there is seemingly no requirement to do that here. |
9316d8f
to
50b3949
Compare
{} | ||
``` | ||
|
||
Get the list of volumes registered with the plugin. |
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.
Possibly copy-pasta; Think this should be something about "Capabilities"?
Note for reviewers, most of the change here is fixing known limitations in the plugin RPC parser/generator + lots of new tests. |
@cpuguy83 Am I able to set the scope on a volume for a plugin that is not "multi-host" ? (like on a volume that is using |
@vdemeester It will be |
@cpuguy83 ah ok good ! Thanks :) |
@cpuguy83 docker/engine-api#216 merged, you can add a |
@cpuguy83 ah right 😝 |
50b3949
to
a985412
Compare
vendor updated this one is ready. |
a985412
to
e73bd72
Compare
} | ||
// import paths have quotes already added in, so need to remove them for name comparison | ||
name = strings.TrimPrefix(name, "\"") | ||
name = strings.TrimSuffix(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.
nit: could be just
name = strings.Trim(name, `"`)
Would it be better to error out if an unknown scope is provided? I'm never really comfortable with silently ignoring invalid values. It could easily lead to backward compatibility issues if it's ignored now, but becomes a valid value in future. |
@thaJeztah Instead what we can do is just not register the driver if it's providing an invalid scope. |
e73bd72
to
c7a6b10
Compare
can you fix the copy/paste typo here; #22077 (comment) ? |
c2f06bc
to
d68468e
Compare
@thaJeztah haha, (finally) fixed. |
d68468e
to
385a9d8
Compare
@cpuguy83 looks like the tests needs to be updated to not use |
Now handles `package.Type` and `*package.Type` Fixes parsing issues with slice and map types. Signed-off-by: Brian Goff <[email protected]>
This is similar to network scopes where a volume can either be `local` or `global`. A `global` volume is one that exists across the entire cluster where as a `local` volume exists on a single engine. Signed-off-by: Brian Goff <[email protected]>
385a9d8
to
2f40b1b
Compare
COBRA! Fixed. |
@cpuguy83 @thaJeztah Is anyone working on adding this to https://github.com/docker/go-plugins-helpers? If no one is doing it, maybe I should give a try. |
Help would be much appreciated. |
not that I'm aware of (@cpuguy83?) so contributions welcome! |
At the time, is it possible to test global volumes? |
@tiagoalves83 scopes are for external tooling, like github.com/docker/swarm, swarm-mode will not be making use of this. Also note that the |
What if a plugin is serving up a set of datastores and some of those are local datastores? How should the plugin report scope - global (there are global datastores) or local? What if the plugin has only local datastores accessible to it (no NFS for example). The user has no control where the volumes are placed anyway. Any plan to enumerate datastores for a plugin and allow users to specify placement. |
@govint Always send local scope? It's probably not the best idea to have a plugin serving two roles. What we'll probably end up doing for swarm mode is something like having the orchestrator pick a node then ask the plugin "does this node have access to the requested data?" |
@cpuguy83 nfs local volumes are not working for swarm workers, just the manager. My Enviroment:
Now, using docker (swarm-mode)
Than, inside swmaster-1, volume is being mounted and it is working
Now, for containers created at the swnode-1, volume is not working
Something that I noticed is that when I remove the volume from the swarm |
Same issue in 1.12.0 version #25202 |
@tiagoalves83 nothing to do with this pr. |
Since Docker 1.12, volumes can report their scope as either "global" or "local". Global scope means volumes are available on all nodes in a Docker Swarm cluster (so Swarm should act accordingly, for example it should not try to remove a global volume on each node). By default (or when started with -scope auto), scope is autodetected, meaning if driver's home is on Virtuozzo Storage, global scope is set and reported, otherwise it's assumed as local. This can be overriden from the command line ("-scope local" or "-scope global"). For more details, see moby/moby#22077 Signed-off-by: Kir Kolyshkin <[email protected]>
Add support for volume scopes
This is similar to network scopes where a volume can either be
local
orglobal
.A
global
volume is one that exists across the entire cluster where as alocal
volume exists on a single engine.Includes fixes to the plugin RPC generator to support passing structs to the plugin which was required for this PR.