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>
Caught by golangci-lint when enabling golint:
libcontainer/cgroups/ebpf/ebpf.go:35:12: SA1019: prog.Attach is deprecated: use link.RawAttachProgram instead. (staticcheck)
if err := prog.Attach(dirFD, ebpf.AttachCGroupDevice, unix.BPF_F_ALLOW_MULTI); err != nil {
^
libcontainer/cgroups/ebpf/ebpf.go:39:13: SA1019: prog.Detach is deprecated: use link.RawDetachProgram instead. (staticcheck)
if err := prog.Detach(dirFD, ebpf.AttachCGroupDevice, unix.BPF_F_ALLOW_MULTI); err != nil {
^
Worth noting that we currently call prog.Detach() with unix.BPF_F_ALLOW_MULTI;
https://github.com/golang/sys/blob/22da62e12c0cd9c1da93581e1113ca4d82a5be14/unix/zerrors_linux.go#L178
BPF_F_ALLOW_MULTI = 0x2
Looking at the source code for prog.Detach(); https://github.com/cilium/ebpf/blob/v0.4.0/prog.go#L579-L581,
this would _always_ produce an error:
if flags != 0 {
return errors.New("flags must be zero")
}
Note that the flags parameter is not used (except for that validation)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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>
Some controllers might still have stats available even if they are
disabled (this is definitely so for cpu.stat -- see earlier commit).
Some stat methods might implement sensible fallbacks (see previous
commit for statPids.
In the view of all that, it makes sense to not check if a particular
controller is available, but rather ignore ENOENT from it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
When getting pids stats, instead of checking whether the pids controller
is available, let's use a fall back function in case pids.current does
not exist. This simplifies the logic in fs2.GetStats.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It is inefficient to create an associative map for the whole purpose of
counting the number of elements in it, especially if the elements are
all unique. It uses more CPU than necessary and creates some work for
the garbage collector.
The file we read contains PIDs and newlines, and the easiest/fastest way
to get the number of PIDs is just to count the newlines.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Giuseppe found out that cpu.stat for a cgroup is available even if
the cpu controller is not enabled for it. So, let's call statCpu
regradress, and ignore ENOENT.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
There is no memory.{current,max} for the root node in cgroup v2, and
thus stats for "/sys/fs/cgroup" return an error.
The same thing works with cgroup v1 (as there are
memory.{usage,limit}_in_bytes files in the root node).
Emulate stats for /sys/fs/cgroup by getting numbers from
/proc/self/meminfo.
NOTE that both memory.current (in cgroup v2) and memory.usage_in_bytes
(in cgroup v1) include page cache etc into the number, so we do the same
when calculating memory usage (as opposed to number reported by "free",
which excludes page cache and buffers).
[v2: check for cgroup files first, as future kernels might add it]
[v3: don't subtract cache from mem_used, simplifying the logic]
[Initially, I wanted to avoid parsing yet another /proc file and
instead mock some numbers using data from memory.stat but was
unable to come up with formulae that make sense.]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
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>
cgroup v1 reports combined mem+swap in stats.MemoryStats.SwapUsage.
In cgroup v2, swap is separate.
For the sake of compatibility, make v2 report mem+swap as well.
This also includes Limit.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This unconditionally enables TasksAccounting for systemd unified (v2)
cgroup driver, making it work the same way as the legacy (v1) driver.
Practically, it is probably a no-op since DefaultTasksAccounting is
usually true.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
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>
Shiming Zhang