Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
10042b9 to
3a1dd2e
Compare
konstantin-s-bogom
left a comment
konstantin-s-bogom
left a comment
There was a problem hiding this comment.
The subtree holds only this pod's sentry and gofers, and the sentry
According to AI review, this is apparently not true for Docker. Prompts for fixing it resulted in some more complicated suggestions.
IMO since it's the last used lookup path, maybe you can just drop it.
|
thanks first time contributor, please sign CLA if you have not |
|
the request is valid iiuc, runc has the similar (or same actually) issue at containerd/containerd#12984, which i believe is still not fixed. |
milantracy
left a comment
milantracy
left a comment
There was a problem hiding this comment.
please see runc at https://github.com/containerd/containerd/blob/fcd43222d6b07379a4be9786bda52438f0dd16a1/pkg/oom/v2/v2.go#L88-L95
runc reads only the container's own cgroup, it doens't not read the paretn
Yeah that's the issue by the time shim tries to read the container's cgroup to determine if the sentry was OOM-killed, systemd had already removed the cgroup. Are you okay with reading the parent's cgroup for this scenario? |
3a1dd2e to
9051f6c
Compare
trantoji
changed the title
Nice find. |
Update the runsc-shim synchronous OOMKill checker to return exit code 137 and reason=OOMKilled in scenarios where the container's cgroup files have already been removed by systemd. Under the systemd cgroup driver, systemd removes the container's cgropu files as soon as its process dies, so the exit-time check (checkOOM) previously failed with ENOENT and left OOM-killed containers at exit code 128. checkOOM now consults witnesses in order: 1. Reads lastOOM map — if the async path already recorded the kill, return. 2. The container cgroup's own memory.events (unchanged, exact). 3. The parent cgroup (the pod slice under Kubernetes), which outlives the scope. Since Linux 5.2, memory.events is hierarchical, so the parent still counts kills from the removed scope. The count is baselined at add() so earlier kills in the pod are not misattributed. The EventChan error handler used to delete the watcher's cgroups and lastOOM entries. Those maps need to be alive when checkOOM runs, so they are removed later instead: checkOOM consumes the cgroups entry, and a new remove() clears both when containerd deletes the container. checkOOM also reports OOM detection separately from TaskOOM publishing, so the exit status is corrected even when the async watcher already announced the kill. Verified on a GKE 1.36 gVisor node pool (cgroup v2, systemd driver): an OOM-killed pod reports reason=OOMKilled with exitCode 137, with no stat-failure warnings. Assisted-by: Claude Code
9051f6c to
b5317ee
Compare
3 participants
Update the runsc-shim synchronous OOMKill checker to return exit code 137 and reason=OOMKilled in scenarios where the container's cgroup files have already been removed by systemd.
Under the systemd cgroup driver, systemd removes the container's transient scope as soon as its process dies, so the exit-time check (
isOOM) previously failed with ENOENT and left OOM-killed containers at exit code 128.isOOMnow consults witnesses in order:isOOM also now reports OOM detection separately from TaskOOM publishing, so the exit status is corrected even when the async watcher already announced the kill.
Verified on a GKE 1.36 gVisor node pool (cgroup v2, systemd driver): an OOM-killed pod reports reason=OOMKilled with exitCode 137, with no stat-failure warnings.
Assisted-by: Claude Code