-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Eliminate redundant parsing of mountinfo #608
Eliminate redundant parsing of mountinfo #608
Conversation
d144b41
to
a81948d
Compare
rootfs parent mount change doesn't look like it preserves behaviour. Pivot root can be not called at all. |
85e9042
to
7351913
Compare
@LK4D4 Thanks! Modified to invoke |
b4ee7b6
to
d259a17
Compare
@LK4D4 Do I need any other fix? |
txt := scanner.Text() | ||
sepIdx := strings.Index(txt, " - ") | ||
if sepIdx == -1 { | ||
return selinuxfs |
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.
is this correct to return here if the line is malformed? I'm not sure it should return.
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.
@crosbymichael Thanks! I referred to cgroups.getCgroupMountsHelper() to parse mountinfo
. Is it better to continue
the loop here for robustness?
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.
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.
I think we should continue and just skip the malformed lines.
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.
@crosbymichael @mrunalp Fixed according to your comments. Thanks!
f88d2c2
to
33cde44
Compare
@crosbymichael Do I need any other fix? |
LGTM |
LGTM, although I am a little worried about the fragility of the /proc/self/mountinfo parsing, but perhaps this was just as bad when we were using "github.com/docker/docker/pkg/mount" |
447b004
to
fbd388d
Compare
Fine with me. |
if len(txt) < sepIdx+12 { | ||
continue | ||
} | ||
if txt[sepIdx+3:sepIdx+12] != "selinuxfs" { |
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 not strings.Contains()
or even just a comment explaining where these magic numbers come from?
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 stuff runs like gazillion times per container stat, so even Splits and Contains in the top of Docker CPU usage :(
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 that case we should cache it, If we don't already. This only needs to be checked once, only way to change this state is to reboot the machine.
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.
Yup, it would be cool. Thanks for info @rhatdan
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.
Could probably be an option to runc and if passed we can skip this check. We can also do the same for the systemd cgroups check.
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.
@cyphar @LK4D4 Thank you very much for your helpful suggestions. I think eliminating the magic numbers makes sense in this context to run a large number of containers, since the primary overhead of mount.GetMounts()
comes from parsing all lines in mountinfo
and creating corresponding objects. Fixed to use strings.Contains()
.
fbd388d
to
3914af9
Compare
Looking at the code, this will only be called once. selinuxfs=unknown to start, First call gets set to "", then if it is in SELInux mode it will get set to the path. All future calls will return "" or SELINUXFSPATH. And never call this code. |
2ea6286
to
962a0eb
Compare
@rhatdan Thank you for your suggestion. When SELinux is enabled, |
scanner := bufio.NewScanner(f) | ||
for scanner.Scan() { | ||
txt := scanner.Text() | ||
sepIdx := strings.Index(txt, " - ") |
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.
I realise this is exactly what the old code did, but Is this field always guaranteed to be -
? Why not just look at the 9th field? Or is this to protect against spaces in the mountpoint names?
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.
57fa063
to
be38788
Compare
@cyphar Does this look good to you? |
2452f27
to
c373395
Compare
@LK4D4 @rhatdan @crosbymichael @mrunalp @cyphar Is this PR ready to be merged? Or, is any other fix required? |
cf4ed0d
to
38b37ce
Compare
LGTM |
38b37ce
to
2368b87
Compare
Avoid parsing the whole lines of mountinfo after all mountpoints of the target subsytems are found, or when the target subsystem is not enabled. Signed-off-by: Tatsushi Inagaki <e29253@jp.ibm.com>
Avoid parsing the whole lines of mountinfo after the mountpoint is found. Signed-off-by: Tatsushi Inagaki <e29253@jp.ibm.com>
Postpone parsing mountinfo until pivot_root() actually failed Signed-off-by: Tatsushi Inagaki <e29253@jp.ibm.com>
2368b87
to
eb0a144
Compare
LGTM |
Fix several format issues found by pdf and html
Fixes #631 (and the corresponding issue moby/moby#20851). The solutions here are:
mountinfo
in FindCgroupMountpoint() and FindCgroupMountpointAndRoot() when a given cgroup subsystem is not enabled. The check is done by testing the subsystem is included in a result of ParseCgroupFile().mountinfo
in getSelinuxMountPoint() when the mount point is found, instead of invoking mount.GetMounts().mountinfo
by invoking rootfsParentMountPrivate() and getParentMount() untilpivot_root()
actually fails.