bump libcontainer to tip of tree #290
Conversation
yihuaf
commented
Aug 29, 2023
yihuaf
commented
- fixed all the api breakage
- implemented new executor validation logic
- fixed all the api breakage - implemented new executor validation logic Signed-off-by: yihuaf <[email protected]>
|
@jprendes PTAL. This turned out to be a little more code change (wider impact) than I anticipated. Let me know if you would like me to make any change. |
| fn can_handle(&self, spec: &Spec) -> bool { | ||
| // TODO: some of these logic is now implemented inside the default |
There was a problem hiding this comment.
We should address this TODO before merging.
There was a problem hiding this comment.
I think it's ok to completely remove this function now.
There was a problem hiding this comment.
There are some ELF magic number checking that is specific to runwasi. I decided to leave the function in for now and we can address the duplication in future PR.
|
One more question, what is the expected behavior of wasm executor when a regular OCI container is passed in? The current implementation will try to check if the container is a wasm container. If not wasm, then we use the default executor to validate or exec. Let me know if this assumption doesn't hold. |
if extension of the binary is incorrect. Signed-off-by: yihuaf <[email protected]>
Signed-off-by: yihuaf <[email protected]>
| fn name(&self) -> &'static str { | ||
| EXECUTOR_NAME | ||
| fn validate(&self, spec: &Spec) -> std::result::Result<(), ExecutorValidationError> { | ||
| match self.can_handle(spec) { |
There was a problem hiding this comment.
nit: I'm not a huge fan of doing this check twice, here and on exec. But it works.
I'll try to think if we can avoid it or not.
There was a problem hiding this comment.
This is one of the downside/trade off of this interface. In both valid and exec, we have to run the can_handle logic to figure out the correct exec. Also due to rust safety features, in exec we have to run a lot of the similar checks to return InvalidArg error. For example, if there is no process block in the spec, we have to check it again inside exec even though we already checked in the valid. It doesn't make sense for us to panic.
|
Shall I merge or are you working on pushing more commits? @yihuaf |
I have one more to address some of the nits and suggestion. I will let you know when this is ready. |
Signed-off-by: yihuaf <[email protected]>
|
@Mossaka Once CI is green, this is ready for merge. |
| }; | ||
| let path = PathBuf::from(module_name); | ||
| Err(ExecutorValidationError::CantHandle(_)) => { | ||
| LinuxContainerExecutor::new(self.stdio.clone()).exec(spec)?; |
There was a problem hiding this comment.
I realized this changes the behavior of previous runwasi's mechanism. Previously, wasmtime shim first checks if container is a Linux binary, and then check if it's a wasm container.
This logic seems to first check if container is a wasm binary, and then a Linux container.
There was a problem hiding this comment.
That's correct.
That's fine in the wasmedge and wasmtime cases where entrypoint should point to wasm files.
For spin/slight/wws the implementation of the executor should be somewhat different.
If you want take a look at the executor in #250, which keeps the old behaviour we has before this PR.
There was a problem hiding this comment.
I had a discussion with @cpuguy83 before, and we reached a conclusion that there isn't a robust way to check a wasm text file (wasm binary file could be checkd by verifying its magic number), thus the reason to check if a container is a Linux binary, or otherwise defaults to wasm / wat.
There was a problem hiding this comment.
I agree that the current check (i.e., the module has .wasm or .wat extension is not robust, but I expect it to work in most cases with wasmedge / wasmtime. But I do think we should find a better way.
For wasmedge / wasmtime, once the module is an OCI artifact the discussion will be different.
