Allow path from normal volume existing to overwrite in start Binds #10365
Conversation
1d29a01 to
ba215d4
Compare
| @@ -80,7 +81,7 @@ func (m *Mount) initialize() error { | |||
| if hostPath, exists := m.container.Volumes[m.MountToPath]; exists { | |||
| // If this is a bind-mount/volumes-from, maybe it was passed in at start instead of create | |||
| // We need to make sure bind-mounts/volumes-from passed on start can override existing ones. | |||
| if !m.volume.IsBindMount && m.from == nil { | |||
| if (!m.volume.IsBindMount && !m.isBind) && m.from == nil { | |||
There was a problem hiding this comment.
what is the difference between IsBindMount and isBind? why would m.from not be nil if it's a bind mount?
There was a problem hiding this comment.
part of the problem why this needs to be refactored cause this looks weird but....
We have m.volume.IsBindMount, which is set if the volume itself is a bind-mount, which in the tested case it is not since it was created as a normal volume.
m.IsBind is (now) set when it is created during parsing HostConfig.Binds.
< 8000 /div>There was a problem hiding this comment.
why do we need another bool when m.from should be nil if it's a bind mount?
There was a problem hiding this comment.
Because the issue this is fixing isn't with volumes-from, it's doing something like docker run -v /var/lib/docker/vfs/dir/55a6f3a06952bf1c6f8a93a85c55493107a6cbe8191a8d9e83ff75fad705e268:/foo ...
|
To clear up some confusion:
|
|
Thanks @cpuguy83, this helps a lot! So if I understand correctly: bind-mounts and normal volumes are essentially the same behavior, what matters to Docker is keeping track of directories it created itself because it's responsible for their disposal. |
|
One extra question: do we absolutely need this in 1.5.0 (i.e., is it a regression from 1.4.1)? If we don't, should we wait for the ongoing refactor? |
(this caught my eye); @cpuguy83 would that mean that with this change, docker will see volumes "copied" during the Fig/Compose re-creation step to be a "bind mount", thus TBH I don't know how Fig/Docker handles this currently, just wondering, because that would make it harder to cleanup volumes. (I think this PR was because of a regression when using Fig/Compose with Docker 1.5) |
|
@cpuguy83 I really don't understand :( Can you explain this magic about |
|
@icecrime This is a regression that occurred in 1.4 when volumes started being created at the same time the container is created (as opposed to just when the container was started). @LK4D4 "volumes" are really two things, mount points and data. The stuff in daemon/volumes is really dealing with mount points. |
|
I got idea, but some day we need to refactor volumes code :) |
|
@crosbymichael Agreed, much of this is already done here: https://github.com/cpuguy83/docker/tree/cleanup_daemon_volumes |
|
@cpuguy83 rebase please! :) |
Fixes moby#9981 Allows a volume which was created by docker (ie, in /var/lib/docker/vfs/dir) to be used as a Bind argument via the container start API and overwrite an existing volume. For example: ```bash docker create -v /foo --name one docker create -v /foo --name two ``` This allows the volume from `one` to be passed into the container start API as a bind to `two`, and it will overwrite it. This was possible before 7107898 Signed-off-by: Brian Goff <cpuguy83@gmail.com>
ba215d4 to
49e1ad4
Compare
|
rebased |
|
ping |
|
That cleanup is here: #10422 |
|
Considering that 1.6.0 will be ~March 22nd, it's highly unlikely we do a 1.5.1. I'm closing this but keeping the milestone in case it happens for some reason. Thanks! |
|
@dmp42 You really have to unauth waffle.io :) |
|
LGTM |
…bind_as_bind Allow path from normal volume existing to overwrite in start Binds
Fixes #9981
Allows a volume which was created by docker (ie, in
/var/lib/docker/vfs/dir) to be used as a Bind argument via the container
start API and overwrite an existing volume.
For example:
This allows the volume from
oneto be passed into the container startAPI as a bind to
two, and it will overwrite it.This was possible before 7107898