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

Provide a simplified API when using libcontainer #293

Merged
merged 2 commits into from
Sep 11, 2023

Conversation

jprendes
Copy link
Collaborator

@jprendes jprendes commented Sep 4, 2023

When using libcontainer many of the functionalities from the sandbox::Instance trait can be delegated to the actual OS container. This can be seen in the current implementation of the shims that build on top of libcontainer, where the same code pattern is seen in all of them.

This PR attempts to capture those patterns and provide a simplified API when wasm container is running inside of an OS container.

This PR introduces an Engine trait. This trait is very similar to libcontainer's Executor trait, but intends to be OS agnostic.
See the WasmEdgeEngine and WasmTimeEngine for reference implementations.

The name Engine is chosen because this type eventually becomes the sandbox::Instance::Engine type in the Instance implementation.

For reference, these are the changes to adopt this API in containerd-wasm-shim: deislabs/containerd-wasm-shims@main...jprendes:containerd-wasm-shims:runwasi-simple-api

This PR superseeds #250

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.

Did my first pass of the review and mostly LGTM. Left some minor comments.

Might need some coordination with PR #281

README.md Outdated Show resolved Hide resolved
crates/containerd-shim-wasm/src/container/context.rs Outdated Show resolved Hide resolved
crates/containerd-shim-wasm/src/container/context.rs Outdated Show resolved Hide resolved
@jprendes
Copy link
Collaborator Author

jprendes commented Sep 6, 2023

I did some small refactor to the RuntimeContext.

The RuntimeContext has a lot of functionalities to deal with env-vars and PATH, but by the time either can_handle or run_wasi get called, libcontianer already populated the env-vars with the correct values, pivoted the rootfs, and did the chdir. That means that we can use the functionalities in std instead of the RuntimeContext implementations.

Something missing from std is resolving things w.r.t. PATH, so I pulled tha tinto an utility PathResolve trait.

@jprendes jprendes force-pushed the simple-container-api branch 5 times, most recently from c728405 to 2234280 Compare September 6, 2023 11:44
@jprendes jprendes requested a review from Mossaka September 6, 2023 11:57
@jprendes jprendes force-pushed the simple-container-api branch 2 times, most recently from e0d7179 to 1ae4b23 Compare September 7, 2023 14:18
@Mossaka
Copy link
Member

Mossaka commented Sep 8, 2023

Need a rebase

Signed-off-by: Jorge Prendes <[email protected]>
@jprendes jprendes force-pushed the simple-container-api branch 2 times, most recently from 7eda9b0 to 75955a7 Compare September 9, 2023 00:15
@jprendes jprendes closed this Sep 9, 2023
@jprendes jprendes reopened this Sep 9, 2023
@jprendes
Copy link
Collaborator Author

jprendes commented Sep 9, 2023

Rebased. E2E failures seem to be sporadic, as re-running produces failures on a different set of nodes.

@jprendes jprendes closed this Sep 9, 2023
@jprendes jprendes reopened this Sep 9, 2023
@jprendes jprendes closed this Sep 9, 2023
@jprendes jprendes reopened this Sep 9, 2023
Mossaka
Mossaka previously approved these changes Sep 11, 2023
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.

lgtm!

README.md Outdated Show resolved Hide resolved
@Mossaka Mossaka merged commit 6a18a04 into containerd:main Sep 11, 2023
12 checks passed
Mossaka added a commit to Mossaka/shims that referenced this pull request Sep 11, 2023
This commits updates to use the latest API from the `containerd-shim-wasm` crate.
It follows the PR containerd/runwasi#293

Signed-off-by: jiaxiao zhou <[email protected]>
Mossaka added a commit to Mossaka/shims that referenced this pull request Sep 19, 2023
This commits updates to use the latest API from the `containerd-shim-wasm` crate.
It follows the PR containerd/runwasi#293

Signed-off-by: jiaxiao zhou <[email protected]>
Mossaka added a commit to Mossaka/shims that referenced this pull request Sep 21, 2023
This commits updates to use the latest API from the `containerd-shim-wasm` crate.
It follows the PR containerd/runwasi#293

Signed-off-by: jiaxiao zhou <[email protected]>
Mossaka added a commit to deislabs/containerd-wasm-shims that referenced this pull request Sep 21, 2023
* chore: use the Engine API from containerd-shim-wasm crate

This commits updates to use the latest API from the `containerd-shim-wasm` crate.
It follows the PR containerd/runwasi#293

Signed-off-by: jiaxiao zhou <[email protected]>
Co-authored-by: Jorge Prendes <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants