Merge pull request #5901 from justxuewei/fix/mpleak

runtime-rs: Clean up mount points shared to guest

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"

src/runtime-rs/crates/resource/src/rootfs/mod.rs

@@ -27,6 +27,7 @@ pub trait Rootfs: Send + Sync {
     async fn get_guest_rootfs_path(&self) -> Result<String>;
     async fn get_rootfs_mount(&self) -> Result<Vec<oci::Mount>>;
     async fn get_storage(&self) -> Option<Storage>;
+    async fn cleanup(&self) -> Result<()>;
 }
 
 #[derive(Default)]
@@ -66,11 +67,10 @@ impl RootFsResource {
             // if rootfs_mounts is empty
             mounts_vec if mounts_vec.is_empty() => {
                 if let Some(share_fs) = share_fs {
-                    let share_fs_mount = share_fs.get_share_fs_mount();
                     // share fs rootfs
                     Ok(Arc::new(
                         share_fs_rootfs::ShareFsRootfs::new(
-                            &share_fs_mount,
+                            share_fs,
                             cid,
                             root.path.as_str(),
                             None,
@@ -86,25 +86,18 @@ impl RootFsResource {
                 // Safe as single_layer_rootfs must have one layer
                 let layer = &mounts_vec[0];
                 let rootfs: Arc<dyn Rootfs> = if let Some(share_fs) = share_fs {
-                    let share_fs_mount = share_fs.get_share_fs_mount();
                     // nydus rootfs
                     if layer.fs_type == NYDUS_ROOTFS_TYPE {
                         Arc::new(
-                            nydus_rootfs::NydusRootfs::new(
+                            nydus_rootfs::NydusRootfs::new(share_fs, hypervisor, sid, cid, layer)
-                                &share_fs_mount,
+                                .await
-                                hypervisor,
+                                .context("new nydus rootfs")?,
-                                sid,
-                                cid,
-                                layer,
-                            )
-                            .await
-                            .context("new nydus rootfs")?,
                         )
                     } else {
                         // share fs rootfs
                         Arc::new(
                             share_fs_rootfs::ShareFsRootfs::new(
-                                &share_fs_mount,
+                                share_fs,
                                 cid,
                                 bundle_path,
                                 Some(layer),

src/runtime-rs/crates/resource/src/rootfs/nydus_rootfs.rs

@@ -9,8 +9,8 @@ use super::{Rootfs, TYPE_OVERLAY_FS};
 use crate::{
     rootfs::{HYBRID_ROOTFS_LOWER_DIR, ROOTFS},
     share_fs::{
-        do_get_guest_path, do_get_guest_share_path, get_host_rw_shared_path, rafs_mount,
+        do_get_guest_path, do_get_guest_share_path, get_host_rw_shared_path, rafs_mount, ShareFs,
-        ShareFsMount, ShareFsRootfsConfig, PASSTHROUGH_FS_DIR,
+        ShareFsRootfsConfig, PASSTHROUGH_FS_DIR,
     },
 };
 use agent::Storage;
@@ -36,12 +36,13 @@ pub(crate) struct NydusRootfs {
 
 impl NydusRootfs {
     pub async fn new(
-        share_fs_mount: &Arc<dyn ShareFsMount>,
+        share_fs: &Arc<dyn ShareFs>,
         h: &dyn Hypervisor,
         sid: &str,
         cid: &str,
         rootfs: &Mount,
     ) -> Result<Self> {
+        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);
@@ -72,7 +73,7 @@ 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(ShareFsRootfsConfig {
+                    .share_rootfs(&ShareFsRootfsConfig {
                         cid: cid.to_string(),
                         source: extra_options.snapshot_dir.clone(),
                         target: SNAPSHOT_DIR.to_string(),
@@ -143,4 +144,10 @@ impl Rootfs for NydusRootfs {
     async fn get_storage(&self) -> Option<Storage> {
         Some(self.rootfs.clone())
     }
+
+    async fn cleanup(&self) -> Result<()> {
+        // TODO: Clean up NydusRootfs after the container is killed
+        warn!(sl!(), "Cleaning up NydusRootfs is still unimplemented.");
+        Ok(())
+    }
 }

src/runtime-rs/crates/resource/src/rootfs/share_fs_rootfs.rs

@@ -7,20 +7,22 @@
 use agent::Storage;
 use anyhow::{Context, Result};
 use async_trait::async_trait;
-use kata_sys_util::mount::Mounter;
+use kata_sys_util::mount::{umount_timeout, Mounter};
 use kata_types::mount::Mount;
 use std::sync::Arc;
 
 use super::{Rootfs, ROOTFS};
-use crate::share_fs::{ShareFsMount, ShareFsRootfsConfig};
+use crate::share_fs::{ShareFs, ShareFsRootfsConfig};
 
 pub(crate) struct ShareFsRootfs {
     guest_path: String,
+    share_fs: Arc<dyn ShareFs>,
+    config: ShareFsRootfsConfig,
 }
 
 impl ShareFsRootfs {
     pub async fn new(
-        share_fs_mount: &Arc<dyn ShareFsMount>,
+        share_fs: &Arc<dyn ShareFs>,
         cid: &str,
         bundle_path: &str,
         rootfs: Option<&Mount>,
@@ -35,19 +37,25 @@ impl ShareFsRootfs {
         } else {
             bundle_path.to_string()
         };
+
+        let share_fs_mount = share_fs.get_share_fs_mount();
+        let config = ShareFsRootfsConfig {
+            cid: cid.to_string(),
+            source: bundle_rootfs.to_string(),
+            target: ROOTFS.to_string(),
+            readonly: false,
+            is_rafs: false,
+        };
+
         let mount_result = share_fs_mount
-            .share_rootfs(ShareFsRootfsConfig {
+            .share_rootfs(&config)
-                cid: cid.to_string(),
-                source: bundle_rootfs.to_string(),
-                target: ROOTFS.to_string(),
-                readonly: false,
-                is_rafs: false,
-            })
             .await
             .context("share rootfs")?;
 
         Ok(ShareFsRootfs {
             guest_path: mount_result.guest_path,
+            share_fs: Arc::clone(share_fs),
+            config,
         })
     }
 }
@@ -65,4 +73,17 @@ impl Rootfs for ShareFsRootfs {
     async fn get_storage(&self) -> Option<Storage> {
         None
     }
+
+    async fn cleanup(&self) -> Result<()> {
+        // 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)
+            .await
+            .context("umount shared rootfs")?;
+
+        // Umount the bundle rootfs
+        umount_timeout(&self.config.source, 0).context("umount bundle rootfs")?;
+        Ok(())
+    }
 }

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

@@ -47,7 +47,7 @@ pub trait ShareFs: Send + Sync {
     fn mounted_info_set(&self) -> Arc<Mutex<HashMap<String, MountedInfo>>>;
 }
 
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 pub struct ShareFsRootfsConfig {
     // TODO: for nydus v5/v6 need to update ShareFsMount
     pub cid: String,
@@ -120,14 +120,16 @@ impl MountedInfo {
 
 #[async_trait]
 pub trait ShareFsMount: Send + Sync {
-    async fn share_rootfs(&self, config: ShareFsRootfsConfig) -> Result<ShareFsMountResult>;
+    async fn share_rootfs(&self, config: &ShareFsRootfsConfig) -> Result<ShareFsMountResult>;
-    async fn share_volume(&self, config: ShareFsVolumeConfig) -> Result<ShareFsMountResult>;
+    async fn share_volume(&self, config: &ShareFsVolumeConfig) -> Result<ShareFsMountResult>;
     /// Upgrade to readwrite permission
     async fn upgrade_to_rw(&self, file_name: &str) -> Result<()>;
     /// Downgrade to readonly permission
     async fn downgrade_to_ro(&self, file_name: &str) -> Result<()>;
     /// Umount the volume
-    async fn umount(&self, file_name: &str) -> Result<()>;
+    async fn umount_volume(&self, file_name: &str) -> Result<()>;
+    /// Umount the rootfs
+    async fn umount_rootfs(&self, config: &ShareFsRootfsConfig) -> Result<()>;
 }
 
 pub fn new(id: &str, config: &SharedFsInfo) -> Result<Arc<dyn ShareFs>> {

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<ShareFsMountResult> {
+    async fn share_rootfs(&self, config: &ShareFsRootfsConfig) -> Result<ShareFsMountResult> {
         // 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<ShareFsMountResult> {
+    async fn share_volume(&self, config: &ShareFsVolumeConfig) -> Result<ShareFsMountResult> {
         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(),
             };
 
@@ -194,10 +194,34 @@ impl ShareFsMount for VirtiofsShareMount {
         Ok(())
     }
 
-    async fn umount(&self, file_name: &str) -> Result<()> {
+    async fn umount_volume(&self, file_name: &str) -> Result<()> {
-        let host_dest = do_get_host_path(file_name, &self.id, "", true, true);
+        let host_dest = do_get_host_path(file_name, &self.id, "", true, false);
-        umount_timeout(&host_dest, 0).context("Umount readwrite host dest")?;
+        umount_timeout(&host_dest, 0).context("umount volume")?;
         // Umount event will be propagated to ro directory
+
+        // Remove the directory of mointpoint
+        if let Ok(md) = fs::metadata(&host_dest) {
+            if md.is_file() {
+                fs::remove_file(&host_dest).context("remove the volume mount point as a file")?;
+            }
+            if md.is_dir() {
+                fs::remove_dir(&host_dest).context("remove the volume mount point as a dir")?;
+            }
+        }
+        Ok(())
+    }
+
+    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")?;
+
+        // Remove the directory of mointpoint
+        if let Ok(md) = fs::metadata(&host_dest) {
+            if md.is_dir() {
+                fs::remove_dir(&host_dest).context("remove the rootfs mount point as a dir")?;
+            }
+        }
+
         Ok(())
     }
 }

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(())
     }

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(())
     }

src/runtime-rs/crates/resource/src/volume/share_fs_volume.rs

@@ -7,7 +7,7 @@
 use std::{
     path::{Path, PathBuf},
     str::FromStr,
-    sync::{Arc, Weak},
+    sync::Arc,
 };
 
 use anyhow::{anyhow, Context, Result};
@@ -24,7 +24,7 @@ use kata_types::mount;
 // device nodes to the guest.
 // skip the volumes whose source had already set to guest share dir.
 pub(crate) struct ShareFsVolume {
-    share_fs: Option<Weak<dyn ShareFs>>,
+    share_fs: Option<Arc<dyn ShareFs>>,
     mounts: Vec<oci::Mount>,
     storages: Vec<agent::Storage>,
 }
@@ -40,7 +40,7 @@ impl ShareFsVolume {
         let file_name = generate_mount_path("sandbox", file_name);
 
         let mut volume = Self {
-            share_fs: share_fs.as_ref().map(Arc::downgrade),
+            share_fs: share_fs.as_ref().map(Arc::clone),
             mounts: vec![],
             storages: vec![],
         };
@@ -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,12 +158,9 @@ impl Volume for ShareFsVolume {
     }
 
     async fn cleanup(&self) -> Result<()> {
-        if self.share_fs.is_none() {
+        let share_fs = match self.share_fs.as_ref() {
-            return Ok(());
+            Some(fs) => fs,
-        }
+            None => return Ok(()),
-        let share_fs = match self.share_fs.as_ref().unwrap().upgrade() {
-            Some(share_fs) => share_fs,
-            None => return Err(anyhow!("The share_fs was released unexpectedly")),
         };
 
         let mounted_info_set = share_fs.mounted_info_set();
@@ -219,7 +216,7 @@ impl Volume for ShareFsVolume {
                 mounted_info_set.remove(&host_source);
                 // Umount the volume
                 share_fs_mount
-                    .umount(&file_name)
+                    .umount_volume(&file_name)
                     .await
                     .context("Umount volume")?
             }

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(())
     }

src/runtime-rs/crates/runtimes/virt_container/src/container_manager/container_inner.rs

@@ -249,6 +249,7 @@ impl ContainerInner {
             .await?;
 
         self.clean_volumes().await.context("clean volumes")?;
+        self.clean_rootfs().await.context("clean rootfs")?;
 
         Ok(())
     }
@@ -279,7 +280,7 @@ impl ContainerInner {
                 unhandled.push(Arc::clone(v));
                 warn!(
                     sl!(),
-                    "Failed to clean volume {:?}, error = {:?}",
+                    "Failed to clean the volume = {:?}, error = {:?}",
                     v.get_volume_mount(),
                     err
                 );
@@ -290,4 +291,23 @@ impl ContainerInner {
         }
         Ok(())
     }
+
+    async fn clean_rootfs(&mut self) -> Result<()> {
+        let mut unhandled = Vec::new();
+        for rootfs in self.rootfs.iter() {
+            if let Err(err) = rootfs.cleanup().await {
+                unhandled.push(Arc::clone(rootfs));
+                warn!(
+                    sl!(),
+                    "Failed to umount rootfs, cid = {:?}, error = {:?}",
+                    self.container_id(),
+                    err
+                );
+            }
+        }
+        if !unhandled.is_empty() {
+            self.rootfs = unhandled;
+        }
+        Ok(())
+    }
 }