This addresses the following TODO in the code (added back in 2015
by commit 845fc65e5):
> // TODO: fix libcontainer's API to better support uid/gid in a typesafe way.
Historically, libcontainer internally uses strings for user, group, and
additional (aka supplementary) groups.
Yet, runc receives those credentials as part of runtime-spec's process,
which uses integers for all of them (see [1], [2]).
What happens next is:
1. runc start/run/exec converts those credentials to strings (a User
string containing "UID:GID", and a []string for additional GIDs) and
passes those onto runc init.
2. runc init converts them back to int, in the most complicated way
possible (parsing container's /etc/passwd and /etc/group).
All this conversion and, especially, parsing is totally unnecessary,
but is performed on every container exec (and start).
The only benefit of all this is, a libcontainer user could use user and
group names instead of numeric IDs (but runc itself is not using this
feature, and we don't know if there are any other users of this).
Let's remove this back and forth translation, hopefully increasing
runc exec performance.
The only remaining need to parse /etc/passwd is to set HOME environment
variable for a specified UID, in case $HOME is not explicitly set in
process.Env. This can now be done right in prepareEnv, which simplifies
the code flow a lot. Alas, we can not use standard os/user.LookupId, as
it could cache host's /etc/passwd or the current user (even with the
osusergo tag).
PS Note that the structures being changed (initConfig and Process) are
never saved to disk as JSON by runc, so there is no compatibility issue
for runc users.
Still, this is a breaking change in libcontainer, but we never promised
that libcontainer API will be stable (and there's a special package
that can handle it -- github.com/moby/sys/user). Reflect this in
CHANGELOG.
For 3998.
[1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.2/config.md#posix-platform-user
[2]: https://github.com/opencontainers/runtime-spec/blob/v1.0.2/specs-go/config.go#L86
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Historically, when HOME is not explicitly set in process.Env,
and UID to run as doesn't have a corresponding entry in container's
/etc/passwd, runc sets HOME=/ as a fallback.
Add the corresponding check, for the sake of backward compatibility
preservation.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Use the old package name as an alias to minimize the patch.
No functional change; this just eliminates a bunch of deprecation
warnings.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Currently, libcontainer/devices contains two things:
1. Device-related configuration data structures and accompanying
methods. Those are used by runc itself, mostly by libct/cgroups.
2. A few functions (HostDevices, DeviceFromPath, GetDevices).
Those are not used by runc directly, but have some external users
(cri-o, microsoft/hcsshim), and they also have a few forks
(containerd/pkg/oci, podman/pkg/util).
This commit moves (1) to a new separate package, config (under
libcontainer/cgroups/devices), adding a backward-compatible aliases
(marked as deprecated so we will be able to remove those later).
Alas it's not possible to move this to libcontainer/cgroups directly
because some IDs (Type, Rule, Permissions) are too generic, and renaming
them (to DeviceType, DeviceRule, DevicePermissions) will break backward
compatibility (mostly due to Rule being embedded into Device).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This release includes a minor breaking API change that requires us to
rework the types of our wrappers, but there is no practical behaviour
change.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
The previous logic caused runc to hang if CloseExecFrom returned an
error, as the defer waiting on logsDone never finished as the parent
process was never started (and it controls the closing of logsDone via
it's logsPipe).
This moves the defer to after we have started the parent, with means all
the logic related to managing the logsPipe should also be running.
Signed-off-by: Evan Phoenix <evan@phx.io>
The current implementation sets all the environment variables passed in
Process.Env in the current process, one by one, then uses os.Environ to
read those back.
As pointed out in [1], this is slow, as runc calls os.Setenv for every
variable, and there may be a few thousands of those. Looking into how
os.Setenv is implemented, it is indeed slow, especially when cgo is
enabled.
Looking into why it was implemented the way it is, I found commit
9744d72c and traced it to [2], which discusses the actual reasons.
It boils down to these two:
- HOME is not passed into container as it is set in setupUser by
os.Setenv and has no effect on config.Env;
- there is a need to deduplicate the environment variables.
Yet it was decided in [2] to not go ahead with this patch, but
later [3] was opened with the carry of this patch, and merged.
Now, from what I see:
1. Passing environment to exec is way faster than using os.Setenv and
os.Environ (tests show ~20x speed improvement in a simple Go test,
and ~3x improvement in real-world test, see below).
2. Setting environment variables in the runc context may result is some
ugly side effects (think GODEBUG, LD_PRELOAD, or _LIBCONTAINER_*).
3. Nothing in runtime spec says that the environment needs to be
deduplicated, or the order of preference (whether the first or the
last value of a variable with the same name is to be used). We should
stick to what we have in order to maintain backward compatibility.
So, this patch:
- switches to passing env directly to exec;
- adds deduplication mechanism to retain backward compatibility;
- takes care to set PATH from process.Env in the current process
(so that supplied PATH is used to find the binary to execute),
also to retain backward compatibility;
- adds HOME to process.Env if not set;
- ensures any StartContainer CommandHook entries with no environment
set explicitly are run with the same environment as before. Thanks
to @lifubang who noticed that peculiarity.
The benchmark added by the previous commit shows ~3x improvement:
│ before │ after │
│ sec/op │ sec/op vs base │
ExecInBigEnv-20 61.53m ± 1% 21.87m ± 16% -64.46% (p=0.000 n=10)
[1]: https://github.com/opencontainers/runc/pull/1983
[2]: https://github.com/docker-archive/libcontainer/pull/418
[3]: https://github.com/docker-archive/libcontainer/pull/432
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Make CommandHook.Command a pointer, which reduces the amount of data
being copied when using hooks, and allows to modify command hooks.
2. Add SetDefaultEnv, which is to be used by the next commit.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is to ensure that changes in Process.Env handling won't affect
StartContainer hook.
Reported-by: lfbzhm <lifubang@acmcoder.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is a slight refactor of TestExecInEnvironment, making it more
strict wrt checking the exec output.
1. Explain why DEBUG is added twice to the env.
2. Reuse the execEnv for the check.
3. Make the check more strict -- instead of looking for substrings,
check line by line.
4. Add a check for extra environment variables.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Here's what it shows on my laptop (with -count 10 -benchtime 10s,
summarized by benchstat):
│ sec/op │
ExecTrue-20 8.477m ± 2%
ExecInBigEnv-20 61.53m ± 1%
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This helper was added for runc-dmz in commit dac417174, but runc-dmz was
later removed in commit 871057d, which forgot to remove the helper.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is an internal implementation detail and should not be either
public or visible.
Amend setIOPriority to do own class conversion.
Fixes: bfbd0305 ("Add I/O priority")
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
For some reason, io priority is set in different places between runc
start/run and runc exec:
- for runc start/run, it is done in the middle of (*linuxStandardInit).Init,
close to the place where we exec runc init.
- for runc exec, it is done much earlier, in (*setnsProcess) start().
Let's move setIOPriority call for runc exec to (*linuxSetnsInit).Init,
so it is in the same logical place as for runc start/run.
Also, move the function itself to init_linux.go as it's part of init.
Should not have any visible effect, except part of runc init is run with
a different I/O priority.
While at it, rename setIOPriority to setupIOPriority, and make it accept
the whole *configs.Config, for uniformity with other similar functions.
Fixes: bfbd0305 ("Add I/O priority")
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This code is not in libcontainer, meaning it is only used by a short lived
binary (runc start/run/exec). Unlike code in libcontainer (see
CreateLibcontainerConfig), here we don't have to care about copying the
structures supplied as input, meaning we can just reuse the pointers
directly.
Fixes: bfbd0305 ("Add I/O priority")
Fixes: 770728e1 ("Support `process.scheduler`")
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
SpecState field of initConfig is only needed to run hooks that are
executed inside a container -- namely CreateContainer and
StartContainer.
If these hooks are not configured, there is no need to fill, marshal and
unmarshal SpecState.
While at it, inline updateSpecState as it is trivial and only has one user.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This allows to omit a call to c.currentOCIState (which can be somewhat
costly when there are many annotations) when the hooks of a given kind
won't be run.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Introduce a common parent struct `containerProcess`,
let both `initProcess` and `setnsProcess` are inherited
from it.
Signed-off-by: lfbzhm <lifubang@acmcoder.com>
Kir Kolyshkin