diff --git a/src/agent/src/sandbox.rs b/src/agent/src/sandbox.rs index aac0f81584..b4331ed4be 100644 --- a/src/agent/src/sandbox.rs +++ b/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); - self.cleanup_sandbox_storage(path)?; + if let Some(storage) = self.storages.remove(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()); diff --git a/src/agent/src/storage/mod.rs b/src/agent/src/storage/mod.rs index 80cc081faa..f312bbd83b 100644 --- a/src/agent/src/storage/mod.rs +++ b/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::{ - StorageDevice, StorageDeviceGeneric, StorageHandlerManager, KATA_SHAREDFS_GUEST_PREMOUNT_TAG, -}; +use kata_types::mount::{StorageDevice, 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>, } +/// An implementation of generic storage device. +#[derive(Default, Debug)] +pub struct StorageDeviceGeneric { + path: Option, +} + +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 !path.is_empty() { - mount_list.push(path.to_string()); + if let Some(path) = state.path() { + if !path.is_empty() { + 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 !path.is_empty() { - mount_list.push(path.clone()); + if let Some(path) = device.path() { + if !path.is_empty() { + 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) + } } diff --git a/src/libs/kata-types/src/mount.rs b/src/libs/kata-types/src/mount.rs index 521a24a4e7..0cccab574d 100644 --- a/src/libs/kata-types/src/mount.rs +++ b/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.