Support mount opts for `local` volume driver #20262

Merged
merged 1 commit into from Mar 3, 2016

Projects

None yet

10 participants

@cpuguy83
Contributor

Allows users to submit options similar to the mount command when
creating a volume with the local volume driver.

For example:

$ docker volume create -d local --opt type=nfs --opt device=myNfsServer:/data --opt o=noatime,nosuid
@jhowardmsft
Contributor

Not compiling on Windows

00:01:18.328 Building: bundles/1.11.0-dev/binary/docker-1.11.0-dev.exe
00:01:36.443 # github.com/docker/docker/volume/local
00:01:36.443 C:\Go\src\github.com\docker\docker\volume\local\local.go:286: v.opts.MountDevice undefined (type *optsConfig has no field or method MountDevice)
00:01:36.444 C:\Go\src\github.com\docker\docker\volume\local\local.go:286: v.opts.MountType undefined (type *optsConfig has no field or method MountType)
00:01:36.445 C:\Go\src\github.com\docker\docker\volume\local\local.go:286: v.opts.MountOpts undefined (type *optsConfig has no field or method MountOpts)
00:01:36.445 C:\Go\src\github.com\docker\docker\volume\local\local.go:315: undefined: validOpts
00:01:36.446 C:\Go\src\github.com\docker\docker\volume\local\local_windows.go:25: not enough arguments to return
00:01:36.447 C:\Go\src\github.com\docker\docker\volume\local\local_windows.go:27: missing return at end of function
00:02:08.440 + ec=1
00:02:08.445 + set +x
@cpuguy83
Contributor

@jhowardmsft fixed

@jhowardmsft jhowardmsft and 1 other commented on an outdated diff Feb 12, 2016
docs/reference/commandline/volume_create.md
@@ -47,4 +47,15 @@ Some volume drivers may take options to customize the volume creation. Use the `
These options are passed directly to the volume driver. Options for
different volume drivers may do different things (or nothing at all).
-*Note*: The built-in `local` volume driver does not currently accept any options.
+The built-in `local` driver accepts options similar to the `mount` command:
@jhowardmsft
jhowardmsft Feb 12, 2016 Contributor

Minor nit, but while updating the docs, can you indicate that no options are supported on Windows.

@cpuguy83
cpuguy83 Feb 16, 2016 Contributor

done

@jhowardmsft
Contributor

Minor nit in docs, but LGTM from a Windows perspective.

@jessfraz
Contributor

Design LGTM

@vdemeester
Member

Design LGTM ๐Ÿถ

@calavera
Contributor

Always be rebasing ๐Ÿ˜›

LGTM

@cpuguy83
Contributor

Rebased.

@jhowardmsft
Contributor

Looks like another merge conflict.....

@calavera
Contributor
calavera commented Mar 1, 2016

:goberserk:

@cpuguy83
Contributor
cpuguy83 commented Mar 1, 2016

rebased

@vdemeester
Member

@cpuguy83 Looks good. Few questions though :

  • Should we add something in inspect that reflect the options :
[
    {
        "Name": "yay",
        "Driver": "local",
        "Mountpoint": "/var/lib/docker/volumes/yay/_data"
    }
]

It's probably not in the scope of the PR because this would need to change the types.Volume struct in engine-api. But I though it was worth mentionning ๐Ÿ˜.

  • Its a nit but I wonder if the short flag -o is even needed. And it can lead to a command with a flag like -o o=size=100m,uid=1000 โ€” which is fun to read.

It's just a couple of nits so I'm LGTM ๐Ÿฐ anyway ๐Ÿ˜‰

@cpuguy83
Contributor
cpuguy83 commented Mar 1, 2016

@vdemeester Yeah we should support something like a Meta field on volume inspect that drivers can implement (or not), but indeed it does need to be in another PR.

The primary reason for having -o=o=<blah> is so we can differentiate from the other options... namely Device and Type, currently, and it also basically matches the mount command opts. It's not horrible if you do --opt o=foo=bar,baz

@vdemeester
Member

yup ๐Ÿ‘ LGTM ๐Ÿ˜‰
switching to docs-review

@calavera
Contributor
calavera commented Mar 1, 2016

docs LGTM

@thaJeztah thaJeztah commented on an outdated diff Mar 1, 2016
docs/reference/commandline/volume_create.md
@@ -47,11 +47,25 @@ Some volume drivers may take options to customize the volume creation. Use the `
These options are passed directly to the volume driver. Options for
different volume drivers may do different things (or nothing at all).
-*Note*: The built-in `local` volume driver does not currently accept any options.
+*Note*: The built-in `local` driver on Windows does not support any options.
+
+*Note*: The built-in `local` driver on Linux accepts options similar to the `mount`
@thaJeztah
thaJeztah Mar 1, 2016 Member

I think we don't need "Note" here.

@thaJeztah
thaJeztah Mar 1, 2016 Member

perhaps "similar to the Linux mount command" (so that people don't start looking for docker mount

Can we add a link to that command somewhere? Or wouldn't that make sense?

@thaJeztah thaJeztah and 1 other commented on an outdated diff Mar 1, 2016
man/docker-volume-create.1.md
@@ -36,7 +36,21 @@ Some volume drivers may take options to customize the volume creation. Use the `
These options are passed directly to the volume driver. Options for
different volume drivers may do different things (or nothing at all).
-*Note*: The built-in `local` volume driver does not currently accept any options.
+The built-in `local` driver on Windows does not support any options.
+
+The built-in `local` driver on Linux accepts options similar to the `mount`
+command:
+
+ ```
+ $ docker volume create --driver local --opt type=tmpfs --opt device=tmpfs --opt o=size=100m,uid=1000
+ ```
@thaJeztah
thaJeztah Mar 1, 2016 Member

Looks like theres a space before these lines

@thaJeztah
thaJeztah Mar 1, 2016 Member

Also can you make it (backticks) bash

@cpuguy83
cpuguy83 Mar 2, 2016 Contributor

These are tabs

@cpuguy83
cpuguy83 Mar 2, 2016 Contributor

Not sure that we should add bash here since this is a man doc.

@thaJeztah thaJeztah commented on an outdated diff Mar 1, 2016
man/docker-volume-create.1.md
@@ -36,7 +36,21 @@ Some volume drivers may take options to customize the volume creation. Use the `
These options are passed directly to the volume driver. Options for
different volume drivers may do different things (or nothing at all).
-*Note*: The built-in `local` volume driver does not currently accept any options.
+The built-in `local` driver on Windows does not support any options.
+
+The built-in `local` driver on Linux accepts options similar to the `mount`
+command:
+
+ ```
+ $ docker volume create --driver local --opt type=tmpfs --opt device=tmpfs --opt o=size=100m,uid=1000
+ ```
+
+Another example:
+
+ ```
+ $ docker volume create --driver local --opt type=btrfs --opt device=/dev/sda2
+ ```
@thaJeztah
thaJeztah Mar 1, 2016 Member

same here

@thaJeztah thaJeztah commented on an outdated diff Mar 1, 2016
docs/reference/commandline/volume_create.md
@@ -47,11 +47,25 @@ Some volume drivers may take options to customize the volume creation. Use the `
These options are passed directly to the volume driver. Options for
different volume drivers may do different things (or nothing at all).
-*Note*: The built-in `local` volume driver does not currently accept any options.
+*Note*: The built-in `local` driver on Windows does not support any options.
+
+*Note*: The built-in `local` driver on Linux accepts options similar to the `mount`
+command:
+
+ ```
+ $ docker volume create --driver local --opt type=tmpfs --opt device=tmpfs --opt o=size=100m,uid=1000
+ ```
+
+Another example:
+
+ ```
+ $ docker volume create --driver local --opt type=btrfs --opt device=/dev/sda2
+ ```
@thaJeztah
thaJeztah Mar 1, 2016 Member

Same comments here as in the Man pages

@cpuguy83
Contributor
cpuguy83 commented Mar 2, 2016

@thaJeztah Made some of the doc updates, ensured it's 2 tabs for the indents instead of 1.
For man docs I don't think we should include the type for the code blocks.

@vdemeester
Member

docs LGTM ๐Ÿ…
/cc @thaJeztah

@thaJeztah thaJeztah commented on an outdated diff Mar 2, 2016
docs/reference/commandline/volume_create.md
$ docker volume create --name hello
hello
$ docker run -d -v hello:/world busybox ls /world
+ ```
@thaJeztah
thaJeztah Mar 2, 2016 Member

Oh, sorry, no, you need to use either indentation (4 spaces) or GitHub style fences. GitHub style fences are nice, because we can control what kind of code-highlighting should be used.

Because you now have both indented and added fences, it becomes:

screen shot 2016-03-02 at 11 01 51

@thaJeztah
Member

ping @cpuguy83 can you fix the markdown, then we can merge :-) #20262 (comment)

@cpuguy83
Contributor
cpuguy83 commented Mar 3, 2016

As soon as I get the opportunity to, thanks.

@cpuguy83 cpuguy83 Support mount opts for `local` volume driver
Allows users to submit options similar to the `mount` command when
creating a volume with the `local` volume driver.

For example:

```go
$ docker volume create -d local --opt type=nfs --opt device=myNfsServer:/data --opt o=noatime,nosuid
```

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
b05b237
@thaJeztah
Member

LGTM, thanks!

@calavera
Contributor
calavera commented Mar 3, 2016

LGTM

@calavera calavera merged commit c4be28d into docker:master Mar 3, 2016

8 checks passed

docker/dco-signed All commits signed
Details
documentation success 2 tests run, 0 skipped, 0 failed.
Details
experimental Jenkins build Docker-PRs-experimental 15799 has succeeded
Details
gccgo Jenkins build Docker-PRs-gccgo 2694 has succeeded
Details
janky Jenkins build Docker-PRs 24587 has succeeded
Details
userns Jenkins build Docker-PRs-userns 6933 has succeeded
Details
win2lin Jenkins build Docker-PRs-Win2Lin 22714 has succeeded
Details
windowsTP4 Jenkins build Docker-PRs-WoW-TP4 2043 has succeeded
Details
@cpuguy83 cpuguy83 deleted the cpuguy83:implemnt_mount_opts_for_local_driver branch Mar 3, 2016
@thaJeztah thaJeztah added this to the 1.11.0 milestone Mar 3, 2016
@blaggacao

@gondor you might be interested in this! (https://github.com/gondor/docker-volume-netshare)

@lmakarov
lmakarov commented Jul 6, 2016

For anyone else trying to get the nfs volume example in this PR's description to work - #24179 added a working example to the docs:

$ docker volume create --driver local --opt type=nfs --opt o=addr=192.168.1.1,rw --opt device=:/path/to/dir --name foo
@DJviolin

Can the --opt flag variables like --opt type=nfs --opt o=addr=192.168.1.1,rw --opt device=:/path/to/dir and --opt o=size=100m,uid=1000 can get to worked under docker-compose driver_opts options? If so, how?

@thaJeztah
Member

@DJviolin please open an issue (or search for an existing one) in the docker compose issue tracker; https://github.com/docker/compose/issues

@DJviolin

I already did, someone told me docker-compose uncapable to do this right now, because driver_opts not have these commands: docker/compose#3715

What I want is to expose a host volume as a named volume to my container service, because this way I can control uid (and maybe gid?). So docker volume create is capable of doing this now, but compose simply not implemented yet? It would be a real game changer to put host folders into named volume and even define uid and gid.

There is a long thread here which lot of people wanted this: #7198

And they got this with this command (or at least partially, gid?). But we need this for compose as well.

Without docker-compose implementation this is useless. I want to define my entire service in a single docker-compose.yml, not set up some basic shell script to use docker volume create command.

@thaJeztah
Member

@DJviolin discussing it here won't help, because it's something that needs to be implemented in docker compose, however (as mentioned on the linked issue), bind-mounts are not volumes, so "driver" options are not available. If you want to bind-mount a host directory as a named volume, you'll need to use a volume plugin that supports this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment