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

Support mount opts for `local` volume driver #20262

Merged
merged 1 commit into from Mar 3, 2016

Conversation

Projects
None yet
10 participants
@cpuguy83
Contributor

cpuguy83 commented Feb 12, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Feb 12, 2016

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
Contributor

jhowardmsft commented Feb 12, 2016

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

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Feb 12, 2016

Contributor

@jhowardmsft fixed

Contributor

cpuguy83 commented Feb 12, 2016

@jhowardmsft fixed

Show outdated Hide outdated 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:

This comment has been minimized.

@jhowardmsft

jhowardmsft Feb 12, 2016

Contributor

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

@jhowardmsft

jhowardmsft Feb 12, 2016

Contributor

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

This comment has been minimized.

@cpuguy83

cpuguy83 Feb 16, 2016

Contributor

done

@cpuguy83

cpuguy83 Feb 16, 2016

Contributor

done

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Feb 12, 2016

Contributor

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

Contributor

jhowardmsft commented Feb 12, 2016

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

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Feb 13, 2016

Contributor

Design LGTM

Contributor

jessfraz commented Feb 13, 2016

Design LGTM

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Feb 13, 2016

Member

Design LGTM 🍶

Member

vdemeester commented Feb 13, 2016

Design LGTM 🍶

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Feb 26, 2016

Contributor

Always be rebasing 😛

LGTM

Contributor

calavera commented Feb 26, 2016

Always be rebasing 😛

LGTM

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Feb 26, 2016

Contributor

Rebased.

Contributor

cpuguy83 commented Feb 26, 2016

Rebased.

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Feb 29, 2016

Contributor

Looks like another merge conflict.....

Contributor

jhowardmsft commented Feb 29, 2016

Looks like another merge conflict.....

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Mar 1, 2016

Contributor

:goberserk:

Contributor

calavera commented Mar 1, 2016

:goberserk:

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Mar 1, 2016

Contributor

rebased

Contributor

cpuguy83 commented Mar 1, 2016

rebased

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Mar 1, 2016

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 😉

Member

vdemeester commented Mar 1, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Mar 1, 2016

Contributor

@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

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

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Mar 1, 2016

Member

yup 👍 LGTM 😉
switching to docs-review

Member

vdemeester commented Mar 1, 2016

yup 👍 LGTM 😉
switching to docs-review

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Mar 1, 2016

Contributor

docs LGTM

Contributor

calavera commented Mar 1, 2016

docs LGTM

Show outdated Hide outdated docs/reference/commandline/volume_create.md
*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`

This comment has been minimized.

@thaJeztah

thaJeztah Mar 1, 2016

Member

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

@thaJeztah

thaJeztah Mar 1, 2016

Member

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

This comment has been minimized.

@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 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?

Show outdated Hide outdated man/docker-volume-create.1.md
```
$ docker volume create --driver local --opt type=tmpfs --opt device=tmpfs --opt o=size=100m,uid=1000
```

This comment has been minimized.

@thaJeztah

thaJeztah Mar 1, 2016

Member

Looks like theres a space before these lines

@thaJeztah

thaJeztah Mar 1, 2016

Member

Looks like theres a space before these lines

This comment has been minimized.

@thaJeztah

thaJeztah Mar 1, 2016

Member

Also can you make it (backticks) bash

@thaJeztah

thaJeztah Mar 1, 2016

Member

Also can you make it (backticks) bash

This comment has been minimized.

@cpuguy83

cpuguy83 Mar 2, 2016

Contributor

These are tabs

@cpuguy83

cpuguy83 Mar 2, 2016

Contributor

These are tabs

This comment has been minimized.

@cpuguy83

cpuguy83 Mar 2, 2016

Contributor

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

@cpuguy83

cpuguy83 Mar 2, 2016

Contributor

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

Show outdated Hide outdated man/docker-volume-create.1.md
```
$ docker volume create --driver local --opt type=btrfs --opt device=/dev/sda2
```

This comment has been minimized.

@thaJeztah

thaJeztah Mar 1, 2016

Member

same here

@thaJeztah

thaJeztah Mar 1, 2016

Member

same here

Show outdated Hide outdated docs/reference/commandline/volume_create.md
```
$ docker volume create --driver local --opt type=btrfs --opt device=/dev/sda2
```

This comment has been minimized.

@thaJeztah

thaJeztah Mar 1, 2016

Member

Same comments here as in the Man pages

@thaJeztah

thaJeztah Mar 1, 2016

Member

Same comments here as in the Man pages

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Mar 2, 2016

Contributor

@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.

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

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Mar 2, 2016

Member

docs LGTM 🐅
/cc @thaJeztah

Member

vdemeester commented Mar 2, 2016

docs LGTM 🐅
/cc @thaJeztah

Show outdated Hide outdated docs/reference/commandline/volume_create.md
$ docker volume create --name hello
hello
$ docker run -d -v hello:/world busybox ls /world
```

This comment has been minimized.

@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

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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 3, 2016

Member

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

Member

thaJeztah commented Mar 3, 2016

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

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Mar 3, 2016

Contributor

As soon as I get the opportunity to, thanks.

Contributor

cpuguy83 commented Mar 3, 2016

As soon as I get the opportunity to, thanks.

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>
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 3, 2016

Member

LGTM, thanks!

Member

thaJeztah commented Mar 3, 2016

LGTM, thanks!

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Mar 3, 2016

Contributor

LGTM

Contributor

calavera commented Mar 3, 2016

LGTM

calavera added a commit that referenced this pull request Mar 3, 2016

Merge pull request #20262 from cpuguy83/implemnt_mount_opts_for_local…
…_driver

Support mount opts for `local` volume driver

@calavera calavera merged commit c4be28d into moby: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

This comment has been minimized.

Show comment
Hide comment

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

@lmakarov

This comment has been minimized.

Show comment
Hide comment
@lmakarov

lmakarov 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

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

This comment has been minimized.

Show comment
Hide comment
@DJviolin

DJviolin Jul 12, 2016

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?

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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 12, 2016

Member

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

Member

thaJeztah commented Jul 12, 2016

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

@DJviolin

This comment has been minimized.

Show comment
Hide comment
@DJviolin

DJviolin Jul 12, 2016

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.

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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 12, 2016

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.

Member

thaJeztah commented Jul 12, 2016

@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