Merge pull request #7819 from jiangliu/storage-cleanup

Improve the way to clean up storage devices for sandbox

src/agent/src/sandbox.rs

@@ -16,7 +16,7 @@ use std::{thread, time};
 
 use anyhow::{anyhow, Context, Result};
 use kata_types::cpu::CpuSet;
-use kata_types::mount::{StorageDevice, StorageDeviceGeneric};
+use kata_types::mount::StorageDevice;
 use libc::pid_t;
 use oci::{Hook, Hooks};
 use protocols::agent::OnlineCPUMemRequest;
@@ -32,11 +32,12 @@ use tokio::sync::Mutex;
 use tracing::instrument;
 
 use crate::linux_abi::*;
-use crate::mount::{get_mount_fs_type, is_mounted, remove_mounts, TYPE_ROOTFS};
+use crate::mount::{get_mount_fs_type, TYPE_ROOTFS};
 use crate::namespace::Namespace;
 use crate::netlink::Handle;
 use crate::network::Network;
 use crate::pci;
+use crate::storage::StorageDeviceGeneric;
 use crate::uevent::{Uevent, UeventMatcher};
 use crate::watcher::BindWatcher;
 
@@ -60,7 +61,7 @@ impl StorageState {
     fn new() -> Self {
         StorageState {
             count: Arc::new(AtomicU32::new(1)),
-            device: Arc::new(StorageDeviceGeneric::new("".to_string())),
+            device: Arc::new(StorageDeviceGeneric::default()),
         }
     }
 
@@ -71,7 +72,7 @@ impl StorageState {
         }
     }
 
-    pub fn path(&self) -> &str {
+    pub fn path(&self) -> Option<&str> {
         self.device.path()
     }
 
@@ -182,30 +183,6 @@ impl Sandbox {
         Ok(state.device)
     }
 
-    // Clean mount and directory of a mountpoint.
-    // This is actually StorageDeviceGeneric::cleanup(), kept here due to dependency chain.
-    #[instrument]
-    fn cleanup_sandbox_storage(&mut self, path: &str) -> Result<()> {
-        if path.is_empty() {
-            return Err(anyhow!("mountpoint path is empty"));
-        } else if !Path::new(path).exists() {
-            return Ok(());
-        }
-
-        if matches!(is_mounted(path), Ok(true)) {
-            let mounts = vec![path.to_string()];
-            remove_mounts(&mounts)?;
-        }
-
-        // "remove_dir" will fail if the mount point is backed by a read-only filesystem.
-        // This is the case with the device mapper snapshotter, where we mount the block device directly
-        // at the underlying sandbox path which was provided from the base RO kataShared path from the host.
-        if let Err(err) = fs::remove_dir(path) {
-            warn!(self.logger, "failed to remove dir {}, {:?}", path, err);
-        }
-        Ok(())
-    }
-
     /// Decrease reference count and destroy the storage object if reference count reaches zero.
     /// Returns `Ok(true)` if the reference count has reached zero and the storage object has been
     /// removed.
@@ -215,8 +192,9 @@ impl Sandbox {
             None => Err(anyhow!("Sandbox storage with path {} not found", path)),
             Some(state) => {
                 if state.dec_and_test_ref_count().await {
-                    self.storages.remove(path);
+                    if let Some(storage) = self.storages.remove(path) {
-                    self.cleanup_sandbox_storage(path)?;
+                        storage.device.cleanup()?;
+                    }
                     Ok(true)
                 } else {
                     Ok(false)
@@ -574,50 +552,6 @@ mod tests {
         );
     }
 
-    #[tokio::test]
-    #[serial]
-    async fn remove_sandbox_storage() {
-        skip_if_not_root!();
-
-        let logger = slog::Logger::root(slog::Discard, o!());
-        let mut s = Sandbox::new(&logger).unwrap();
-
-        let tmpdir = Builder::new().tempdir().unwrap();
-        let tmpdir_path = tmpdir.path().to_str().unwrap();
-
-        let srcdir = Builder::new()
-            .prefix("src")
-            .tempdir_in(tmpdir_path)
-            .unwrap();
-        let srcdir_path = srcdir.path().to_str().unwrap();
-
-        let destdir = Builder::new()
-            .prefix("dest")
-            .tempdir_in(tmpdir_path)
-            .unwrap();
-        let destdir_path = destdir.path().to_str().unwrap();
-
-        let emptydir = Builder::new()
-            .prefix("empty")
-            .tempdir_in(tmpdir_path)
-            .unwrap();
-
-        assert!(s.cleanup_sandbox_storage("").is_err());
-
-        let invalid_dir = emptydir.path().join("invalid");
-
-        assert!(s
-            .cleanup_sandbox_storage(invalid_dir.to_str().unwrap())
-            .is_ok());
-
-        assert!(bind_mount(srcdir_path, destdir_path, &logger).is_ok());
-
-        assert!(s.cleanup_sandbox_storage(destdir_path).is_ok());
-
-        // remove a directory without umount
-        s.cleanup_sandbox_storage(srcdir_path).unwrap();
-    }
-
     #[tokio::test]
     #[serial]
     async fn unset_and_remove_sandbox_storage() {
@@ -649,6 +583,10 @@ mod tests {
         assert!(bind_mount(srcdir_path, destdir_path, &logger).is_ok());
 
         s.add_sandbox_storage(destdir_path).await;
+        let storage = StorageDeviceGeneric::new(destdir_path.to_string());
+        assert!(s
+            .update_sandbox_storage(destdir_path, Arc::new(storage))
+            .is_ok());
         assert!(s.remove_sandbox_storage(destdir_path).await.is_ok());
 
         let other_dir_str;
@@ -663,6 +601,10 @@ mod tests {
             other_dir_str = other_dir_path.to_string();
 
             s.add_sandbox_storage(other_dir_path).await;
+            let storage = StorageDeviceGeneric::new(other_dir_path.to_string());
+            assert!(s
+                .update_sandbox_storage(other_dir_path, Arc::new(storage))
+                .is_ok());
         }
 
         assert!(s.remove_sandbox_storage(&other_dir_str).await.is_ok());

src/agent/src/storage/mod.rs

@@ -12,9 +12,7 @@ use std::sync::Arc;
 
 use anyhow::{anyhow, Context, Result};
 use kata_sys_util::mount::{create_mount_destination, parse_mount_options};
-use kata_types::mount::{
+use kata_types::mount::{StorageDevice, StorageHandlerManager, KATA_SHAREDFS_GUEST_PREMOUNT_TAG};
-    StorageDevice, StorageDeviceGeneric, StorageHandlerManager, KATA_SHAREDFS_GUEST_PREMOUNT_TAG,
-};
 use nix::unistd::{Gid, Uid};
 use protocols::agent::Storage;
 use protocols::types::FSGroupChangePolicy;
@@ -32,7 +30,7 @@ use crate::device::{
     DRIVER_LOCAL_TYPE, DRIVER_NVDIMM_TYPE, DRIVER_OVERLAYFS_TYPE, DRIVER_SCSI_TYPE,
     DRIVER_VIRTIOFS_TYPE, DRIVER_WATCHABLE_BIND_TYPE,
 };
-use crate::mount::{baremount, is_mounted};
+use crate::mount::{baremount, is_mounted, remove_mounts};
 use crate::sandbox::Sandbox;
 
 pub use self::ephemeral_handler::update_ephemeral_mounts;
@@ -55,6 +53,71 @@ pub struct StorageContext<'a> {
     sandbox: &'a Arc<Mutex<Sandbox>>,
 }
 
+/// An implementation of generic storage device.
+#[derive(Default, Debug)]
+pub struct StorageDeviceGeneric {
+    path: Option<String>,
+}
+
+impl StorageDeviceGeneric {
+    /// Create a new instance of `StorageStateCommon`.
+    pub fn new(path: String) -> Self {
+        StorageDeviceGeneric { path: Some(path) }
+    }
+}
+
+impl StorageDevice for StorageDeviceGeneric {
+    fn path(&self) -> Option<&str> {
+        self.path.as_deref()
+    }
+
+    fn cleanup(&self) -> Result<()> {
+        let path = match self.path() {
+            None => return Ok(()),
+            Some(v) => {
+                if v.is_empty() {
+                    // TODO: Bind watch, local, ephemeral volume has empty path, which will get leaked.
+                    return Ok(());
+                } else {
+                    v
+                }
+            }
+        };
+        if !Path::new(path).exists() {
+            return Ok(());
+        }
+
+        if matches!(is_mounted(path), Ok(true)) {
+            let mounts = vec![path.to_string()];
+            remove_mounts(&mounts)?;
+        }
+        if matches!(is_mounted(path), Ok(true)) {
+            return Err(anyhow!("failed to umount mountpoint {}", path));
+        }
+
+        let p = Path::new(path);
+        if p.is_dir() {
+            let is_empty = p.read_dir()?.next().is_none();
+            if !is_empty {
+                return Err(anyhow!("directory is not empty when clean up storage"));
+            }
+            // "remove_dir" will fail if the mount point is backed by a read-only filesystem.
+            // This is the case with the device mapper snapshotter, where we mount the block device
+            // directly at the underlying sandbox path which was provided from the base RO kataShared
+            // path from the host.
+            let _ = fs::remove_dir(p);
+        } else if !p.is_file() {
+            // TODO: should we remove the file for bind mount?
+            return Err(anyhow!(
+                "storage path {} is neither directory nor file",
+                path
+            ));
+        }
+
+        Ok(())
+    }
+}
+
 /// Trait object to handle storage device.
 #[async_trait::async_trait]
 pub trait StorageHandler: Send + Sync {
@@ -103,9 +166,10 @@ pub async fn add_storages(
         let path = storage.mount_point.clone();
         let state = sandbox.lock().await.add_sandbox_storage(&path).await;
         if state.ref_count().await > 1 {
-            let path = state.path();
+            if let Some(path) = state.path() {
-            if !path.is_empty() {
+                if !path.is_empty() {
-                mount_list.push(path.to_string());
+                    mount_list.push(path.to_string());
+                }
             }
             // The device already exists.
             continue;
@@ -128,9 +192,10 @@ pub async fn add_storages(
                         .update_sandbox_storage(&path, device.clone())
                     {
                         Ok(d) => {
-                            let path = device.path().to_string();
+                            if let Some(path) = device.path() {
-                            if !path.is_empty() {
+                                if !path.is_empty() {
-                                mount_list.push(path.clone());
+                                    mount_list.push(path.to_string());
+                                }
                             }
                             drop(d);
                         }
@@ -140,7 +205,12 @@ pub async fn add_storages(
                             {
                                 warn!(logger, "failed to remove dummy sandbox storage {:?}", e);
                             }
-                            device.cleanup();
+                            if let Err(e) = device.cleanup() {
+                                error!(
+                                    logger,
+                                    "failed to clean state for storage device {}, {}", path, e
+                                );
+                            }
                             return Err(anyhow!("failed to update device for storage"));
                         }
                     }
@@ -319,9 +389,11 @@ pub fn recursive_ownership_change(
 #[cfg(test)]
 mod tests {
     use super::*;
+    use anyhow::Error;
+    use nix::mount::MsFlags;
     use protocols::agent::FSGroup;
     use std::fs::File;
-    use tempfile::tempdir;
+    use tempfile::{tempdir, Builder};
     use test_utils::{
         skip_if_not_root, skip_loop_by_user, skip_loop_if_not_root, skip_loop_if_root, TestUserType,
     };
@@ -649,4 +721,69 @@ mod tests {
             }
         }
     }
+
+    #[tokio::test]
+    #[serial_test::serial]
+    async fn cleanup_storage() {
+        skip_if_not_root!();
+
+        let logger = slog::Logger::root(slog::Discard, o!());
+
+        let tmpdir = Builder::new().tempdir().unwrap();
+        let tmpdir_path = tmpdir.path().to_str().unwrap();
+
+        let srcdir = Builder::new()
+            .prefix("src")
+            .tempdir_in(tmpdir_path)
+            .unwrap();
+        let srcdir_path = srcdir.path().to_str().unwrap();
+        let empty_file = Path::new(srcdir_path).join("emptyfile");
+        fs::write(&empty_file, "test").unwrap();
+
+        let destdir = Builder::new()
+            .prefix("dest")
+            .tempdir_in(tmpdir_path)
+            .unwrap();
+        let destdir_path = destdir.path().to_str().unwrap();
+
+        let emptydir = Builder::new()
+            .prefix("empty")
+            .tempdir_in(tmpdir_path)
+            .unwrap();
+
+        let s = StorageDeviceGeneric::default();
+        assert!(s.cleanup().is_ok());
+
+        let s = StorageDeviceGeneric::new("".to_string());
+        assert!(s.cleanup().is_ok());
+
+        let invalid_dir = emptydir
+            .path()
+            .join("invalid")
+            .to_str()
+            .unwrap()
+            .to_string();
+        let s = StorageDeviceGeneric::new(invalid_dir);
+        assert!(s.cleanup().is_ok());
+
+        assert!(bind_mount(srcdir_path, destdir_path, &logger).is_ok());
+
+        let s = StorageDeviceGeneric::new(destdir_path.to_string());
+        assert!(s.cleanup().is_ok());
+
+        // fail to remove non-empty directory
+        let s = StorageDeviceGeneric::new(srcdir_path.to_string());
+        s.cleanup().unwrap_err();
+
+        // remove a directory without umount
+        fs::remove_file(&empty_file).unwrap();
+        s.cleanup().unwrap();
+    }
+
+    fn bind_mount(src: &str, dst: &str, logger: &Logger) -> Result<(), Error> {
+        let src_path = Path::new(src);
+        let dst_path = Path::new(dst);
+
+        baremount(src_path, dst_path, "bind", MsFlags::MS_BIND, "", logger)
+    }
 }

src/libs/kata-types/src/mount.rs

@@ -7,7 +7,6 @@
 use anyhow::{anyhow, Context, Error, Result};
 use std::collections::hash_map::Entry;
 use std::convert::TryFrom;
-use std::fmt::Formatter;
 use std::{collections::HashMap, fs, path::PathBuf};
 
 /// Prefix to mark a volume as Kata special.
@@ -429,41 +428,13 @@ impl TryFrom<&NydusExtraOptions> for KataVirtualVolume {
     }
 }
 
-/// An implementation of generic storage device.
-pub struct StorageDeviceGeneric {
-    path: String,
-}
-
-impl std::fmt::Debug for StorageDeviceGeneric {
-    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
-        f.debug_struct("StorageState")
-            .field("path", &self.path)
-            .finish()
-    }
-}
-
-impl StorageDeviceGeneric {
-    /// Create a new instance of `StorageStateCommon`.
-    pub fn new(path: String) -> Self {
-        StorageDeviceGeneric { path }
-    }
-}
-
-impl StorageDevice for StorageDeviceGeneric {
-    fn path(&self) -> &str {
-        &self.path
-    }
-
-    fn cleanup(&self) {}
-}
-
 /// Trait object for storage device.
 pub trait StorageDevice: Send + Sync {
     /// Path
-    fn path(&self) -> &str;
+    fn path(&self) -> Option<&str>;
 
     /// Clean up resources related to the storage device.
-    fn cleanup(&self);
+    fn cleanup(&self) -> Result<()>;
 }
 
 /// Manager to manage registered storage device handlers.