Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a dedicated zygote process per container #828

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jprendes
Copy link
Collaborator

This PR creates a dedicated zygote process per container.
This fixes issues with libcontainer changing the shim process global state (like chdir), which leads to race conditions on multithreaded programs.
This also helps with throughput, as the global zygote process was a bottleneck (since it's single threaded).

TODO: Add tests

@Mossaka
Copy link
Member

Mossaka commented Jan 30, 2025

Great performance increase! I just ran a stress test on 1000 containers and with concurrency=32

Before
999 tasks succeeded, 0 tasks failed, 1 tasks didn't finish
elapsed time: 5s 898ms 226us 35ns

After
1000 tasks succeeded, 0 tasks failed, 0 tasks didn't finish
elapsed time: 2s 48ms 936us 565ns

@Mossaka
Copy link
Member

Mossaka commented Jan 30, 2025

Strangely, the labeler does not work for this PR.

This is the error msg:

An unknown config option was under C-containerd-shim-wasm: any-glob-to-any-file

Hope this PR could fix it: #829


Fixed it

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

crates/containerd-shim-wasm/src/sys/unix/container/container.rs:39

  • The error message 'Failed to obtain PID' could be more descriptive to help with debugging.
.context("Failed to obtain PID")

Copy link
Member

@Mossaka Mossaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change is worth a CHANGELOG mentioning how the perf is now different.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants