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

Faster mountinfo parser #2255

Closed
wants to merge 5 commits into from
Closed

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Mar 13, 2020

This is part 1 of improving runc wrt /proc/self/mountinfo parsing.

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 be racy (see https://github.com/kolyshkin/procfs-test)

This PR

My plans are

  • part 2: switch the code that parses mountinfo on its own to use this package
  • part 3: look into cgroups code, trying to minimize calls to parse mountinfo

Commit 3ca4c78 drops the Moby (then Docker) pkg/mount
code here, and it was mostly forgotten since.

In the meantime, Moby's mountinfo parser has much improved,
and this commit brings it here.

The new parser deliberately changes the API, so its users are
converted accordingly.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
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
Copy link
Member

Do you have benchmark results?

@kolyshkin
Copy link
Contributor Author

Do you have benchmark results?

I have just updated the description. Haven't performed any benchmarks for this one, but e.g. moby/moby#36091 claims 8x speed improvement :)

@AkihiroSuda
Copy link
Member

Maybe we should begin with splitting the repo first moby/moby#40684 (comment)

@kolyshkin
Copy link
Contributor Author

@AkihiroSuda is it OK to have runc depend on moby repos? I am asking because commit 3ca4c78 specifically says "get rid of docker/docker dependency". Or is a dependency upon a smaller thing is fine?

@AkihiroSuda
Copy link
Member

Or is a dependency upon a smaller thing is fine?

I think fine

@crosbymichael
Copy link
Member

no, we don't want to dependency on moby/moby. moby already imports runc and we don't want it to be the other way around. keep the code here :)

@kolyshkin
Copy link
Contributor Author

no, we don't want to dependency on moby/moby. moby already imports runc and we don't want it to be the other way around. keep the code here :)

I never proposed a dependency on moby/moby, this is out of the question.

I don't like copy-pasting code around either (as it creates a maintenance nightmare -- instead of fixing the code in one place, it has to be fixed in a few).

So, for now I have split out moby/moby/pkg/mount to two new packages:

Anyway, moving along according to the new plan: #2256

@kolyshkin
Copy link
Contributor Author

replaced by #2256

@kolyshkin kolyshkin closed this Mar 14, 2020
@kolyshkin kolyshkin deleted the mountinfo branch March 30, 2020 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants