Skip to content
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 volume api #14242

Merged
merged 1 commit into from Aug 26, 2015

Conversation

@cpuguy83
Copy link
Contributor

cpuguy83 commented Jun 29, 2015

Posting this up here for discussion.

TODO:

  • Add CLI docs
  • Add CLI tests
  • Accounting for the local volume driver is not good enough and allows for things like calling docker volume rm on a volume multiple times to remove.
@cpuguy83 cpuguy83 mentioned this pull request Jun 29, 2015
0 of 2 tasks complete
@cpuguy83 cpuguy83 force-pushed the cpuguy83:add_volume_api branch from 56bf8ad to 79e1426 Jun 29, 2015
@cpuguy83 cpuguy83 force-pushed the cpuguy83:add_volume_api branch 4 times, most recently from 29a7147 to 3f81a20 Jun 29, 2015
description += fmt.Sprintf(" %-10.10s%s\n", cmd[0], cmd[1])
}

description += "\nRun 'docker volume COMMNAD --help' for more information on a command."

This comment has been minimized.

Copy link
@kojiromike

kojiromike Jul 8, 2015

Contributor

sp COMMAND

@yasker

This comment has been minimized.

Copy link
Contributor

yasker commented Jul 8, 2015

And is it possible to omit the driver option when rm/inspect? Docker should know which volume used which driver when created?

@cpuguy83

This comment has been minimized.

Copy link
Contributor Author

cpuguy83 commented Jul 8, 2015

@yasker No, not really.
Volumes are not managed by docker at all, and there is no guarantee of uniqueness of the name between drivers.

@yasker

This comment has been minimized.

Copy link
Contributor

yasker commented Jul 8, 2015

@cpuguy83 I see, thanks.

Just curious, would we have ability to create docker disposable volume through docker volume command? Like what I shown above, without "-d"?

@cpuguy83

This comment has been minimized.

Copy link
Contributor Author

cpuguy83 commented Jul 8, 2015

Without "-d" would just use the default driver, which is currently hard coded to the "local" driver.

@jonasrosland

This comment has been minimized.

Copy link

jonasrosland commented Jul 9, 2015

I see we have this and #13924, how much of the work from that one could be reused here?

@cpuguy83

This comment has been minimized.

Copy link
Contributor Author

cpuguy83 commented Jul 9, 2015

@jonasrosland They are completely different things.
#13924 is refactoring the internal implementation, this is adding a public/remote API/CLI to these drivers.

@jonasrosland

This comment has been minimized.

Copy link

jonasrosland commented Jul 9, 2015

Perfect, thanks for the explanation!

@cpuguy83 cpuguy83 mentioned this pull request Jul 10, 2015
0 of 5 tasks complete
@cpuguy83 cpuguy83 force-pushed the cpuguy83:add_volume_api branch 6 times, most recently from 865fff0 to bd32f23 Jul 11, 2015
@cpuguy83 cpuguy83 changed the title [WIP] Add volume api Add volume api Jul 11, 2015
@cpuguy83

This comment has been minimized.

Copy link
Contributor Author

cpuguy83 commented Jul 11, 2015

Removed [WIP]
This is ready.

@yasker

This comment has been minimized.

Copy link
Contributor

yasker commented Jul 13, 2015

@cpuguy83 It's strange, seems github got a glitch. I once commented on old commit that the specifying of driver/name should be consistent across the commands. Now I cannot find it anywhere...

In short, the create/rm/inspect should use same order/parameter for input driver and volume name. Now inspect/rm using [DRIVER] [NAME], but create use "--name [NAME] --driver [DRIVER]"

I think they should all be: "--name [NAME]" with "--driver [DRIVER]" as optional. If DRIVER is first augment, it cannot be omitted.

Something like this would be useful:

docker volume create vol1
docker volume create vol1 -d volume_driver_1
docker volume rm vol1
docker volume rm vol1 -d volume_driver_1
docker volume inspect vol1
docker volume inspect vol1 -d volume_driver_1

And without driver it would default to "local" as we discussed.

I omitted the "--name" above because it's pretty required, but not the driver option.

@calavera

This comment has been minimized.

Copy link
Contributor

calavera commented Jul 13, 2015

I agree with @yasker. The name should always be the first argument for every command and the driver should always be a flag.

@cpuguy83

This comment has been minimized.

Copy link
Contributor Author

cpuguy83 commented Jul 13, 2015

I agree as it is right here is not good (and really horrible to actually use as I've learned the last couple of days!).
However I don't think driver as a flag for inspect/rm is good as you may end up hitting the wrong volume in this case.
It also makes piping commands together extremely difficult.

What I have now, just making some test/doc tweaks, is expecting a volume to be specified as <driver>/<name>.
Then with docker volume ls -q, it outputs <driver>/<name>\n... since a name by itself is useless.
This allows you to combine commands together similar to with docker ps, like docker volume inspect $(docker volume ls -q)

webus added a commit to webus/docker-thumbor that referenced this pull request Sep 15, 2015
Warning: the mapping "data:/data" in the volumes config for service "thumbor" is ambiguous. In a future version of Docker, it will designate a "named" volume (see moby/moby#14242). To prevent unexpected behaviour, change it to "./data:/data"
Warning: the mapping "logs:/logs" in the volumes config for service "thumbor" is ambiguous. In a future version of Docker, it will designate a "named" volume (see moby/moby#14242). To prevent unexpected behaviour, change it to "./logs:/logs"
toolness added a commit to GovLab/noi2 that referenced this pull request Sep 17, 2015
Here's an example warning this suppresses:

> Warning: the mapping "conf/:/etc/nginx/conf.d/" in the volumes
> config for service "web" is ambiguous. In a future version of Docker,
> it will designate a "named" volume (see
> moby/moby#14242). To prevent unexpected
> behaviour, change it to "./conf/:/etc/nginx/conf.d/"

This warning appears on Docker 1.8.2.
@wlan0 wlan0 mentioned this pull request Sep 22, 2015
@coding2012

This comment has been minimized.

Copy link

coding2012 commented Sep 24, 2015

I have a use-case to DISABLE VOLUME tags inside Dockerfiles. For example let's say I'm automating the build and deployment of dev and feature branches and possibly QA. For these environments I want to be able to build a pre-existing postgres image that has my schema and testing data saved, and pushed to a public repository. When such pre-existing database images have VOLUME dockerfile instructions, I pretty much have to either copy and paste the dockerfile without the VOLUME directive OR create a data-only container, tag/commit and push (haven't tested this route). Both of these are not desireable for staging data solutions that you want to push to a docker repo.

@kojiromike

This comment has been minimized.

Copy link
Contributor

kojiromike commented Sep 24, 2015

yes, this ^^ @coding2012; however, isn't it a separate issue?

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Sep 24, 2015

@coding2012 @kojiromike that's indeed not really related to this PR. See #8177 (and #3465) for a prior discussion on that.

viranch added a commit to viranch/docker-compose-files that referenced this pull request Oct 8, 2015
hao-opentown added a commit to hao-opentown/elastic-mongo that referenced this pull request Oct 10, 2015
Warning: the mapping "elasticsearch/logging.yml:/etc/elasticsearch/logging.yml" in the volumes config for service "elasticsearch" is ambiguous. In a future version of Docker, it will designate a "named" volume (see moby/moby#14242). To prevent unexpected behaviour, change it to "./elasticsearch/logging.yml:/etc/elasticsearch/logging.yml"
@omribahumi

This comment has been minimized.

Copy link

omribahumi commented Oct 15, 2015

What about volume export/import ?

@cpuguy83

This comment has been minimized.

Copy link
Contributor Author

cpuguy83 commented Oct 15, 2015

@omribahuni this is not implemented in docker and should be a concern of the volume driver itself.
You can also fire up a container with the volume and tar/gzip it for a naive backup.

@omribahumi

This comment has been minimized.

Copy link

omribahumi commented Oct 15, 2015

@cpuguy83 I was expecting docker to do that exactly, like docker save.

@cpuguy83

This comment has been minimized.

Copy link
Contributor Author

cpuguy83 commented Oct 15, 2015

@omribahumi For persistent data, the storage backend probably already has a solution for backing up data, probably much more efficiently than docker could, and if not running a container and manually backing up with tar/gzip is a simple/elegant solution.

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Oct 15, 2015

@cpuguy83 may be something we should document; i.e. using the plugin's tools, or how to do an export

noahsw added a commit to winsleague/winsleague that referenced this pull request Nov 8, 2015
…onfig for service "webapp" is ambiguous. In a future version of Docker, it will designate a "named" volume (see moby/moby#14242). To prevent unexpected behaviour, change it to "./webapp:/webapp")
@trkoch

This comment has been minimized.

Copy link

trkoch commented Nov 12, 2015

Thanks for implementing this! Makes handling volumes a whole lot easier.

After reading through docs (and skimming comments here) I'm still unsure about the approach taken to seed data of named volumes. When creating containers with auto-created (anonymous) volumes, mounting a volume to an existing folder results in existing data to be copied from the image to the volume. This does not apply to host mounted volumes. What about named volumes?

@trkoch

This comment has been minimized.

Copy link

trkoch commented Nov 12, 2015

It appears named volumes are treated just like host mounted volumes.

Anonymous volumes

$ docker run --rm -it -v /etc debian ls -1 /etc | wc -l | awk '{print $1}'
92

Host mounted volumes

$ docker run --rm -it -v $(pwd)/etc:/etc debian ls -1 /etc | wc -l | awk '{print $1}'
3

Named volumes

$ docker run --rm -it -v test:/etc debian ls -1 /etc | wc -l | awk '{print $1}'
3
sokrat added a commit to consultnn/yii2-docker-app-advanced that referenced this pull request Nov 27, 2015
Warning: the mapping "project:/www" in the volumes config for service "nginx" is ambiguous. In a future version of Docker, it will designate a "named" volume (see moby/moby#14242). To prevent unexpected behaviour, change it to "./project:/www"
KengoTODA added a commit to KengoTODA/brownie that referenced this pull request Dec 5, 2015
wsargent pushed a commit to wsargent/docker-cheat-sheet that referenced this pull request Jul 18, 2017
Will Sargent
Fixed by moby/moby#14242
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.