if bfq is not loaded, then io.bfq.weight is not available. io.weight
should always be available and is the next best equivalent thing.
Signed-off-by: Daniel Dao <dqminh89@gmail.com>
bfq weight controller (i.e. io.bfq.weight if present) is still using the
same bfq weight scheme (i.e 1->1000, see [1].) Unfortunately the
documentation for this was wrong, and only fixed recently [2].
Therefore, if we map blkio weight to io.bfq.weight, there's no need to
do any conversion. Otherwise, we will try to write invalid value which
results in error such as:
```
time="2021-02-03T14:55:30Z" level=error msg="container_linux.go:367: starting container process caused: process_linux.go:495: container init caused: process_linux.go:458: setting cgroup config for procHooks process caused: failed to write \"7475\": write /sys/fs/cgroup/runc-cgroups-integration-test/test-cgroup/io.bfq.weight: numerical result out of range"
```
[1] https://github.com/torvalds/linux/blob/master/Documentation/block/bfq-iosched.rst
[2] https://github.com/torvalds/linux/commit/65752aef0a407e1ef17ec78a7fc31ba4e0b360f9
Signed-off-by: Daniel Dao <dqminh89@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>
This makes the code simpler and more future-proof, in case
any more values will appear in hugetlb.*.events.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Generalize the libct/getValueFromCgroup() as fscommon.GetValueByKey(),
and document it.
No changes other than using fscommon.ParseUint to convert the value.
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>
All cgroup managers has Apply() and Set() methods:
- Apply is used to create a cgroup (and, in case of systemd,
a systemd unit) and/or put a PID into the cgroup (and unit);
- Set is used to set various cgroup resources and limits.
The fs/fs2 cgroup manager implements the functionality as described above.
The systemd v1/v2 manager deviate -- it sets *most* of cgroup limits
(those that can be projected to systemd unit properties) in Apply(),
and then again *all* cgroup limits in Set (first indirectly via systemd
properties -- same as in Apply, then via cgroupfs).
This commit removes setting the cgroup limits from Apply,
so now the systemd manager behaves the same way as the fs manager.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Checking the access mode as bellow
if (R3 & bpfAccess == 0 /* use R1 as a temp var */) goto next
does not handle correctly device file probing with:
access(dev_name, F_OK)
F_OK does not trigger read or write access. Instead the access type in
R3 in that case will be zero and the check will not pass even if "rw" is
allowed for the device file. Comparing the 'masked' access type with the
requested one solves the issue:
if (R3 & bpfAccess != R3 /* use R1 as a temp var */) goto next
Signed-off-by: Vasiliy Ulyanov <vulyanov@suse.de>
In the past we incorrectly handled eBPF errors in two ways:
1. We would only ignore errors if there was an allow rule in the list
(this doesn't make sense because for security purposes we only care
if a *deny* rule is being ignored). Arguably this is a security flaw
but you would only get an error from bpf(2) in rare cases, and thus
is not a big enough deal to go through security review.
2. If we were in a rootless container we would still return an error
even though bpf(2) is blocked for rootless containers.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.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>
The merge 6eed6e5795 broke the build because ab27e12ceb ("Implement
GetStat for cpuset cgroup.") dropped the errors import which was used by
c85cd2b325 ("libct/cg/fs/cpuset: don't parse mountinfo") and the CI
wasn't retriggered.
Fix this by just importing "github.com/pkg/errors" again.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
A recent commit a35cad3b22 added warnings about systemd being too
old. While those warnings are valid, they break some existing tests,
and also don't add much value to a user (IOW no one is going to upgrade
systemd because runc says it's old).
Demote those to warnings.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It was already explained why we ignore the error, so let's ignore this
deliberately.
This fixes
> name.go:22:7: Error return value of `join` is not checked (errcheck)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Rewrite getPageUsageByNUMA
1. Be less strict to unknown contents, i.e. skip it. This makes the
function more future-proof. Before this commit, if a line like
"a=b" is encountered, the function returns an error, which is
propagated all the way up to and returned by (CgroupManager).GetStats.
2. Be more strict to contents it recognizes, i.e. return an error.
In case the first field in the line is recognized (e.g. "total=123",
the rest of the line should be in format "N<id>=<value> ...".
3. Optimize. Before this commit, addNUMAStatsByType was called for every
item in the line, which is excessive and might even be slow in case
there are many NUMA nodes. It is enough to look up the field once.
4. Remove a bunch of global numaNode* and numaStat* constants. Those
were used by only one function, and it does not make sense to have
them defined globally. Some were moved to the function, some were
eliminated entirely.
5. Improve readability and added code comments.
Finally, add some test cases for good and bad contents.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
strings.SplitN not always return N fields if not staify, sometimes
cgroup interface add some custom fields make parse memory.numa_stat
fails, it will case panic
Signed-off-by: acetang <aceapril@126.com>
This:
> === RUN TestGetHugePageSizeImpl
> utils_test.go:504: (input [hugepages-akB], error strconv.Atoi: parsing "a": invalid syntax)
feels like an error but it's not.
Only log errors.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. Add a check to unifiedResToSystemdProps that systemd is recent enough
to support AllowedCPUs/AllowedMemoryNodes unit properties, and skip
setting the property if it is not supported.
Note that this is not an error as the setting is still applied to
the underlying cgroupfs -- it's just systemd unit property that is
being skipped.
2. In all the places we skip an unsupported property, warn about it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Support for systemd properties AllowedCPUs and AllowedMemoryNodes
was added by commit 13afa58d0e, but only for unified resources
of systemd v2 driver.
This adds support for Cpu.Cpus and Cpu.Mems resources to
both systemd v1 and v2 cgroup drivers.
An integration test is added to check that the settings work.
[v2: check for systemd version]
[v3: same in the test]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
As the caller of this function just logs the error, it does not make
sense to pass it. Instead, log it (once) and return -1.
This is a preparation for the second user.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Here, we always create a parent directory, so using MkdirAll
is redundant. Use Mkdir instead.
One difference between MkdirAll and Mkdir is the former ignores
EEXIST, and since we sometimes try to create a directory that
already exists, we need to explicitly ignore that.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In cgroup v1, the parent of a cgroup mount is on tmpfs
(or maybe any other fs but definitely not cgroupfs).
Use this to traverse up the tree until we reach non-cgroupfs.
This should work in any setups (nested container, non-standard
cgroup mounts) so issue [1] won't happen.
Theoretically, the only problematic case is when someone mounts
cpuset cgroupfs into a directory on another cgroupfs. Let's
assume people don't do that -- if they do, they will get other
error (e.g. inability to read cpuset.cpus file).
[1] https://github.com/opencontainers/runc/issues/1367
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Drop the custom mountinfo parser, reuse the cgroups.GetCgroupMounts()
to get the cgroup root. Faster, and much less code.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Apparently it is inevitable that we have to read mountinfo multiple
times when dealing with cgroup v1. It seems we can only do it once
and reuse the data, without major modifications to the code.
This commit does a few things.
1. Drop our custom mountinfo parser implementation in favor of
moby/sys/mountinfo. While the custom parser is faster
(about 2x according to benchmark) for this particular case,
the one from the package is more correct and future-proof.
2. Read mountinfo only once, caching all the entries with fstype of
cgroup. With this, there's no need to worry about performance
degradation introduced above.
3. Drop "isSubsystemAvailable" optimization (introduced by commit
2a1a6cdf44) because now with the cache it is probably slowing
things down.
4. Modify the tests accordingly.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
> libcontainer/cgroups/utils.go:282:4: SA4006: this value of `paths` is never used (staticcheck)
> paths = make(map[string]string)
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In case we get ENOSYS from openat2(2), this is expected, so log that
we're falling back to using securejoin as debug.
Otherwise, log it as a warning (as the error is unexpected, but we're
still good to go).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Akihiro Suda