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

feat: Support wasmedge plugins #307

Merged
merged 1 commit into from
Oct 3, 2023

Conversation

CaptainVincent
Copy link
Contributor

@CaptainVincent CaptainVincent commented Sep 11, 2023

It's a feature PR

By the way, please note that this feature currently only functions when dynamically linking the WasmEdge library to the WasmEdge shim.

For building this, you can use the following command:
cargo build --package containerd-shim-wasmedge --no-default-features

In addition, I have provided all the WasmEdge runtime demo cases via containerd (here), including the wasinn plugin demo related to this feature. Considering that these demos may only be supported with the WasmEdge shim, I plan to relocate and maintain them under the WasmEdge or Second State Org's repository. At the same time, I will ensure that they run demos daily in CI and guarantee that these demos are always executed based on the latest upstream code.

@CaptainVincent CaptainVincent force-pushed the support-wasmedge-plugins branch 2 times, most recently from caff532 to 30ddec5 Compare September 11, 2023 08:26
@CaptainVincent CaptainVincent force-pushed the support-wasmedge-plugins branch from 30ddec5 to 901668c Compare September 11, 2023 20:13
@Mossaka Mossaka closed this Sep 12, 2023
@Mossaka Mossaka reopened this Sep 12, 2023
@CaptainVincent CaptainVincent force-pushed the support-wasmedge-plugins branch 2 times, most recently from 0216e3b to 4c4e9cf Compare September 15, 2023 04:07
@Mossaka Mossaka changed the title Support wasmedge plugins feat: Support wasmedge plugins Sep 18, 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.

Thanks! Added a few comments.

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

Where are the plugins supposed to be found?
In the host? Or in the container image?

If inside the container image, I don't see why we need to fiddle with the mount points, as libcontainer will have already handled that for us.
If it's in the host, by this point it's too late and the process doesn't have access to any path ouside the container root.

@CaptainVincent
Copy link
Contributor Author

Where are the plugins supposed to be found? In the host? Or in the container image?

If inside the container image, I don't see why we need to fiddle with the mount points, as libcontainer will have already handled that for us. If it's in the host, by this point it's too late and the process doesn't have access to any path ouside the container root.

The plugin is a .so file loaded through dlopen, and it is not specifically restricted to running within either the host or the container. As mentioned earlier, this mount aspect belongs to legacy code to handle filesystem for wasm, and I will remove it and modify the way my example is used.

@CaptainVincent CaptainVincent force-pushed the support-wasmedge-plugins branch 3 times, most recently from 19c7489 to 8080010 Compare September 20, 2023 15:09
@CaptainVincent CaptainVincent force-pushed the support-wasmedge-plugins branch from 8080010 to 8efdf4d Compare September 20, 2023 20:33
@CaptainVincent CaptainVincent marked this pull request as draft September 20, 2023 20:48
@CaptainVincent CaptainVincent marked this pull request as ready for review September 20, 2023 20:48
@CaptainVincent
Copy link
Contributor Author

CaptainVincent commented Sep 22, 2023

The statically linked version is also about to support plugin functionality, but the changes made only depend on the Wasmedge library. Therefore, after merging this PR, upstream will only need to await the release of the new SDK version to enable it in both ways.

Copy link
Collaborator

@jprendes jprendes left a comment

Choose a reason for hiding this comment

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

LGTM!

@jsturtevant
Copy link
Contributor

LGTM

Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

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

lgtm! Thank you, @CaptainVincent.

@devigned devigned merged commit 5e0dc8b into containerd:main Oct 3, 2023
77 of 78 checks passed
@CaptainVincent CaptainVincent deleted the support-wasmedge-plugins branch October 11, 2023 17:26
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.

5 participants