Repository navigation
fix(sandbox): keep idle execs and disconnect freezes off the shared blocking pool - #5217
Conversation
|
/ok to test 4b43d56 |
mrunalp
added
area:sandbox
|
Label |
|
/ok to test 4b43d56 |
FrostGod
left a comment
FrostGod
left a comment
There was a problem hiding this comment.
Could we add a disconnect test with a full blocking pool and a setsid child? The current tests leave spare threads, so they miss this case.
4b43d56 to
2065ac7
Compare
|
/ok to test 2065ac7 |
…hreads Exec stdin/stdout/stderr and the PTY master were wrapped in tokio::fs::File, so every pending read on an idle pipe parked one tokio blocking-pool thread until data arrived or the peer closed. That pool is shared with the control plane (gRPC exchange, exec spawn/reap, AcceptNetwork), and enough idle exec sessions exhausted it, blocking new control connections. Adopt the descriptors with a readiness-based AsyncFd adapter instead: reads and writes only run when the descriptor is ready, and idle execs park no pool threads. Flush and shutdown keep the tokio::fs::File semantics the consumers were written against, and close-on-drop remains the EOF mechanism, so no consumer changes. An isolated boundary test caps the blocking pool at 16 threads, starts 12 idle execs, and asserts a fresh exec and a control request still complete; without the fix it fails when the sixth idle exec stalls. Signed-off-by: Emilien Macchi <emacchi@redhat.com>
transport_disconnected froze the workload while holding the supervisor_connection mutex, and the freeze signaled every descendant through up to four synchronous /proc scans, so losing the supervisor stalled a runtime worker and the lock for as long as the scans took, which grows with the number of processes in the sandbox. Freeze and resume now stop and continue the registered root groups synchronously, so a confirmed recovery still implies the roots were continued, and defer the descendant sweeps to a dedicated thread: the shared blocking pool can be fully occupied, and a sweep queued behind it would leave detached children running while the boundary reports itself frozen. If thread creation itself fails, the sweep runs synchronously rather than leave the freeze unenforced. Each sweep collects the owned set without holding any lock and applies the signal under a signal mutex only while the transition generation it captured is still current, so a stale deferred SIGSTOP can never land after a resume SIGCONT. A sweep that fails to deliver reports the gap as an OCSF process activity naming the recovery. The scans now run under neither supervisor_connection nor the signal mutex; a transition that immediately follows another waits at most one signal application, never a scan. Closes NVIDIA#5184 Signed-off-by: Emilien Macchi <emacchi@redhat.com>
Head branch was pushed to by a user without write access
2065ac7 to
a5a3934
Compare
|
Rebased onto main ( No conflicts, and both commits are byte-identical to before (range-diff clean). Clippy, fmt and the full The |
|
/ok to test a5a3934 |
|
All the red lanes on the latest run are the Nov 11 Azure DNS outage, not the PR. The vm-e2e and podman-e2e jobs all died in the apt setup step ("Temporary failure resolving 'azure.archive.ubuntu.com'") before any test ran, tmachine hit the same inside its Dockerfile, and Trivy/Core E2E result are just a cancelled twin run plus the aggregate of those. I can't re-run failed jobs myself without admin rights on the repo. Could you hit "Re-run failed jobs" on the Branch E2E Checks run for a5a3934? Everything else on that SHA is green. |
Summary
Exec pipes and PTY masters were wrapped in
tokio::fs::File, so every idle read parked a blocking-pool thread. That pool is shared with the control plane (gRPC exchange, exec spawn/reap,AcceptNetwork), so enough idle execs starved new control connections. Losing the supervisor had a related problem: the freeze ran up to four synchronous/procscans while holding thesupervisor_connectionmutex, which stalled a runtime worker and the lock.Related Issue
Closes #5184.
Changes
AsyncFdadapter replacestokio::fs::Filefor exec stdin/stdout/stderr and the PTY master; idle execs park no pool threads. Flush and shutdown keep the semantics consumers were written against, so nothing downstream changes./procsweeps to a dedicated thread, so a full blocking pool cannot leave detached children running behind a boundary that reports itself frozen. A transition generation checked under a signal mutex drops stale batches, so a deferredSIGSTOPcan never land after a resumeSIGCONT. Terminal transitions keep their immediate group signaling and run their scans outside the mutex./procscan.Release Notes
Testing
-D warnings, fmt, the fullopenshell-sandboxsuite)e2e:podman:ci(38 tests) passes;openshell-sandbox-backendandopenshell-supervisor-processsuites passChecklist