Remove --read-only restriction when user ns enabled #25540
Conversation
| func testReadOnlyFile(c *check.C, filenames ...string) { | ||
| // Not applicable on Windows which does not support --read-only | ||
| testRequires(c, DaemonIsLinux, NotUserNamespace) | ||
| func testReadOnlyFile(c *check.C, testPriv bool, filenames ...string) { | ||
| touch := "touch " + strings.Join(filenames, " ") |
There was a problem hiding this comment.
doesnt this still want testRequires(c, DaemonIsLinux)?
There was a problem hiding this comment.
It's only called from a function that already has that testRequires on Linux, so seemed reasonable to remove it. Actually felt lazy, because maybe this function is not necessary as it is only called from the one test.. don't know the history...
|
The userns failure looks real... (if odd) |
|
Oh yuck.. looks like there may be environmental issues that make this not a home run across all distros/setups? :( Definitely not having the dev remount error on Ubuntu 16.04 LTS |
|
Hey @mrunalp, do you have any thoughts on this re: kernel and/or distro peculiarities? |
|
@estesp No, I don't really have a matrix :/ I can try on few Fedoras and RHELs and get back. |
|
@estesp I think we'll have to figure out the minimum supported kernel to do this. |
thaJeztah
added
the
status/failing-ci
|
@mrunalp: I wonder if it is 3.19? Given this specific change around dev remount? torvalds/linux@87c31b3#diff-3fbed1fd4d15699b74b30cabf5be8133L2100 |
|
I feel like it would be good to get rid of the restriction with a note that, like other Docker capabilities, may rely on a specific kernel level for proper operation. Should I break out the test re-enable into a separate PR and have a discussion about whether some CI systems should be testing more modern kernels + features enabled by those kernels? ping @docker/core-engine-maintainers |
|
I like that idea as it can be used where possible. Sent from my iPhone
|
|
Any update? |
estesp
force-pushed
the
ro-plus-userns
branch
2 times, most recently
from
7010266
to
1e781fa
Compare
|
Ok, updated with a compromise--added a integration-cli requirement that checks for RO mount capability when user namespaces enabled. This way the tests run on as many platforms as will support it and are only skipped if the kernel is too old to deal with RO mounts + userns. |
| if os.Getenv("DOCKER_REMAP_ROOT") == "" { | ||
| return true | ||
| } | ||
| if _, _, err := dockerCmdWithError("run", "--read-only", "busybox", "date"); err != nil { |
There was a problem hiding this comment.
I'm guessing you also need to run this with --rm or make sure its cleaned up correctly?
estesp
removed
the
status/failing-ci
The restriction is no longer necessary given changes at the runc layer related to mount options of the rootfs. Also cleaned up the docs on restrictions left for userns enabled mode. Re-enabled tests related to --read-only when testing a userns-enabled daemon in integration-cli. Docker-DCO-1.1-Signed-off-by: Phil Estes <estesp@linux.vnet.ibm.com> (github: estesp)
estesp
force-pushed
the
ro-plus-userns
branch
from
1e781fa
to
6062ae5
Compare
|
LGTM |
|
LGTM 🐸 |
|
Docs LGTM - though it needs a release note |
|
LGTM @estesp can you write a short line for the changelog, and add it to your top description? |
|
@thaJeztah added, thx! |
|
Thanks @estesp 😃 |
The restriction is no longer necessary given changes at the runc layer
related to mount options of the rootfs. Also cleaned up the docs on
restrictions left for userns enabled mode. Re-enabled tests related to
--read-only when testing a userns-enabled daemon in integration-cli.
Docker-DCO-1.1-Signed-off-by: Phil Estes estesp@linux.vnet.ibm.com (github: estesp)
Changelog entry: Use of
--read-onlyis no longer restricted from use when user namespaces are enabled in the Docker engine. Requires modern kernel which does not restrict remount as read-only in a user namespace.