deprecate bind path auto-create by cpuguy83 · Pull Request #16349 · moby/moby

cpuguy83 · 2015-09-16T18:32:12Z

Fixes #16302
Fixes #12776
Fixes #13468
I'm sure a few more that I could not find again.

ping @calavera

calavera · 2015-09-16T19:27:18Z

@cpuguy83 I think we should show a warning in the client when this happens.

@docker/maintainers Please, take a look. This is source of much confusion among users and we'd like to deprecate it.

tiborvass · 2015-09-16T19:29:19Z

warning is fine for me

tianon · 2015-09-16T19:33:11Z

Definitely +1 for deprecation, and +1 for a warning 👍 🤘

duglin · 2015-09-16T19:36:08Z

sgtm

calavera · 2015-09-16T20:12:58Z

Just to clarify, I meant to show a warning saying that we auto-created the directory but it's a deprecated feature and we're going to stop doing it.

abronan · 2015-09-16T21:53:25Z

Agreed for deprecating/removing it, SGTM

thaJeztah · 2015-09-16T22:00:40Z

+1 for removing this "feature", and +1 for outputting a warning now (including a "deprecation" message?)

(If we want a "warning" in 1.9, please add that change to this PR)

The documentation changes in this PR can use a slight touch-up, but I'll wait until it's been agreed on.

cpuguy83 · 2015-09-16T23:20:23Z

Presenting the user with a warning is not a small change.
Warning the daemon logs is simple enough.

Not sure about presenting a warning to the user here unless we just do it for all binds.

cpuguy83 · 2015-09-16T23:21:00Z

And then it would just be on the client.

moxiegirl · 2015-09-17T02:49:07Z

LGTM @thaJeztah touch up at will it looks ok by me. Didn't we just have a pr touching on bind mount --- #13776? Does that language still work in light of this...bit late for me...maybe I'm not clear

stevvooe · 2015-09-17T09:55:06Z

👍

calavera · 2015-09-17T15:24:33Z

@cpuguy83 maybe the daemon log is good enough, that's what we did with LXC:

https://github.com/docker/docker/pull/15255/files

Can you add it here?

cpuguy83 · 2015-09-17T15:41:52Z

Pushed with daemon log added.

stevvooe · 2015-09-18T08:54:14Z

daemon/volumes.go

This needs to check for an error that is not a os.IsNotExist and handle that appropriately:

if fi, err := os.Stat(m.Source); err != nil { if !os.IsNotExist(err) { //.. handle error } logrus.Warnf("Auto-creating non-existant volume host path %s, this is deprecated and will be removed soon", m.Source) }

I suppose we can, though this would picked up by the subsequent os.MkdirAll anyway.
But since we're already adding the stat call, might as well handle earlier.

hah, the worst part is we were already doing this, and this is already wrapped in a stat call... silly me.

Signed-off-by: Brian Goff <[email protected]>

LK4D4 · 2015-09-18T17:03:18Z

LGTM

calavera · 2015-09-18T19:53:01Z

LGTM. Merging since #16287 takes care of the deprecation note in the run page mentioned by @moxiegirl .

deprecate bind path auto-create

m59peacemaker · 2016-07-10T17:07:47Z

The way I'm understanding this, if /foo doesn't exist on the host and I do:

docker run -v /foo:/foo etc

/foo shouldn't be created on the host. I'm on 1.11 and the directory is still created.

There isn't even a warning. Am I misreading this?

thaJeztah · 2016-07-10T18:21:40Z

@m59peacemaker this change was later reverted; see #19953 (comment), and #21666

m59peacemaker · 2016-07-10T18:41:51Z

@thaJeztah Well, that's unfortunate.. I commented my issue here: #19953 (comment)

Thanks for letting me know.

stevvooe · 2016-07-15T02:53:14Z

@thaJeztah Auto-creation of bind paths is why we can't have nice things. This model is extremely broken.

Any way we can re-deprecate this?

thaJeztah · 2016-07-15T06:36:53Z

@stevvooe I know 😞. To re-deprecate, I think best start would be to open an issue or PR for discussion

tiborvass · 2016-07-15T18:02:53Z

We can't deprecate this, there are scripts relying on this misfeature.

cpuguy83 · 2016-07-15T18:04:30Z

I wish we got the new mounts (for containers) API in for 1.12 so that at least services didn't have to propagate this nastiness.

m59peacemaker · 2016-07-15T18:35:19Z

@tiborvass The nocreate option you mentioned here is all I need. You said it wouldn't be useful, but I think my situation calls for it. See #19953 (comment)

diego-treitos · 2017-01-11T11:01:33Z

In my opinion the directory auto-creation and mounting a host file as a data volume are incompatible options.
If you can truly mount any type of file (directory, normal file, socket, pipe, etc) then you cannot auto-create in case it does not exist because you dont know what to create and even if you knew you cannot control the permissions it will have on creation so it might create problems too. This feature makes no sense.

If you truly want to have a feature like this you would need options like:

create-d: Auto-create directory if missing. Default option for backwards compatibility.
create-p: Auto-create pipe if missing
create-s: Auto-create socket if missing
create-f: Auto-create regular file if missing
create-none: Disable auto-creation

and also have an option for <user_id>:<group_id>:<permissions> of the file being autocreated. So you could end up with something like:

$ docker run --rm -it -v '/var/run/mysqld/mysqld.sock:/var/run/mysqld/mysqld.sock:rw:create-s:mysql:mysql:777' ubuntu /bin/bash

This way you have full control over the result of starting a container.

For now, just being able to disable autocreation with nocreate would help to fix many systems although I dont think it is a full solution in the long term in case you want a fully functional auto-create feature.

thaJeztah · 2017-01-11T11:26:21Z

Plans are to provide an "advanced" syntax for docker run, similar to the --mount flag on docker service create. When using the advanced syntax, auto-creation will be disabled for bind-mounts, and the user will be responsible for creating the mounted directory, file (or socket) before the container is started. The intent of bind-mounting something from the host is to allow a container access to something that exists on the host, not to have docker create something on the host (i.e. docker manages anything inside /var/lib/docker, but outside of that, it should not be managing things).

The current "shorthand" syntax for bind-mounts -v /foo/bar:/container/path will likely not be modified (for reasons of backward compatibility).

diego-treitos · 2017-01-11T11:33:02Z

@thaJeztah thanks a lot for the update. That sounds really good and makes a lot of sense. Looking forward to see those changes :)

fwanggg · 2017-09-28T15:01:12Z

wise decision to leave existing syntax's behavior unchanged. some monitoring solutions leverages the auto-create feature to install their software.

thaJeztah · 2017-09-28T15:19:39Z

To update my earlier comment: the --mount syntax was implemented in #32251, and is included in docker 17.05 and up

GordonTheTurtle added the status/3-docs-review label Sep 16, 2015

cpuguy83 force-pushed the 16302_deprecate_autocreate_binds branch from a1f61a6 to b66102b Compare September 16, 2015 18:32

cpuguy83 added status/1-design-review and removed status/3-docs-review labels Sep 16, 2015

calavera added the impact/changelog label Sep 16, 2015

thaJeztah added the impact/deprecation label Sep 16, 2015

cpuguy83 force-pushed the 16302_deprecate_autocreate_binds branch from b66102b to 39d4b12 Compare September 17, 2015 15:41

calavera added status/2-code-review and removed status/1-design-review labels Sep 17, 2015

mikebrow mentioned this pull request Sep 17, 2015

man update for docker run with host volumes #16287

Merged

stevvooe reviewed Sep 18, 2015
View reviewed changes

deprecate bind path auto-create

249f45b

Signed-off-by: Brian Goff <[email protected]>

cpuguy83 force-pushed the 16302_deprecate_autocreate_binds branch from 39d4b12 to 249f45b Compare September 18, 2015 14:28

calavera added a commit that referenced this pull request Sep 18, 2015

Merge pull request #16349 from cpuguy83/16302_deprecate_autocreate_binds

e61abac

deprecate bind path auto-create

calavera merged commit e61abac into moby:master Sep 18, 2015

cpuguy83 deleted the 16302_deprecate_autocreate_binds branch September 18, 2015 20:01

thaJeztah mentioned this pull request Sep 24, 2015

docker unexpectedly creates non-existent host volumes #16055

Closed

thaJeztah mentioned this pull request Dec 16, 2015

Named volumes do not copy existing data from image path to volume. #18670

Closed

thaJeztah mentioned this pull request Oct 11, 2017

Remove error return on !exists in validateMountConfigReg #35159

Closed

Conversation

cpuguy83 commented Sep 16, 2015

Uh oh!

calavera commented Sep 16, 2015

Uh oh!

tiborvass commented Sep 16, 2015

Uh oh!

tianon commented Sep 16, 2015

Uh oh!

duglin commented Sep 16, 2015

Uh oh!

calavera commented Sep 16, 2015

Uh oh!

abronan commented Sep 16, 2015

Uh oh!

thaJeztah commented Sep 16, 2015

Uh oh!

cpuguy83 commented Sep 16, 2015

Uh oh!

cpuguy83 commented Sep 16, 2015

Uh oh!

moxiegirl commented Sep 17, 2015

Uh oh!

stevvooe commented Sep 17, 2015

Uh oh!

calavera commented Sep 17, 2015

Uh oh!

cpuguy83 commented Sep 17, 2015

Uh oh!

stevvooe Sep 18, 2015

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Sep 18, 2015

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Sep 18, 2015

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Sep 18, 2015

Uh oh!

calavera commented Sep 18, 2015

Uh oh!

m59peacemaker commented Jul 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Jul 10, 2016

Uh oh!

m59peacemaker commented Jul 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevvooe commented Jul 15, 2016

Uh oh!

thaJeztah commented Jul 15, 2016

Uh oh!

tiborvass commented Jul 15, 2016

Uh oh!

cpuguy83 commented Jul 15, 2016

Uh oh!

m59peacemaker commented Jul 15, 2016

Uh oh!

diego-treitos commented Jan 11, 2017

Uh oh!

thaJeztah commented Jan 11, 2017

Uh oh!

diego-treitos commented Jan 11, 2017

Uh oh!

fwanggg commented Sep 28, 2017

Uh oh!

thaJeztah commented Sep 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

m59peacemaker commented Jul 10, 2016 •

edited

Loading

m59peacemaker commented Jul 10, 2016 •

edited

Loading