1. Use strings.TrimPrefix instead of fmt.Sscanf and simplify the code.
2. Add a test case and a benchmark.
The benchmark shows some improvement, compared to the old
implementation:
name old time/op new time/op delta
FindDeviceGroup-4 39.7µs ± 2% 26.8µs ± 2% -32.63% (p=0.008 n=5+5)
name old alloc/op new alloc/op delta
FindDeviceGroup-4 6.08kB ± 0% 4.23kB ± 0% -30.39% (p=0.008 n=5+5)
name old allocs/op new allocs/op delta
FindDeviceGroup-4 117 ± 0% 6 ± 0% -94.87% (p=0.008 n=5+5)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 343951a22b added a call to os.Stat for the device path
when generating systemd device properties, to avoid systemd warning for
non-existing devices. The idea was, since systemd uses stat(2) to look
up device properties for a given path, it will fail anyway. In addition,
this allowed to suppress a warning like this from systemd:
> Couldn't stat device /dev/char/10:200
NOTE that this was done because:
- systemd could not add the rule anyway;
- runs puts its own set of rules on top of what systemd does.
Apparently, the above change broke some setups, resulting in inability
to use e.g. /dev/null inside a container. My guess is this is because
in cgroup v2 we add a second eBPF program, which is not used if the
first one (added by systemd) returns "access denied".
Next, commit 3b9582895b fixed that by adding a call to os.Stat for
"/sys/"+path (meaning, if "/dev/char/10:200" does not exist, we retry
with "/sys/dev/char/10:200", and if it exists, proceed with adding a
device rule with the original (non-"/sys") path).
How that second fix ever worked was a mystery, because the path we gave
to systemd still doesn't exist.
Well, I think now I know.
Since systemd v240 (commit 74c48bf5a8005f20) device access rules
specified as /dev/{block|char}/MM:mm are no longer looked up on the
filesystem, instead, if possible, those are parsed from the string.
So, we need to do different things, depending on systemd version:
- for systemd >= v240, use the /dev/{char,block}/MM:mm as is, without
doing stat() -- since systemd doesn't do stat() either;
- for older version, check if the path exists, and skip passing it on
to systemd otherwise.
- the check for /sys/dev/{block,char}/MM:mm is not needed in either
case.
Pass the systemd version to the function that generates the rules, and
fix it accordingly.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
These functions were added in ancient times, facilitating the
docker-in-docker case when cgroup namespace was not available.
As pointed out in commit 2b28b3c276, using init 1 cgroup is not
correct because it won't work in case of host PID namespace.
The last user of GetInitCgroup was removed by commit
54e20217a8. GetInitCgroupPath was never used
as far as I can see, nor was I able to find any external users.
Remove both functions. Modify the comment in libct/cg/fs.subsysPath
to not refer to GetInitCgroupPath.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Systemd v252 (available in CentOS Stream 9 in our CI) added support
for setting cpu.idle (see [1]). The way it works is:
- if CPUWeight == 0, cpu.idle is set to 1;
- if CPUWeight != 0, cpu.idle is set to 0.
This commit implements setting cpu.idle in systemd cgroup driver via a
unit property. In case CPUIdle is set to non-zero value, the driver sets
adds CPUWeight=0 property, which will result in systemd setting cpu.idle
to 1.
Unfortunately, there's no way to set cpu.idle to 0 without also changing
the CPUWeight value, so the driver doesn't do anything if CPUIdle is
explicitly set to 0. This case is handled by the fs driver which is
always used as a followup to setting systemd unit properties.
Also, handle cpu.idle set via unified map. In case it is set to non-zero
value, add CPUWeight=0 property, and ignore cpu.weight (otherwise we'll
get two different CPUWeight properties set).
Add a unit test for new values in unified map, and an integration test case.
[1] https://github.com/systemd/systemd/pull/23299
[2] https://github.com/opencontainers/runc/issues/3786
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In code that checks that the resource name is in the for
Using strings.SplitN is an overkill in this case, resulting in
allocations and thus garbage to collect.
Using strings.IndexByte and checking that result is not less than 1
(meaning there is a period, and it is not the first character) is
sufficient here.
Fixes: 0cb8bf67a3
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In case a systemd unit fails (for example, timed out or OOM-killed),
systemd keeps the unit. This prevents starting a new container with
the same systemd unit name.
The fix is to call reset-failed in case UnitExists error is returned,
and retry once.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit d223e2adae ("Ignore error when starting transient unit
that already exists" modified the code handling errors from startUnit
to ignore UnitExists error.
Apparently it was done so that kubelet can create the same pod slice
over and over without hitting an error (see [1]).
While it works for a pod slice to ensure it exists, it is a gross bug
to ignore UnitExists when creating a container. In this case, the
container init PID won't be added to the systemd unit (and to the
required cgroup), and as a result the container will successfully
run in a current user cgroup, without any cgroup limits applied.
So, fix the code to only ignore UnitExists if we're not adding a process
to the systemd unit. This way, kubelet will keep working as is, but
runc will refuse to create containers which are not placed into a
requested cgroup.
[1] https://github.com/opencontainers/runc/pull/1124
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since commit 39914db679 this function is not used by runc (see
that commit to learn why this function is not that good).
I was not able to find any external users either.
Since it's not a good function, with no users, and it is rather trivial,
let's remove it right away (rather than mark as deprecated).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Sometimes, the init process is not in the root cgroup.
This can be noted by GetInitPath, which already scrubs the path of `init.scope`.
This was encountered when trying to patch the Kubelet to handle systemd being in a separate cpuset
from root (to allow load balance disabling for containers). At present, there's no way to have libcontainer or runc
manage cgroups in a hierarchy outside of the one init is in (unless the path contains `init.scope`, which is limiting)
Signed-off-by: Peter Hunt <pehunt@redhat.com>
There is some kind of a race in CentOS 7 which sometimes result in one
of these tests failing like this:
systemd_test.go:136: mkdir /sys/fs/cgroup/hugetlb/system.slice/system-runc_test_pods.slice: no such file or directory
or
systemd_test.go:187: open /sys/fs/cgroup/cpuset/system.slice/system-runc_test_pods.slice/cpuset.mems: no such file or directory
As this is only happening on CentOS 7, let's skip this test on this
platform.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
centos-9 unit test sometimes fails with:
=== RUN TestPodSkipDevicesUpdate
systemd_test.go:114: container stderr not empty: basename: missing operand
Try 'basename --help' for more information.
--- FAIL: TestPodSkipDevicesUpdate (0.11s)
I'm not sure why the container output is an error in basename. It seems
likely that the bashrc in that distro is kind of broken. Let's just run
a sleep command and forget about bash.
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
Rewrite systemdVersionAtoi to not use regexp, and fix two issues:
1. It was returning 0 (rather than -1) for some errors.
2. The comment was saying that the input string is without quotes,
while in fact it is.
Note the new function, similar to the old one, works on input either
with or without quotes. Amend the test to add test cases without quotes.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is aimed at solving the problem of cgroup v2 memory controller
behavior which is not compatible with that of cgroup v1.
In cgroup v1, if the new memory limit being set is lower than the
current usage, setting the new limit fails.
In cgroup v2, same operation succeeds, and the container is OOM killed.
Introduce a new setting, memory.checkBeforeUpdate, and use it to mimic
cgroup v1 behavior.
Note that this is not 100% reliable because of TOCTOU, but this is the
best we can do.
Add some test cases.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Runc parses cpuset range to bits in the case of cgroup v2 + systemd as cgroup driver.
The byte order representation differs from systemd expectation, which will set
different cpuset range in systemd transient unit if the length of parsed byte array exceeds one.
# cat config.json
...
"resources": {
...
"cpu": {
"cpus": "10-23"
}
},
...
# runc --systemd-cgroup run test
# cat /run/systemd/transient/runc-test.scope.d/50-AllowedCPUs.conf
# This is a drop-in unit file extension, created via "systemctl set-property"
# or an equivalent operation. Do not edit.
[Scope]
AllowedCPUs=0-7 10-15
The cpuset.cpus in cgroup will also be set to wrong value after reloading systemd manager configuration.
# systemctl daemon-reload
# cat /sys/fs/cgroup/system.slice/runc-test.scope/cpuset.cpus
0-7,10-15
Signed-off-by: seyeongkim <seyeong.kim@canonical.com>
Signed-off-by: Chengen, Du <chengen.du@canonical.com>
This is a forward port of https://github.com/opencontainers/runc/pull/3620
The original code depended on the origin filesystem to have
/dev/{block,char} populated. This is done by udev normally and while is
very common non-containerized systemd installs, it's very easy to start
systemd in a container created by runc itself and not have
/dev/{block,char} populated. When this occurs, the following error
output is observed:
$ docker run hello-world
docker: Error response from daemon: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: error during container init: error reopening /dev/null inside container: open /dev/null: operation not permitted: unknown.
/dev/null can't be opened because it was not added to the
deviceAllowList, as there was no /dev/char directory. The change here
utilizes the fact that when sysfs in in use, there is a
/sys/dev/{block,char} that is kernel maintained that we can check.
Signed-off-by: Evan Phoenix <evan@phx.io>
It's more idiomatic Go to define interfaces on the receiver, and constructors to
return concrete types.
This patch changes various constructors to return a concrete type, with the
exceptions of NewWithPaths, which needs the abstraction as it switches between
implementations.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
A regression reported for runc v1.1.3 says that "runc exec -t" fails
after doing "systemctl daemon-reload":
> exec failed: unable to start container process: open /dev/pts/0: operation not permitted: unknown
Apparently, with commit 7219387eb7 we are no longer adding
"DeviceAllow=char-pts rwm" rule (because os.Stat("char-pts") returns
ENOENT).
The bug can only be seen after "systemctl daemon-reload" because runc
also applies the same rules manually (by writing to devices.allow for
cgroup v1), and apparently reloading systemd leads to re-applying the
rules that systemd has (thus removing the char-pts access).
The fix is to do os.Stat only for "/dev" paths.
Also, emit a warning that the path was skipped. Since the original idea
was to emit less warnings, demote the level to debug.
Note this also fixes the issue of not adding "m" permission for block-*
and char-* devices.
A test case is added, which reliably fails before the fix
on both cgroup v1 and v2.
Fixes: https://github.com/opencontainers/runc/issues/3551
Fixes: 7219387eb7
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since Go 1.19, godoc recognizes lists, code blocks, headings etc. It
also reformats the sources making it more apparent that these features
are used.
Fix a few places where it misinterpreted the formatting (such as
indented vs unindented), and format the result using the gofumpt
from HEAD, which already incorporates gofmt 1.19 changes.
Some more fixes (and enhancements) might be required.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
systemd emits very loud warnings when the path specified doesn't exist
(which can be the case for some of our default rules). We don't need the
ruleset we give systemd to be completely accurate (we discard some kinds
of wildcard rules anyway) so we can safely skip adding these.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
These are only used from inside the package, and we don't want them to
be public.
The only two methods left are Enable and Disable.
While at it, fix or suppress found lint-extra warnings.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This commit separates the functionality of setting cgroup device
rules out of libct/cgroups to libct/cgroups/devices package. This
package, if imported, sets the function variables in libct/cgroups and
libct/cgroups/systemd, so that a cgroup manager can use those to manage
devices. If those function variables are nil (when libct/cgroups/devices
are not imported), a cgroup manager returns the ErrDevicesUnsupported
in case any device rules are set in Resources.
It also consolidates the code from libct/cgroups/ebpf and
libct/cgroups/ebpf/devicefilter into libct/cgroups/devices.
Moved some tests in libct/cg/sd that require device management to
libct/sd/devices.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This moves the functionality related to devices, SkipDevices, and
SkipFreezeOnSet to a separate file, in preparation for the next commit.
No code changes.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Looking through git blame, this was added by commit 9fac18329
aka "Initial commit of runc binary", most probably by mistake.
Obviously, a container should not have access to tun/tap device, unless
it is explicitly specified in configuration.
Now, removing this might create a compatibility issue, but I see no
other choice.
Aside from the obvious misconfiguration, this should also fix the
annoying
> Apr 26 03:46:56 foo.bar systemd[1]: Couldn't stat device /dev/char/10:200: No such file or directory
messages from systemd on every container start, when runc uses systemd
cgroup driver, and the system runs an old (< v240) version of systemd
(the message was presumably eliminated by [1]).
[1] https://github.com/systemd/systemd/pull/10996/commits/d5aecba6e0b7c73657c4cf544ce57289115098e7
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In case statfs("/sys/fs/cgroup/unified") fails with any error other
than ENOENT, current code panics. As IsCgroup2HybridMode is called from
libcontainer/cgroups/fs's init function, this means that any user of
libcontainer may panic during initialization, which is ugly.
Avoid panicking; instead, do not enable hybrid hierarchy support and
report the error (under debug level, not to confuse anyone).
Basically, replace the panic with "turn off hybrid mode support"
(which makes total sense since we were unable to statfs its root).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since LinuxFactory has become the means to specify containers state
top directory (aka --root), and is only used by two methods (Create
and Load), it is easier to pass root to them directly.
Modify all the users and the docs accordingly.
While at it, fix Create and Load docs (those that were originally moved
from the Factory interface docs).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Apparently, "systemctl --user --no-pager show-environment" is useless
without DBUS_SESSION_BUS_ADDRESS or XDG_RUNTIME_DIR set:
$ echo $DBUS_SESSION_BUS_ADDRESS, $XDG_RUNTIME_DIR
unix:path=/run/user/1000/bus, /run/user/1000
$ systemctl --user --no-pager show-environment | grep DBUS_SESS
DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1000/bus
$ unset DBUS_SESSION_BUS_ADDRESS
$ systemctl --user --no-pager show-environment | grep DBUS_SESS
DBUS_SESSION_BUS_ADDRESS=unix:path=/run/user/1000/bus
$ unset XDG_RUNTIME_DIR
$ systemctl --user --no-pager show-environment | grep DBUS_SESS
Failed to connect to bus: $DBUS_SESSION_BUS_ADDRESS and $XDG_RUNTIME_DIR not defined (consider using --machine=<user>@.host --user to connect to bus of other user)
So, it does not make sense to try it to get the address.
Also, it does not make sense to suggest "systemctl --user start dbus"
either, for the same reason, so remove that suggestion from the error
message text.
Since DBUS_SESSION_BUS_ADDRESS environment variable, on which the code
relies, is et by dbus-run-session (or dbus-launch, or something similar
that is supposed to be run during the login process), add a suggestion
to re-login.
Finally, fix the following linter warning:
> error-strings: error strings should not be capitalized or end with punctuation or a newline (revive)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Apparently, not all files listed in /sys/kernel/cgroup/delegate must
exist in every cgroup, so we should ignore ENOENT.
Dot not ignore ENOENT on the directory itself though.
Change cgroupFilesToChown to not return ".", and refactor it to not do
any dynamic slice appending in case we're using the default built-in
list of files.
Fixes: 35d20c4e0
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since commit 12e99a0f8d we do require Go >= 1.16, so this file
is no longer needed.
Also, this actually ensures that go >= 1.16 is used (otherwise
libcontainer/cgroups/getallpids.go won't compile).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It never returns any error, so let's drop it (in case it needs to be
re-added, it is easy to do so).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Delegating cgroups to the container enables more complex workloads,
including systemd-based workloads. The OCI runtime-spec was
recently updated to explicitly admit such delegation, through
specification of cgroup ownership semantics:
https://github.com/opencontainers/runtime-spec/pull/1123
Pursuant to the updated OCI runtime-spec, change the ownership of
the container's cgroup directory and particular files therein, when
using cgroups v2 and when the cgroupfs is to be mounted read/write.
As a result of this change, systemd workloads can run in isolated
user namespaces on OpenShift when the sandbox's cgroupfs is mounted
read/write.
It might be possible to implement this feature in other cgroup
managers, but that work is deferred.
Signed-off-by: Fraser Tweedale <ftweedal@redhat.com>
Move test case comments to doc strings, and use t.Run.
Suggested-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Instead of distinguishing between errors and warnings, let's treat all
errors as warnings, thus simplifying the code. This changes the
function behaviour for input like hugepages-BadNumberKb --
previously, the error from Atoi("BadNumber") was considered fatal,
now it's just another warnings.
2. Move the warning logging to HugePageSizes, thus simplifying the test
case, which no longer needs to read what logrus writes. Note that we
do not want to log all the warnings (as chances are very low we'll
get any, and if we do this means the code need to be updated), only
the first one.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
I have noticed that libct/cg/fs allocates 8K during init on every runc
execution:
> init github.com/opencontainers/runc/libcontainer/cgroups/fs @1.5 ms, 0.028 ms clock, 8512 bytes, 13 allocs
Apparently this is caused by global HugePageSizes variable init, which
is only used from GetStats (i.e. it is never used by runc itself).
Remove it, and use HugePageSizes() directly instead. Make it init-once,
so that GetStats (which, I guess, is periodically called by kubernetes)
does not re-read huge page sizes over and over.
This also removes 12 allocs and 8K from libct/cg/fs init section:
> $ time GODEBUG=inittrace=1 ./runc --help 2>&1 | grep cgroups/fs
> init github.com/opencontainers/runc/libcontainer/cgroups/fs @1.5 ms, 0.003 ms clock, 16 bytes, 1 allocs
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Kir Kolyshkin