Skip to content

fix(ps): return all host-visible PIDs via /proc walk#731

Closed
Chennamma-Hotkar wants to merge 1 commit into
urunc-dev:mainfrom
Chennamma-Hotkar:fix/issue-637-ps-show-all-pids
Closed

fix(ps): return all host-visible PIDs via /proc walk#731
Chennamma-Hotkar wants to merge 1 commit into
urunc-dev:mainfrom
Chennamma-Hotkar:fix/issue-637-ps-show-all-pids

Conversation

@Chennamma-Hotkar

@Chennamma-Hotkar Chennamma-Hotkar commented May 30, 2026

Copy link
Copy Markdown
Contributor

Description

Previously, urunc ps only returned the sandbox monitor PID stored in state.json. This left VMM sub-processes (e.g. QEMU threads) and virtiofsd invisible to operators and container orchestrators, breaking cgroup accounting and process visibility.

This PR adds getAllDescendants() which recursively walks /proc/<pid>/task/<pid>/children to collect all host-visible PIDs
rooted at the monitor process. Since virtiofsd is spawned before syscall.Exec(), it naturally becomes a child of the monitor PID and is included in the walk automatically.

Related issues

How was this tested?

Unit tests added in cmd/urunc/ps_test.go:

  • TestGetAllDescendants_IncludesRoot — root PID is always present
  • TestGetAllDescendants_NonExistentPID — graceful handling of dead process
  • TestGetAllDescendants_IncludesChild — spawned child process is detected

All existing unit tests pass:
go test ./cmd/urunc/... ./pkg/unikontainers/... -short -timeout 60s

Test Results

Screenshot 2026-05-30 202629

LLM usage

N/A

Checklist

  • I have read the contribution guide.
  • The linter passes locally (make lint).
  • The e2e tests of at least one tool pass locally (make test_ctr, make test_nerdctl, make test_docker, make test_crictl).
  • If LLMs were used: I have read the llm policy.

Previously ps only returned the monitor PID. Now getAllDescendants()
recursively walks /proc/<pid>/task/<pid>/children to include all VMM
sub-processes and virtiofsd in the output.

Closes urunc-dev#637

Signed-off-by: Chennamma-Hotkar <channuhotkar@gmail.com>
@netlify

netlify Bot commented May 30, 2026

Copy link
Copy Markdown

Deploy Preview for urunc ready!

Name Link
🔨 Latest commit e53be60
🔍 Latest deploy log https://app.netlify.com/projects/urunc/deploys/6a1af961e1f27800087ba4cd
😎 Deploy Preview https://deploy-preview-731--urunc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@cmainas

cmainas commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Hello @Chennamma-Hotkar ,

thank you for the PR, but as discussed in the issue, trying to manually find all descendants would be hard with quite some unnecessary overhead. Instead, we can use cgroups when we finally add support for them.

@Chennamma-Hotkar

Chennamma-Hotkar commented Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Hi @cmainas, that makes sense - cgroups would be a much cleaner long-term solution for tracking all container processes without the /proc walk overhead.

Should I:

  1. Close this PR and wait for cgroup support to land first, or
  2. Keep this as a stop-gap fix until cgroups are implemented?

Happy to help with cgroup support if that's the direction the project wants to go - just let me know how I can contribute there.

@cmainas

cmainas commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

We are currently working in cgroup support. We can close this PR for the time being.

@Chennamma-Hotkar

Copy link
Copy Markdown
Contributor Author

Understood, thank you! I'll close this PR.

Would love to help with the cgroup support work if there's room to contribute. Is there an existing issue or PR I could look at to see where things stand?

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.

Display all process IDs in urunc ps

2 participants