I was unable to add integration tests for this PR without resorting to some hacky methods, but I tested whether this issue was resolved in the kubernetes-sigs/kind repository.

In brief, I discovered this issue while working in the kubernetes/kubernetes repo to propagate kubelet's context to the container runtime. The issue manifested as the test job being unable to tear down after the k/k repo's e2e tests completed, because the leaked runc init process and its corresponding systemd scope prevented systemd from shutting down.

Therefore, I opened a PR in the kubernetes-sigs/kind repo to debug this issue by manually replacing the containerd/runc binaries in the CI environment. After building the code from this PR and replacing the binaries in the CI environment, the test job no longer failed to tear down due to systemd being unable to shut down, as the leaked processes were resolved.

Ref: kubernetes-sigs/kind#3903 (Some job failures occurred due to the instability of the k/k repo e2e tests, but they are unrelated to this issue.)

I also conducted some manual tests targeting the scenarios where the leftover container is in the paused and stopped states.

Paused:

diff --git a/vendor/github.com/opencontainers/cgroups/systemd/v1.go b/vendor/github.com/opencontainers/cgroups/systemd/v1.go
index 8453e9b4..bbe3524c 100644
--- a/vendor/github.com/opencontainers/cgroups/systemd/v1.go
+++ b/vendor/github.com/opencontainers/cgroups/systemd/v1.go
@@ -6,6 +6,7 @@ import (
        "path/filepath"
        "strings"
        "sync"
+       "time"

        systemdDbus "github.com/coreos/go-systemd/v22/dbus"
        "github.com/sirupsen/logrus"
@@ -361,6 +362,7 @@ func (m *LegacyManager) Set(r *cgroups.Resources) error {
                }
        }
        setErr := setUnitProperties(m.dbus, unitName, properties...)
+       time.Sleep(time.Second * 30)
        if needsThaw {
                if err := m.doFreeze(cgroups.Thawed); err != nil {
                        logrus.Infof("thaw container after SetUnitProperties failed: %v", err)

1. Create a container:
./runc --systemd-cgroup create mycontainer

2. Check container processes:
ps -ef | grep runc
root        2944     694  0 15:36 pts/2    00:00:00 ./runc --systemd-cgroup create mycontainer
root        2956    2944  0 15:36 ?        00:00:00 ./runc init
root        2963     688  0 15:36 pts/1    00:00:00 grep runc

3. Kill the runc create process:
kill -9 2944

4. Check if the runc init process is left behind:
ps -ef | grep runc
root        2956       1  0 15:36 ?        00:00:00 ./runc init
root        2965     688  0 15:37 pts/1    00:00:00 grep runc

5. Check the current container state:
./runc list
ID            PID         STATUS      BUNDLE              CREATED                OWNER
mycontainer   2953        paused      /root/mycontainer   0001-01-01T00:00:00Z   root

6. Delete the container:
./runc delete -f mycontainer
writing sync procError: write sync: broken pipe
EOF

7. Verify if the runc init process has been cleaned up:
ps -ef | grep runc
root        3067     688  0 15:39 pts/1    00:00:00 grep runc

stopped:

diff --git a/libcontainer/process_linux.go b/libcontainer/process_linux.go
index 96e3ca5f..350e3660 100644
--- a/libcontainer/process_linux.go
+++ b/libcontainer/process_linux.go
@@ -613,6 +613,7 @@ func (p *initProcess) start() (retErr error) {
                        return fmt.Errorf("unable to apply cgroup configuration: %w", err)
                }
        }
+         time.Sleep(time.Second * 30)
        if p.intelRdtManager != nil {
                if err := p.intelRdtManager.Apply(p.pid()); err != nil {
                        return fmt.Errorf("unable to apply Intel RDT configuration: %w", err)

1. Create a container:
./runc --systemd-cgroup create mycontainer

2. Check container processes:
ps -ef | grep runc
root        3124     694  0 15:45 pts/2    00:00:00 ./runc --systemd-cgroup create mycontainer
root        3132    3124  0 15:45 pts/2    00:00:00 ./runc init
root        3140     688  0 15:45 pts/1    00:00:00 grep runc

3. Kill the runc create process:
kill -9 3124

4. Check if the runc init process is left behind (There will be no runc init process left behind):
ps -ef | grep runc
root        3142     688  0 15:45 pts/1    00:00:00 grep runc

5. Check the current container state:
./runc list
ID            PID         STATUS      BUNDLE              CREATED                OWNER
mycontainer   0           stopped     /root/mycontainer   0001-01-01T00:00:00Z   root

6. Delete the container:
./runc delete -f mycontainer

/cc @kolyshkin @AkihiroSuda @rata

See also: #2575

@HirazawaUi thanks! So my comment was on-spot, but you didn't need to remove the assignment?

For testing, I'd like to have something. It should be simple and kind of reliable. Here are some ideas, but we don't need a test if we don't find a reasonable and simple way to test this:

• I wonder if creating a PID namespace with a low limit can emulate it (only if it's simple, I guess it is?). We can then increase the limit and call runc delete to see it is deleted correctly?
• Or maybe we can use fanotify, to block some operation and send a SIGKILL at that point?
• Or maybe in unit tests, we can maybe override the start() function and create a process with the API that will be blocked there, before the state file is created?

but you didn't need to remove the assignment?

I believe that removing this assignment and delaying the assignment process until after updateState is pointless. Regardless of whether it is removed here, the container will enter the stopped state if the creation process is interrupted before the cgroup is frozen, and stoppedState.destroy() can properly clean up residual files in this scenario.
ref:

• I wonder if creating a PID namespace with a low limit can emulate it (only if it's simple, I guess it is?). We can then increase the limit and call runc delete to see it is deleted correctly?
• Or maybe we can use fanotify, to block some operation and send a SIGKILL at that point?
• Or maybe in unit tests, we can maybe override the start() function and create a process with the API that will be blocked there, before the state file is created?

I will try testing it in the direction of Suggestion 2 (it seems the most effective). If it cannot be implemented, I will promptly provide feedback here :)

Test case has been added.

While attempting to use fanotify to monitor the open events of state.json and terminate the runc create process upon detecting an open event, I suddenly realized a blind spot I had never considered before: why not try running runc create and then sending it SIGKILL signal to terminate it within a very short time frame?

Compared to event monitoring, this approach better aligns with the scenario we encountered and is completely asynchronous. The only downside seems to be its fragility, but I added numerous device rules to slow down cgroup creation and restricted it to cgroup v1 only, which reduces the likelihood of errors (in my last code submission, all tests performed well with no errors).

@rata Do you think this test case sufficiently covers the scenarios for this PR?

ping @kolyshkin @AkihiroSuda @rata Could you take another look at this PR? Any feedback would be greatly appreciated.

Encountered errors during rebase, investigating...

@HirazawaUi you need to sing-off the commits before we merge. Can you do that?

ou need to sing-off the commits before we merge. Can you do that?

Thanks for the reminder, signed.

Oh, @lifubang requested changes, although he wrote to feel free to ignore if we want to merge the test, that is why I wanted to do that. @HirazawaUi Can you remove the test, then? That way we can easily merge now.

You can open another PR with the test, if you want. Although I feel we need to explore more options to have a reliable test (and not sure it's worth it? Maybe it is). Something that might work is using seccomp notify and make it hang in some syscall, but it is also fragile. Maybe if we use a rare syscall, only when compiled with some build tags (like the access syscall), and then compile and run runc like that for the tests. The test needs more thought, definitely :)

@HirazawaUi Can you remove the test, then? That way we can easily merge now.

Removed, while I'd prefer to keep it given the considerable effort invested in designing and implementing this testing approach, I respect the consensus to remove it. Perhaps the journey of exploration matters more than the outcome itself.

@lifubang PTAL

Oh, yeah, I can dismiss the review but let's just wait for @lifubang to take another look...

@HirazawaUi Would you like this change to be backported to release-1.3?

Would you like this change to be backported to release-1.3?

I'd be very happy to backport this to still maintained older releases, I'll do this tomorrow :)