Suppport executing wasm workloads with wasmer #548
Conversation
Codecov Report
@@ Coverage Diff @@
## main #548 +/- ##
==========================================
+ Coverage 61.18% 69.77% +8.58%
==========================================
Files 85 84 -1
Lines 12553 10972 -1581
==========================================
- Hits 7681 7656 -25
+ Misses 4872 3316 -1556 |
|
This is really interesting. I'm wondering how you plan to distinguish between a container and a wasm workload, and how a high level runtime might integrate with this. This could be a really interesting alternative to something like Krustlet which could allow you to run wasm and container workloads on the same kubelet, whereas krustlet only supports wasm workloads on the hosts running it. |
|
@tsturzl This is based on the compat variant of the Wasm Image Specification. The config.json needs to have an annotation of either run.oci.handler or module.wasm.image/variant in order for us to detect that a wasm workload has be run. The linked spec also describes how to build an image that is compliant with the Wasm Image spec. Here is an example of how you can use it with CRI-O. |
Furisto
force-pushed
the
wasm-wasmer
branch
2 times, most recently
from
d4ca2cf
to
c99757a
Compare
Furisto
changed the title
|
Wow! This is a very wonderful feature. I'm sorry but as It is hard for me to understand it because I am not familiar with wasm, I need some time to review this PR. Please give me some time to review 🙇 |
|
I will also review in addition to @utam0k. This is a pretty big change so it's probably good to have a few eyes on it. This is awesome work though. |
There was a problem hiding this comment.
First of all this is really cool stuff, and it's awesome that you took the time and initiative to do this work. So thank you for that. I'm not entirely on board with using dynamic dispatch here. I think perhaps I'd consider refactoring to get rid of the CompositeExecutor and opt for a ExecutorManager that doesn't need to satisfy the Executor trait. This ExecutorManager would instead select hard coded Executors with a match block, and therefore only instantiate and allocate the Executor that is determined is actually needed. You can probably still use can_handle just don't have can_handle require a reference to self, in fact you can probably completely avoid the need to instantiate Executors and have all of the trait methods be static (no ref to self) just like we currently do with cgroups controllers, self is never used anyway (other than CompositeExecutor) and there's no point in instantiating these empty structs.
Let me know what you think of this. It's possible there's an argument for dynamic dispatch that I'm missing, or maybe others prefer that approach also. Even if we were to decide dynamic dispatch is best I'd say the CompositeExecutor should probably just be an ExecutorManager that doesn't satisfy the Executor trait and all Executors should just be static structs that never need to be instantiated.
| It also needs to specifiy a valid .wasm (webassembly binary) or .wat (webassembly test) module as entrypoint for the container. If a wat module is specified it will be compiled to a wasm module by youki before it is executed. The module also needs to be available in the root filesystem of the container obviously. | ||
|
|
||
|
|
||
| ```json |
There was a problem hiding this comment.
Should this tutorial be more in line with the crun example, or the example from wasm/spec-compat? They both seem to use buildah as a way to create a WASM image to run through a high level runtime. Maybe we could have both the youki only example and the example of how to build an image (or at least link to one)? I think the interest here will probably be running this through a high level runtime, whereas podman seems like a good option for the example. It would also be good to verify that this is working with some kind of high level runtime, and then give instructions on how to use this feature through that highlevel runtime.
There was a problem hiding this comment.
Overall, I'd say it's probably worth providing a couple of links to helpful references here. Like the links I posted in my comment, and some of the references you linked in the PR comments.
| fn name(&self) -> &str; | ||
| } | ||
| pub struct CompositeExecutor { | ||
| executors: Vec<Box<dyn Executor>>, |
There was a problem hiding this comment.
You probably don't need a Vector since it's statically sized, and you know the size. You could use an Array and set the size based on whether the wasm feature flag is present.
Also I'm not totally sure I agree with dynamic dispatch here. How many Executors are we actually going to add here? It might be worth saving the Box allocation. It might even be better to not have to load Executors you don't actually need.
|
|
||
| impl Executor for CompositeExecutor { | ||
| fn exec(&self, spec: &Spec) -> Result<()> { | ||
| for executor in &self.executors { |
There was a problem hiding this comment.
I think iterators would be more idiomatic and readable here.
There was a problem hiding this comment.
Mind you this loop may go away if we agree against dynamic dispatch.
| fn exec(&self, spec: &Spec) -> Result<()> { | ||
| for executor in &self.executors { | ||
| if executor | ||
| .can_handle(spec) |
There was a problem hiding this comment.
Again, I'm not sure I'm totally on board with dynamic dispatch here. It seems weird to loop through all the executors and settle on the first one that satisfies the spec, what if more than one Executor were to match at some point in the future? It also seems like if a number of parameters or spec properties go into selecting the Executor then you'd be better served by static control flow like a match block, and moving the can_run logic into the top level, and instead of having a CompositeExecutor, you'd have a ExecutorManager that probably doesn't even need to satisfy the Executor trait. You're also always allocating a copy of every Executor Youki was compiled with into heap. If you figured out what Executor you need before instantiating an Executor things would be more efficient.
Consider this for example: You have 3 regular container executors, and 3 WASM executors. In the dynamic dispatch approach your worst case is you check all 6. If you had a match block that could narrow down between knowing if the user wanted a "regular" or "wasm" executor, then you could narrow down to 4 checks as your worst case. Checking if it's wasm or regular, then checking the 3 for either. I suppose with dynamic dispatch you could handle that by having a regular and wasm composite Executor, but then you're still instantiating and allocating Executors you don't actually need.
There was a problem hiding this comment.
And of course, I'm not demanding the change, but perhaps it's worth the conversation. Maybe there's things I'm not considering or overlooking here, or maybe others find the dynamic dispatch approach to be nicer also.
| executors: Vec<Box<dyn Executor>>, | ||
| } | ||
|
|
||
| impl Executor for CompositeExecutor { |
There was a problem hiding this comment.
What's the benefit for having a CompositeExecutor? It seems the major benefit for an Executor is that is creates a common interface for Executors, could this maybe just be a ExecutorManager that doesn't need to satisfy Executor? I don't imagine we'll ever need the name function here, and that's just dead code.
| Ok(()) | ||
| } | ||
|
|
||
| fn can_handle(&self, spec: &Spec) -> Result<bool> { |
There was a problem hiding this comment.
In all the instances of can_handle Result isn't ever really needed, and always returns Ok. This seems like unnecessary indirection.
|
|
||
| pub trait Executor { | ||
| /// Executes the workload | ||
| fn exec(&self, spec: &Spec) -> Result<()>; |
There was a problem hiding this comment.
Do any of the functions for Executor actually need a reference to self? The CompositeExecutor is the only one that uses it, and it probably doesn't even need to satisfy this trait. If none of the Executors needed a &self, you'd also avoid the need to ever instantiate Executors, which aside from the CompositeExecutor are all empty structs.
|
@utam0k Could you also look through my review and provide your input? |
|
@Furisto |
@Furisto |
|
If so, may it be difficult to separate |
|
@tsturzl @utam0k Instead of responding to individual comments I think it is better for me to explain what my current thinking around this feature is and where I want to go with this. Quite a few webassembly runtimes exist nowadays with the most prominent ones being wasmer, wasmtime and wasmedge. These runtimes offer different features and make different tradeoffs (wasmedge for example currently does not support compiling .wat files while wasmer does). Therefore I plan to support different webassembly runtimes to accommodate different user requirements. It is likely that the current method of activating a handler using a feature flag will go away as I would like to avoid that everyone has to build youki with the appropriate configuration for their specific use case. Statically linking support for all webassembly runtimes would be problematic as this would bloat the size of the youki binary considerably (even with just wasmer the size of the binary roughly doubles) and would not be of use to anyone who is not wanting to run webassembly workloads (which is still the majority for the foreseeable future). Therefore dependencies will probably linked dynamically in the future. I am also thinking about allowing users to to specify the preferred order in which webassembly runtimes are asked to handle the workload in the config.json. It might even be possible that a configuration file located at a well known location can be provided that allows enables/disables certain handlers globally. This might be overkill though and we would want to check if other runtimes also plan to support such a feature. Lastly one intended use case for this will be running hooks in a webassembly sandbox, so hooks will have to be adapted as well in the future. What I want to say with this is: I would not focus to much on how exactly an executor is called because this is likely to change anyway. I would like to get this feature in first and then iterate on it with the knowledge gained from implementing support for further webassembly runtimes. I welcome comments on the general direction of this feature. |
Great thoughts 💯 #[cfg(not(feature = "wasm-wasmer"))]
fn choose_executor() -> Result<Box<dyn Executor>> {
Ok(Box::new(DefaultExecutor {}))
}
#[cfg(feature = "wasm-wasmer")]
fn choose_executor() -> Result<Box<dyn Executor>> {
if true {
Ok(Box::new(WasmerExecutor {}))
} else {
Ok(Box::new(DefaultExecutor {}))
}
} |
|
@tsturzl seems to be busy, so I will go ahead and merge this now. But don't worry, there will be a new opportunity in the next PR for this. |
|
Apologies for my absence. I've been on-boarding new hires at work. I did take a brief look through, and thought this looked fine. I'm excited to see the feature get merged in. I may hack on it a bit on my free time to familiarize myself with the code base. |
See docs