[1.1] Fix systemd cgroup driver's Apply (and make CI green again) #3806
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Move error handling earlier, removing "if err == nil" block. No change of logic. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit c6e8cb7) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit d223e2a ("Ignore error when starting transient unit that already exists" modified the code handling errors from startUnit to ignore UnitExists error. Apparently it was done so that kubelet can create the same pod slice over and over without hitting an error (see [1]). While it works for a pod slice to ensure it exists, it is a gross bug to ignore UnitExists when creating a container. In this case, the container init PID won't be added to the systemd unit (and to the required cgroup), and as a result the container will successfully run in a current user cgroup, without any cgroup limits applied. So, fix the code to only ignore UnitExists if we're not adding a process to the systemd unit. This way, kubelet will keep working as is, but runc will refuse to create containers which are not placed into a requested cgroup. [1] opencontainers#1124 Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit c253342) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In case a systemd unit fails (for example, timed out or OOM-killed), systemd keeps the unit. This prevents starting a new container with the same systemd unit name. The fix is to call reset-failed in case UnitExists error is returned, and retry once. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> (cherry picked from commit 1d18743) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
As this is currently not possible to add a PID into an existing systemd unit, plus this feature will be deprected in runc 1.2 (see commit d08bc0c ("runc run: warn on non-empty cgroup"), let's reject sharing a systemd unit between two containers, and fix the test case accordingly. We still allow this to happen in case cgroupfs driver is used, to minimize the potential compatibility issues in a stable branch. This is an adaptation of main branch commit 82bc89c for 1.1. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
kolyshkin
added
area/ci
area/systemd
backport/1.1-pr
|
close/reopen to re-kick CI |
This was referenced Apr 3, 2023
|
CI failure in CS9 is a flake being fixed by commit 4d0a60c which we need to backport. CI restarted |
|
This makes CI green again, yay 👯 |
kolyshkin
requested review from
cyphar,
thaJeztah,
hqhq and
AkihiroSuda
and removed request for
cyphar
kolyshkin
changed the title
Merged
|
@AkihiroSuda @mrunalp PTAL |
1 similar comment
This comment was marked as duplicate.
This comment was marked as duplicate.
AkihiroSuda
approved these changes
Apr 5, 2023
mrunalp
approved these changes
Apr 5, 2023
This was referenced Apr 6, 2023
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a backport of #3782 for 1.1 branch. The difference is, here we still allow a container to share a cgroup with another container, but only when systemd cgroup driver is NOT used.
In case of systemd cgroup driver, which can't add a PID into an existing cgroup, we error out early.
The reasons behind this approach is described in #3782 (comment)
Fixes: #3780