runc delete is supposed to remove all the container's artefacts.
In case systemd cgroup driver is used, and the systemd unit has failed
(e.g. oom-killed), systemd won't remove the unit (that is, unless the
"CollectMode: inactive-or-failed" property is set).
Call reset-failed from manager.Destroy so the failed unit will be
removed during "runc delete".
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Sometimes we call resetFailedUnit as a cleanup measure, and we don't
care if it fails or not. So, move error reporting to its callers, and
ignore error in cases we don't really expect it to succeed.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In code we have frozen the cgroup to avoid the processes get
an occasional "permission denied" error, while the systemd's application of device
rules is done disruptively. When the processes in the container can not
be frozen over 2 seconds (which defined in fs/freezer.go),
we still update the cgroup which resulting the container get an occasional
"permission denied" error in some cases.
Return error directly without updating cgroup, when freeze fails.
Fixes: #3803
Signed-off-by: Zoe <hi@zoe.im>
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>
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>
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>
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>
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>
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>
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>
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>
Currently, we can create subcgroup in a rootless container with systemd cgroupv2 on centos8.
But after the container exited, the container cgroup and its subcgroup will not be removed.
Fix this by removing all directories recursively.
Fixes: https://github.com/opencontainers/runc/issues/3225
Signed-off-by: Kang Chen <kongchen28@gmail.com>
Currently the parent process of the container is moved to the right
cgroup v2 tree when systemd is using a hybrid model (last line with 0::):
$ runc --systemd-cgroup run myid
/ # cat /proc/self/cgroup
12:cpuset:/system.slice/runc-myid.scope
11:blkio:/system.slice/runc-myid.scope
10:devices:/system.slice/runc-myid.scope
9:hugetlb:/system.slice/runc-myid.scope
8:memory:/system.slice/runc-myid.scope
7:rdma:/
6:perf_event:/system.slice/runc-myid.scope
5:net_cls,net_prio:/system.slice/runc-myid.scope
4:freezer:/system.slice/runc-myid.scope
3:pids:/system.slice/runc-myid.scope
2:cpu,cpuacct:/system.slice/runc-myid.scope
1:name=systemd:/system.slice/runc-myid.scope
0::/system.slice/runc-myid.scope
However, if a second process is executed in the same container, it is
not moved to the right cgroup v2 tree:
$ runc exec myid /bin/sh -c 'cat /proc/self/cgroup'
12:cpuset:/system.slice/runc-myid.scope
11:blkio:/system.slice/runc-myid.scope
10:devices:/system.slice/runc-myid.scope
9:hugetlb:/system.slice/runc-myid.scope
8:memory:/system.slice/runc-myid.scope
7:rdma:/
6:perf_event:/system.slice/runc-myid.scope
5:net_cls,net_prio:/system.slice/runc-myid.scope
4:freezer:/system.slice/runc-myid.scope
3:pids:/system.slice/runc-myid.scope
2:cpu,cpuacct:/system.slice/runc-myid.scope
1:name=systemd:/system.slice/runc-myid.scope
0::/user.slice/user-1000.slice/session-8.scope
This commit makes that processes executed with exec are placed into the
right cgroup v2 tree. The implementation checks if systemd is using a
hybrid mode (by checking if cgroups v2 is mounted in
/sys/fs/cgroup/unified), if yes, the path of the cgroup v2 slice for
this container is saved into the cgroup path list.
The fs group driver has a similar issue, in this case none of the runc
run or runc exec commands put the process in the right cgroups v2. This
commit also fixes that.
Having the processes of the container in its own cgroup v2 is useful
for any BPF programs that rely on bpf_get_current_cgroup_id(), like
https://github.com/kinvolk/inspektor-gadget/ for instance.
[@kolyshkin: rebased]
Signed-off-by: Mauricio Vásquez <mauricio@kinvolk.io>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Cgroup controllers should never panic, and yet sometimes they do.
Add a unit test to check that controllers never panic when called with
nil arguments and/or resources, and fix a few found cases.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This way we
- won't re-initialize the paths if they were provided;
- will always have paths ready for every method.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Make Rootless and Systemd flags part of config.Cgroups.
2. Make all cgroup managers (not just fs2) return error (so it can do
more initialization -- added by the following commits).
3. Replace complicated cgroup manager instantiation in factory_linux
by a single (and simple) libcontainer/cgroups/manager.New() function.
4. getUnifiedPath is simplified to check that only a single path is
supplied (rather than checking that other paths, if supplied,
are the same).
[v2: can't -> cannot]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
As ExpandSlice("system.slice") returns "/system.slice", there is no need
to call it for such paths (and the slash will be added by path.Join
anyway).
The same optimization was already done for v2 as part of commit
bf15cc99b1.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
We were checking if a unit is a slice two times. Consolidate those
checks, and improve comments while we're at it.
The code is the same in v1 and v2 but it's too complicated to factor it
out, thus we just do the same changes in v1.go and v2.go.
No functional change.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
These are not needed as these packages (libcontainer/cgroups,
libcontainer/cgroups/fs, and libcontainer/cgroups/systemd) can
not be built under non-linux anyway (for various reasons).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Only some libcontainer packages can be built on non-linux platforms
(not that it make sense, but at least go build succeeds). Let's call
these "good" packages.
For all other packages (i.e. ones that fail to build with GOOS other
than linux), it does not make sense to have linux build tag (as they
are broken already, and thus are not and can not be used on anything
other than Linux).
Remove linux build tag for all non-"good" packages.
This was mostly done by the following script, with just a few manual
fixes on top.
function list_good_pkgs() {
for pkg in $(find . -type d -print); do
GOOS=freebsd go build $pkg 2>/dev/null \
&& GOOS=solaris go build $pkg 2>/dev/null \
&& echo $pkg
done | sed -e 's|^./||' | tr '\n' '|' | sed -e 's/|$//'
}
function remove_tag() {
sed -i -e '\|^// +build linux$|d' $1
go fmt $1
}
SKIP="^("$(list_good_pkgs)")"
for f in $(git ls-files . | grep .go$); do
if echo $f | grep -qE "$SKIP"; then
echo skip $f
continue
fi
echo proc $f
remove_tag $f
done
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is helpful to kubernetes in cases it knows for sure that the freeze
is not required (since it created the systemd unit with no device
restrictions).
As the code is trivial, no tests are required.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Add a test for freezeBeforeSet, checking various scenarios including
those that were failing before the fix in the previous commit.
[v2: add more cases, add a check before creating a unit.]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This fixes the behavior intended to avoid freezing containers/control
groups without it being necessary. This is important for end users of
libcontainer who rely on the behavior of no freeze.
The previous implementation would always get error trying to get
DevicePolicy from the Unit via dbus, since the Unit interface doesn't
contain DevicePolicy.
Signed-off-by: Odin Ugedal <odin@uged.al>
Kir Kolyshkin