Separate plugin sockets and specs. by calavera · Pull Request #13951 · moby/moby

calavera · 2015-06-15T22:39:12Z

Check if there is a plugin socket first under /run/docker/plugins/NAME.sock.
If there is no socket for a plugin, check /etc/docker/plugins/NAME.spec and
/usr/lib/docker/plugins/NAME.spec for spec files.

Fixes #13859

/cc @lukemarsden

Signed-off-by: David Calavera [email protected]

Requires moby/moby#13951

proppy · 2015-06-30T22:26:43Z

experimental/plugin_api.md

maybe we should mention per plugin directory here with a specific example.

@proppy that's define at the bottom of this section, like 42. Is that good enough?

cpuguy83 · 2015-07-01T17:25:12Z

Design LGTM

LK4D4 · 2015-07-01T18:28:27Z

@calavera What motivation behind separation unix sockets from specs?

calavera · 2015-07-02T16:39:52Z

@LK4D4 I think that's more where people expect to find them. Unix sockets under /run, like other unix sockets and specs under a "configuration" directory.

This is where we decided to go with this approach:

#13514 (comment)

LK4D4 · 2015-07-02T17:34:19Z

okay

erikh · 2015-07-06T22:29:15Z

integration-cli/docker_cli_start_volume_driver_unix_test.go

as I understand it, these changes leave the /usr/share path untested. is that accurate?

the /usr/share path is completely gone, we are not going to make this stable with that path.

ok, good to know.

On Jul 6, 2015, at 3:35 PM, David Calavera [email protected] wrote:

In integration-cli/docker_cli_start_volume_driver_unix_test.go #13951 (comment):

@@ -129,11 +129,11 @@ func (s *DockerExternalVolumeSuite) SetUpSuite(c *check.C) {
fmt.Fprintln(w, {})
})

if err := os.MkdirAll("/usr/share/docker/plugins", 0755); err != nil {

if err := os.MkdirAll("/etc/docker/plugins", 0755); err != nil {
the /usr/share path is completely gone, we are not going to make this stable with that path.

—
Reply to this email directly or view it on GitHub https://github.com/docker/docker/pull/13951/files#r33990203.

erikh · 2015-07-06T22:30:50Z

LGTM other than the comments; but historically this stuff lives in /var/run -- is that a better place for it maybe?

tomdee · 2015-07-09T22:21:59Z

Any update on this PR? Without it I'm unable to test network plugins on CoreOS.

And FWIW, I've been trying to test with this branch but I can't get it to work (with .sock files). I'm trying to create a network. i.e.

docker network create --driver=calico 0c6db1c987123
Error response from daemon: Plugin not found

The calico.sock file exists.

Does this change require a corresponding change to libnetwork?

calavera · 2015-07-10T04:58:22Z

@tomdee I made a small change but it doesn't look to be the error you get. I don't think networking requires anything special. Can you point me to your code? This is my gluster plugin being used with this PR:

root@gfs-client-1:/# ./docker-1.8.0-dev run --volume-driver glusterfs --rm -it -v data-store:/data alpine:latest ash
/ # ls /data/
helo
root@gfs-client-1:/# ls -la /run/docker/plugins/glusterfs.sock
srw-rw---- 1 root root 0 Jul 10 04:29 /run/docker/plugins/glusterfs.sock
root@gfs-client-1:/#

tomdee · 2015-07-13T20:50:02Z

I've retested this and it's working fine with Calico (as a libnetwork remote plugin) now. Either your small change did it, or I had a permission problem on the /run/docker directory.

I would love to see this merged ASAP - it would be great to start testing on CoreOS.

(And FWIW - anyone wanting to do manual testing of this - nc -lU /run/docker/plugins/test.sock is pretty handy for simulating a plugin. Running docker network create --driver test $RANDOM should produce output from the nc command if everything is getting wired up properly)

abronan · 2015-07-14T17:51:11Z

LGTM

tiborvass · 2015-07-15T16:17:21Z

pkg/plugins/discovery.go

i know it's unrelated to this PR, but it's unfortunate we have another thing called Registry that has nothing to do with a docker registry :( we should consider renaming it to PluginService or PluginStore what do you think? Can be done in another PR, but just wanted to bring it to the attention

yeah, I had the same feeling from the beginning, but naming is hard 😸

Let's think about something better somewhere else.

tiborvass · 2015-07-15T19:01:27Z

LGTM

thaJeztah · 2015-07-16T21:04:28Z

doc changes LGTM

Keep in mind that this is an experimental feature; docs only need a "casual" review, but are not part of the official documentation, so it's mostly important to make sure that the information is correct

thaJeztah · 2015-07-16T21:08:34Z

experimental/plugin_api.md

hm, thinking of this; perhaps you should mention in what order the directories are scanned, is that useful information?

I added a short explanation about the scan ordering.

calavera · 2015-07-16T21:16:05Z

@thaJeztah the volume plugins are going in the 1.8 release. The docs will be moved to the right place after this lands on master. See #14659.

Check if there is a plugin socket first under `/run/docker/plugins/NAME.sock`. If there is no socket for a plugin, check `/etc/docker/plugins/NAME.spec` and `/usr/lib/docker/plugins/NAME.spec` for spec files. Signed-off-by: David Calavera <[email protected]>

thaJeztah · 2015-07-16T21:36:05Z

@calavera then I suggest to "dot the i's and cross the t's" in #14659. The right meta-data headers need to be added to the docs as well as part of that PR (and we can do some tweaking if needed)

calavera · 2015-07-16T21:37:36Z

SGTM

icecrime · 2015-07-17T16:00:29Z

I'm a bit confused here.

We're are merging this for 1.8
This impacts the docs
As per another PR, volume plugins are making it out of experimental
This other PR will hold the docs

Tentatively moving this to status/4-ready-to-merge assuming I got this right. Can you confirm @thaJeztah?

thaJeztah · 2015-07-17T19:11:27Z

@icecrime IIUC, this not really touches the "docs" (as in, the docs here are still in the "experimental" directory). #14659 will promote this from "experimental" to "mainstream" and where the docs will be moved and touched-up to match the standards for the documentation.

So yes, I think ready to merge is good. @calavera also pinged Mary to have a look, but I think that can be done on #14659. So I'll merge so we can proceed 👍

Separate plugin sockets and specs.

thaJeztah · 2015-07-17T19:11:54Z

@calavera merged!

Requires moby/moby#13951 Former-commit-id: ef6f0896168dc7fd7bd514652ce6a73f66f837ec

GordonTheTurtle added the status/0-triage label Jun 15, 2015

calavera force-pushed the plugins_path branch from c45aa6a to f2c6849 Compare June 15, 2015 22:41

calavera added status/1-design-review and removed status/0-triage labels Jun 16, 2015

calavera mentioned this pull request Jun 18, 2015

Proposal: Abstract Volume Driver Proxy to allow for multiple drivers and native external storage management #13924

Closed

3 tasks

tomdee added a commit to projectcalico/calicoctl that referenced this pull request Jun 19, 2015

Use new location of calico.sock

ef6f089

Requires moby/moby#13951

calavera added the /core/plugins label Jun 24, 2015

This was referenced Jun 24, 2015

pkg/plugins: add plugin name in discovery path #14135

Closed

Volume extensions: from experimental to stable #14213

Closed

calavera force-pushed the plugins_path branch 3 times, most recently from 17335be to 894f7e2 Compare June 30, 2015 21:53

proppy reviewed Jun 30, 2015
View reviewed changes

calavera force-pushed the plugins_path branch from 894f7e2 to d60e556 Compare June 30, 2015 22:30

erikh reviewed Jul 6, 2015
View reviewed changes

calavera added the status/needs-attention Calls for a collective discussion during a review session label Jul 8, 2015

calavera added this to the 1.8.0 milestone Jul 8, 2015

calavera force-pushed the plugins_path branch from d60e556 to 9227b52 Compare July 10, 2015 04:55

calavera added status/2-code-review and removed status/1-design-review labels Jul 14, 2015

tiborvass reviewed Jul 15, 2015
View reviewed changes

tiborvass added status/3-docs-review and removed status/2-code-review labels Jul 15, 2015

calavera mentioned this pull request Jul 15, 2015

Promote volume drivers from experimental to master. #14659

Merged

thaJeztah reviewed Jul 16, 2015
View reviewed changes

calavera force-pushed the plugins_path branch from 9227b52 to 6c07957 Compare July 16, 2015 21:20

icecrime added status/4-merge and removed status/3-docs-review labels Jul 17, 2015

thaJeztah added a commit that referenced this pull request Jul 17, 2015

Merge pull request #13951 from calavera/plugins_path

a763637

Separate plugin sockets and specs.

thaJeztah merged commit a763637 into moby:master Jul 17, 2015

calavera deleted the plugins_path branch July 17, 2015 19:35

squaremo mentioned this pull request Jul 23, 2015

Assumed location of plugin sockets has changed weaveworks-experiments/docker-plugin#30

Closed

tomdee added a commit to projectcalico/libcalico that referenced this pull request Jul 28, 2015

Use new location of calico.sock

cb5c22c

Requires moby/moby#13951 Former-commit-id: ef6f0896168dc7fd7bd514652ce6a73f66f837ec

Conversation

calavera commented Jun 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Jul 1, 2015

Uh oh!

LK4D4 commented Jul 1, 2015

Uh oh!

calavera commented Jul 2, 2015

Uh oh!

LK4D4 commented Jul 2, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikh commented Jul 6, 2015

Uh oh!

tomdee commented Jul 9, 2015

Uh oh!

calavera commented Jul 10, 2015

Uh oh!

tomdee commented Jul 13, 2015

Uh oh!

abronan commented Jul 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiborvass commented Jul 15, 2015

Uh oh!

thaJeztah commented Jul 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calavera commented Jul 16, 2015

Uh oh!

thaJeztah commented Jul 16, 2015

Uh oh!

calavera commented Jul 16, 2015

Uh oh!

icecrime commented Jul 17, 2015

Uh oh!

thaJeztah commented Jul 17, 2015

Uh oh!

thaJeztah commented Jul 17, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants