diff --git a/crates/kit/src/cache_metadata.rs b/crates/kit/src/cache_metadata.rs index 52dfa9e..4a8ae79 100644 --- a/crates/kit/src/cache_metadata.rs +++ b/crates/kit/src/cache_metadata.rs @@ -9,14 +9,13 @@ //! - The container image digest for visibility and tracking use crate::install_options::InstallOptions; -use cap_std_ext::cap_std::{self, fs::Dir}; +use cap_std_ext::cap_std::fs::Dir; use cap_std_ext::dirext::CapStdExtDirExt; use color_eyre::{eyre::Context, Result}; use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; use std::ffi::OsStr; use std::fs::File; -use std::path::Path; /// Extended attribute name for storing bootc cache hash const BOOTC_CACHE_HASH_XATTR: &str = "user.bootc.cache_hash"; @@ -115,32 +114,13 @@ impl DiskImageMetadata { Ok(()) } - /// Read image digest from a file path using extended attributes - pub fn read_image_digest_from_path(path: &Path) -> Result> { - // First check if file exists - if !path.exists() { - return Ok(None); - } - - // Get the parent directory and file name - // Use current directory if parent is empty (for bare filenames like "disk.img") - let parent = path - .parent() - .filter(|p| !p.as_os_str().is_empty()) - .unwrap_or(Path::new(".")); - let file_name = path - .file_name() - .ok_or_else(|| color_eyre::eyre::eyre!("Path has no file name"))?; - - // Open the parent directory with cap-std - let dir = Dir::open_ambient_dir(parent, cap_std::ambient_authority()) - .with_context(|| format!("Failed to open directory {:?}", parent))?; - + /// Read image digest from a file using extended attributes + pub fn read_image_digest_from_path(dir: &Dir, file_name: &OsStr) -> Result> { // Get the image digest xattr let digest_data = match dir.getxattr(file_name, OsStr::new(BOOTC_IMAGE_DIGEST_XATTR))? { Some(data) => data, None => { - tracing::debug!("No image digest xattr found on {:?}", path); + tracing::debug!("No image digest xattr found on {:?}", file_name); return Ok(None); } }; @@ -148,7 +128,7 @@ impl DiskImageMetadata { let digest = std::str::from_utf8(&digest_data) .with_context(|| "Invalid UTF-8 in image digest xattr")?; - tracing::debug!("Read image digest from {:?}: {}", path, digest); + tracing::debug!("Read image digest from {:?}: {}", file_name, digest); Ok(Some(digest.to_string())) } } @@ -179,12 +159,15 @@ pub(crate) enum ValidationError { /// Check if a cached disk image can be reused by comparing cache hashes pub fn check_cached_disk( - path: &Path, + dir: &Dir, + file_name: impl AsRef, image_digest: &str, install_options: &InstallOptions, ) -> Result> { - if !path.exists() { - tracing::debug!("Disk image {:?} does not exist", path); + let file_name = file_name.as_ref(); + + if !dir.try_exists(file_name)? { + tracing::debug!("Disk image {:?} does not exist", file_name); return Ok(Err(ValidationError::MissingFile)); } @@ -193,24 +176,12 @@ pub fn check_cached_disk( let expected_hash = expected_meta.compute_cache_hash(); // Read the cache hash from the disk image - // Use current directory if parent is empty (for bare filenames like "disk.img") - let parent = path - .parent() - .filter(|p| !p.as_os_str().is_empty()) - .unwrap_or(Path::new(".")); - let file_name = path - .file_name() - .ok_or_else(|| color_eyre::eyre::eyre!("Path has no file name"))?; - - let dir = Dir::open_ambient_dir(parent, cap_std::ambient_authority()) - .with_context(|| format!("Failed to open directory {:?}", parent))?; - let cached_hash = match dir.getxattr(file_name, OsStr::new(BOOTC_CACHE_HASH_XATTR))? { Some(data) => std::str::from_utf8(&data) .with_context(|| "Invalid UTF-8 in cache hash xattr")? .to_string(), None => { - tracing::debug!("No cache hash xattr found on {:?}", path); + tracing::debug!("No cache hash xattr found on {:?}", file_name); return Ok(Err(ValidationError::MissingXattr)); } }; @@ -218,16 +189,16 @@ pub fn check_cached_disk( let matches = expected_hash == cached_hash; if matches { tracing::debug!( - "Found cached disk image at {:?} matching cache hash {}", - path, + "Found cached disk image {:?} matching cache hash {}", + file_name, expected_hash ); Ok(Ok(())) } else { tracing::debug!( - "Cached disk at {:?} does not match requirements. \ + "Cached disk {:?} does not match requirements. \ Expected hash: {}, found: {}", - path, + file_name, expected_hash, cached_hash ); diff --git a/crates/kit/src/libvirt/base_disks.rs b/crates/kit/src/libvirt/base_disks.rs index 73e7f01..9eeb43b 100644 --- a/crates/kit/src/libvirt/base_disks.rs +++ b/crates/kit/src/libvirt/base_disks.rs @@ -7,9 +7,11 @@ use crate::cache_metadata::DiskImageMetadata; use crate::install_options::InstallOptions; use camino::{Utf8Path, Utf8PathBuf}; +use cap_std_ext::cap_std; +use cap_std_ext::cap_std::fs::Dir; +use cap_std_ext::dirext::CapStdExtDirExt; use color_eyre::eyre::{eyre, Context}; use color_eyre::Result; -use std::fs; use tracing::{debug, info}; /// Find or create a base disk for the given parameters @@ -33,47 +35,56 @@ pub fn find_or_create_base_disk( let base_disk_name = format!("bootc-base-{}.qcow2", short_hash); // Get storage pool path + let rootfs = Dir::open_ambient_dir("/", cap_std::ambient_authority())?; let pool_path = super::run::get_libvirt_storage_pool_path(connect_uri)?; + let pool_path = pool_path.strip_prefix("/").unwrap_or(&pool_path); let base_disk_path = pool_path.join(&base_disk_name); // Check if base disk already exists with valid metadata - if base_disk_path.exists() { - debug!("Checking existing base disk: {:?}", base_disk_path); + if let Some(pool_dir) = rootfs.open_dir_optional(pool_path)? { + if pool_dir.try_exists(&base_disk_name)? { + debug!("Checking existing base disk: {:?}", base_disk_path); + + if crate::cache_metadata::check_cached_disk( + &pool_dir, + &base_disk_name, + image_digest, + install_options, + )? + .is_ok() + { + return Ok(base_disk_path); + } else { + info!("Base disk exists but metadata doesn't match, will recreate"); + pool_dir.remove_file(&base_disk_name).with_context(|| { + format!("Failed to remove stale base disk: {:?}", base_disk_path) + })?; + } + } - if crate::cache_metadata::check_cached_disk( - base_disk_path.as_std_path(), + // Base disk doesn't exist or was stale, create it + // Multiple concurrent processes may race to create this, but each uses + // a unique temp file, so they won't conflict + info!("Creating base disk: {:?}", base_disk_path); + create_base_disk( + &pool_dir, + Utf8Path::new(&base_disk_name), + source_image, image_digest, install_options, - )? - .is_ok() - { - return Ok(base_disk_path); - } else { - info!("Base disk exists but metadata doesn't match, will recreate"); - fs::remove_file(&base_disk_path).with_context(|| { - format!("Failed to remove stale base disk: {:?}", base_disk_path) - })?; - } - } + connect_uri, + )?; - // Base disk doesn't exist or was stale, create it - // Multiple concurrent processes may race to create this, but each uses - // a unique temp file, so they won't conflict - info!("Creating base disk: {:?}", base_disk_path); - create_base_disk( - &base_disk_path, - source_image, - image_digest, - install_options, - connect_uri, - )?; - - Ok(base_disk_path) + Ok(base_disk_path) + } else { + Err(eyre!("Pool directory does not exist: {}", pool_path)) + } } /// Create a new base disk fn create_base_disk( - base_disk_path: &Utf8Path, + dir: &Dir, + base_disk_name: &Utf8Path, source_image: &str, image_digest: &str, install_options: &InstallOptions, @@ -84,18 +95,29 @@ fn create_base_disk( // Use a unique temporary file to avoid conflicts when multiple processes // race to create the same base disk - let temp_file = tempfile::Builder::new() - .prefix(&format!("{}.", base_disk_path.file_stem().unwrap())) - .suffix(".tmp.qcow2") - .tempfile_in(base_disk_path.parent().unwrap()) - .with_context(|| { - format!( - "Failed to create temp file in {:?}", - base_disk_path.parent() - ) - })?; - - let temp_disk_path = Utf8PathBuf::from(temp_file.path().to_str().unwrap()); + // We use a temp name pattern that includes a unique component to avoid collisions + let temp_name = format!(".{}.{}.tmp", base_disk_name, std::process::id()); + + // Get the actual path to use (need absolute path for to_disk) + let pool_path = super::run::get_libvirt_storage_pool_path(connect_uri)?; + let temp_disk_path = pool_path.join(&temp_name); + + // Create a guard that will clean up the temp file on drop + struct TempFileGuard<'a> { + dir: &'a Dir, + name: String, + } + + impl Drop for TempFileGuard<'_> { + fn drop(&mut self) { + let _ = self.dir.remove_file(&self.name); + } + } + + let _temp_guard = TempFileGuard { + dir, + name: temp_name.clone(), + }; // Keep the temp file open so it gets cleaned up automatically if we error out // We'll persist it manually on success @@ -122,64 +144,59 @@ fn create_base_disk( }; // Run bootc install - if it succeeds, the disk is valid - // On error, temp_file is automatically cleaned up when dropped + // On error, temp_guard is automatically cleaned up when dropped crate::to_disk::run(to_disk_opts) .with_context(|| format!("Failed to install bootc to base disk: {:?}", temp_disk_path))?; // If we got here, bootc install succeeded - verify metadata was written - let metadata_valid = crate::cache_metadata::check_cached_disk( - temp_disk_path.as_std_path(), - image_digest, - install_options, - ) - .context("Querying cached disk")?; - - match metadata_valid { - Ok(()) => { - // All validations passed - persist temp file to final location - // If another concurrent process already created the file, that's fine - match temp_file.persist(base_disk_path) { - Ok(_) => { - debug!("Successfully created base disk: {:?}", base_disk_path); - } - Err(e) if e.error.kind() == std::io::ErrorKind::AlreadyExists => { - // Another process won the race and created the base disk - debug!( - "Base disk already created by another process: {:?}", - base_disk_path - ); - // temp file is cleaned up when e is dropped - } - Err(e) => { - return Err(e.error).with_context(|| { - format!("Failed to persist base disk to {:?}", base_disk_path) - }); - } - } - - // Refresh libvirt storage pool so the new disk is visible to virsh - let mut cmd = super::run::virsh_command(connect_uri)?; - cmd.args(&["pool-refresh", "default"]); - - if let Err(e) = cmd - .output() - .with_context(|| "Failed to run virsh pool-refresh") - { - debug!("Warning: Failed to refresh libvirt storage pool: {}", e); - // Don't fail if pool refresh fails, the disk was created successfully - } + // Verify by reading back the xattrs using cap-std + match crate::cache_metadata::check_cached_disk(dir, &temp_name, image_digest, install_options)? + { + Ok(()) => {} + Err(e) => { + return Err(eyre!("Generated disk metadata validation failed: {e}")); + } + } - info!( - "Successfully created and validated base disk: {:?}", - base_disk_path + // All validations passed - rename temp file to final location + // If another concurrent process already created the file, that's fine + match dir.rename(&temp_name, dir, base_disk_name) { + Ok(_) => { + debug!("Successfully created base disk: {:?}", base_disk_name); + // Don't clean up the temp file since we successfully renamed it + std::mem::forget(_temp_guard); + } + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { + // Another process won the race and created the base disk + debug!( + "Base disk already created by another process: {:?}", + base_disk_name ); - Ok(()) + // temp file is cleaned up when _temp_guard is dropped } Err(e) => { - // temp_file will be automatically cleaned up when dropped - Err(eyre!("Generated disk metadata validation failed: {e}")) + return Err(e) + .with_context(|| format!("Failed to persist base disk to {:?}", base_disk_name)); } } + + // Refresh libvirt storage pool so the new disk is visible to virsh + let mut cmd = super::run::virsh_command(connect_uri)?; + cmd.args(&["pool-refresh", "default"]); + + if let Err(e) = cmd + .output() + .with_context(|| "Failed to run virsh pool-refresh") + { + debug!("Warning: Failed to refresh libvirt storage pool: {}", e); + // Don't fail if pool refresh fails, the disk was created successfully + } + + info!( + "Successfully created and validated base disk: {:?}", + base_disk_name + ); + Ok(()) } /// Clone a base disk to create a VM-specific disk @@ -191,7 +208,12 @@ pub fn clone_from_base( vm_name: &str, connect_uri: Option<&str>, ) -> Result { + let rootfs = Dir::open_ambient_dir("/", cap_std::ambient_authority())?; let pool_path = super::run::get_libvirt_storage_pool_path(connect_uri)?; + let pool_path = pool_path.strip_prefix("/").unwrap_or(&pool_path); + let pool_dir = rootfs + .open_dir(pool_path) + .with_context(|| format!("Failed to open pool directory {:?}", pool_path))?; // Use predictable disk name let vm_disk_name = format!("{}.qcow2", vm_name); @@ -231,9 +253,10 @@ pub fn clone_from_base( } // Also remove the file if it exists but wasn't tracked by libvirt - if vm_disk_path.exists() { + if pool_dir.try_exists(&vm_disk_name)? { debug!("Removing untracked disk file: {:?}", vm_disk_path); - fs::remove_file(&vm_disk_path) + pool_dir + .remove_file(&vm_disk_name) .with_context(|| format!("Failed to remove disk file: {:?}", vm_disk_path))?; } @@ -243,7 +266,7 @@ pub fn clone_from_base( ); // Get the virtual size of the base disk to use for the new volume - let info = crate::qemu_img::info(base_disk_path)?; + let info = crate::qemu_img::info(&pool_dir, base_disk_path)?; let virtual_size = info.virtual_size; // Create volume with backing file using vol-create-as @@ -289,6 +312,8 @@ pub fn clone_from_base( pub fn list_base_disks(connect_uri: Option<&str>) -> Result> { use super::run::list_storage_pool_volumes; + let rootfs = Dir::open_ambient_dir("/", cap_std::ambient_authority())?; + let pool_path = super::run::get_libvirt_storage_pool_path(connect_uri)?; let mut base_disks = Vec::new(); @@ -305,37 +330,43 @@ pub fn list_base_disks(connect_uri: Option<&str>) -> Result> { }) .collect(); - if let Ok(entries) = fs::read_dir(&pool_path) { - for entry in entries.flatten() { - if let Ok(file_name) = entry.file_name().into_string() { - // Check if this is a base disk - if file_name.starts_with("bootc-base-") && file_name.ends_with(".qcow2") { - let path = pool_path.join(&file_name); - - // Try to read metadata - let image_digest = - crate::cache_metadata::DiskImageMetadata::read_image_digest_from_path( - path.as_std_path(), - ) - .unwrap_or(None); - - // Get file size and creation time - let metadata = entry.metadata().ok(); - let size = metadata.as_ref().map(|m| m.len()); - let created = metadata.and_then(|m| m.created().ok()); - - // Count references - let ref_count = count_base_disk_references(&path, &vm_disks)?; - - base_disks.push(BaseDiskInfo { - path, - image_digest, - size, - ref_count, - created, - }); - } + if let Some(d) = rootfs.open_dir_optional(&pool_path)? { + for entry in d.entries()? { + let entry = entry?; + let file_name = entry.file_name(); + let Some(name) = file_name.to_str().map(Utf8Path::new) else { + continue; + }; + let path = pool_path.join(name); + let Some("qcow2") = name.extension() else { + continue; + }; + if !name.starts_with("bootc-base-") { + continue; } + + // Try to read metadata + let image_digest = + crate::cache_metadata::DiskImageMetadata::read_image_digest_from_path( + &d, &file_name, + ) + .unwrap_or(None); + + // Get file size and creation time + let metadata = entry.metadata()?; + let size = Some(metadata.len()); + let created = metadata.created()?.into_std(); + + // Count references + let ref_count = count_base_disk_references(&d, &name, &vm_disks)?; + + base_disks.push(BaseDiskInfo { + path, + image_digest, + size, + ref_count, + created, + }); } } @@ -349,13 +380,19 @@ pub struct BaseDiskInfo { pub image_digest: Option, pub size: Option, pub ref_count: usize, - pub created: Option, + pub created: std::time::SystemTime, } /// Prune unreferenced base disks pub fn prune_base_disks(connect_uri: Option<&str>, dry_run: bool) -> Result> { use super::run::list_storage_pool_volumes; + let rootfs = Dir::open_ambient_dir("/", cap_std::ambient_authority())?; + let pool_path = super::run::get_libvirt_storage_pool_path(connect_uri)?; + let pool_dir = rootfs + .open_dir(pool_path.strip_prefix("/").unwrap_or(&pool_path)) + .with_context(|| format!("Failed to open pool directory {:?}", pool_path))?; + let base_disks = list_base_disks(connect_uri)?; let all_volumes = list_storage_pool_volumes(connect_uri)?; @@ -375,7 +412,7 @@ pub fn prune_base_disks(connect_uri: Option<&str>, dry_run: bool) -> Result, dry_run: bool) -> Result Result { - let base_disk_name = base_disk.file_name().unwrap(); +fn count_base_disk_references( + dir: &Dir, + base_disk: impl AsRef, + vm_disks: &[&Utf8PathBuf], +) -> Result { + let base_disk = base_disk.as_ref(); + let base_disk_name = base_disk + .file_name() + .ok_or_else(|| eyre!("Missing filename {base_disk}"))?; let mut count = 0; for vm_disk in vm_disks { // Use qemu-img info with --force-share to allow reading even if disk is locked by a running VM - let info = match crate::qemu_img::info(vm_disk) { + let info = match crate::qemu_img::info(dir, vm_disk) { Ok(info) => info, Err(_) => { // If we can't read the disk, skip it for counting purposes @@ -452,12 +496,16 @@ fn count_base_disk_references(base_disk: &Utf8Path, vm_disks: &[&Utf8PathBuf]) - } /// Check if a base disk is referenced by any VM disk (via qcow2 backing file) -fn check_base_disk_referenced(base_disk: &Utf8Path, vm_disks: &[&Utf8PathBuf]) -> Result { +fn check_base_disk_referenced( + dir: &Dir, + base_disk: &Utf8Path, + vm_disks: &[&Utf8PathBuf], +) -> Result { let base_disk_name = base_disk.file_name().unwrap(); for vm_disk in vm_disks { // Use qemu-img info with --force-share to allow reading even if disk is locked by a running VM - let info = match crate::qemu_img::info(vm_disk) { + let info = match crate::qemu_img::info(dir, vm_disk) { Ok(info) => info, Err(e) => { // If we can't read the disk info, be conservative and assume it DOES reference this base diff --git a/crates/kit/src/libvirt/base_disks_cli.rs b/crates/kit/src/libvirt/base_disks_cli.rs index 1771d2f..1f3b63d 100644 --- a/crates/kit/src/libvirt/base_disks_cli.rs +++ b/crates/kit/src/libvirt/base_disks_cli.rs @@ -80,7 +80,8 @@ fn run_list(connect_uri: Option<&str>, opts: ListOpts) -> Result<()> { let created = disk .created - .and_then(|t| t.duration_since(std::time::UNIX_EPOCH).ok()) + .duration_since(std::time::UNIX_EPOCH) + .ok() .and_then(|d| chrono::DateTime::from_timestamp(d.as_secs() as i64, 0)) .map(|dt| dt.format("%Y-%m-%d %H:%M").to_string()) .unwrap_or_else(|| "unknown".to_string()); diff --git a/crates/kit/src/qemu_img.rs b/crates/kit/src/qemu_img.rs index 1047ac4..ee259d5 100644 --- a/crates/kit/src/qemu_img.rs +++ b/crates/kit/src/qemu_img.rs @@ -1,6 +1,7 @@ //! Helper functions for interacting with qemu-img use camino::Utf8Path; +use cap_std_ext::{cap_std::fs::Dir, cmdext::CapStdExtCommandExt}; use color_eyre::{eyre::Context, Result}; use serde::Deserialize; use std::process::Command; @@ -32,9 +33,10 @@ pub struct QemuImgInfo { /// /// The `--force-share` flag allows reading disk info even when the image /// is locked by a running VM. -pub fn info(path: &Utf8Path) -> Result { +pub fn info(dir: &Dir, path: &Utf8Path) -> Result { let output = Command::new("qemu-img") .args(["info", "--force-share", "--output=json", path.as_str()]) + .cwd_dir(dir.try_clone()?) .output() .with_context(|| format!("Failed to run qemu-img info on {:?}", path))?; diff --git a/crates/kit/src/to_disk.rs b/crates/kit/src/to_disk.rs index 3d85e67..b0b493e 100644 --- a/crates/kit/src/to_disk.rs +++ b/crates/kit/src/to_disk.rs @@ -74,6 +74,7 @@ //! ``` use std::io::IsTerminal; +use std::sync::Arc; use crate::cache_metadata::DiskImageMetadata; use crate::install_options::InstallOptions; @@ -81,6 +82,9 @@ use crate::run_ephemeral::{run_detached, CommonVmOpts, RunEphemeralOpts}; use crate::run_ephemeral_ssh::wait_for_ssh_ready; use crate::{images, ssh, utils}; use camino::Utf8PathBuf; +use cap_std_ext::cap_std; +use cap_std_ext::cap_std::fs::Dir; +use cap_std_ext::cmdext::CapStdExtCommandExt; use clap::{Parser, ValueEnum}; use color_eyre::eyre::{eyre, Context}; use color_eyre::Result; @@ -301,8 +305,14 @@ impl ToDiskOpts { /// Main entry point for the bootc installation process. See module-level documentation /// for details on the installation workflow and architecture. pub fn run(opts: ToDiskOpts) -> Result<()> { + let rootfs = &Dir::open_ambient_dir("/", cap_std::ambient_authority())?; + let target_disk = opts + .target_disk + .strip_prefix("/") + .unwrap_or(&opts.target_disk); + // Phase 0: Check for existing cached disk image - if opts.target_disk.exists() { + if rootfs.try_exists(target_disk)? { debug!( "Target disk {} already exists, checking cache metadata", opts.target_disk @@ -314,10 +324,13 @@ pub fn run(opts: ToDiskOpts) -> Result<()> { // Check if cached disk matches our requirements match crate::cache_metadata::check_cached_disk( - opts.target_disk.as_std_path(), + &rootfs, + target_disk, &image_digest, &opts.install, - )? { + ) + .context("Checking cached disk")? + { Ok(()) => { println!( "Reusing existing cached disk image (digest {image_digest}) at: {}", @@ -327,10 +340,6 @@ pub fn run(opts: ToDiskOpts) -> Result<()> { } Err(e) => { debug!("Existing disk does not match requirements, recreating: {e}"); - // Remove the existing disk so we can recreate it - std::fs::remove_file(&opts.target_disk).with_context(|| { - format!("Failed to remove existing disk {}", opts.target_disk) - })?; } } } @@ -351,12 +360,13 @@ pub fn run(opts: ToDiskOpts) -> Result<()> { let disk_size = opts.calculate_disk_size()?; + // Open the target + let file = std::fs::File::create(&opts.target_disk) + .with_context(|| format!("Opening {}", opts.target_disk))?; + // Create disk image based on format match opts.additional.format { Format::Raw => { - // Create sparse file - only allocates space as data is written - let file = std::fs::File::create(&opts.target_disk) - .with_context(|| format!("Opening {}", opts.target_disk))?; file.set_len(disk_size)?; // TODO pass to qemu via fdset drop(file); @@ -366,13 +376,8 @@ pub fn run(opts: ToDiskOpts) -> Result<()> { debug!("Creating qcow2 with size {} bytes", disk_size); let size_arg = disk_size.to_string(); let output = std::process::Command::new("qemu-img") - .args([ - "create", - "-f", - "qcow2", - opts.target_disk.as_str(), - &size_arg, - ]) + .args(["create", "-f", "qcow2", "/proc/self/fd/3", &size_arg]) + .take_fd_n(Arc::new(file.into()), 3) .output() .with_context(|| { format!("Failed to run qemu-img create for {}", opts.target_disk)