plugins: container-rootfs-relative paths by tiborvass · Pull Request #26398 · moby/moby

tiborvass · 2016-09-07T23:05:16Z

Legacy plugins expect host-relative paths (such as for Volume.Mount).
However, a containerized plugin cannot respond with a host-relative
path. Therefore, this commit modifies new volume plugins' paths in Mount
and List to prepend the container's rootfs path.

Volume plugins that do mounts and want it to propagate to the host namespace, need to mount inside /mnt.

Signed-off-by: Tibor Vass tibor@docker.com

tiborvass · 2016-09-07T23:15:52Z

plugin/manager.go

@anusha-ragunathan @vieux I changed this to accommodate legacy plugins that have hardcoded /run/docker/plugins already. I had initially chosen /run/docker because it was shorter. To be clear: this is the path inside the container where we expect the socket to be.

Works for me. It reduces the code change required by the legacy plugins to move to pluginv2.

@tiborvass : you dont need to change this for plugins. This is the host directory that gets bind mounted to the plugin. The plugin run time path is defaultPluginRuntimeDestination which is already set to /run/docker/plugins https://github.com/docker/docker/blob/fa7c1a68a68ffb7c6851c59d2c0b41091b9d6e5e/plugin/v2/plugin.go#L29

We could change runRoot back to /run/docker/plugins to indicate that the directories that get created under there are plugins related.

For a separate PR, but this should not be hard-coded to /run/docker but instead use the daemon exec root

The reason to hard code is that, this holds the plugin socket. There's a socket path limitation on Linux to 108 bytes. The daemon exec root doesnt have any socket and is not bound by this limitation. So we have to choose a path that always works.

anusha-ragunathan · 2016-09-08T18:14:55Z

plugin/v2/plugin.go

p.Rootfs can also be used in https://github.com/docker/docker/blob/master/plugin/backend.go#L132

anusha-ragunathan · 2016-09-08T18:18:26Z

Lets keep this PR specifically for container-rootfs-relative path fix and open new ones for the other bug fixes. Keeps a cleaner history.

anusha-ragunathan · 2016-09-08T18:27:55Z

plugin/v2/plugin.go

Lets fix this todo by adding to Manifest's privilege section. Something like "REQUEST_DEVICE_CREATION".

tiborvass · 2016-09-09T19:10:04Z

plugin/store/store.go

@anusha-ragunathan I could not change this in the caller function because I have only access to CompatPlugin interface values, and not *Plugin objects.

Fair enough.

anusha-ragunathan · 2016-09-09T21:11:16Z

@tiborvass

FAIL: docker_cli_daemon_experimental_test.go:177: DockerDaemonSuite.TestVolumePlugin

[d0f844bc9335c] waiting for daemon to start
[d0f844bc9335c] daemon started
docker_cli_daemon_experimental_test.go:223:
    c.Assert(err, checker.IsNil, check.Commentf(out))
... value *exec.ExitError = &exec.ExitError{ProcessState:(*os.ProcessState)(0xc421c5b0e0), Stderr:[]uint8(nil)} ("exit status 127")
... Unable to find image 'busybox:latest' locally
latest: Pulling from library/busybox
8ddc19f16526: Pulling fs layer
8ddc19f16526: Download complete
8ddc19f16526: Pull complete
Digest: sha256:a59906e33509d14c036c8678d687bd4eec81ed7c4b8ce907b888c607f6a1e0e6
Status: Downloaded newer image for busybox:latest
/go/src/github.com/docker/docker/bundles/1.13.0-dev/binary-client/docker: Error response from daemon: oci runtime error: rootfs_linux.go:53: mounting "/tmp/data" to rootfs "/go/src/github.com/docker/docker/bundles/1.13.0-dev/test-integration-cli/d0f844bc9335c/root/vfs/dir/fe75f9766650efb2ecd73bee99b3c9eb67301379cccf3e119582c92016c3a318" caused "stat /go/src/github.com/docker/docker/bundles/1.13.0-dev/test-integration-cli/d0f844bc9335c/root/plugins/2666fee9e7017759705396aa5301f13af1385ca24e8665106e0cdcd687385816/rootfs/data/plugin-volume: no such file or directory".

ekristen · 2016-10-10T13:09:34Z

Please let me know if I can assist. Looking forward to seeing this get merged for 1.13.

tiborvass · 2016-10-10T14:26:25Z

Thanks, I'm planning to update this soon.

thaJeztah · 2016-10-20T18:44:08Z

ping @tiborvass what's the status here?

anusha-ragunathan · 2016-12-02T23:42:03Z

Need a fresh pair of eyes reviewing this. @cpuguy83 ?

anusha-ragunathan · 2016-12-02T23:46:38Z

plugin/v2/plugin.go

anusha-ragunathan · 2016-12-02T23:47:00Z

plugin/manager.go

anusha-ragunathan · 2016-12-02T23:47:43Z

plugin/manager.go

Please see #26398 (comment)

anusha-ragunathan · 2016-12-02T23:55:05Z

integration-cli/docker_cli_daemon_plugins_test.go

We are simply printing out the mounts, which doesnt test much. Can we use findmnt to see if the mount propagated?

cpuguy83 · 2016-12-03T02:13:23Z

Beyond the already commented items, code seems ok.

vdemeester · 2016-12-06T15:28:44Z

integration-cli/docker_cli_plugins_test.go

Shouldn't this be dockerCmd instead ? Doesn't seem right to not care about error at all 😓

@vdemeester I kept err and asserted it non-nil. Realized that dockerCmd will assume non-nil error...

anusha-ragunathan · 2016-12-07T23:15:37Z

plugin/manager.go

Is this still applicable? When does the mount leak?

../../../../etc

anusha-ragunathan · 2016-12-07T23:18:40Z

plugin/v2/plugin.go

Lets not introduce Public fields in this struct. Refer to #29191 is making changes to make this all private and move necessary fields to a controller struct in the manager. Please review that PR and follow similar design on the new fields.

anusha-ragunathan · 2016-12-07T23:19:38Z

api/types/plugin.go

https://github.com/docker/docker/blob/master/docs/extend/config.md needs to be updated. Please do that alongside.

anusha-ragunathan · 2016-12-08T23:25:25Z

LGTM (if tests pass)

cpuguy83 · 2016-12-09T14:58:55Z

plugin/manager.go

filepath.Base(p.PluginObj.Config.PropagatedMount)?

Making sure I understand this... this is for mounting the propagated path to the plugin root (outside rootfs)?
Maybe even just use mnt instead of the mount path.

@tiborvass : You should be using filepath.Join(p.LibRoot, p.PluginObj.ID, p.PluginObj.Config.PropagatedMount). Else mounts from different plugins with the same name will collide. Another option is to fix p.LibRoot to include the pluginID (It should have been this way in the first place)

We dont want to use hard coded mnt. We want plugin devs to specify the mount that needs propagation.

@cpuguy83 : PropagatedMount is strictly for those plugins (typically volume and graph drivers) that need to setup mounts that need to propagated to the host.
We want plugin devs to specify which mounts need to be propagated, rather than hard coding it to a specific path.

Right, but isn't this a singleton that's the "host" side of the mount?

Yes. Having the same dir as the mount in the config could be less confusing. On the other hand, having a standard dir such as mnt is also reasonable. I'll let @tiborvass decide. Also, we dont have to do this in this PR.

For start up reasons, this field is also set in plugin/manager.go. And I just realized it's set to something else. So I'll go with p.Rootfs for now.

Legacy plugins expect host-relative paths (such as for Volume.Mount). However, a containerized plugin cannot respond with a host-relative path. Therefore, this commit modifies new volume plugins' paths in Mount and List to prepend the container's rootfs path. This introduces a new PropagatedMount field in the Plugin Config. When it is set for volume plugins, RootfsPropagation is set to rshared and the path specified by PropagatedMount is bind-mounted with rshared prior to launching the container. This is so that the daemon code can access the paths returned by the plugin from the host mount namespace. Signed-off-by: Tibor Vass <tibor@docker.com>

GordonTheTurtle added the status/0-triage label Sep 7, 2016

tiborvass force-pushed the plugin-fixes branch from de699b3 to 895ef95 Compare September 7, 2016 23:13

tiborvass reviewed Sep 7, 2016
View reviewed changes

tiborvass force-pushed the plugin-fixes branch from 895ef95 to cc0760a Compare September 7, 2016 23:35

ehazlett added status/1-design-review status/failing-ci Indicates that the PR in its current state fails the test suite and removed status/0-triage labels Sep 8, 2016

anusha-ragunathan reviewed Sep 8, 2016
View reviewed changes

tiborvass force-pushed the plugin-fixes branch from cc0760a to 3cce395 Compare September 9, 2016 19:08

tiborvass reviewed Sep 9, 2016
View reviewed changes

tiborvass force-pushed the plugin-fixes branch 2 times, most recently from 3eb7294 to 1362b42 Compare September 9, 2016 19:12

anusha-ragunathan added status/2-code-review and removed status/1-design-review labels Sep 9, 2016

anusha-ragunathan added this to the 1.13.0 milestone Sep 9, 2016

anusha-ragunathan added the area/plugins label Sep 9, 2016

tiborvass force-pushed the plugin-fixes branch from 1362b42 to 74bb539 Compare October 26, 2016 17:31

anusha-ragunathan reviewed Dec 2, 2016

View reviewed changes

plugin/v2/plugin.go Outdated

Copy link
Copy Markdown

Contributor

anusha-ragunathan Dec 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

anusha-ragunathan reviewed Dec 2, 2016

View reviewed changes

plugin/manager.go Outdated

Copy link
Copy Markdown

Contributor

anusha-ragunathan Dec 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

vdemeester reacted with laugh emoji vieux reacted with heart emoji

anusha-ragunathan reviewed Dec 2, 2016

View reviewed changes

plugin/manager.go Outdated

Copy link
Copy Markdown

Contributor

anusha-ragunathan Dec 2, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see #26398 (comment)

anusha-ragunathan reviewed Dec 2, 2016

View reviewed changes

vdemeester reviewed Dec 6, 2016

View reviewed changes

tiborvass force-pushed the plugin-fixes branch 5 times, most recently from 2e89b78 to 46c343f Compare December 7, 2016 18:48

anusha-ragunathan reviewed Dec 7, 2016

View reviewed changes

tiborvass force-pushed the plugin-fixes branch from 46c343f to a0b59db Compare December 8, 2016 23:22

tiborvass added rebuild/experimental labels Dec 9, 2016

GordonTheTurtle removed rebuild/experimental labels Dec 9, 2016

cpuguy83 reviewed Dec 9, 2016

View reviewed changes

tiborvass force-pushed the plugin-fixes branch from a0b59db to c54b717 Compare December 9, 2016 18:16

tiborvass added the rebuild/windowsRS1 label Dec 9, 2016

GordonTheTurtle removed the rebuild/windowsRS1 label Dec 9, 2016

anusha-ragunathan merged commit 0a072e9 into moby:master Dec 9, 2016

vieux mentioned this pull request Dec 9, 2016

1.13.0-rc4 cherry-picks: part1 #29229

Merged

Conversation

tiborvass commented Sep 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tiborvass Sep 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

anusha-ragunathan commented Sep 8, 2016

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!

anusha-ragunathan commented Sep 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ekristen commented Oct 10, 2016

Uh oh!

tiborvass commented Oct 10, 2016

Uh oh!

thaJeztah commented Oct 20, 2016

Uh oh!

anusha-ragunathan commented Dec 2, 2016

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!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Dec 3, 2016

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!

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!

anusha-ragunathan commented Dec 8, 2016

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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

tiborvass commented Sep 7, 2016 •

edited

Loading

tiborvass Sep 7, 2016 •

edited

Loading

anusha-ragunathan commented Sep 9, 2016 •

edited

Loading