runtime-rs: firecracker hypervisor backend #8070

Pyrromanis · 2023-09-26T19:36:09Z

Add a basic runtime-rs Hypervisor trait implementation for Firecracker (FC).

implement fc-api requests to setup and start an FC sandbox
account for jailer directory hacks
add separate config for runtime-rs (might need to change as there are only minor diffs with the go config)

Notes:

only devmapper is currently supported
no sharefs functionality, copy files in the sandbox
nerdctl spawn is broken (TODO: runtime: run prestart hooks before starting VM for FC #7703)
pending FC-specific doc

Fixes: #5268

katacontainersbot · 2023-09-26T19:39:08Z

Can one of the admins verify this patch?

ananos · 2023-09-26T20:14:52Z

/test

ananos · 2023-09-27T14:01:34Z

/test

ananos · 2023-10-04T11:46:25Z

/test

ananos · 2023-10-06T17:39:40Z

/test

pmores · 2024-01-15T17:17:19Z

src/runtime-rs/crates/runtimes/virt_container/src/sandbox.rs

@@ -570,9 +572,31 @@ impl Persist for VirtSandbox {
        let sandbox_state = crate::sandbox_persist::SandboxState {
            sandbox_type: VIRTCONTAINER.to_string(),
            resource: Some(self.resource_manager.save().await?),
-            hypervisor: Some(self.hypervisor.save_state().await?),
+            //hypervisor: Some(self.hypervisor.save_state().await?),


This line could probably be removed, right?

ack, will clean up once we get the correct implementation on this snippet.

pmores · 2024-01-15T17:23:41Z

src/runtime-rs/crates/runtimes/virt_container/src/sandbox.rs

+            hypervisor: match self.hypervisor.save_state().await?.hypervisor_type.as_str() {
+                // TODO support other hypervisors
+                HYPERVISOR_DRAGONBALL => Ok(Some(self.hypervisor.save_state().await?)),
+                HYPERVISOR_FIRECRACKER => Ok(Some(self.hypervisor.save_state().await?)),


This looks odd to me, why is save_state() called again? If the purpose of the first call (in the match expression above) is only to get the hypervisor type, isn't there a more direct way to do that?

exactly! maybe we should call save state earlier and keep it as a variable to pass it on there. Not sure, what do you think?

On a closer look, the Hypervisor interface doesn't seem to make it possible to get hypervisor_type directly, interestingly enough. So short of adding a Hypervisor::hypervisor_type() function to the interface, I can't think of a better solution at the moment than what you suggest - call save_state() ahead of the match expression, then match on the state and return it.

ACK, thanks!

pmores · 2024-01-30T10:22:30Z

src/runtime-rs/crates/runtimes/src/manager.rs

@@ -597,3 +601,18 @@ fn update_component_log_level(config: &TomlConfig) {
        updated_inner
    });
 }
+
+fn determine_jailed(config: &TomlConfig) -> Result<String> {
+    let Some(hypervisor_config) = config.hypervisor.get(&config.runtime.hypervisor_name) else {


Does this compile? It kind of looks like if let was meant but the following code seems to contradict that.

It seems like a rust idiom: https://doc.rust-lang.org/rust-by-example/flow_control/let_else.html

Oh I missed that addition to the language, thanks! :-)

pmores · 2024-01-30T10:26:28Z

src/runtime-rs/crates/runtimes/src/manager.rs

+
+fn determine_jailed(config: &TomlConfig) -> Result<String> {
+    let Some(hypervisor_config) = config.hypervisor.get(&config.runtime.hypervisor_name) else {
+        todo!()


There seems to be uncertainty how to handle this, however (assuming I'm guessing the intent of the previous line correctly) this block will only execute if hypervisor_name points to an unconfigured hypervisor, right? If so then that would clearly be a major misconfiguration, warranting a return with an Err. Or am I misunderstanding completely?

correct -- AFAIU todo!() will raise an error. If not, we should definitely raise an error there.

I believe returning an Err here (probably with an empty string but that doesn't seem to matter much) would be better here than todo!() since todo!() basically just panics whereas with an Err return we can hope for a nice error stack message.

If this block gets executed it means that user pointed runtime.hypervisor_name to a hypervisor they haven't configured at all. There's no possible recovery from this kind of a gross configuration error as far as I can see, and failing with a nice informative error message in the log is all we can hope for here IMHO.

pmores · 2024-01-30T11:26:01Z

src/runtime-rs/crates/runtimes/virt_container/src/sandbox.rs

+        // jailed_path = h.<>
+        // and somehow store the sandbox state into the jail:
+        // persist::to_disk(&sandbox_state, &self.sid, jailed_path)?;
+        // Issue is, how to handle restore.


Do I get the issue right that it's the chicken-egg problem where in order to restore a hypervisor you need to get the corresponding jailed path, for which you need the very same hypervisor you're trying to restore?

again, correct -- and we don't like the way we've implemented that (at all...) so we're open to suggestions about what to do here ...

Honestly I don't know if there can even be any other way apart from either 1) not saving the state to the jailed path, or 2) store the jailed path also somewhere outside of the state (which would require building something of a registry of sandbox state paths...).

lifupan · 2024-06-19T01:53:59Z

src/libs/kata-types/src/config/hypervisor/firecracker.rs

+            }
+            if firecracker.boot_info.image.is_empty() {
+                return Err(eother!(
+                    "Both guest boot image and initrd for firecracker are empty"


It seemed firecracker didn't support initrd, am I right?

actually it does, although we haven't added that in kata just yet ;-)

lifupan · 2024-06-19T02:05:25Z

src/libs/kata-types/src/config/hypervisor/firecracker.rs

+
+            if (firecracker.cpu_info.default_vcpus > 0
+                && firecracker.cpu_info.default_vcpus as u32 > default::MAX_FIRECRACKER_VCPUS)
+                || firecracker.cpu_info.default_maxvcpus > default::MAX_FIRECRACKER_VCPUS


firecracker didn't support cpu hotplug, thus there's no need to check the default_maxvcpus.

the rationale here was to make sure that no vCPU setting is above the maximum number of vCPUs supported by the hypervisor. But I agree, let's just ignore the default_maxvcpus setting, as it is not used.

lifupan · 2024-06-19T02:32:56Z

src/libs/shim-interface/src/lib.rs

-            Ok(format!(
-                "unix://{}",
-                kata_run_path
+            let p = match jailed_path {


Here the uds is used for managing the shim, instead of the hypervisor, so why should we put the shim's management uds in the jail path?

that's true -- we thought it would be better if all container-related files are inside the jail, but, indeed, we can keep the monitor shim at the generic (non-jailed) dir.

lifupan · 2024-06-19T02:42:28Z

src/runtime-rs/Makefile

@@ -164,8 +170,11 @@ DEFMSIZE9P := 8192
 DEFVFIOMODE := guest-kernel
 ##VAR DEFSANDBOXCGROUPONLY=<bool> Default cgroup model
 DEFSANDBOXCGROUPONLY ?= false
+DEFSANDBOXCGROUPONLY_DB ?= true


sandbox cgroup only isn't hypervisor specific value, why should we define dg/fc specific var here?

issue is that if we want a separate default option for hypervisors, we should have a per-hypervisor variable.

lifupan · 2024-06-19T02:50:17Z

src/runtime-rs/crates/hypervisor/Cargo.toml

@@ -45,6 +45,9 @@ tempdir = "0.3.7"

 [target.'cfg(not(target_arch = "s390x"))'.dependencies]
 dragonball = { path = "../../../dragonball", features = ["atomic-guest-memory", "virtio-vsock", "hotplug", "virtio-blk", "virtio-net", "virtio-fs", "vhost-net", "dbs-upcall", "virtio-mem", "virtio-balloon", "vhost-user-net", "host-device"] }
+dbs-utils = { path = "../../../dragonball/src/dbs_utils" }


If the dragonball utils are useful to other hypervisors, maybe we can split them out into the hypervisor utils?

we only use the MacAddr from dbs_utils -- maybe we could use another crate for that.

lifupan · 2024-06-19T08:13:24Z

src/runtime-rs/crates/hypervisor/src/firecracker/inner.rs

+impl FcInner {
+    pub fn new() -> FcInner {
+        let mut capabilities = Capabilities::new();
+        capabilities.set(CapabilityBits::BlockDeviceSupport);


Did the FC need to set HybridVsockSupport support specifically?

lifupan · 2024-06-19T08:33:08Z

src/runtime-rs/crates/hypervisor/src/firecracker/inner.rs

+                let exit_status = child.wait().await?;
+                error!(
+                    sl!(),
+                    "[Firecracker] Process exited, status: {:?}", exit_status


Maybe you can redefine a sl! macro including the Firecracker sub system instead of writing them in the log message every time?

lifupan · 2024-06-19T08:43:08Z

Hi @Pyrromanis

Thanks for the FC supporting effort, most of the patch LGTM except some small comments.

ananos · 2024-06-19T09:35:56Z

Hi @Pyrromanis

Thanks for the FC supporting effort, most of the patch LGTM except some small comments.

thanks @lifupan for the comments -- all look reasonable and prob. easy to address. We'll report back once we get to it!

Add a basic runtime-rs `Hypervisor` trait implementation for AWS Firecracker - Add basic hypervisor operations (setup / start / stop / add_device) - Implement AWS Firecracker API on a separate file `fc_api.rs` - Add support for running jailed (include all sandbox-related content) - Add initial device support (limited as hotplug is not supported) - Add separate config for runtime-rs (FC) Notes: - devmapper is the only snapshotter supported - to account for no sharefs support, we copy files in the sandbox (as in the GO runtime) - nerdctl spawn is broken (TODO: kata-containers#7703) Fixes: kata-containers#5268 Signed-off-by: George Pyrros <gpyrros@nubificus.co.uk> Signed-off-by: Anastassios Nanos <ananos@nubificus.co.uk> Signed-off-by: Charalampos Mainas <cmainas@nubificus.co.uk> Signed-off-by: George Ntoutsos <gntouts@nubificus.co.uk>

katacontainersbot added the size/huge Largest and most complex task (probably needs breaking into small pieces) label Sep 26, 2023

Pyrromanis force-pushed the feat_add-fc-runtime-rs branch from 64797ac to e7128a8 Compare September 26, 2023 19:43

ananos added no-backport-needed area/firecracker Issues specific to the Firecracker hypervisor lang/rust Rust code runtime-rs size/large Task of significant size rust Pull requests that update Rust code labels Sep 26, 2023

ananos requested review from jodh-intel, Tim-0731-Hzt, bergwolf and fidencio September 26, 2023 20:16

katacontainersbot removed the size/large Task of significant size label Sep 27, 2023

ananos force-pushed the feat_add-fc-runtime-rs branch from 248ad38 to 67c7079 Compare September 27, 2023 13:48

ananos added the ok-to-test label Sep 27, 2023

ananos force-pushed the feat_add-fc-runtime-rs branch from 67c7079 to f3e7820 Compare October 1, 2023 09:01

ananos force-pushed the feat_add-fc-runtime-rs branch from f3e7820 to 6aac3c3 Compare October 6, 2023 17:36

ananos added no-backport-needed ok-to-test and removed no-backport-needed ok-to-test labels Oct 7, 2023

pmores reviewed Jan 15, 2024

View reviewed changes

ananos force-pushed the feat_add-fc-runtime-rs branch from 4a41e8f to fab0845 Compare January 16, 2024 10:45

pmores reviewed Jan 30, 2024

View reviewed changes

ananos force-pushed the feat_add-fc-runtime-rs branch 2 times, most recently from e9eb376 to 4c2a941 Compare February 29, 2024 11:52

ananos force-pushed the feat_add-fc-runtime-rs branch from 4c2a941 to 8c05cc2 Compare April 9, 2024 08:01

stevenhorsman removed no-backport-needed labels Apr 10, 2024

Apokleos requested review from Apokleos, lifupan, studychao and justxuewei May 6, 2024 02:21

ananos force-pushed the feat_add-fc-runtime-rs branch from 8c05cc2 to 98ea15f Compare May 29, 2024 14:50

ananos force-pushed the feat_add-fc-runtime-rs branch from 98ea15f to e7f7bfc Compare June 6, 2024 08:44

lifupan reviewed Jun 19, 2024

View reviewed changes

ananos force-pushed the feat_add-fc-runtime-rs branch 4 times, most recently from 74729cb to e8757d8 Compare June 20, 2024 14:52

lifupan approved these changes Jun 25, 2024

View reviewed changes

ananos force-pushed the feat_add-fc-runtime-rs branch 3 times, most recently from f9ab685 to d2f75b2 Compare June 26, 2024 10:19

ananos force-pushed the feat_add-fc-runtime-rs branch from d2f75b2 to 9020b16 Compare June 26, 2024 19:32

runtime-rs: firecracker hypervisor backend #8070

Are you sure you want to change the base?

runtime-rs: firecracker hypervisor backend #8070

Conversation

Pyrromanis commented Sep 26, 2023

katacontainersbot commented Sep 26, 2023

ananos commented Sep 26, 2023

ananos commented Sep 27, 2023

ananos commented Oct 4, 2023

ananos commented Oct 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lifupan commented Jun 19, 2024

ananos commented Jun 19, 2024