Volume mounts need to use "Binds" API field #23947
Conversation
| @@ -207,7 +206,7 @@ func getMountMask(m *api.Mount) string { | |||
| func (c *containerConfig) hostConfig() *enginecontainer.HostConfig { | |||
| return &enginecontainer.HostConfig{ | |||
| Resources: c.resources(), | |||
| Binds: c.bindMounts(), | |||
| Binds: append(c.bindMounts(), c.volumes()...), | |||
There was a problem hiding this comment.
And now you might see why I think the engine side API is horrible :)
There was a problem hiding this comment.
Double check this works with an empty source...
When I tried to implement this, volumes that were just the dst/target were required to be in Volumes.
There was a problem hiding this comment.
Should we even allow an empty source?
There was a problem hiding this comment.
It fills in the role of an anonymous volume. It's a use case we support today and I'm not sure we can actually leave out.
There was a problem hiding this comment.
Ok... one thing I don't like here (different topic) is docker will automatically create the volume with the given name if it doesn't already exist.
For example:
$ docker service create --mount type=volume,source=test,target=/foo busybox top
Even though we've not provided any create options, we'll get a volume with the specified name, on the local driver.
There was a problem hiding this comment.
The following should also work, given docker's current model:
docker service create --mount type=volume,target=/foo busybox top
It's going to be a big change to not automagically create volumes. These are declared this way all over the place, including in docker files.
The solution, as you have suggested, is to bind the lifecycle of these volumes to the scope of the container lifecycle.
dnephin
added
the
kind/bug
|
LGTM
😢 Does swarmkit need this fix, as well? |
|
Is it possible to use the mount structures directly in the engine code? |
|
@cpuguy83 bump.
I ask this because swarmkit tries to put source-less volumes in the |
|
@stevvooe By swarmkit do you mean swarmd? |
|
@cpuguy83 I mean |
Swarm was putting volume type mounts into the container config's "Volumes" field, but really these need to go into "Binds". "Volumes" is only for normal "-v /foo" volumes, not named volumes or anything else. Signed-off-by: Brian Goff <cpuguy83@gmail.com>
cpuguy83
force-pushed
the
fix_mount_target
branch
from
19a1bd2
to
2bc2165
Compare
|
@stevvooe This is updated. Will have to look at swarmkit. |
|
LGTM |
1 similar comment
|
LGTM |
|
❤️ |
Swarm was putting volume type mounts into the container config's
"Volumes" field, but really these need to go into "Binds".
"Volumes" is only for normal "-v /foo" volumes, not named volumes or
anything else.
I was looking at adding an integration test for this, however it dawned on me I was testing implementation detail as there is not really an easy way to actually get at the underlying container object to have a look at it, or really any good way to dive into this service.