From fd77eebd4d783dadf4707e3074910eb3c97c33cb Mon Sep 17 00:00:00 2001 From: Xuewei Niu Date: Thu, 15 Dec 2022 11:22:03 +0800 Subject: [PATCH] runtime-rs: fix the issues mentioned in the code review In order to avoid cloning, changed the signature of `ShareFsMount::share_rootfs`, `ShareFsMount::share_volume`, and `ShareFsMount::umount_rootfs` to receive a reference to a config. Fixes: #5898 Signed-off-by: Xuewei Niu --- src/runtime-rs/Cargo.lock | 15 +++++++++++++++ .../resource/src/rootfs/nydus_rootfs.rs | 19 +++++++++---------- .../resource/src/rootfs/share_fs_rootfs.rs | 4 ++-- .../crates/resource/src/share_fs/mod.rs | 6 +++--- .../src/share_fs/virtio_fs_share_mount.rs | 8 ++++---- .../resource/src/volume/block_volume.rs | 1 + .../resource/src/volume/default_volume.rs | 1 + .../resource/src/volume/share_fs_volume.rs | 10 +++++----- .../crates/resource/src/volume/shm_volume.rs | 1 + 9 files changed, 41 insertions(+), 24 deletions(-) diff --git a/src/runtime-rs/Cargo.lock b/src/runtime-rs/Cargo.lock index 060f7a76f..90129b4c5 100644 --- a/src/runtime-rs/Cargo.lock +++ b/src/runtime-rs/Cargo.lock @@ -1223,6 +1223,7 @@ dependencies = [ "seccompiler", "serde", "serde_json", + "shim-interface", "slog", "slog-scope", "thiserror", @@ -1919,6 +1920,7 @@ dependencies = [ "safe-path", "serde", "serde_json", + "shim-interface", ] [[package]] @@ -2343,6 +2345,7 @@ dependencies = [ "logging", "oci", "persist", + "shim-interface", "slog", "slog-scope", "tokio", @@ -2474,6 +2477,7 @@ dependencies = [ "logging", "persist", "runtimes", + "shim-interface", "slog", "slog-scope", "tokio", @@ -2539,12 +2543,23 @@ dependencies = [ name = "shim-ctl" version = "0.1.0" dependencies = [ + "anyhow", "common", "logging", "runtimes", "tokio", ] +[[package]] +name = "shim-interface" +version = "0.1.0" +dependencies = [ + "anyhow", + "hyper", + "hyperlocal", + "tokio", +] + [[package]] name = "signal-hook-registry" version = "1.4.0" diff --git a/src/runtime-rs/crates/resource/src/rootfs/nydus_rootfs.rs b/src/runtime-rs/crates/resource/src/rootfs/nydus_rootfs.rs index 4fa095164..16f9c48dd 100644 --- a/src/runtime-rs/crates/resource/src/rootfs/nydus_rootfs.rs +++ b/src/runtime-rs/crates/resource/src/rootfs/nydus_rootfs.rs @@ -42,19 +42,11 @@ impl NydusRootfs { cid: &str, rootfs: &Mount, ) -> Result { - let share_fs = Arc::clone(share_fs); let share_fs_mount = share_fs.get_share_fs_mount(); let extra_options = NydusExtraOptions::new(rootfs).context("failed to parse nydus extra options")?; info!(sl!(), "extra_option {:?}", &extra_options); let rafs_meta = &extra_options.source; - let config = ShareFsRootfsConfig { - cid: cid.to_string(), - source: extra_options.snapshot_dir.clone(), - target: SNAPSHOT_DIR.to_string(), - readonly: true, - is_rafs: false, - }; let (rootfs_storage, rootfs_guest_path) = match extra_options.fs_version.as_str() { // both nydus v5 and v6 can be handled by the builtin nydus in dragonball by using the rafs mode. // nydus v6 could also be handled by the guest kernel as well, but some kernel patch is not support in the upstream community. We will add an option to let runtime-rs handle nydus v6 in the guest kernel optionally once the patch is ready @@ -81,7 +73,13 @@ impl NydusRootfs { let rootfs_guest_path = do_get_guest_path(ROOTFS, cid, false, false); // bind mount the snapshot dir under the share directory share_fs_mount - .share_rootfs(config.clone()) + .share_rootfs(&ShareFsRootfsConfig { + cid: cid.to_string(), + source: extra_options.snapshot_dir.clone(), + target: SNAPSHOT_DIR.to_string(), + readonly: true, + is_rafs: false, + }) .await .context("share nydus rootfs")?; let mut options: Vec = Vec::new(); @@ -148,7 +146,8 @@ impl Rootfs for NydusRootfs { } async fn cleanup(&self) -> Result<()> { - warn!(sl!(), "Cleaning up Nydus Rootfs is still unimplemented."); + // TODO: Clean up NydusRootfs after the container is killed + warn!(sl!(), "Cleaning up NydusRootfs is still unimplemented."); Ok(()) } } diff --git a/src/runtime-rs/crates/resource/src/rootfs/share_fs_rootfs.rs b/src/runtime-rs/crates/resource/src/rootfs/share_fs_rootfs.rs index ee46de805..b5d4136c1 100644 --- a/src/runtime-rs/crates/resource/src/rootfs/share_fs_rootfs.rs +++ b/src/runtime-rs/crates/resource/src/rootfs/share_fs_rootfs.rs @@ -48,7 +48,7 @@ impl ShareFsRootfs { }; let mount_result = share_fs_mount - .share_rootfs(config.clone()) + .share_rootfs(&config) .await .context("share rootfs")?; @@ -78,7 +78,7 @@ impl Rootfs for ShareFsRootfs { // Umount the mount point shared to guest let share_fs_mount = self.share_fs.get_share_fs_mount(); share_fs_mount - .umount_rootfs(self.config.clone()) + .umount_rootfs(&self.config) .await .context("umount shared rootfs")?; diff --git a/src/runtime-rs/crates/resource/src/share_fs/mod.rs b/src/runtime-rs/crates/resource/src/share_fs/mod.rs index c6fbd73c7..4865db17b 100644 --- a/src/runtime-rs/crates/resource/src/share_fs/mod.rs +++ b/src/runtime-rs/crates/resource/src/share_fs/mod.rs @@ -120,8 +120,8 @@ impl MountedInfo { #[async_trait] pub trait ShareFsMount: Send + Sync { - async fn share_rootfs(&self, config: ShareFsRootfsConfig) -> Result; - async fn share_volume(&self, config: ShareFsVolumeConfig) -> Result; + async fn share_rootfs(&self, config: &ShareFsRootfsConfig) -> Result; + async fn share_volume(&self, config: &ShareFsVolumeConfig) -> Result; /// Upgrade to readwrite permission async fn upgrade_to_rw(&self, file_name: &str) -> Result<()>; /// Downgrade to readonly permission @@ -129,7 +129,7 @@ pub trait ShareFsMount: Send + Sync { /// Umount the volume async fn umount_volume(&self, file_name: &str) -> Result<()>; /// Umount the rootfs - async fn umount_rootfs(&self, config: ShareFsRootfsConfig) -> Result<()>; + async fn umount_rootfs(&self, config: &ShareFsRootfsConfig) -> Result<()>; } pub fn new(id: &str, config: &SharedFsInfo) -> Result> { diff --git a/src/runtime-rs/crates/resource/src/share_fs/virtio_fs_share_mount.rs b/src/runtime-rs/crates/resource/src/share_fs/virtio_fs_share_mount.rs index 2e49c47fd..30d6a9336 100644 --- a/src/runtime-rs/crates/resource/src/share_fs/virtio_fs_share_mount.rs +++ b/src/runtime-rs/crates/resource/src/share_fs/virtio_fs_share_mount.rs @@ -38,7 +38,7 @@ impl VirtiofsShareMount { #[async_trait] impl ShareFsMount for VirtiofsShareMount { - async fn share_rootfs(&self, config: ShareFsRootfsConfig) -> Result { + async fn share_rootfs(&self, config: &ShareFsRootfsConfig) -> Result { // TODO: select virtiofs or support nydus let guest_path = utils::share_to_guest( &config.source, @@ -56,7 +56,7 @@ impl ShareFsMount for VirtiofsShareMount { }) } - async fn share_volume(&self, config: ShareFsVolumeConfig) -> Result { + async fn share_volume(&self, config: &ShareFsVolumeConfig) -> Result { let mut guest_path = utils::share_to_guest( &config.source, &config.target, @@ -103,7 +103,7 @@ impl ShareFsMount for VirtiofsShareMount { source: guest_path, fs_type: String::from("bind"), fs_group: None, - options: config.mount_options, + options: config.mount_options.clone(), mount_point: watchable_guest_mount.clone(), }; @@ -211,7 +211,7 @@ impl ShareFsMount for VirtiofsShareMount { Ok(()) } - async fn umount_rootfs(&self, config: ShareFsRootfsConfig) -> Result<()> { + async fn umount_rootfs(&self, config: &ShareFsRootfsConfig) -> Result<()> { let host_dest = do_get_host_path(&config.target, &self.id, &config.cid, false, false); umount_timeout(&host_dest, 0).context("umount rootfs")?; diff --git a/src/runtime-rs/crates/resource/src/volume/block_volume.rs b/src/runtime-rs/crates/resource/src/volume/block_volume.rs index 67d0bc7af..da8ef03f9 100644 --- a/src/runtime-rs/crates/resource/src/volume/block_volume.rs +++ b/src/runtime-rs/crates/resource/src/volume/block_volume.rs @@ -30,6 +30,7 @@ impl Volume for BlockVolume { } async fn cleanup(&self) -> Result<()> { + // TODO: Clean up BlockVolume warn!(sl!(), "Cleaning up BlockVolume is still unimplemented."); Ok(()) } diff --git a/src/runtime-rs/crates/resource/src/volume/default_volume.rs b/src/runtime-rs/crates/resource/src/volume/default_volume.rs index bc14ba959..8855a8e03 100644 --- a/src/runtime-rs/crates/resource/src/volume/default_volume.rs +++ b/src/runtime-rs/crates/resource/src/volume/default_volume.rs @@ -34,6 +34,7 @@ impl Volume for DefaultVolume { } async fn cleanup(&self) -> Result<()> { + // TODO: Clean up DefaultVolume warn!(sl!(), "Cleaning up DefaultVolume is still unimplemented."); Ok(()) } diff --git a/src/runtime-rs/crates/resource/src/volume/share_fs_volume.rs b/src/runtime-rs/crates/resource/src/volume/share_fs_volume.rs index 38aad4cde..95bc2edfb 100644 --- a/src/runtime-rs/crates/resource/src/volume/share_fs_volume.rs +++ b/src/runtime-rs/crates/resource/src/volume/share_fs_volume.rs @@ -112,7 +112,7 @@ impl ShareFsVolume { } else { // Not mounted ever let mount_result = share_fs_mount - .share_volume(ShareFsVolumeConfig { + .share_volume(&ShareFsVolumeConfig { // The scope of shared volume is sandbox cid: String::from(""), source: m.source.clone(), @@ -158,10 +158,10 @@ impl Volume for ShareFsVolume { } async fn cleanup(&self) -> Result<()> { - if self.share_fs.is_none() { - return Ok(()); - } - let share_fs = self.share_fs.as_ref().unwrap(); + let share_fs = match self.share_fs.as_ref() { + Some(fs) => fs, + None => return Ok(()), + }; let mounted_info_set = share_fs.mounted_info_set(); let mut mounted_info_set = mounted_info_set.lock().await; diff --git a/src/runtime-rs/crates/resource/src/volume/shm_volume.rs b/src/runtime-rs/crates/resource/src/volume/shm_volume.rs index 53f3addf8..5805106d2 100644 --- a/src/runtime-rs/crates/resource/src/volume/shm_volume.rs +++ b/src/runtime-rs/crates/resource/src/volume/shm_volume.rs @@ -100,6 +100,7 @@ impl Volume for ShmVolume { } async fn cleanup(&self) -> Result<()> { + // TODO: Clean up ShmVolume warn!(sl!(), "Cleaning up ShmVolume is still unimplemented."); Ok(()) }