-
Notifications
You must be signed in to change notification settings - Fork 95
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(wasmtime): support wasm components #401
Conversation
b2fbb24
to
beb52e9
Compare
Not needed as part of this but we will also need a way to compose together multiple components? |
Some(WasmBinaryType::Component) => { | ||
let component = Component::from_binary(&self.engine, &data)?; | ||
let file_perms = wasi_preview2::FilePerms::all(); | ||
let dir_perms = wasi_preview2::DirPerms::all(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is the way it's done today but in the case of the OCI layers I don't think we need these permissions. Any thoughts on how we can make this more configurable? It feels like this is where a OCI artifact with configuration that had info like this in it would come into play
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we need to figure out the story for this.
I can see the argument where te runtime provides an fs component with access to the container root, but you can choose to use your own fs component instead, and ignore/proxy the one provided by the runtime.
If we want to introduce config files as custom layers, it would be good to have a discussion with a wider group (wasi-wg, or even wasi-cg)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to introduce config files as custom layers, it would be good to have a discussion with a wider group (wasi-wg, or even wasi-cg)
They may have already thought of possible solutions, or many people even outside container (e.g., wasmtime itself) could have a similar pain point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to introduce config files as custom layers, it would be good to have a discussion with a wider group (wasi-wg, or even wasi-cg)
+1 to wider discussion, wasn't intending to figure it out on this PR just calling it out. I don't think it should be a custom layer either, that feels like its too open. I think this is more evidence that we really want a "real" "OCI artifact" with its own custom config media type that contains WASI specific meta data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Raised an issue here for further discussion: #413
beb52e9
to
eb299ac
Compare
This PR is ready to review. Could you PTAL? @jsturtevant ? |
I am looking for a full fleged wasip2 Command component for testing purposes, but failed to find any existing ones. Asked this question in the Zulipchat: https://bytecodealliance.zulipchat.com/#narrow/stream/219900-wasi/topic/Looking.20for.20a.20wasip2.20component.20for.20testing.20purposes/near/405901766 |
8418b5b
to
795f180
Compare
} | ||
Source::Oci([module]) => { | ||
log::info!("loading module wasm OCI layers"); | ||
module.layer.clone() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the clone required here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes because the function returns a Vec<u8>
but module is a &WasmLayer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was done by design to avoid coping data. Can this return &[u8] instead of vec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could roll back to the original one as I am not sure if there is a way to return &[u8] since the file is read in-function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by roll back to the original?
I see the problem, the Source::File
would create a temporary variable that can't be returned as borrowed. Maybe need to rethink the source, The read of the file could be moved to the call on entry point
and that would make it a non-local variable but this would have cascading changes. Another option might be to have a "intermediate step" that capture that value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The read of the file could be moved to the call on
entrypoint()
.
I would be a bit more careful here since that means a huge slowdown to entrypoint invocation, which is used in can_handle()
and is_linux_container()
functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted the code back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely looks cleaner than the reverted code, lets revisit this after this PR merges and see if can figure out a way to clean it up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup I will have a separate PR to clean that
let wasi_data = WasiData { | ||
wasi_preview1: wasi_preview1_ctx, | ||
wasi_preview2: wasi_preview2_ctx, | ||
wasi_preview2_table: wasi_preview2::Table::new(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we be going through the effort to build both of these contexts when it is only one type (either module/component)? If not, the build of the store happened insinde the execute_component
then this could become something like
fn prepare_wasi_ctx(
ctx: &impl RuntimeContext,
envs: Vec<(String, String)>,
type: wasmbinarytype,
) -> Result<WasiData, anyhow::Error> {
match type {
Some(WasmBinaryType::Module) => return data { wasi1_data}
Some(WasmBinaryType::Component) => return data {wasi2_data},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, but I think the current appraoch is nice because we only have one wasi_ctx type that contains both wasip1 and wasip2 APIs. This could potentially benefits a future case where we want to run wasm modules with wasip2 api.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could potentially benefits a future case where we want to run wasm modules with wasip2 api
what does this look like?
Added a new commit to support |
d21141a
to
5a133fb
Compare
5a133fb
to
9f65edb
Compare
Signed-off-by: jiaxiao zhou <[email protected]>
This commit adds a simple component testcase to the wasmtime shim in order to test the component model feature. It also refactors some of the common logic to the core crate. It adds a new module named "wasm.rs" to differentiate wasm module from wasm components. Signed-off-by: jiaxiao zhou <[email protected]>
Signed-off-by: jiaxiao zhou <[email protected]>
Signed-off-by: jiaxiao zhou <[email protected]>
Signed-off-by: jiaxiao zhou <[email protected]>
Signed-off-by: jiaxiao zhou <[email protected]>
Signed-off-by: jiaxiao zhou <[email protected]>
76192c4
to
208da25
Compare
Signed-off-by: jiaxiao zhou <[email protected]>
Signed-off-by: jiaxiao zhou <[email protected]>
Signed-off-by: jiaxiao zhou <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
needs some linter fixes |
Signed-off-by: jiaxiao zhou <[email protected]>
a1c1987
to
715b1bb
Compare
This PR adds wasm components support to the wasmtime shim which enables it with the
component-model
feature on. The component loading, linking and execution has a different set of APIs than the wasm module one. I usedwasmparser
to determine if a byte array is a wasm module or a wasm component, and then dispatch to two different paths.To finish this work completely, I hope to add some integration tests
As a follow up PR, I hope to add
wasmtime serve
functionality to the wasmtime shim so that we have HTTP capability for the shim.Closes #384