-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
Capability to specify per volume mount propagation mode #17034
Conversation
If I specify two volumes one as shared and one as private
Won't propogation end up being RSLAVE? |
I first check if there are any shared volumes and set propagation to "shared" and return. I think you missed the "return" statement. |
Simpler would be to just check if a higher level is set and then don't change it. if root prop = shared don't set to private, or slave, |
@rhatdan I had thought about it. I think it is a good idea to implement it. Will do. |
a1ab552
to
b68aad2
Compare
|
HOW TO TEST
HOW TO DETERMINE SOURCE MOUNT
HOW TO CHECK PROPAGATION PROPERTY OF A MOUNT HOW TO CHANGE PROPAGATION PROPERTY OF A MOUNT |
@@ -288,18 +289,42 @@ func (d *Driver) setupMounts(container *configs.Config, c *execdriver.Command) e | |||
|
|||
for _, m := range c.Mounts { | |||
flags := syscall.MS_BIND | syscall.MS_REC | |||
var propagationFlags []int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this an array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case it does not have to be. Will change it to integer.
b68aad2
to
f712fb8
Compare
@crosbymichael Fixed the array issue. Also I have added another patch which verifies that source mount point has appropriate propagation properties for volume mount to work, otherwise container start will fail. PTAL. |
Looks like your additional change broke some stuff and there was a bad rebase |
f712fb8
to
f69ba4d
Compare
@crosbymichael Found the bug I had introduced. PropagationFlags need to be nil if there is no propagation flag to be applied. With the second round of changes, it got initialized to 0 and libcontainer was trying to apply that and failing. Fixed it. Also rebased on top of latest code. This volume support on windows moved quite a few things around. |
Build error on windows binaries |
hmm..., Need to define volume.GetPropagation() for windows. Will do. |
f69ba4d
to
8632406
Compare
Defined a function GetPropagation() for windows which returns nil string. Hopefully that fixes windows build. lets see. |
return rwModes[strings.ToLower(mode)] | ||
} | ||
|
||
// Windows does not support mount propagation. Just return empty string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be godoc'd starting with the function name - something like "GetPropagation is a platform-specific function to ..... This is not supported on Windows".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jhowardmsft Will do. Thanks.
Apart from the godoc nit, LGTM from a Windows perspective. @rhvgoyal Apologies for the rebase pain with the introduction of volume support on Windows. |
8632406
to
d3cc2ba
Compare
Took care of validate-lint issue. Added a comment starting with GetPropagation. |
d3cc2ba
to
ebcd3fb
Compare
@crosbymichael Did some more changes. PTAL.
|
@crosbymichael Right now these are 3 patches. Do let me know if you want me to merge all this in single patch. |
My newly introduced integration test FAIL: docker_cli_run_test.go:3615: DockerSuite.TestRunVolumesMountedAsShared docker_cli_run_test.go:3644: |
Oh look, this is exactly what I need for a thing I'm doing at this moment. When can a release including this functionality be expected to appear? |
@griwes this will be in the 1.10 release, probably in February. If you want to give it a test, you can either try the builds from master (https://master.dockerproject.org) or the experimental builds https://experimental.docker.com. |
Is it assumed knowledge that
Without this I got an error indicating that |
Yes this is expected. Note that RHEL7, Fedora, And Centos come this way by default. |
|
||
To change propagation properties of a mount point use `mount` command. For | ||
example, if one wants to bind mount source directory `/foo` one can do | ||
`mount --bind /foo /foo` and `mount --make-private --make-shared /foo`. This |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this be mount --make-shared
(i.e. without --make-private
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usage of --make-private
decouples the mount from any other sharing already going on. So while this is not strictly necessary, this might be little less confusing in some of the configurations when propagation actually happens.
Make a unit file or service script to do this. |
@kiwenlau on 14.10+ with systemd you can set the MountFlags=shared and you wont need to set --make-shared each time. You can also set that in /etc/fstab with 14.10+. on 14.04 you are limited to some type of init script for this (can also be a udev trigger depending on if that makes the situation better) |
@rhatdan @SamYaple I use Ubuntu 14.04. However, I can not set shared mount for it in fstab file. I have to manually set it when I reboot ubuntu It's OK for me, and I will try some init script to do it. Thanks. |
@kiwenlau Yes i believe I stated that this only works on 14.10+ when set in /etc/fstab or with the mount binary. If you pullin the mount binary from 14.10+ it would work, but may break other things |
As mentioned in moby/moby#17034 in order to follow the netns corresponding file when it gets bind mounted, the netns directory bind mounted in the skydive agent container with a docker volume option needs to have the "shared" propagation flag set. By default, the propagation flag of a docker volume is "private", not "shared". This works since the usual source (/var/run/netns) also has the "shared" propagation flag. Otherwise, one has to set it on the source before starting the skydive agent container. Change-Id: Ic321681269aba267a51bc0782f68683660c9eca0 Reviewed-on: https://softwarefactory-project.io/r/6397 Reviewed-by: Nicolas PLANEL <nplanel@redhat.com> Reviewed-by: Sylvain Baubeau <sbaubeau@redhat.com> Tested-by: Sylvain Baubeau <sbaubeau@redhat.com> Workflow: Sylvain Baubeau <sbaubeau@redhat.com> Tested-by: Jenkins CI <jenkins@softwarefactory-project.io>
Fixes #14630
This patch set allows one to specify mount propagation setting of a volume. Allowed values are "shared" or "slave" or "private".
For example.
docker run -ti -v /root/mnt-source:/root/mnt-dest:slave fedora bash