Fix linux mount calls not applying propagation type changes #30416
Conversation
|
ping @cpuguy83 @justincormack PTAL |
|
Not sure the jenkins errors are related to this PR? |
|
Can we add some tests for these, as clearly there were not sufficient ones before or it would have been noticed... |
stevenh
force-pushed
the
linux-mount-fixes
branch
from
dcde4d5
to
d87a157
Compare
|
@justincormack tests added |
|
@stevenh the tests seem to be failing... |
stevenh
force-pushed
the
linux-mount-fixes
branch
2 times, most recently
from
d5c2092
to
73819b4
Compare
|
I think there may be something kernel / OS specific about slave mount reporting as this latest version works fine here on ubuntu 16 LTS here, need to investigate more. |
stevenh
force-pushed
the
linux-mount-fixes
branch
from
73819b4
to
a07a209
Compare
|
The underlying issue was false assumptions about the parent mounts propagation type in the test, locally its was shared on jenkins they must be private, so the resulting Optionals where unstable. I've restructured the test to start with a source that has known flags, so hopefully it should be good now. |
|
@justincormack this good for you now? |
|
ping @justincormack |
|
It looked good, will review again just to check.
On 1 Feb 2017 9:18 p.m., "Alexander Morozov" <notifications@github.com> wrote:
ping @justincormack <https://github.com/justincormack>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#30416 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAdcPOMTZyWIvQQZIRm2eJE2DCVtJMtxks5rYPaDgaJpZM4Lsl-9>
.
|
|
ping @justincormack |
pkg/mount/mounter_linux.go
Outdated
| func mount(device, target, mType string, flags uintptr, data string) error { | ||
| oflags := flags &^ ptypes | ||
| if !isremount(device, flags) { | ||
| // Initial call applying all none propagation flags. |
stevenh
force-pushed
the
linux-mount-fixes
branch
from
a07a209
to
3194a14
Compare
pkg/mount/mounter_linux.go
Outdated
| // isremount returns true if either device name or flags identify a remount request, false otherwise. | ||
| func isremount(device string, flags uintptr) bool { | ||
| switch { | ||
| case flags&syscall.MS_REMOUNT != 0, device == "", device == "none": |
There was a problem hiding this comment.
ok, unsure here. Why is device=="" or device="none" a remount? I can mount a tmpfs with these options. Why is presense of MS_REMOUNT not sufficient?
There was a problem hiding this comment.
The problem with only looking at MS_REMOUNT is that typical use cases don't do this.
An example of this can be seen in sharedsubtree_linux:
Mount(mountPoint, mountPoint, "none", "bind,rw")
These are covered in the test cases so removing the "" and "none" case you'll see the following failures:
--- FAIL: TestMount (0.22s)
--- FAIL: TestMount/none-bind,slave (0.00s)
mounter_linux_test.go:82: no such device
--- FAIL: TestMount/none-bind,rw,slave (0.00s)
mounter_linux_test.go:82: no such device
--- FAIL: TestMount/none-bind,ro,slave (0.00s)
mounter_linux_test.go:82: no such device
--- FAIL: TestSubtreePrivate (0.02s)
sharedsubtree_linux_test.go:66: no such device
--- FAIL: TestSubtreeShared (0.00s)
sharedsubtree_linux_test.go:143: no such device
--- FAIL: TestSubtreeSharedSlave (0.00s)
sharedsubtree_linux_test.go:222: no such device
--- FAIL: TestSubtreeUnbindable (0.00s)
sharedsubtree_linux_test.go:303: no such device
There was a problem hiding this comment.
Ah yes. Could we add a note in the code explaining that as it is non obvious, especially as the pkg/ code is intended for use outside Docker and external users might be confused...
Propagation type changes must be done as a separate call, in the same way as read only bind mounts. To fix this: 1. Ensure propagation type change flags aren't included in other calls. 2. Apply propagation type change in a separate call. Also: * Make it clear which parameters are ignored by passing them as empty. * Add tests to ensure Mount options are applied correctly. Fixes moby#30415 Signed-off-by: Steven Hartland <steven.hartland@multiplay.co.uk>
stevenh
force-pushed
the
linux-mount-fixes
branch
from
3194a14
to
b3b14b9
Compare
|
Windows failure unrelated and passed before comments changed. |
Propagation type changes must be done as a separate call, in the same way as read only bind mounts.
To fix this:
Fixes #30415
Signed-off-by: Steven Hartland steven.hartland@multiplay.co.uk