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

runc exec is 5x slower than crun exec #3181

Open
kolyshkin opened this issue Aug 25, 2021 · 9 comments
Open

runc exec is 5x slower than crun exec #3181

kolyshkin opened this issue Aug 25, 2021 · 9 comments

Comments

@kolyshkin
Copy link
Contributor

root@ubu2004:/home/kir/git/runc/tst# time for ((i=0; i<1000;i++)); do runc exec xx45 true; done

real	0m20.726s
user	0m11.635s
sys	0m14.094s
root@ubu2004:/home/kir/git/runc/tst# time for ((i=0; i<1000;i++)); do crun exec xx4 true; done

real	0m4.802s
user	0m2.634s
sys	0m2.283s

Need to

  • figure out why the difference is so high
  • eliminate some of the bottlenecks
@kolyshkin
Copy link
Contributor Author

One thing runc exec does and crun exec does not is parsing /etc/passwd and /etc/group. I do not understand why it's needed, since we pass uid:gid as numbers (stringified but still) so the only need for passwd/group lookup is to $HOME value ... which is not used, unless not available from environment.

This needs to be fixed.

@AkihiroSuda
Copy link
Member

Is this a recent performance regression?

@kolyshkin
Copy link
Contributor Author

Is this a recent performance regression?

Good question. I don't believe it is, but let's check....

I compared with rc10, compiled by go 1.10.8, and the performance seems to be similar.

Choosing the best run among 10, here are the numbers.

git tip:

root@ubu2004:/home/kir/git/runc/tst# time for ((i=0; i<1000;i++)); do ../runc exec xx55 true; done

real	0m21.230s
user	0m10.349s
sys	0m13.599s

rc10:

root@ubu2004:/home/kir/git/runc/tst# time for ((i=0; i<1000;i++)); do ../runc-rc10 exec xx55 true; done

real	0m20.166s
user	0m8.895s
sys	0m16.125s

@dqminh
Copy link
Contributor

dqminh commented Aug 25, 2021

One thing runc exec does and crun exec does not is parsing /etc/passwd and /etc/group. I do not understand why it's needed

i think this is because runc supports passing username/groupname so we need to read the 2 files to get correct ids out. Does crun support that as well ?

@kolyshkin
Copy link
Contributor Author

kolyshkin commented Aug 25, 2021

i think this is because runc supports passing username/groupname so we need to read the 2 files to get correct ids out.

No, we (at least in CLI) only accept numeric IDs.

[kir@kir-rhat runc]$ runc exec --help | grep user
   --user value, -u value             UID (format: <uid>[:<gid>])

@eero-t
Copy link

eero-t commented Sep 17, 2021

Docker supports passing username/groupname, but it reads them from inside the container, not from host which IMHO makes that feature useless (only reason I can think of for using names is matching host IDs for things that correspond to host, like device files).

@cyphar
Copy link
Member

cyphar commented Sep 18, 2021

@kolyshkin

One thing runc exec does and crun exec does not is parsing /etc/passwd and /etc/group. I do not understand why it's needed, since we pass uid:gid as numbers (stringified but still) so the only need for passwd/group lookup is to $HOME value ... which is not used, unless not available from environment.

We fetch the supplementary group IDs from the container even if the user ID specified is numeric, which Docker doesn't do for us IIRC. This is also something we do with the regular config so I'm not sure we can change the behaviour at this late stage. The whole stringification logic is a quirk from when libcontainer used by use directly by Docker, but we probably can't change that either because it's saved to the on-disk state format and changing that leads to very bad issues when upgrading runc.

I'm surprised that crun doesn't do this at all, because it's something AFAIK they need to do in order to have parity with what Docker expects of runc.

@eero-t

Docker supports passing username/groupname, but it reads them from inside the container, not from host which IMHO makes that feature useless (only reason I can think of for using names is matching host IDs for things that correspond to host, like device files).

It's not useless -- how else would you run a program as a non-root user that is configured inside the container (note: this is the mechanism by which the USER directive in Dockerfiles works as well)? Require sudo be installed in every container? Arguably it makes less sense to use host users because the user configuration on the host is unrelated to the one in the container (outside of volume mounts -- which is related to the example usage you mentioned -- but that's a more generic issue which id mapped mounts might help solve).

@eero-t
Copy link

eero-t commented Oct 4, 2021

@cyphar

It's not useless -- how else would you run a program as a non-root user that is configured inside the container (note: this is the mechanism by which the USER directive in Dockerfiles works as well)?

The point of using name for host mapping is that you do not know what are the numeric IDs, because those differ between distributions & individual host machines, and container orchestration systems are not in control of them. Whereas names for few common groups (e.g. render) and users (e.g. nobody) are fairly standard between distributions.

Unlike with hosts (which are "randomly" assigned by container orchestration), container users have full control of the container contents, they specify what is being run and can check (and often also override) them as needed, so using correct numeric values for container contents is trivial. Expecting that to be the case with hosts, is hopeless without full control of the hosts.

@ningmingxiao
Copy link

ningmingxiao commented Jul 20, 2024

use " github.com/syndtr/gocapability" will call "os.Open("/proc/sys/kernel/cap_last_cap")" (it will run in init() ) even though we don't need it. I find we need it just run runc init.
https://github.com/syndtr/gocapability/blob/42c35b4376354fd554efc7ad35e0b7f94e3a0ffb/capability/capability_linux.go#L37
If we can move it to runc "github.com/opencontainers/runc/libcontainer/capabilities" and rewite it, we will use less resource.
this package been a long time since the last update.

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

No branches or pull requests

6 participants