runtime-rs: bringing virtio-fs device in device-manager #7932

Apokleos · 2023-09-13T11:58:59Z

Currently, virtio-fs is still outside of the device manager. This causes some management issues, such as the inability to unify PCI address management.
To do Enhancement of device manager, it will bring virtio-fs device in device-manager for unified management

Fixes: #7915

Apokleos · 2023-09-18T02:38:20Z

/test

quanweiZhou

Thanks, @Apokleos, few comments.

src/runtime-rs/crates/hypervisor/src/device/device_manager.rs

src/runtime-rs/crates/hypervisor/src/device/driver/virtio_fs.rs

Apokleos · 2023-09-22T00:12:32Z

/test

Apokleos · 2023-09-25T01:52:47Z

/retest

Apokleos · 2023-09-25T09:13:10Z

/test

src/runtime-rs/crates/hypervisor/src/device/mod.rs

Apokleos · 2023-09-26T06:24:40Z

/test

gkurz · 2023-11-15T17:28:18Z

@Apokleos This PR still has a very huge commit that I believe could have been broken down further but it is definitely an improvement over the previous tentative. Please address pending comments. I'll ask approvers to review again and we'll merge it.

pmores · 2023-11-16T12:50:08Z

src/runtime-rs/crates/hypervisor/src/device/device_manager.rs

@@ -325,6 +330,21 @@ impl DeviceManager {
                // No need to do find device for hybrid vsock device.
                Arc::new(Mutex::new(HybridVsockDevice::new(&device_id, hvconfig)))
            }
+            DeviceConfig::ShareFsCfg(config) => {
+                // Try to find the sharefs device. If found, just return.
+                if let Some(device_id_matched) = self.find_device(config.shared_path.clone()).await


I'm wondering why find_device() takes an owned string instead of a string slice, forcing all this clone()ing... But that's a case for another PR, ignore me here. ;-)

Yeah，I will consider to refactor it future!

pmores · 2023-11-16T13:10:49Z

src/runtime-rs/crates/hypervisor/src/device/device_manager.rs

+                    // update the mount_config.
+                    device.config.mount_config = config.mount_config.clone();
+                }
+                Arc::new(Mutex::new(device.clone()))


Is clone() necessary here? get_device_info() returns DeviceType by value and is commonly implemented by clone()ing self. So target_device is a clone already and owned by this function. If that's the case it might make more sense to drop the clone() call here. That would leave target_device partially moved from but I don't see it referenced after this match statement so it should be ok.

pmores · 2023-11-16T13:30:01Z

src/runtime-rs/crates/resource/src/share_fs/share_virtio_fs.rs

-    let result = h
-        .add_device(DeviceType::ShareFsMount(virtio_fs))
+    // update virtio-fs device with ShareFsMountConfig
+    do_update_device(d, &DeviceConfig::ShareFsCfg(sharefs_config.clone()))


Do we need to clone() sharefs_config here? I don't see it referenced anymore after this point.

pmores · 2023-11-16T13:33:38Z

src/runtime-rs/crates/resource/src/share_fs/share_virtio_fs.rs

-    h.add_device(DeviceType::ShareFsMount(virtio_fs))
+
+    // update virtio-fs device with ShareFsMountConfig
+    do_update_device(d, &DeviceConfig::ShareFsCfg(sharefs_config.clone()))


Just like a similar piece of code in setup_inline_virtiofs(), is clone() necessary here?

pmores

lgtm, thanks @Apokleos for all the work you put into this PR!

amshinde · 2023-11-17T22:13:35Z

src/runtime-rs/crates/hypervisor/src/device/device_manager.rs

@@ -472,6 +547,19 @@ pub async fn do_handle_device(
    Ok(device_info)
 }

+pub async fn do_update_device(
+    d: &RwLock<DeviceManager>,
+    update_config: &DeviceConfig,


This should be updated_config here as well, as we want to avoid the use of verbs in the function params.

@Apokleos Can you make this small fix? Otherwise the PR looks good to me.

@amshinde Thx, sorry, I forget to correct this one.

Apokleos · 2023-11-20T11:33:18Z

/test

gkurz · 2023-11-20T11:49:18Z

src/runtime-rs/crates/hypervisor/src/device/device_manager.rs

+        .await
+        .try_update_device(updated_config)
+        .await
+        .context("failed to update deivce")?;


Typo: s/deivce/device

gkurz

Thanks for taking the time to address all the comments @Apokleos !

It mainly focus on the two parts: (1) redesign the ShareFsConfig with ShareFsMountConfig The device mount operation must depend on the fact that sharefs device exists, and re-design the structure of SharesFsConfig and move the ShareFsMountConfig into it with Option type, which is to describe the relation between ShareFsConfig and ShareFsMountConfig. (2) move virtiofs into device manager Currently, virtio-fs is still outside of the device manager. To do Enhancement of device manager, it will bring virtio-fs device in device-manager for unified management Fixes: kata-containers#7915 Signed-off-by: alex.lyn <alex.lyn@antgroup.com>

Fixes: kata-containers#7915 Signed-off-by: alex.lyn <alex.lyn@antgroup.com>

Apokleos · 2023-11-20T12:17:08Z

Thanks for taking the time to address all the comments @Apokleos !

Thx @gkurz very much !

amshinde · 2023-11-20T20:34:28Z

/test

Apokleos · 2023-11-21T00:17:40Z

/retest

Apokleos · 2023-11-21T01:21:07Z

/test-s390x

Apokleos · 2023-11-21T02:33:31Z

/retest-s390x

Apokleos · 2023-11-21T02:34:17Z

/test-ubuntu

BbolroC · 2023-11-21T03:47:05Z

/test-s390x

katacontainersbot added the size/large Task of significant size label Sep 13, 2023

Apokleos force-pushed the wrap-virtiofs-in-dm branch from af6b8b4 to 2af1160 Compare September 18, 2023 02:17

Apokleos marked this pull request as ready for review September 18, 2023 02:22

Apokleos changed the title ~~WIP runtime-rs: bringing virtio-fs device in device-manager~~ runtime-rs: bringing virtio-fs device in device-manager Sep 18, 2023

Apokleos requested review from lifupan, Tim-0731-Hzt, quanweiZhou and ananos September 18, 2023 02:23

Apokleos requested a review from Ji-Xinyou September 18, 2023 02:58

quanweiZhou reviewed Sep 19, 2023

View reviewed changes

src/runtime-rs/crates/hypervisor/src/device/device_manager.rs Outdated Show resolved Hide resolved

src/runtime-rs/crates/hypervisor/src/device/device_manager.rs Outdated Show resolved Hide resolved

src/runtime-rs/crates/hypervisor/src/device/driver/virtio_fs.rs Outdated Show resolved Hide resolved

Apokleos force-pushed the wrap-virtiofs-in-dm branch 2 times, most recently from 05f77d2 to 7e73bbf Compare September 21, 2023 08:46

katacontainersbot added size/small Small and simple task size/large Task of significant size and removed size/large Task of significant size size/small Small and simple task labels Sep 21, 2023

Apokleos force-pushed the wrap-virtiofs-in-dm branch from aa8fbc4 to dff8cf0 Compare September 21, 2023 09:03

Apokleos force-pushed the wrap-virtiofs-in-dm branch from dff8cf0 to a425d73 Compare September 25, 2023 00:50

katacontainersbot added size/small Small and simple task size/large Task of significant size and removed size/large Task of significant size size/small Small and simple task labels Sep 25, 2023

Apokleos force-pushed the wrap-virtiofs-in-dm branch from dc03a4b to 1edd76f Compare September 25, 2023 01:12

quanweiZhou reviewed Sep 25, 2023

View reviewed changes

src/runtime-rs/crates/hypervisor/src/device/mod.rs Outdated Show resolved Hide resolved

src/runtime-rs/crates/hypervisor/src/device/mod.rs Outdated Show resolved Hide resolved

Apokleos force-pushed the wrap-virtiofs-in-dm branch from 1edd76f to 39ef7b4 Compare September 26, 2023 02:06

gkurz requested review from quanweiZhou, lifupan, Tim-0731-Hzt, studychao and gkurz November 15, 2023 17:27

pmores reviewed Nov 16, 2023

View reviewed changes

Apokleos force-pushed the wrap-virtiofs-in-dm branch from 8192c2a to b6a0445 Compare November 17, 2023 02:22

pmores approved these changes Nov 17, 2023

View reviewed changes

amshinde reviewed Nov 17, 2023

View reviewed changes

Apokleos force-pushed the wrap-virtiofs-in-dm branch from b6a0445 to 3080d3d Compare November 20, 2023 03:00

Apokleos added the runtime-rs label Nov 20, 2023

gkurz reviewed Nov 20, 2023

View reviewed changes

gkurz approved these changes Nov 20, 2023

View reviewed changes

Apokleos added 2 commits November 20, 2023 20:04

runtime-rs: Name the ShareFs Mount Option type more accurately

fe62e65

Fixes: kata-containers#7915 Signed-off-by: alex.lyn <alex.lyn@antgroup.com>

Apokleos force-pushed the wrap-virtiofs-in-dm branch from 3080d3d to fe62e65 Compare November 20, 2023 12:09

amshinde approved these changes Nov 20, 2023

View reviewed changes

Apokleos merged commit 4fd2914 into kata-containers:main Nov 21, 2023
158 of 164 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

runtime-rs: bringing virtio-fs device in device-manager #7932

runtime-rs: bringing virtio-fs device in device-manager #7932

Apokleos commented Sep 13, 2023

Apokleos commented Sep 18, 2023

quanweiZhou left a comment

Apokleos commented Sep 22, 2023

Apokleos commented Sep 25, 2023

Apokleos commented Sep 25, 2023

Apokleos commented Sep 26, 2023

gkurz commented Nov 15, 2023 •

edited

pmores Nov 16, 2023

Apokleos Nov 17, 2023

pmores Nov 16, 2023 •

edited

Apokleos Nov 17, 2023

pmores Nov 16, 2023

Apokleos Nov 17, 2023

pmores Nov 16, 2023

Apokleos Nov 17, 2023

pmores left a comment

amshinde Nov 17, 2023

amshinde Nov 17, 2023

Apokleos Nov 20, 2023

Apokleos Nov 20, 2023

Apokleos commented Nov 20, 2023

gkurz Nov 20, 2023

Apokleos Nov 20, 2023

gkurz left a comment

Apokleos commented Nov 20, 2023

amshinde commented Nov 20, 2023

Apokleos commented Nov 21, 2023

Apokleos commented Nov 21, 2023

Apokleos commented Nov 21, 2023

Apokleos commented Nov 21, 2023

BbolroC commented Nov 21, 2023

runtime-rs: bringing virtio-fs device in device-manager #7932

runtime-rs: bringing virtio-fs device in device-manager #7932

Conversation

Apokleos commented Sep 13, 2023

Apokleos commented Sep 18, 2023

quanweiZhou left a comment

Choose a reason for hiding this comment

Apokleos commented Sep 22, 2023

Apokleos commented Sep 25, 2023

Apokleos commented Sep 25, 2023

Apokleos commented Sep 26, 2023

gkurz commented Nov 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmores Nov 16, 2023 • edited

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

pmores left a 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

Apokleos commented Nov 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkurz left a comment

Choose a reason for hiding this comment

Apokleos commented Nov 20, 2023

amshinde commented Nov 20, 2023

Apokleos commented Nov 21, 2023

Apokleos commented Nov 21, 2023

Apokleos commented Nov 21, 2023

Apokleos commented Nov 21, 2023

BbolroC commented Nov 21, 2023

gkurz commented Nov 15, 2023 •

edited

pmores Nov 16, 2023 •

edited