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 is a very simple test that checks that container.Run do not leak
opened file descriptors.
In fact it does, so we have to add two exclusions:
1. /sys/fs/cgroup is opened once per lifetime in prepareOpenat2(),
provided that cgroupv2 is used and openat2 is available. This
works as intended ("it's not a bug, it's a feature").
2. ebpf program fd is leaked every time we call setDevices() for
cgroupv2 (iow, every container.Run or container.Set leaks 1 fd).
This needs to be fixed, thus FIXME.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Apparently, the parent never closes execFifo fd. Not a problem for runc
per se, but can be an issue for a user of libcontainer.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
First, add runc --debug exec test cases, very similar to those in
debug.bats but for runc exec (rather than runc run). Do not include json
tests as it is already tested in debug.bats.
Second, add logrus debug to late stages of runc init, and amend the
integration tests to check for those messages. This serves two purposes:
- demonstrate that runc init can be amended with debug logrus which is
properly forwarded to and logged by the parent runc create/run/exec;
- improve the chances to catch the race fixed by the previous commit.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Sometimes debug.bats test cases are failing like this:
> not ok 27 global --debug to --log --log-format 'json'
> # (in test file tests/integration/debug.bats, line 77)
> # `[[ "${output}" == *"child process in init()"* ]]' failed
It happens more when writing to disk.
This issue is caused by the fact that runc spawns log forwarding goroutine
(ForwardLogs) but does not wait for it to finish, resulting in missing
debug lines from nsexec.
ForwardLogs itself, though, never finishes, because it reads from a
reading side of a pipe which writing side is not closed. This is
especially true in case of runc create, which spawns runc init and
exits; meanwhile runc init waits on exec fifo for arbitrarily long
time before doing execve.
So, to fix the failure described above, we need to:
1. Make runc create/run/exec wait for ForwardLogs to finish;
2. Make runc init close its log pipe file descriptor (i.e.
the one which value is passed in _LIBCONTAINER_LOGPIPE
environment variable).
This is exactly what this commit does:
1. Amend ForwardLogs to return a channel, and wait for it in start().
2. In runc init, save the log fd and close it as late as possible.
PS I have to admit I still do not understand why an explicit close of
log pipe fd is required in e.g. (*linuxSetnsInit).Init, right before
the execve which (thanks to CLOEXEC) closes the fd anyway.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
- add check, checkWait, and finish helpers;
- move test cleanup to runLogForwarding;
- introduce and use log struct.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
I have noticed that ConfigureLogs do not return an error in case logging
was already configured -- instead it just warns about it. So I went
ahead and changed the warning to the actual error...
... only to discover I broke things badly, because in case of runc init
logging is configured twice. The fix is to not configure logging in case
we are init.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Error handling is slightly cleaner this way.
While at it, do minor refactoring and fix error logging
in processEntry.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Apparently, scripts/validate-c is not working in CI (or maybe
maintainers ignored the failures from it) -- current C code
gets some changes if we run indent on it.
This commit fixes this, simplifying things along the way.
In particular:
1. Remove "validate" make target, add "cfmt" target that just runs
indent on all *.c files in the repository (NOTE that *.h files
are not included, as before).
This may help a contributor to fix their code -- they just need
to run "make cfmt" now instead of running "make validate" and
copy-pasting the indent command and options from the hint.
2. Split GHA validate/misc into validate/release and validate/cfmt.
The latter checks that the sources are not changed after "make cfmt".
3. Adds a few more options to indent. This was mostly motivated by
trying to save the existing formatting, minimizing the amount of
changes indent produces.
The new options are:
* -il0: sets the offset for goto labels to 0 (currently all labels
but one are not indented -- let's keep it that way);
* -ppi2: sets the indentation for nested preprocessor directives
to 2 spaces (same as it is done in "SYS_memfd_create" defines);
* -cp1: sets the indentation between #else / #endif and the
following comment to 1 space.
4. Reformat the code using the new indent options.
5. Remove the now-unused script/{.validate,validate-c}.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Remove comments with architectures when defining SYS_memfd_create,
as they are redundant, and indent has a funny way of indenting them.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This block apparently does nothing except for creating
a need for additional indentation. Remove it.
While at it, break a long line in this code.
No functional change.
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>
An exec may fail due to memory shortage (cgroup memory limits being too
tight), and an error message provided in this case is clueless:
> $ sudo ../runc/runc exec xx56 top
> ERRO[0000] exec failed: container_linux.go:367: starting container process caused: read init-p: connection reset by peer
Same as the previous commit for run/start, check the OOM kill counter
and report an OOM kill.
The differences from run are
1. The container is already running and OOM kill counter might not be
zero. This is why we have to read the counter before exec and after
it failed.
2. An unrelated OOM kill event might occur in parallel with our exec
(and I see no way to find out which process was killed, except to
parse kernel logs which seems excessive and not very reliable).
This is why we report _possible_ OOM kill.
With this commit, the error message looks like:
> ERRO[0000] exec failed: container_linux.go:367: starting container process caused: process_linux.go:105: possibly OOM-killed caused: read init-p: connection reset by peer
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>
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>
This is to benefit from openat2() implementation, on kernels
that support it. Theoretically this also improves security.
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>
Kir Kolyshkin