libct: use os.Process.WithHandle - #5414
Draft
kolyshkin wants to merge 3 commits into
Draft
Conversation
kolyshkin
force-pushed
the
go-1.26-withhandle
branch
2 times, most recently
from
August 24, 2026 19:41
fa6e5ab to
92b4996
Compare
kolyshkin
changed the title
kolyshkin
marked this pull request as ready for review
August 24, 2026 19:55
kolyshkin
force-pushed
the
go-1.26-withhandle
branch
from
August 26, 2026 21:34
92b4996 to
16579f3
Compare
These unit tests are built on two mocks, mockCgroupManager and mockProcess, and the mocks cost more than the tests are worth. mockCgroupManager implements the entire cgroups.Manager interface, so oc/cgroups can not add a method to it without breaking the runc build (see opencontainers/cgroups#61). mockProcess is in the same position with respect to our own parentProcess interface: every method added there has to be added to the mock, which never does anything meaningful with it. The tests themselves do not pay for that: - TestGetContainerPids merely asserted that Container.Processes returns what the mock cgroup manager was configured to return. - TestGetContainerState reimplemented the namespace type to /proc file name mapping that it was meant to verify, so all it could detect was drift between the two copies. - TestGetContainerStateAfterUpdate round-tripped the container state through a real state directory, which is genuinely useful, but with the cgroup manager mocked out it could not tell whether Set did anything beyond rewriting state.json. Package coverage goes from 7.4% to 5.8%. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
waitForFifoReady used to call pidfd_open(2) on the init pid it was given.
When we are init's parent, though, os/exec already holds a pidfd for it,
so opening a second one by pid is both an extra syscall and a (small)
pid-reuse race: the pid is read from c.initProcess, and between that and
pidfd_open the process could in principle be reaped and its pid reused.
Add a withHandle method to the parentProcess interface, which hands out a
handle that is guaranteed to refer to the process for the duration of the
callback, and let each implementation provide it the best way it can:
- containerProcess and restoredProcess have an exec.Cmd, so they reuse
the handle os.Process already owns, with no extra syscall and no race;
- nonChildProcess (a container loaded from its state directory, e.g. by
"runc start" run as a separate process) has nothing but a pid, so it
has to look the process up. Do that once and keep the result, like
every other implementation does, rather than on every call.
The lookup is lazy because most things one can do with a loaded
container (state, list, ps, ...) never need it, and "runc list" loads
every container in the state directory, so taking an fd for each one
up front would be a waste at best.
Note this does not close the pid reuse window in signalInit(), which
calls hasInit() -- comparing the /proc/<pid> start time -- and only
then signals: with a lazy lookup, the handle is still acquired after
that check. Closing that window would mean taking the handle before
it, i.e. at load time, which is exactly the fd cost being avoided.
handleFifo now takes the parentProcess rather than a bare pid. Behavior is
otherwise unchanged: when no handle is available (kernel without pidfd
support, or init already reaped), we fall back to the same polling loop.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pidGetFd used to open its own pidfd for a process we are the parent of, and thus already have a handle for. Pass the parentProcess and use withHandle instead: one syscall less, and pidfd_getfd(2) can no longer land on a recycled pid. This does mean pidGetFd now fails if no handle is available. That is not a practical concern: pidfd_getfd(2) requires a 5.6 kernel, while a handle is available from 5.4 on, so any kernel that could have served the old code can serve this one. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kolyshkin
force-pushed
the
go-1.26-withhandle
branch
from
August 28, 2026 08:40
16579f3 to
5c1e0ae
Compare
kolyshkin
marked this pull request as draft
August 29, 2026 08:45
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
🗒️ Currently a draft as we want to merge #5431 first.
libct: remove container_linux_test.goReason for removal is discussed in #5382; TL;DR: those tests are mostly useless and stand in a way of oc/cgroups development. Supersedes #5382, which I am closing.
libct: use os.Process.WithHandle for the exec fifo waitSince we're on Go 1.26 minimum, we can use
os.Process.WithHandleto use existing os.Process and its pidfd rather than callingpidfd_open(2). When no handle is available we fall back to the same polling loop as before.libct: get the seccomp notify fd via the process handleSame idea:
pidGetFdopened its own pidfd for a process we are already the parent of.Notes for review
handleFifocomment used to say "kernels supporting pidfd_open(2) (>= 5.3)". I dropped the version, as the threshold is now Go's. We might lose the fast path on kernel 5.3 (only); not a big deal.pidGetFdnow fails outright without a handle. Not a practical concern:pidfd_getfd(2)needs 5.6, a handle is there from 5.4.nonChildProcesslookup is lazy, sincestate/list/psnever need it andrunc listloads every container in the state dir. The flip side: this does not close the pid-reuse window insignalInit(), which checkshasInit()and only then signals. Closing it means taking the handle at load time -- the fd cost being avoided here. Happy to flip if you prefer.pid(): cgroups and Intel RDT need the number forcgroup.procs/tasks, as do/proc/<pid>/...,sched_setaffinity(2),prlimit(2), CRIU, andstate.json.