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>
As reported in issue 3084, sometimes setting CPU quota period fails
when a new period is lower and a parent cgroup has CPU quota limit set.
This happens as in cgroup v1 the quota and the period can not be set
together (this is fixed in v2), and since the period is being set first,
new_limit = old_quota/new_period may be higher than the parent cgroup
limit.
The fix is to retry setting the period after the quota, to cover all
possible scenarios.
Add a test case to cover a regression caused by an earlier version of
this patch (ignoring a failure of setting invalid period when quota is
not set).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The two exceptions I had to add to codespellrc are:
- CLOS (used by intelrtd);
- creat (syscall name used in tests/integration/testdata/seccomp_*.json).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This was initially added by commits 41d9d26513 and 4a8f0b4db4,
apparently to implement docker run --cgroup container:ID, which was
never merged. Therefore, this code is not and was never used.
It needs to be removed mainly because having it makes it much harder to
understand how cgroup manager works (because with this in place we have
not one or two but three sets of cgroup paths to think about).
Note if the paths are known and there is a need to add a PID to existing
cgroup, cgroup manager is not needed at all -- something like
cgroups.WriteCgroupProc or cgroups.EnterPid is sufficient (and the
latter is what runc exec uses in (*setnsProcess).start).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
If a control group is frozen, all its descendants will report FROZEN
in freezer.state cgroup file.
OTOH cgroup v2 cgroup.freeze is not reporting the cgroup as frozen
unless it is frozen directly (i.e. not via an ancestor).
Fix the discrepancy between v1 and v2 drivers behavior by
looking into freezer.self_freezing cgroup file, which, according
to kernel documentation, will show 1 iff the cgroup was frozen directly.
Co-authored-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Odin Ugedal <odin@uged.al>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Errors from unix.* are always bare and thus can be used directly.
Add //nolint:errorlint annotation to ignore errors such as these:
libcontainer/system/xattrs_linux.go:18:7: comparing with == will fail on wrapped errors. Use errors.Is to check for a specific error (errorlint)
case errno == unix.ERANGE:
^
libcontainer/container_linux.go:1259:9: comparing with != will fail on wrapped errors. Use errors.Is to check for a specific error (errorlint)
if e != unix.EINVAL {
^
libcontainer/rootfs_linux.go:919:7: comparing with != will fail on wrapped errors. Use errors.Is to check for a specific error (errorlint)
if err != unix.EINVAL && err != unix.EPERM {
^
libcontainer/rootfs_linux.go:1002:4: switch on an error will fail on wrapped errors. Use errors.Is to check for specific errors (errorlint)
switch err {
^
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This builds on top of recently introduced fscommon.ParseError.
Errors returned from parsers (mostly ones used by GetStats()) are all
different, and many are incomplete. For example, in many cases errors
from strconv.ParseUint are returned as is, meaning there is no context
telling which file we were reading. Similarly, errors from
fscommon.ParseKeyValue should be wrapped to add more context.
Same is true for scanner.Err().
One special case that repeats a few times is "malformed line: xxx".
Add and use a helper for that to simplify things.
OTOH, errors from fscommon.GetCgroup* do have enough context and there
is no need to wrap them.
Fix all the above.
While at it, add a missing scanner.Err() check.
[v2: use parseError not ParseError]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Do not wrap errors returned from fscommon.GetCgroupParamUint -- those
errors already have enough context.
2. Instead of parsing "max" ourselves, use GetCgroupParamUint which does
it, and then convert MaxUint64 to 0 (we do it historically since
commit 087b953dc5, and while using MaxUint64 as is seems fine,
there may be some existing users who rely on the old behavior).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The error from fscommon.GetCgroup* already contains the file name and so
on, so there's no need to wrap it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
For per-device weight, you can set weight and/or leaf weight.
The problem is, with the recent fix to use BFQ on cgroup v1,
if per-device weights are set, the code tries to set device
weight to blkio.bfq.weight, and the leaf weight to
blkio.leaf_weight_device. The latter file does not exist on
kernels v5.0, meaning one can not set any per-device weights
at all.
The fix is to only set weights if they are non-zero (i.e. set).
The test case will come in a following commit.
Fixes: 6339d8a0dd
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is a better place as cgroups itself is using these.
Should help with moving more stuff common in between fs and fs2 to
fscommon.
Looks big, but this is just moving the code around:
fscommon/{fscommon,open}.go -> cgroups/file.go
fscommon/fscommon_test.go -> cgroups/file_test.go
and fixes for TestMode moved to a different package.
There's no functional change.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
- Update the blkio cgroup to support the BFQ I/O Scheduler, that has
replaced CFQ in the Linux kernel.
- BFQ is controlled through blkio.bfq.weight[_device] instead of
CFQ's blkio.weight[_device] in cgroups v1.
- BFQ does not support blkio.leaf_weight[_device], so that behavior
remains untouched.
- Do not change behavior on legacy CFQ systems.
- Enable using blkio weights on BFQ systems.
Signed-off-by: Antti Kervinen <antti.kervinen@intel.com>
These functions are called from multiple places,
and if t.Helper() is not used, the context is not clear.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
gofumpt (mvdan.cc/gofumpt) is a fork of gofmt with stricter rules.
Brought to you by
git ls-files \*.go | grep -v ^vendor/ | xargs gofumpt -s -w
Looking at the diff, all these changes make sense.
Also, replace gofmt with gofumpt in golangci.yml.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Most of these were false positives or cases where we want to ignore the
lint, but the change to the BPF generation is actually useful.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
I hate to keep adding those kludges, but lately TestFreeze (and
TestSystemdFreeze) from libcontainer/integration fails a lot. The
failure comes and goes, and is probably this is caused by a slow host
allocated for the test, and a slow VM on top of it.
To remediate, add a small sleep on every 25th iteration in between
asking the kernel to freeze and checking its status.
In the worst case scenario (failure to freeze), this adds about 0.4 ms
(40 x 10 us) to the duration of the call.
It is hard to measure how this affects CI as GHA plays a roulette when
allocating a node to run the test on, but it seems to help. With
additional debug info, I saw somewhat frequent "frozen after 24 retries"
or "frozen after 49 retries", meaning it succeeded right after the added
sleep.
While at it, rewrite/improve the comments.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
A cgroup manager's Set method sets cgroup resources, but historically
it was accepting configs.Cgroups.
Refactor it to accept resources only. This is an improvement from the
API point of view, as the method can not change cgroup configuration
(such as path to the cgroup etc), it can only set (modify) its
resources/limits.
This also lays the foundation for complicated resource updates, as now
Set has two sets of resources -- the one that was previously specified
during cgroup manager creation (or the previous Set), and the one passed
in the argument, so it could deduce the difference between these. This
is a long term goal though.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 88e8350de2, among the other things, replaced filepath.Join with
securejoin.SecureJoin for both reads and writes to cgroupfs.
Commits e76ac1c054 and 31f0f5b7e0 switched more code to use
fscommon.ReadFile (and thus securejoin). Commit 0228226e6d introduced
fscommon.OpenFile (which uses securejoin as the fallback if openat2(2)
is not available, which is the case for older kernels), and commit
c95e69007c switched most of cgroup/fs[2] code to use it.
As a result, fs.GetStats() method became noticeable slower, mostly due
to securejoin calling os.Lstat and filepath.Clean.
Using securejoin as a security measure for cgroupfs files is
not well justified, as cgroupfs do not contain symlinks, and none of the
code using it have uncleaned paths. In particular, fs/fs2/systemd
managers do check and sanitize their paths.
This commit modifies the code to not use securejoin. Instead, it checks
that the opened file is indeed on cgroupfs.
Using BenchmarkGetStats on a CentOS 8 VM, I see the following
improvement:
Before:
> BenchmarkGetStats-8 8376 625135 ns/op
After:
> BenchmarkGetStats-8 12226 485015 ns/op
An intermediate version, with no fstatfs to check fstype:
> BenchmarkGetStats-8 13162 452281 ns/op
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Function (*FreezerGroup).Set has a few paths where in can return an
error. In any case, if an error is returned, we failed to freeze,
and we need to thaw to avoid leaving the cgroup in a stuck state.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 5d0ffbf9c8 added OOM kill count checking and better container
start/run/exec error reporting in case we hit OOM.
It also introduced warnings like these:
> level=warning msg="unable to get oom kill count" error="openat2
> /sys/fs/cgroup/user.slice/user-1000.slice/user@1000.service/test_hello/memory.events:
> no such file or directory"
In case of rootless containers, unless cgroup is delegated or systemd is
used, runc can not create a cgroup and thus it fails to get OOM kill
count. This is expected, and the warning should not be shown in this
case.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
For fs, commit fc620fdf81 made rootless field private,
and for fs2, it was always private, and yet comments in both
mention it as m.Rootless.
Fix it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is somewhat radical approach to deal with kernel memory.
Per-cgroup kernel memory limiting was always problematic. A few
examples:
- older kernels had bugs and were even oopsing sometimes (best example
is RHEL7 kernel);
- kernel is unable to reclaim the kernel memory so once the limit is
hit a cgroup is toasted;
- some kernel memory allocations don't allow failing.
In addition to that,
- users don't have a clue about how to set kernel memory limits
(as the concept is much more complicated than e.g. [user] memory);
- different kernels might have different kernel memory usage,
which is sort of unexpected;
- cgroup v2 do not have a [dedicated] kmem limit knob, and thus
runc silently ignores kernel memory limits for v2;
- kernel v5.4 made cgroup v1 kmem.limit obsoleted (see
https://github.com/torvalds/linux/commit/0158115f702b).
In view of all this, and as the runtime-spec lists memory.kernel
and memory.kernelTCP as OPTIONAL, let's ignore kernel memory
limits (for cgroup v1, same as we're already doing for v2).
This should result in less bugs and better user experience.
The only bad side effect from it might be that stat can show kernel
memory usage as 0 (since the accounting is not enabled).
[v2: add a warning in specconv that limits are ignored]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Moving these utilities to a separate package, so that consumers of this
package don't have to pull in the whole "system" package.
Looking at uses of these utilities (outside of runc itself);
`RunningInUserNS()` is used by [various external consumers][1],
so adding a "Deprecated" alias for this.
[1]: https://grep.app/search?current=2&q=.RunningInUserNS
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Existing code ignores ENOENT error in case we're reading data from
controls that might not be enabled. While this is correct, the code
can be improved:
1. Check name != "" instead of moduleName != "memory", as these checks
are equivalent but the new one is faster.
2. It does not make sense to ignore subsequent errors -- if the control
is not available, we won't hit this codepath.
3. Add a comment explaining why we ignore the error.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In some cases, container init fails to start because it is killed by
the kernel OOM killer. The errors returned by runc in such cases are
semi-random and rather cryptic. Below are a few examples.
On cgroup v1 + systemd cgroup driver:
> process_linux.go:348: copying bootstrap data to pipe caused: write init-p: broken pipe
> process_linux.go:352: getting the final child's pid from pipe caused: EOF
On cgroup v2:
> process_linux.go:495: container init caused: read init-p: connection reset by peer
> process_linux.go:484: writing syncT 'resume' caused: write init-p: broken pipe
This commits adds the OOM method to cgroup managers, which tells whether
the container was OOM-killed. In case that has happened, the original error
is discarded (unless --debug is set), and the new OOM error is reported
instead:
> ERRO[0000] container_linux.go:367: starting container process caused: container init was OOM-killed (memory limit too low?)
Also, fix the rootless test cases that are failing because they expect
an error in the first line, and we have an additional warning now:
> unable to get oom kill count" error="no directory specified for memory.oom_control
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. This is the only function in the package with Get prefix
that does not read a file (but parses a string). Rename
accordingly, and convert the callers.
GetCgroupParamKeyValue -> ParseKeyValue
2. Use strings.Split rather than strings.Fields. Split by a space
is 2x faster, plus we can limit the splitting. The downside is
we have to strip a newline in one of the callers.
3. Improve the doc and the code flow.
4. Fix a test case with invalid data (spaces at BOL).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
EBUSY when trying to set memory limit may mean the new limit is too low
(lower than the current usage, and the kernel can't do anything).
Provide a more specific error for such case.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Factor out setMemory and setSwap
2. Pass cgroup.Resources (rather than cgroup) to setMemoryAndSwap().
3. Merge the duplicated "set memory, set swap" case.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Currently, we read and parse 5 different files while we only need 1.
Use GetCgroupParamUint() directly to get current limit.
While at it, remove the workaround previously needed for the unit test,
and make it a bit more verbose.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It appears that briefly thawing the cgroup while freezing
greatly increases its chances to freeze successfully.
The test case I used is doing runc exec in a look parallel with runc
pause/resume in another loop, and the failure to freeze rate reduced
from 40 to 0 per minute (tested inside a VM using a busybox container
running sleep 1h, doing about 1500 pause/resumes and 650 execs per
minute), with max retries being 150 (of 1000).
This is still a game of chances, so failures are possible.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Before this commit, Set() used GetState() to check the freezer state
and retry the operation if the actual state still differs from requested.
This should help with the situation when a new process (such as one
added by runc exec) is added to the container's cgroup while it's being
freezed by the kernel, but it's not working as it should.
The problem is, GetState() never returns FREEZING state, looping until
the state is either FROZEN or THAWED, so Set() does not have a chance
to repeate the freeze attempt.
As a result, the container might end up stuck in a FREEZING state,
with GetState() never returning (which in turn blocks some other
operations).
One way to fix this would be to have GetState returning FREEZING state
instead of retrying ad infinitum. It would result in changing the public
API, and no callers of GetState expects it to return this.
To fix, let's not use GetState() from Set(). Instead, read the
freezer.state file directly and act accordingly -- return success
on FROZEN, retry on FREEZING, and error out on any other (unexpected)
value.
While at it, further improve the code:
- limit the number of retries;
- if retries are exceeded, thaw and return an error;
- don't retry (or read the state back) on THAW.
I played a lot with various reproducers for this bug, including
- parallel runc execs and runc pause/resumes
- parallel runc execs and runc --systemd-cgroup update
(the latter performs freeze/unfreeze);
- continuously running /bin/printf inside container
in parallel with runc pause/resume;
- running pthread bomb (from criu test suite) in parallel
with runc pause/resume;
and I was not able to make freeze work 100%, meaning sometimes
runc pause fails, or runc --systemd-cgroup update produces a warning.
With that said, it's still a big improvement over the previous
state of affairs where container is stuck in FREEZING state,
and GetState() (and all its users) are also stuck.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Kir Kolyshkin