Skip to content
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

Explicitly set `rw` / `ro` for mounts #38785

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@thaJeztah
Copy link
Member

thaJeztah commented Feb 23, 2019

The current code assumed that mounts were mounted "read-write" by default, and added "ro" to make them read-only.

In some situations, omitting the "rw" option could result in sysfs mounts becoming read-only (possibly inheriting defaults from the host).

This patch changes the approach to always set either "ro" or "rw", instead of omitting these options (thus ambiguous).

A similar fix was already implemented in containerd/cri
containerd/cri@a5d1332

relates to #24000

Explicitly set `rw` / `ro` for mounts
The current code assumed that mounts were mounted "read-write"
by default, and added "ro" to make them read-only.

In some situations, omitting the "rw" option could result in
`sysfs` mounts becoming read-only (possibly inheriting defaults
from the host).

This patch changes the approach to always set either "ro" or
"rw", instead of omitting these options (thus ambiguous).

A similar fix was already implemented in containerd/cri
containerd/cri@a5d1332

Reported-by: Lantao Liu <lantaol@google.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah

This comment has been minimized.

Copy link
Member Author

thaJeztah commented Feb 23, 2019

@Random-Liu

This comment has been minimized.

Copy link

Random-Liu commented Feb 23, 2019

@thaJeztah Actually, I'm not sure whether that actually fixes the issue. It seems that @lbernail still saw the issue recently.

@thaJeztah

This comment has been minimized.

Copy link
Member Author

thaJeztah commented Feb 23, 2019

oh, thanks for the heads-up @Random-Liu I'll remove the "fixes". Not sure if this change still makes sense (although it may make things more consistent)

@Random-Liu

This comment has been minimized.

Copy link

Random-Liu commented Feb 23, 2019

@thaJeztah We can wait for confirmation from @lbernail. I don't remember exactly whether he hit the issue in containerd 1.1 or 1.2. The fix is only included in 1.2.

But anyway, we haven't root cause this yet, explicitly specifying rw is just one blind try to fix this, and it does make things more consistent :p

@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 23, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@cbb885b). Click here to learn what that means.
The diff coverage is 80%.

@@            Coverage Diff            @@
##             master   #38785   +/-   ##
=========================================
  Coverage          ?   36.48%           
=========================================
  Files             ?      613           
  Lines             ?    45849           
  Branches          ?        0           
=========================================
  Hits              ?    16729           
  Misses            ?    26830           
  Partials          ?     2290
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.