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

Remove --read-only restriction when user ns enabled #25540

Merged
merged 1 commit into from Sep 14, 2016

Conversation

Projects
None yet
9 participants
@estesp
Contributor

estesp commented Aug 9, 2016

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-only is 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.

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, " ")

This comment has been minimized.

@justincormack

justincormack Aug 9, 2016

Contributor

doesnt this still want testRequires(c, DaemonIsLinux)?

@justincormack

justincormack Aug 9, 2016

Contributor

doesnt this still want testRequires(c, DaemonIsLinux)?

This comment has been minimized.

@estesp

estesp Aug 9, 2016

Contributor

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...

@estesp

estesp Aug 9, 2016

Contributor

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...

@justincormack

This comment has been minimized.

Show comment
Hide comment
@justincormack

justincormack Aug 9, 2016

Contributor

The userns failure looks real... (if odd)

Contributor

justincormack commented Aug 9, 2016

The userns failure looks real... (if odd)

@estesp

This comment has been minimized.

Show comment
Hide comment
@estesp

estesp Aug 9, 2016

Contributor

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

Contributor

estesp commented Aug 9, 2016

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

@estesp

This comment has been minimized.

Show comment
Hide comment
@estesp

estesp Aug 9, 2016

Contributor

Hey @mrunalp, do you have any thoughts on this re: kernel and/or distro peculiarities?

Contributor

estesp commented Aug 9, 2016

Hey @mrunalp, do you have any thoughts on this re: kernel and/or distro peculiarities?

@mrunalp

This comment has been minimized.

Show comment
Hide comment
@mrunalp

mrunalp Aug 9, 2016

Contributor

@estesp No, I don't really have a matrix :/ I can try on few Fedoras and RHELs and get back.

Contributor

mrunalp commented Aug 9, 2016

@estesp No, I don't really have a matrix :/ I can try on few Fedoras and RHELs and get back.

@mrunalp

This comment has been minimized.

Show comment
Hide comment
@mrunalp

mrunalp Aug 9, 2016

Contributor

@estesp I think we'll have to figure out the minimum supported kernel to do this.

Contributor

mrunalp commented Aug 9, 2016

@estesp I think we'll have to figure out the minimum supported kernel to do this.

@estesp

This comment has been minimized.

Show comment
Hide comment
@estesp

estesp Aug 9, 2016

Contributor

@mrunalp: I wonder if it is 3.19? Given this specific change around dev remount? torvalds/linux@87c31b3#diff-3fbed1fd4d15699b74b30cabf5be8133L2100

Contributor

estesp commented Aug 9, 2016

@mrunalp: I wonder if it is 3.19? Given this specific change around dev remount? torvalds/linux@87c31b3#diff-3fbed1fd4d15699b74b30cabf5be8133L2100

@estesp

This comment has been minimized.

Show comment
Hide comment
@estesp

estesp Aug 23, 2016

Contributor

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

Contributor

estesp commented Aug 23, 2016

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

@mrunalp

This comment has been minimized.

Show comment
Hide comment
@mrunalp

mrunalp Aug 23, 2016

Contributor

I like that idea as it can be used where possible.

Sent from my iPhone

On Aug 22, 2016, at 6:59 PM, Phil Estes notifications@github.com wrote:

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


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Contributor

mrunalp commented Aug 23, 2016

I like that idea as it can be used where possible.

Sent from my iPhone

On Aug 22, 2016, at 6:59 PM, Phil Estes notifications@github.com wrote:

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


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Sep 8, 2016

Contributor

Any update?

Contributor

cpuguy83 commented Sep 8, 2016

Any update?

@estesp

This comment has been minimized.

Show comment
Hide comment
@estesp

estesp Sep 8, 2016

Contributor

@cpuguy83 I'm happy to update the PR to remove the test enablement until we figure out CI strategy for more modern kernel testing.. but was hoping for more than just @mrunalp's opinion, although I value it highly :) :)

Contributor

estesp commented Sep 8, 2016

@cpuguy83 I'm happy to update the PR to remove the test enablement until we figure out CI strategy for more modern kernel testing.. but was hoping for more than just @mrunalp's opinion, although I value it highly :) :)

@estesp

This comment has been minimized.

Show comment
Hide comment
@estesp

estesp Sep 9, 2016

Contributor

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.

Contributor

estesp commented Sep 9, 2016

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.

Remove --read-only restriction when user ns enabled
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)
@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Sep 9, 2016

Contributor

LGTM

Contributor

crosbymichael commented Sep 9, 2016

LGTM

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Sep 10, 2016

Member

LGTM 🐸
Moving to docs-review
/cc @thaJeztah @SvenDowideit @sfsmithcha

Member

vdemeester commented Sep 10, 2016

LGTM 🐸
Moving to docs-review
/cc @thaJeztah @SvenDowideit @sfsmithcha

@SvenDowideit

This comment has been minimized.

Show comment
Hide comment
@SvenDowideit

SvenDowideit Sep 12, 2016

Contributor

Docs LGTM - though it needs a release note

Contributor

SvenDowideit commented Sep 12, 2016

Docs LGTM - though it needs a release note

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Sep 14, 2016

Member

LGTM

@estesp can you write a short line for the changelog, and add it to your top description?

Member

thaJeztah commented Sep 14, 2016

LGTM

@estesp can you write a short line for the changelog, and add it to your top description?

@thaJeztah thaJeztah merged commit 8ac2000 into moby:master Sep 14, 2016

7 checks passed

docker/dco-signed All commits signed
Details
documentation success
Details
experimental Jenkins build Docker-PRs-experimental 23540 has succeeded
Details
janky Jenkins build Docker-PRs 32121 has succeeded
Details
userns Jenkins build Docker-PRs-userns 14143 has succeeded
Details
win2lin Jenkins build Docker-PRs-Win2Lin 30845 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 2925 has succeeded
Details

@estesp estesp deleted the estesp:ro-plus-userns branch Sep 14, 2016

@estesp

This comment has been minimized.

Show comment
Hide comment
@estesp

estesp Sep 14, 2016

Contributor

@thaJeztah added, thx!

Contributor

estesp commented Sep 14, 2016

@thaJeztah added, thx!

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Sep 14, 2016

Member

Thanks @estesp 😃

Member

thaJeztah commented Sep 14, 2016

Thanks @estesp 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment