Conversation
95f3f1b to
c2489eb
Compare
98b9acc to
5644242
Compare
|
This PR is ready for review. Please take a look when you have a moment. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for libkrun to youki, enabling containers to run inside lightweight KVM-based virtual machines. The implementation adds a new executor that interfaces with the libkrun library to provide VM-based container isolation.
- Introduces a new libkrun executor with VM configuration and KVM device management
- Modifies the executor architecture to support pre-execution hooks for VM setup
- Adds feature flag support for optional libkrun functionality
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/youki/src/workload/mod.rs | Adds conditional module import for libkrun support |
| crates/youki/src/workload/libkrun.rs | New module implementing the libkrun executor with VM management |
| crates/youki/src/workload/executor.rs | Updates DefaultExecutor to include libkrun support and pre-execution hooks |
| crates/youki/Cargo.toml | Adds libkrun feature flag and required dependencies |
| crates/libcontainer/src/workload/mod.rs | Adds pre_exec trait method to Executor interface |
| crates/libcontainer/src/workload/default.rs | Implements default pre_exec method |
| crates/libcontainer/src/error.rs | Adds ExecutorError to LibcontainerError enum |
| crates/libcontainer/src/container/init_builder.rs | Integrates pre_exec call into container building process |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
crates/youki/src/workload/libkrun.rs
Outdated
| use nix::sys::stat::{major, minor, stat, Mode}; | ||
|
|
||
| const EXECUTOR_NAME: &str = "libkrun"; | ||
| const LIBKRUN_NAME: &str = "libkrun.so.1"; |
There was a problem hiding this comment.
The hardcoded library name 'libkrun.so.1' may not be portable across different systems. Consider making this configurable or using a more flexible library loading approach that can handle different versions.
There was a problem hiding this comment.
+1. How about using the annotation and default value?
There was a problem hiding this comment.
I modified it to use annotations and default value.
| krun.set_root(ctx_id, &root)?; | ||
|
|
||
| let res = krun.start_enter(ctx_id)?; | ||
| std::process::exit(res) |
There was a problem hiding this comment.
Direct call to std::process::exit() makes the code difficult to test and can cause issues in library contexts. Consider returning the exit code and letting the caller handle the exit.
| fn pre_exec(&self, spec: Spec) -> Result<Spec, ExecutorError> { | ||
| #[cfg(feature = "libkrun")] | ||
| { | ||
| let res = self.libkrun.pre_exec(spec.clone()); |
There was a problem hiding this comment.
Cloning the entire spec can be expensive for large specifications. Consider using references or only cloning when necessary based on the libkrun executor's needs.
| let res = self.libkrun.pre_exec(spec.clone()); | |
| let res = self.libkrun.pre_exec(&spec); |
crates/youki/Cargo.toml
Outdated
| tracing-subscriber = { version = "0.3.20", features = ["json", "env-filter"] } | ||
| tracing-journald = "0.3.1" | ||
| libloading = { version = "0.8", optional = true } | ||
| thiserror = { version ="2.0.16", optional = true } |
There was a problem hiding this comment.
Missing space after the equals sign. Should be 'version = "2.0.16"' for consistency with other dependencies.
| @@ -1,4 +1,6 @@ | |||
| pub mod executor; | |||
| #[cfg(feature = "libkrun")] | |||
There was a problem hiding this comment.
Do we really need the feature flag? The libkrun feature has already been protected by the annotation to prevent accidental running. And I don't think this feature will increase the binary and memory footprint size, right?(We should measure that.)
There was a problem hiding this comment.
Do we need the additional dependency to run or build youki?
There was a problem hiding this comment.
I added libloading to call (dynamically load) the libkrun library. With the current code, the resulting binary sizes were:
- youki (release):6.2 MB
- youki + libkrun: 6.4 MB
- (youki + WasmEdge): 111 MB
If the differences above aren’t an issue, I’ll remove the features flag.
78eea37 to
b91b3f2
Compare
utam0k
left a comment
There was a problem hiding this comment.
I ran out of time today, but I've left a few comments on things that caught my attention.
crates/youki/src/workload/libkrun.rs
Outdated
| } | ||
|
|
||
| // Add /dev/kvm to `linux.devices` so libkrun can access KVM device. | ||
| pub fn modify_spec_device(spec: &mut Spec) -> Result<(), KrunError> { |
There was a problem hiding this comment.
Let's make the name a bit more precise. It's too abstract. What this function does is exactly as described in the explanation just above it.
crates/youki/src/workload/libkrun.rs
Outdated
|
|
||
| // Add an allow rule for /dev/kvm to linux.resources.devices | ||
| // if resources.devices is None or empty, it's effectively permissive, so skip. | ||
| pub fn modify_spec_resource_device(spec: &mut Spec) -> Result<(), KrunError> { |
There was a problem hiding this comment.
Let's make the name a bit more precise. It's too abstract. What this function does is exactly as described in the explanation just above it.
|
|
||
| pub trait Executor: CloneBoxExecutor { | ||
| /// Pre Executes the workload | ||
| fn pre_exec(&self, spec: Spec) -> Result<Spec, ExecutorError> { |
There was a problem hiding this comment.
I've reconsidered, and including exec could be misleading.
Also, we need to clarify for the library users:
- a: When is it called?
- b: Where are the methods called from?
'a' is already expressed in the method name. What about 'b'?
My proposal is to separate the trait based on whether it's within a container or on the host. For example:
trait HostExecutor {
fn modify_spec(&self, spec: Spec) -> Result<Spec, ExecutorError> {
Ok(spec)
}
}
trait ContainerExecutor {
fn exec(&self, spec: &Spec) -> Result<(), ExecutorError>;
fn validate(&self, spec: &Spec) -> Result<(), ExecutorValidationError>;
}
trait Executor: HostExecutor + ContainerExecutor + CloneBoxExecutor {}There was a problem hiding this comment.
Thank you for the review.
If we follow the approach above, do we need to split the existing executors that implement Executor (e.g., DefaultExecutor, WasmedgeExecutor, etc.) into HostExecutor and ContainerExecutor implementations?
There was a problem hiding this comment.
We need it. But, I'd like to get other @youki-dev/youki's opinion.
There was a problem hiding this comment.
I split it into HostExecutor and ContainerExecutor.
As a result, I updated the other executors as well (e.g., DefaultExecutor, WasmedgeExecutor, etc.). I confirmed that the build succeeds.
| let (kvm_major, kvm_minor) = match stat_dev_numbers("/dev/kvm") { | ||
| Ok(v) => v, | ||
| Err(e) if e.kind() == io::ErrorKind::NotFound => return Ok(()), | ||
| Err(e) => return Err(KrunError::Other(format!("stat `/dev/kvm`: {e}"))), | ||
| }; |
There was a problem hiding this comment.
How about adding a validation to return as early as possible?
There was a problem hiding this comment.
I updated it so that validate_kvm function is executed inside modify_spec to check whether KVM is available.
crates/youki/src/workload/libkrun.rs
Outdated
| // Add an allow rule for /dev/kvm to linux.resources.devices | ||
| // if resources.devices is None or empty, it's effectively permissive, so skip. | ||
| pub fn modify_spec_resource_device(spec: &mut Spec) -> Result<(), KrunError> { | ||
| let mut linux = match spec.linux() { |
There was a problem hiding this comment.
Please use mutating getters. Everything else is the same.
There was a problem hiding this comment.
I updated the code to use mutating getters.
|
Could you please write the user documentation for this feature? |
Signed-off-by: Yusuke Sakurai <yusuke.sakurai@3-shake.com>
| let (kvm_major, kvm_minor) = match stat_dev_numbers("/dev/kvm") { | ||
| Ok(v) => v, | ||
| Err(e) if e.kind() == io::ErrorKind::NotFound => return Ok(()), | ||
| Err(e) => return Err(KrunError::Other(format!("stat `/dev/kvm`: {e}"))), | ||
| }; |
There was a problem hiding this comment.
I updated it so that validate_kvm function is executed inside modify_spec to check whether KVM is available.
| // see: https://github.com/containers/crun/blob/92977c0fc843e4649fe4611a97ba12b06cb5073f/src/libcrun/handlers/krun.c#L514C1-L515C72 | ||
| pub fn write_krun_config(rootfs: &Path, json_spec: &str) -> Result<(), KrunError> { | ||
| let mut root = | ||
| Root::open(rootfs).map_err(|e| KrunError::Other(format!("failed to open rootfs {e}")))?; |
There was a problem hiding this comment.
I changed this implementation to use libpathrs.
I added documentation and believe I’ve addressed the items raised in the review. Sorry for the trouble, but I’d appreciate it if you could take another look. |
| @@ -0,0 +1,97 @@ | |||
| # Libkrun | |||
There was a problem hiding this comment.
Technically, it seems to be in lowercase. Also, isn't it more of a VM than libkrun?
https://github.com/containers/libkrun
| # Libkrun | |
| # VM Support(libkrun) |
| @@ -0,0 +1,97 @@ | |||
| # Libkrun | |||
|
|
|||
| youki can launch microVM-based containers by dynamically loading the libkrun shared library. Here, we describe how to launch a container with youki using libkrun. | |||
There was a problem hiding this comment.
What's microVM-based containers? It'd be better to put one big picture (using Excalidraw?)
|
|
||
| ## Prepare KVM on the host | ||
|
|
||
| Libkrun uses KVM to start a microVM. |
There was a problem hiding this comment.
| Libkrun uses KVM to start a microVM. | |
| libkrun uses KVM to start a microVM. |
| LD_LIBRARY_PATH=/usr/local/lib64 youki run -b tutorial container | ||
| ``` | ||
|
|
||
| ### Keys |
|
|
||
| ## Install libkrun and libkrunfw | ||
|
|
||
| To use libkrun with youki, you need to install libkrun and libkrunfw in advance. |
There was a problem hiding this comment.
| To use libkrun with youki, you need to install libkrun and libkrunfw in advance. | |
| To use youki with libkrun, you need to install libkrun and libkrunfw in advance. |
| ```bash | ||
| ls /usr/local/lib64 | ||
| libkrun.so libkrun.so.1 libkrun.so.1.17.0 libkrunfw.so libkrunfw.so.5 libkrunfw.so.5.1.0 pkgconfig | ||
| ``` |
There was a problem hiding this comment.
Should the versions be specified?
| } | ||
|
|
||
| pub trait Executor: CloneBoxExecutor { | ||
| pub trait HostExecutor { |
There was a problem hiding this comment.
Could you write the description and why these traits are separated?
| } | ||
|
|
||
| impl Krun { | ||
| fn load(libkrun_path: String) -> Result<Self, ExecutorError> { |
|
|
||
| #[derive(Clone)] | ||
| pub struct LibkrunExecutor { | ||
| lib: Rc<OnceCell<Rc<Krun>>>, |
There was a problem hiding this comment.
Do we really Rc and OnceCell? I'd like to avoid them if possible.
Description
Add support libkrun
See here for more details.: https://gist.github.com/saku3/7594fe4d2bd1a8d0edbb8b1d0e2e255c
How to
Build
Run
Assuming libkrun and libkrunfw are installed.
Impact Scope
libcontainer
ContainerBuilder::buildinvokespre_exec.Executor
execruns only when launching libkrun-based containers, so there’s no impact on others.lib,ctx_id), we adjusted DefaultExecutor to reuse a single executor instance (callingget_executorrepeatedly would re-initialize state).sequenceDiagram participant youki participant Main Process participant Intermediate Process participant Init Process participant Executor participant libkrun participant kvm Note over youki: load config.json rect rgb(235,250,235) youki->>Executor: pre_exec() opt feature "libkrun" Executor->>libkrun: load libkrun.so Executor->>libkrun: krun_create_ctx() Note over Executor: modify config.json Note over Executor: setup .krun_config.json end end youki->>Main Process: container_main_process() Main Process->>Intermediate Process: clone3 Intermediate Process->>Init Process: clone3 Note over Init Process: pivot_root Init Process->>Executor: exec() rect rgb(235,250,235) opt feature "libkrun" Executor->>libkrun: krun_set_vm_config() Executor->>libkrun: krun_set_root() Executor->>libkrun: krun_set_log_level(1) Executor->>libkrun: krun_start_enter() libkrun->>kvm: start end endType of Change
Testing
Related Issues
Part of # #3196
Additional Context