Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

crun delete: call systemd's reset-failed #1295

Merged
merged 1 commit into from
Sep 7, 2023

Conversation

kolyshkin
Copy link
Collaborator

According to the OCI runtime spec (https://github.com/opencontainers/runtime-spec/blob/main/runtime.md#delete), runtime's delete is supposed to remove all the container's artefacts.

In case systemd cgroup driver is used, and the systemd unit has failed (e.g. oom-killed), systemd won't remove the unit (that is, unless the "CollectMode: inactive-or-failed" property is set).

Leaving a leftover failed unit is a violation of runtime spec; in addition, a leftover unit result in inability to start a container with the same systemd unit name (such operation will fail with "unit already exists" error).

Call reset-failed from systemd's cgroup manager destroy_cgroup call, so the failed unit will be removed (by systemd) after "crun delete".

This change is similar to the one in runc (see opencontainers/runc#3888). A (slightly modified) test case from runc added by the above change was used to check that the bug is fixed.

For bigger picture, see:

To test manually, systemd >= 244 is needed. Create a container config that runs sleep 10 and has the following systemd annotations:

org.systemd.property.RuntimeMaxUSec: "uint64 2000000"
org.systemd.property.TimeoutStopUSec: "uint64 1000000"

Start a container using --systemd-cgroup option.

The container will be killed by systemd in 2 seconds, thus its systemd unit status will be "failed". Once it has failed, the systemctl status $UNIT_NAME should have exit code of 3 (meaning "unit is not active").

Now, run crun delete $CTID and repeat systemctl status $UNIT_NAME. It should result in exit code of 4 (meaning "no such unit").

According to the OCI runtime spec [1], runtime's delete is supposed to
remove all the container's artefacts.

In case systemd cgroup driver is used, and the systemd unit has failed
(e.g. oom-killed), systemd won't remove the unit (that is, unless the
"CollectMode: inactive-or-failed" property is set).

Leaving a leftover failed unit is a violation of runtime spec; in
addition, a leftover unit result in inability to start a container with
the same systemd unit name (such operation will fail with "unit already
exists" error).

Call reset-failed from systemd's cgroup manager destroy_cgroup call,
so the failed unit will be removed (by systemd) after "crun delete".

This change is similar to the one in runc (see [2]). A (slightly
modified) test case from runc added by the above change was used to
check that the bug is fixed.

For bigger picture, see [3] (issue A) and [4].

To test manually, systemd >= 244 is needed. Create a container config
that runs "sleep 10" and has the following systemd annotations:

	org.systemd.property.RuntimeMaxUSec: "uint64 2000000"
	org.systemd.property.TimeoutStopUSec: "uint64 1000000"

Start a container using --systemd-cgroup option.

The container will be killed by systemd in 2 seconds, thus its systemd
unit status will be "failed". Once it has failed, the "systemctl status
$UNIT_NAME" should have exit code of 3 (meaning "unit is not active").

Now, run "crun delete $CTID" and repeat "systemctl status $UNIT_NAME".
It should result in exit code of 4 (meaning "no such unit").

[1] https://github.com/opencontainers/runtime-spec/blob/main/runtime.md#delete
[2] opencontainers/runc#3888
[3] opencontainers/runc#3780
[4] cri-o/cri-o#7035

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin kolyshkin requested a review from giuseppe September 7, 2023 02:29
@kolyshkin
Copy link
Collaborator Author

kolyshkin commented Sep 7, 2023

Oh, and here's how to use runc test to test crun:

  1. Get runc repo:
$ git clone https://github.com/opencontainers/runc
$ cd runc
  1. Apply the following diff (required because runc recognizes that *Sec property argument needs to be uint64, see opencontainers/runc@1cd71df):
[kir@kir-rhat runc]$ git diff
diff --git a/tests/integration/delete.bats b/tests/integration/delete.bats
index adcca2c4..4363b976 100644
--- a/tests/integration/delete.bats
+++ b/tests/integration/delete.bats
@@ -175,8 +175,8 @@ EOF
        set_cgroups_path
        # shellcheck disable=SC2016
        update_config '   .annotations += {
-                               "org.systemd.property.RuntimeMaxSec": "2",
-                               "org.systemd.property.TimeoutStopSec": "1"
+                               "org.systemd.property.RuntimeMaxUSec": "uint64 2000000",
+                               "org.systemd.property.TimeoutStopUSec": "uint64 1000000"
                           }
                        | .process.args |= ["/bin/sleep", "10"]'
 
  1. Run the test:
$ sudo -s
# RUNC=/path/to/crun RUNC_USE_SYSTEMD=yes bats tests/integration/delete.bats

You will see an unrelated failure from a different test (which I'm going to fix). The main thing is, "runc delete removes failed systemd unit" should fail before and pass after the fix.

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@giuseppe giuseppe merged commit 9669989 into containers:main Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants