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>
This is helpful to kubernetes in cases it knows for sure that the freeze
is not required (since it created the systemd unit with no device
restrictions).
As the code is trivial, no tests are required.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Add a test for freezeBeforeSet, checking various scenarios including
those that were failing before the fix in the previous commit.
[v2: add more cases, add a check before creating a unit.]
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This fixes the behavior intended to avoid freezing containers/control
groups without it being necessary. This is important for end users of
libcontainer who rely on the behavior of no freeze.
The previous implementation would always get error trying to get
DevicePolicy from the Unit via dbus, since the Unit interface doesn't
contain DevicePolicy.
Signed-off-by: Odin Ugedal <odin@uged.al>
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>
TestPodSkipDevicesUpdate checks that updating a pod having SkipDevices: true
does not result in spurious "permission denied" errors in a container
running under the pod. The test is somewhat similar in nature to the
@test "update devices [minimal transition rules]" in tests/integration,
but uses a pod.
This tests the validity of freezeBeforeSet in v1.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This was initially added by commit 3e5c199708 because Set (with
r.Freezer = Frozen) was not able to freeze a container.
Now (see a few previous commits) Set can do the freeze, so the explicit
Freeze is no longer needed.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Introduce freezeBeforeSet, which contains the logic of figuring out
whether we need to freeze/thaw around setting systemd unit properties.
In particular, if SkipDevices is set, and the current unit properties
allow all devices, there is no need to freeze and thaw, as systemd
won't write any device rules in this case.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
m.Freeze method changes m.cgroups.Resources.Freezer field, which should
not be done while we're temporarily freezing the cgroup in Set. If this
field is changed, and r == m.cgroups.Resources (as it often happens),
this results in inability to freeze the container using Set().
To fix, add and use a method which does not change r.Freezer field.
A test case for the bug will be added separately.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This test the issues fixed by the two preceding commits.
Co-Authored-By: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Odin Ugedal <odin@uged.al>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since device updates in cgroup v2 are atomic for systemd, there is no
need to freeze the processes before running the updates.
Signed-off-by: Odin Ugedal <odin@uged.al>
1. The meaning of SkipDevices is what it is -- do not set any
device-related options.
2. Reverts the part of commit 108ee85b82 which skipped the freeze
when the SkipDevices is set. Apparently, the freeze is needed on
update even if no Device* properties are being set.
3. Add "runc update" to "runc run [device cgroup deny]" test.
Fixes: 752e7a8249
Fixes: 108ee85b82
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Noticed that the check of trying to use both rootful and rootless
in NewDbusConnManager never worked, as we never set dbusInited to true.
Do that. While at it, protect this with the mutex (against the
case of two goroutines simultaneously calling NewDbusConnManager).
This is a rare call, so taking read-only then read-write mutex does not
make sense.
Fixes: c7f847ed3a
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This fixes isDbusError function, introduced by commit bacfc2c. Due to a
type error it was not working at all.
This also fixes the whole "retry on dbus disconnect" logic.
This also fixes a regression in startUnit (and cgroupManager.Apply()),
which should never return "unit already exists" error but it did.
Fixes: bacfc2c
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>
The idea is to mimic what kubelet is doing, with minimum amount of code.
First, create a slice with SkipDevices=true. It should have access to
all devices.
Next, create a scope within the above slice, allowing access to /dev/full
only.
Check that within that scope we can only access /dev/full and not other
devices (such as /dev/null).
Repeat the test with SkipDevices=false, make sure we can not access any
devices (as they are disallowed by a parent cgroup). This is done only
to assess the test correctness.
NOTE that cgroup v1 and v2 behave differently for SkipDevices=false
case, and thus the check is different. Cgroup v1 returns EPERM on
writing to devices.allow, so cgroup manager's Set() fails, and we check
for a particular error from m.Set(). Cgroup v2 allows to create a child
cgroup, but denies access to any device (despite access being enabled)
-- so we check the error from the shell script running in that cgroup.
Again, this is only about SkipDevices=false case.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 108ee85b82 adds SkipDevices flag, which is used by kubernetes
to create cgroups for pods.
Unfortunately the above commit falls short, and systemd DevicePolicy and
DeviceAllow properties are still set, which requires kubernetes to set
"allow everything" rule.
This commit fixes this: if SkipDevices flag is set, we return
Device* properties to allow all devices.
Fixes: 108ee85b82
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The devices cgroup emulator is also useful for removing unneeded rules
as well as computing what the final default-allow state of the filter
will be (allow-list or deny-list).
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Historically, we never returned an error from failed startUnit
or stopUnit. The startUnit case was fixed by commit 3844789.
It is time to fix stopUnit, too. The reasons are:
1. Ignoring an error from stopUnit means an unexpected trouble down the
road, for example a failure to create a container with the same name:
> time="2021-05-07T19:51:27Z" level=error msg="container_linux.go:380: starting container process caused: process_linux.go:385: applying cgroup configuration for process caused: Unit runc-test_busybox.scope already exists."
2. A somewhat short timeout of 1 second means the cgroup might
actually be removed a few seconds later but we might have a
race between removing the cgroup and creating another one
with the same name, resulting in the same error as amove.
So, return an error if removal failed, and increase the timeout.
Now, modify the systemd cgroup v1 manager to not mask the error from
stopUnit (stopErr) with the subsequent one from cgroups.RemovePath,
as stopErr is most probably the reason why RemovePath failed.
Note that for v1 we do want to remove the paths even in case of a
failure from stopUnit, as some were not created by systemd.
There's no need to do that for v2, thanks to unified hierarchy,
so no changes there.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Sometimes Path() is called before m.path is initialized (in particular,
this happens from (*linuxContainer).newInitConfig), so we do need to
make sure to call initPath.
This fixes the following integration tests (for cgroup v2 + systemd case,
currently not enabled -- to be enabled by further commits):
* runc run (blkio weight)
* runc run (cgroupv2 mount inside container)
Fixes: ff692f289b ("Fix cgroup2 mount for rootless case")
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Using per cgroup manager dbus connection instances means
that every cgroup manager instance gets a new connection,
and those connections are never closed, ultimately resulting
in file descriptors limit being hit.
Revert back to using a single global dbus connection for everything,
without changing the callers.
NOTE that it is assumed a runtime can't use both root and rootless
dbus at the same time. If this happens, we panic.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 47ef9a104f forgot to wrap GetManagerProperty("ControlGroup")
into retryOnDisconnect. Since there's one other user of
GetManagerProperty, add getManagerProperty wrapper and use it.
Fixes: 47ef9a104f
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>
For some reason, systemd cgroup v2 driver's Set is not using its
container argument when generating systemd unit properties.
This bug is not detected by our update tests as we run a new binary
every time and thus a new instance of a cgroup manager.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Instead of reconnecting to dbus after some failed operations, and
returning an error (so a caller has to retry), reconnect AND retry
in place for all such operations.
This should fix issues caused by a stale dbus connection after e.g.
a dbus daemon restart.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
[@kolyshkin: doc nits, use dbus.ErrClosed and isDbusError]
Signed-off-by: Shiming Zhang <wzshiming@foxmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Generalize isUnitExists as isDbusError, and use errors.As while at it
(which can handle wrapped errors as well).
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>
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>
Aleksa Sarai