The warnings fixed were:
libcontainer/configs/config_test.go:205:12: printf: non-constant format string in call to (*testing.common).Errorf (govet)
t.Errorf(fmt.Sprintf("Expected error to not occur but it was %+v", err))
^
libcontainer/cgroups/fs/blkio_test.go:481:13: printf: non-constant format string in call to (*testing.common).Errorf (govet)
t.Errorf(fmt.Sprintf("test case '%s' failed unexpectedly: %s", testCase.desc, err))
^
libcontainer/cgroups/fs/blkio_test.go:595:13: printf: non-constant format string in call to (*testing.common).Errorf (govet)
t.Errorf(fmt.Sprintf("test case '%s' failed unexpectedly: %s", testCase.desc, err))
^
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
There's too much logic here figuring out which CPUs to use. Runc is a
low level tool and is not supposed to be that "smart". What's worse,
this logic is executed on every exec, making it slower. Some of the
logic in (*setnsProcess).start is executed even if no annotation is set,
thus making ALL execs slow.
Also, this should be a property of a process, rather than annotation.
The plan is to rework this.
This reverts commit afc23e3397.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It has been pointed out that some controllers can not accept multiple
lines of output at once. In particular, io.max can only set one device
at a time.
Practically, the only multi-line resource values we can get come from
unified.* -- let's write those line by line.
Add a test case.
Reported-by: Tao Shen <shentaoskyking@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
When swap is being disabled (as set to 0), or set to max, ignore
non-existent memory.swap.max cgroup file.
If swap is being set explicitly to some value, do return an error like
before.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This test panics if userns is detected (such as when run in a rootless
docker container) because SetV1 does nothing in this case.
We could fix the panic, but it doesn't make sense to run the test at
all.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The issue is the same as in commit 1b2adcf but for RT scheduler;
the fix is also the same.
Test case by ls-ggg.
Co-authored-by: ls-ggg <335814617@qq.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
If CPU burst knob is non-existent, the current implementation (added in
commit e1584831) still tries to set it again after setting the new CPU
quota, which is useless (and we have to ignore ENOENT again).
Fix this.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit b6967fa84c moved the functionality of managing cgroup devices
into a separate package, and decoupled libcontainer/cgroups from it.
Yet, some software (e.g. cadvisor) may need to use libcontainer package,
which imports libcontainer/cgroups/devices, thus making it impossible to
use libcontainer without bringing in cgroup/devices dependency.
In fact, we only need to manage devices in runc binary, so move the
import to main.go.
The need to import libct/cg/dev in order to manage devices is already
documented in libcontainer/cgroups, but let's
- update that documentation;
- add a similar note to libcontainer/cgroups/systemd;
- add a note to libct README.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This handles a corner case when joining a container having all
the processes running exclusively on isolated CPU cores to force
the kernel to schedule runc process on the first CPU core within the
cgroups cpuset.
The introduction of the kernel commit
46a87b3851f0d6eb05e6d83d5c5a30df0eca8f76 has affected this deterministic
scheduling behavior by distributing tasks across CPU cores within the
cgroups cpuset. Some intensive real-time application are relying on this
deterministic behavior and use the first CPU core to run a slow thread
while other CPU cores are fully used by real-time threads with SCHED_FIFO
policy. Such applications prevents runc process from joining a container
when the runc process is randomly scheduled on a CPU core owned by a
real-time thread.
Introduces isolated CPU affinity transition OCI runtime annotation
org.opencontainers.runc.exec.isolated-cpu-affinity-transition to restore
the behavior during runc exec.
Fix issue with kernel >= 6.2 not resetting CPU affinity for container processes.
Signed-off-by: Cédric Clerget <cedric.clerget@gmail.com>
We auto-close this file descriptor in the final exec step, but it's
probably a good idea to not possibly leak the file descriptor to "runc
init" (we've had issues like this in the past) especially since it is a
directory handle from the host mount namespace.
In practice, on runc 1.1 this does leak to "runc init" but on main the
handle has a low enough file descriptor that it gets clobbered by the
ForkExec of "runc init".
OPEN_TREE_CLONE would let us protect this handle even further, but the
performance impact of creating an anonymous mount namespace is probably
not worth it.
Also, switch to using an *os.File for the handle so if it goes out of
scope during setup (i.e. an error occurs during setup) it will get
cleaned up by the GC.
Fixes: GHSA-xr7r-f8xq-vfvv CVE-2024-21626
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
With the idmap work, we will have a tainted Go thread in our
thread-group that has a different mount namespace to the other threads.
It seems that (due to some bad luck) the Go scheduler tends to make this
thread the thread-group leader in our tests, which results in very
baffling failures where /proc/self/mountinfo produces gibberish results.
In order to avoid this, switch to using /proc/thread-self for everything
that is thread-local. This primarily includes switching all file
descriptor paths (CLONE_FS), all of the places that check the current
cgroup (technically we never will run a single runc thread in a separate
cgroup, but better to be safe than sorry), and the aforementioned
mountinfo code. We don't need to do anything for the following because
the results we need aren't thread-local:
* Checks that certain namespaces are supported by stat(2)ing
/proc/self/ns/...
* /proc/self/exe and /proc/self/cmdline are not thread-local.
* While threads can be in different cgroups, we do not do this for the
runc binary (or libcontainer) and thus we do not need to switch to
the thread-local version of /proc/self/cgroups.
* All of the CLONE_NEWUSER files are not thread-local because you
cannot set the usernamespace of a single thread (setns(CLONE_NEWUSER)
is blocked for multi-threaded programs).
Note that we have to use runtime.LockOSThread when we have an open
handle to a tid-specific procfs file that we are operating on multiple
times. Go can reschedule us such that we are running on a different
thread and then kill the original thread (causing -ENOENT or similarly
confusing errors). This is not strictly necessary for most usages of
/proc/thread-self (such as using /proc/thread-self/fd/$n directly) since
only operating on the actual inodes associated with the tid requires
this locking, but because of the pre-3.17 fallback for CentOS, we have
to do this in most cases.
In addition, CentOS's kernel is too old for /proc/thread-self, which
requires us to emulate it -- however in rootfs_linux.go, we are in the
container pid namespace but /proc is the host's procfs. This leads to
the incredibly frustrating situation where there is no way (on pre-4.1
Linux) to figure out which /proc/self/task/... entry refers to the
current tid. We can just use /proc/self in this case.
Yes this is all pretty ugly. I also wish it wasn't necessary.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
The current code is only doing retries in RemovePaths, which is only
used for cgroup v1 (cgroup v2 uses RemovePath, which makes no retries).
Let's remove all retry logic and logging from RemovePaths, together
with:
- os.Stat check from RemovePaths (its usage probably made sense before
commit 19be8e5ba5 but not after);
- error/warning logging from RemovePaths (this was added by commit
19be8e5ba5 in 2020 and so far we've seen no errors other
than EBUSY, so reporting the actual error proved to be useless).
Add the retry logic to rmdir, and the second retry bool argument.
Decrease the initial delay and increase the number of retries from the
old implementation so it can take up to ~1 sec before returning EBUSY
(was about 0.3 sec).
Hopefully, as a result, we'll have less "failed to remove cgroup paths"
errors.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
CentOS 7 is showing its age and we'd rather skip some tests on it than
find out why they are flaky.
Add internal/testutil package, and move the generalized version of
SkipOnCentOS7 from libcontainer/cgroups/devices to there.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This prevents potential exploit of using "../" in cgroups.OpenFile
(as well as other methods that use OpenFile) to read or write to
other cgroups.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This field reports swap-only usage. For cgroupv1, `Usage` and `Failcnt`
are set by subtracting memory usage from memory+swap usage. For cgroupv2,
`Usage`, `Limit`, and `MaxUsage` are set. This commit also export `MaxUsage`
of memory under cgroupv2 mode, using `memory.peak` introduced in kernel 5.19.
Signed-off-by: Heran Yang <heran55@126.com>
There is no point in showing the underlying error when path == "",
because it is ENOENT.
Revert the change done in commit e1584831b6.
Fixes: e1584831b6
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit f34eb2c00 introduced a workaround to retry on EINTR due to changes in Go 1.14.
It was fixed in Go 1.15 [1], meaning a custom retry loop is no longer
necessary.
Keep the test case to avoid future regressions.
[1] https://github.com/golang/go/issues/38033
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This adds support for hugetlb.<pagesize>.rsvd limiting and accounting.
The previous non-rsvd max/limit_in_bytes does not account for reserved
huge page memory, making it possible for a processes to reserve all the
huge page memory, without being able to allocate it (due to cgroup
restrictions).
In practice this makes it possible to successfully mmap more huge page
memory than allowed via the cgroup settings, but when using the memory
the process will get a SIGBUS and crash. This is bad for applications
trying to mmap at startup (and it succeeds), but the program crashes
when starting to use the memory. eg. postgres is doing this by default.
This also keeps writing to the old max/limit_in_bytes, for backward
compatibility.
More info can be found here: https://lkml.org/lkml/2020/2/3/1153
(commit message mostly written by Odin Ugedal)
Co-authored-by: Odin Ugedal <odin@ugedal.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Burstable CFS controller is introduced in Linux 5.14. This helps with
parallel workloads that might be bursty. They can get throttled even
when their average utilization is under quota. And they may be latency
sensitive at the same time so that throttling them is undesired.
This feature borrows time now against the future underrun, at the cost
of increased interference against the other system users, by introducing
cfs_burst_us into CFS bandwidth control to enact the cap on unused
bandwidth accumulation, which will then used additionally for burst.
The patch adds the support/control for CFS bandwidth burst.
runtime-spec: https://github.com/opencontainers/runtime-spec/pull/1120
Co-authored-by: Akihiro Suda <suda.kyoto@gmail.com>
Co-authored-by: Nadeshiko Manju <me@manjusaka.me>
Signed-off-by: Kailun Qin <kailun.qin@intel.com>
golangci-lint v1.54.2 comes with errorlint v1.4.4, which contains
the fix [1] whitelisting all errno comparisons for errors coming from
x/sys/unix.
Thus, these annotations are no longer necessary. Hooray!
[1] https://github.com/polyfloyd/go-errorlint/pull/47
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
A few cases relied on the fact that systemd is used, and thus
/sys/fs/cgroup/user.slice is available.
Guess what, in case of "make unittest" it might not be.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Split the test into two -- for fs and systemd cgroup managers, and only
run the second one if systemd is available.
Prevents the following failure during `make unittest`:
> === RUN TestNilResources
> manager_test.go:27: systemd not running on this host, cannot use systemd cgroups manager
> --- FAIL: TestNilResources (0.22s)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This aligns v2 usage calculations more closely with v1.
Current node-level reporting for v1 vs v2 on the same
machine under similar load may differ by ~250-750Mi.
Also return usage as combined swap + memory usage, aligned
with v1 and non-root v2 cgroups.
`mem_cgroup_usage` in the kernel counts NR_FILE_PAGES
+ NR_ANON_MAPPED + `nr_swap_pages` (if swap enabled) [^0].
Using total - free results in higher "usage" numbers.
This is likely due to various types of reclaimable
memory technically counted as in use (e.g. inactive anon).
See also https://github.com/kubernetes/kubernetes/issues/118916 for more context
[^0]: https://github.com/torvalds/linux/blob/06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5/mm/memcontrol.c#L3673-L3680
Signed-off-by: Alexander Eldeib <alexeldeib@gmail.com>
Since systemd v240 (commit 8e8b5d2e6d91180a), one can use
/dev/{char,block}-MAJOR syntax to specify that all MAJOR:*
devices are allowed.
Use it, if available, since it's more straightforward, plus
we can skip somewhat expensive parsing of /proc/devices.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
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>
Replace a panic with a warning, unless it's ENOENT and we're running in
a user namespace. In the latter case, do the same as before, i.e. report
the error but using a Debug logging level.
This prevents software that uses libcontainer from panicking in
some exotic setups.
This will also print a warning on some very old systems which does not
use /sys/fs/cgroup for cgroup mount point. My bet is such systems no
longer exist.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
We read output from the following files if they exists:
- cpu.pressure
- memory.pressure
- io.pressure
Each are in format:
```
some avg10=0.00 avg60=0.00 avg300=0.00 total=0
full avg10=0.00 avg60=0.00 avg300=0.00 total=0
```
Signed-off-by: Daniel Dao <dqminh89@gmail.com>
Co-authored-by: Sandor Szücs <sandor.szuecs@zalando.de>
Co-authored-by: Kir Kolyshkin <kolyshkin@gmail.com>
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>
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>
Rafael Roquetto