Use faster mountinfo parser (part 1) by kolyshkin · Pull Request #2256 · opencontainers/runc

kolyshkin · 2020-03-14T02:44:36Z

This is part 1 of improving runc wrt /proc/self/mountinfo parsing (and an alternative to #2255).

It needs improvements because:

the current code has 5 different implementations of runc parser
a single runc run command reads mountinfo from from 72 to 116 times (depending on whether --systemd-cgroup is set) on my system.
reading mountinfo from the kernel might be slow if there are many containers running (and thus many mounts)
reading mountinfo might also be racy (see https://github.com/kolyshkin/procfs-test)

This PR

removes the old libcontainer/mount mountinfo parser (which was taken from Docker in Nov 2017)
makes use of moby/sys/mountinfo package which evolves from the same code as above but is fast (for background on improvements, see pkg/mount improvements moby/moby#36091, fix parseInfoFile does not handle spaces in filenames moby/moby#38977, Yet better mountinfo parser moby/moby#40684)
converts existing users to benefit from the above.

The new parser itself is up to 8x faster than the one removed; in addition, it implements mountinfo filters that may result in additional savings (faster, less garbage to collect).

History

v2: switch to golang 1.13
v3: switch to go modules (after Add support for Go Modules #2073 is merged)

My future plans (not in this PR) are

part 2: make other runc code that parses mountinfo on its own to use the above package
part 3: look into cgroups code, trying to minimize calls to parse mountinfo

kolyshkin · 2020-03-14T18:58:27Z

This is failing because we still use golang 1.12 :(

kolyshkin · 2020-03-15T20:23:17Z

Added a temporary commit switching to golang 1.13. Ultimately, we need to merge #2234 and #2239

AkihiroSuda · 2020-03-17T16:54:52Z

@@ -1,4 +1,4 @@
-FROM golang:1.12-stretch
+FROM golang:1.14-stretch


Either 1.13 or 1.14 should work just fine. As 1.13 will eventually be unsupported, I'm jumping straight to 1.14. Can use 1.13 as well here, let me know if you want it that way.

Ideally we should merge #2239 first and just rebase this one on top of it, but it's still marked as draft by @thaJeztah.

1.14 has breaking changes on SIGURG&EINTR stuff https://golang.org/doc/go1.14#runtime

It was marked "draft", because I separated it from #2234. Hopefully that one could be merged soon, and I'll rebase #2239

AkihiroSuda

Please roll back to go 1.13

kolyshkin · 2020-03-18T00:40:45Z

Please roll back to go 1.13

done

kolyshkin · 2020-03-18T21:24:51Z

@opencontainers/runc-maintainers PTAL

kolyshkin · 2020-03-19T01:34:20Z

IDK what's going on on travis, it doesn't like oldstable as go version. Works fine on my laptop though, and same gimme version.

My laptop:

$ gimme -r oldstable
1.13.8
$ gimme -V
v1.5.3

Travis:

gimme version
v1.5.3
...
gimme: version specifier 'oldstable' unknown
error: GIMME_GO_VERSION not recognized as valid
  got: oldstable
Failed to run gimme
The command "travis_setup_go" failed and exited with 86 during .

Anyway I'll just use 1.13.x and stable (which currently equals 1.14.x)

kolyshkin · 2020-03-19T16:30:31Z

Some glitch in CI, rebasing to re-trigger it

gzip: stdin: not in gzip format
1452tar: Child returned status 1
1453tar: Error is not recoverable: exiting now
1454The command '/bin/sh -c mkdir -p /usr/src/criu && curl -sSL https://github.com/checkpoint-restore/criu/archive/${CRIU_VERSION}.tar.gz | tar -v -C /usr/src/criu/ -xz --strip-components=1 && cd /usr/src/criu && make install-criu && rm -rf /usr/src/criu' returned a non-zero code: 2

kolyshkin · 2020-03-19T19:25:09Z

CI is good now, was a glitch

AkihiroSuda · 2020-03-20T17:41:53Z

      script:
        - make verify-dependencies
-    - go: 1.12.x
+    - go: stable


stable -> 1.13.x ?

No, currently stable means 1.14.x. I didn't want to use an explicit version here since it will need to be changed every once in a while, but an implicit version like stable can stay as is.

Do you want it to be explicit?

We aren't ready for 1.14 yet

this is CI only and it passes.

OK I'll switch to explicitly 1.13

Run CI with go 1.13 and 1.14 (aka "stable"). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Delete libcontainer/mount in favor of github.com/moby/sys/mountinfo, which is fast mountinfo parser. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

1. rootfs is already validated to be kosher by (*ConfigValidator).rootfs() 2. mount points from /proc/self/mountinfo are absolute and clean, too Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

AkihiroSuda · 2020-03-21T17:52:11Z

LGTM

mrunalp · 2020-03-24T21:26:56Z

LGTM

mrunalp · 2020-03-24T21:27:18Z

@crosbymichael @cyphar @giuseppe ptal.

AkihiroSuda · 2020-03-27T02:09:31Z

@mrunalp Can we merge this, or do you want one more LGTM?

Make use of errors.Is() and errors.As() where appropriate to check the underlying error. The biggest motivation is to simplify the code. The feature requires go 1.13 but since merging opencontainers#2256 we are already not supporting go 1.12 (which is an unsupported release anyway). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin mentioned this pull request Mar 14, 2020

Faster mountinfo parser #2255

Closed

2 tasks

kolyshkin force-pushed the mountinfo-alt branch from 527fa7e to 79f1207 Compare March 14, 2020 18:57

This was referenced Mar 16, 2020

mountinfo: parse empty strings in source #1829

Closed

Make public ParseMountinfo #1970

Closed

kolyshkin force-pushed the mountinfo-alt branch from 79f1207 to 6322359 Compare March 16, 2020 16:13

AkihiroSuda reviewed Mar 17, 2020

View reviewed changes

kolyshkin changed the title ~~Use faster mountinfo parser~~ Use faster mountinfo parser (part 1) Mar 18, 2020

AkihiroSuda requested changes Mar 18, 2020

View reviewed changes

kolyshkin force-pushed the mountinfo-alt branch from 6322359 to 27ab232 Compare March 18, 2020 00:40

kolyshkin force-pushed the mountinfo-alt branch 2 times, most recently from 83bb000 to 2ae97a2 Compare March 19, 2020 16:30

AkihiroSuda reviewed Mar 20, 2020

View reviewed changes

kolyshkin added 6 commits March 21, 2020 10:33

Switch to golang 1.13, drop unsupported versions

3147c32

Run CI with go 1.13 and 1.14 (aka "stable"). Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

libcontainer: switch to moby/sys/mountinfo package

c7ab2c0

Delete libcontainer/mount in favor of github.com/moby/sys/mountinfo, which is fast mountinfo parser. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

libct/TestFactoryNewTmpfs: benefit from GetMounts

81d8452

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

getParentMount: benefit from GetMounts filter

dce0de8

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

libct/msMoveRoot: rm redundant filepath.Abs() calls

fc4357a

1. rootfs is already validated to be kosher by (*ConfigValidator).rootfs() 2. mount points from /proc/self/mountinfo are absolute and clean, too Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

libct/msMoveRoot: benefit from GetMounts filter

dd7b346

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin force-pushed the mountinfo-alt branch from 2ae97a2 to dd7b346 Compare March 21, 2020 17:33

kolyshkin mentioned this pull request Mar 23, 2020

WIP/RFC: libcontainer/cgroups: add mountinfo cache #2265

Closed

AkihiroSuda approved these changes Mar 27, 2020

View reviewed changes

mrunalp merged commit 53ad1d5 into opencontainers:master Mar 27, 2020

kolyshkin deleted the mountinfo-alt branch March 30, 2020 16:02

This was referenced Apr 1, 2020

Actually check for syscall.ENODEV when checking if a container is paused #2279

Merged

Use errors.Unwrap() where possible #2280

Merged

kolyshkin mentioned this pull request Apr 3, 2020

Use errors.As() and errors.Is() to unwrap errors #2291

Merged

		@@ -1,4 +1,4 @@
		FROM golang:1.12-stretch
		FROM golang:1.14-stretch

Conversation

kolyshkin commented Mar 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

History

Uh oh!

kolyshkin commented Mar 14, 2020

Uh oh!

kolyshkin commented Mar 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Mar 18, 2020

Uh oh!

kolyshkin commented Mar 18, 2020

Uh oh!

kolyshkin commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kolyshkin commented Mar 19, 2020

Uh oh!

kolyshkin commented Mar 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda commented Mar 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrunalp commented Mar 24, 2020 • edited by AkihiroSuda Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrunalp commented Mar 24, 2020

Uh oh!

AkihiroSuda commented Mar 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kolyshkin commented Mar 14, 2020 •

edited

Loading

kolyshkin commented Mar 19, 2020 •

edited

Loading

AkihiroSuda commented Mar 21, 2020 •

edited

Loading

mrunalp commented Mar 24, 2020 •

edited by AkihiroSuda

Loading