From e4727827ab4293d99cc97112426e59cbf6b437cb Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Sun, 16 Nov 2025 12:27:32 -0500 Subject: [PATCH] libvirt: Add --replace to run, make rm -f stop by default When I say `--force` I mean it; needing to stop the VM manually was just annoying. Add run --replace/-R to forcibly destroy and recreate a VM with the same name, matching podman run --replace behavior. This is useful for quick iteration during development and testing. Assisted-by: Claude Code (Sonnet 4.5) Signed-off-by: Colin Walters --- Cargo.lock | 1 + crates/integration-tests/Cargo.toml | 1 + .../src/tests/libvirt_verb.rs | 211 +++++++++++++++--- crates/kit/src/libvirt/rm.rs | 152 +++++++++---- crates/kit/src/libvirt/run.rs | 20 +- docs/src/man/bcvk-libvirt-rm.md | 4 +- docs/src/man/bcvk-libvirt-run.md | 4 + 7 files changed, 311 insertions(+), 82 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6ee4150..ea97a0f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1320,6 +1320,7 @@ dependencies = [ "libtest-mimic", "linkme", "paste", + "rand 0.9.2", "regex", "serde", "serde_json", diff --git a/crates/integration-tests/Cargo.toml b/crates/integration-tests/Cargo.toml index c86cbe6..4b388ef 100644 --- a/crates/integration-tests/Cargo.toml +++ b/crates/integration-tests/Cargo.toml @@ -34,6 +34,7 @@ camino = "1.1.12" regex = "1" linkme = "0.3.30" paste = "1.0" +rand = { workspace = true } [lints] workspace = true diff --git a/crates/integration-tests/src/tests/libvirt_verb.rs b/crates/integration-tests/src/tests/libvirt_verb.rs index 71e5380..66815e8 100644 --- a/crates/integration-tests/src/tests/libvirt_verb.rs +++ b/crates/integration-tests/src/tests/libvirt_verb.rs @@ -17,6 +17,16 @@ use crate::{ }; use bcvk::xml_utils::parse_xml_dom; +/// Generate a random alphanumeric suffix for VM names to avoid collisions +fn random_suffix() -> String { + use rand::{distr::Alphanumeric, Rng}; + rand::rng() + .sample_iter(&Alphanumeric) + .take(8) + .map(char::from) + .collect() +} + /// Test libvirt list functionality (lists domains) fn test_libvirt_list_functionality() -> Result<()> { let bck = get_bck_command()?; @@ -210,13 +220,7 @@ fn test_libvirt_comprehensive_workflow() -> Result<()> { let bck = get_bck_command()?; // Generate unique domain name for this test - let domain_name = format!( - "test-workflow-{}", - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap() - .as_secs() - ); + let domain_name = format!("test-workflow-{}", random_suffix()); println!( "Testing comprehensive libvirt workflow for domain: {}", @@ -383,7 +387,128 @@ fn test_libvirt_comprehensive_workflow() -> Result<()> { ); println!("✓ VM is running and accessible"); - // Cleanup domain + // Test 6: Test `rm -f` stops running VMs by default + println!("Test 6: Testing `rm -f` stops running VMs without --stop flag..."); + let rm_test_domain = create_test_vm_and_assert("test-rm", &test_image, &domain_name)?; + + // Verify it's running + let rm_dominfo_output = Command::new("virsh") + .args(&["dominfo", &rm_test_domain]) + .output() + .expect("Failed to run virsh dominfo for rm test VM"); + + let rm_info = String::from_utf8_lossy(&rm_dominfo_output.stdout); + assert!( + rm_info.contains("running") || rm_info.contains("idle"), + "Test VM should be running before rm test" + ); + println!("✓ Test VM is running: {}", rm_test_domain); + + // Test rm -f WITHOUT --stop flag (should succeed and stop the VM) + println!( + "Running `bcvk libvirt rm -f {}` (without --stop)...", + rm_test_domain + ); + let rm_output = + run_bcvk(&["libvirt", "rm", "-f", &rm_test_domain]).expect("Failed to run libvirt rm -f"); + + assert!( + rm_output.success(), + "rm -f should succeed in stopping and removing running VM without --stop flag. stderr: {}", + rm_output.stderr + ); + println!("✓ rm -f successfully stopped and removed running VM"); + + // Verify the VM is actually removed + let list_check = Command::new("virsh") + .args(&["list", "--all", "--name"]) + .output() + .expect("Failed to list domains"); + + let domain_list = String::from_utf8_lossy(&list_check.stdout); + assert!( + !domain_list.contains(&rm_test_domain), + "VM should be removed after rm -f" + ); + println!("✓ VM successfully removed from domain list"); + + // Test 7: Test `run --replace` replaces existing VM + println!("Test 7: Testing `run --replace` replaces existing VM..."); + let replace_test_domain = create_test_vm_and_assert("test-replace", &test_image, &domain_name)?; + + // Verify initial VM exists + let initial_list = Command::new("virsh") + .args(&["list", "--all", "--name"]) + .output() + .expect("Failed to list domains"); + + let initial_domain_list = String::from_utf8_lossy(&initial_list.stdout); + assert!( + initial_domain_list.contains(&replace_test_domain), + "Initial VM should exist before replace" + ); + println!("✓ Initial VM exists: {}", replace_test_domain); + + // Run with --replace flag (should replace existing VM) + println!( + "Running `bcvk libvirt run --replace --name {}`...", + replace_test_domain + ); + let replace_output = run_bcvk(&[ + "libvirt", + "run", + "--replace", + "--name", + &replace_test_domain, + "--label", + LIBVIRT_INTEGRATION_TEST_LABEL, + "--filesystem", + "ext4", + &test_image, + ]) + .expect("Failed to run libvirt run --replace"); + + if !replace_output.success() { + cleanup_domain(&replace_test_domain); + cleanup_domain(&domain_name); + panic!( + "Failed to replace VM with --replace flag: {}", + replace_output.stderr + ); + } + println!("✓ Successfully replaced VM with --replace flag"); + + // Verify VM still exists with same name + let replaced_list = Command::new("virsh") + .args(&["list", "--all", "--name"]) + .output() + .expect("Failed to list domains"); + + let replaced_domain_list = String::from_utf8_lossy(&replaced_list.stdout); + assert!( + replaced_domain_list.contains(&replace_test_domain), + "Replaced VM should exist with same name" + ); + println!("✓ Replaced VM exists with same name"); + + // Verify it's a fresh VM (should be running) + let replaced_dominfo = Command::new("virsh") + .args(&["dominfo", &replace_test_domain]) + .output() + .expect("Failed to run virsh dominfo for replaced VM"); + + let replaced_info = String::from_utf8_lossy(&replaced_dominfo.stdout); + assert!( + replaced_info.contains("running") || replaced_info.contains("idle"), + "Replaced VM should be running" + ); + println!("✓ Replaced VM is running (fresh instance)"); + + // Cleanup replace test VM + cleanup_domain(&replace_test_domain); + println!("✓ Cleaned up replace test VM"); + + // Cleanup original domain cleanup_domain(&domain_name); println!("✓ Comprehensive workflow test passed"); @@ -419,6 +544,44 @@ fn cleanup_domain(domain_name: &str) { } } +/// Helper function to create a test VM and assert success +/// +/// Creates a VM using run_bcvk with the given prefix and test image. +/// Cleans up the original domain on error and returns the created domain name. +fn create_test_vm_and_assert( + domain_prefix: &str, + test_image: &str, + original_domain: &str, +) -> Result { + let domain_name = format!("{}-{}", domain_prefix, random_suffix()); + + println!("Creating test VM: {}", domain_name); + let create_output = run_bcvk(&[ + "libvirt", + "run", + "--name", + &domain_name, + "--label", + LIBVIRT_INTEGRATION_TEST_LABEL, + "--filesystem", + "ext4", + test_image, + ]) + .expect("Failed to create test VM"); + + if !create_output.success() { + cleanup_domain(&domain_name); + cleanup_domain(original_domain); + panic!( + "Failed to create test VM '{}': {}", + domain_name, create_output.stderr + ); + } + + println!("✓ Test VM created: {}", domain_name); + Ok(domain_name) +} + /// Check if libvirt supports readonly virtiofs (requires libvirt 11.0+) /// Returns true if supported, false if not supported fn check_libvirt_supports_readonly_virtiofs() -> Result { @@ -564,13 +727,7 @@ fn test_libvirt_run_bind_storage_ro() -> Result<()> { let test_image = get_test_image(); // Generate unique domain name for this test - let domain_name = format!( - "test-bind-storage-{}", - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap() - .as_secs() - ); + let domain_name = format!("test-bind-storage-{}", random_suffix()); println!("Testing --bind-storage-ro with domain: {}", domain_name); @@ -709,13 +866,7 @@ fn test_libvirt_run_no_storage_opts_without_bind_storage() -> Result<()> { let test_image = get_test_image(); // Generate unique domain name for this test - let domain_name = format!( - "test-no-storage-opts-{}", - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap() - .as_secs() - ); + let domain_name = format!("test-no-storage-opts-{}", random_suffix()); println!( "Testing that STORAGE_OPTS are not injected without --bind-storage-ro for domain: {}", @@ -855,13 +1006,7 @@ fn test_libvirt_run_transient_vm() -> Result<()> { let test_image = get_test_image(); // Generate unique domain name for this test - let domain_name = format!( - "test-transient-{}", - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap() - .as_secs() - ); + let domain_name = format!("test-transient-{}", random_suffix()); println!("Testing transient VM with domain: {}", domain_name); @@ -1021,13 +1166,7 @@ fn test_libvirt_run_bind_mounts() -> Result<()> { let test_image = get_test_image(); // Generate unique domain name for this test - let domain_name = format!( - "test-bind-mounts-{}", - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap() - .as_secs() - ); + let domain_name = format!("test-bind-mounts-{}", random_suffix()); println!("Testing bind mounts and kargs with domain: {}", domain_name); diff --git a/crates/kit/src/libvirt/rm.rs b/crates/kit/src/libvirt/rm.rs index 0739bcb..3078d41 100644 --- a/crates/kit/src/libvirt/rm.rs +++ b/crates/kit/src/libvirt/rm.rs @@ -12,15 +12,110 @@ pub struct LibvirtRmOpts { /// Name of the domain to remove pub name: String, - /// Force removal without confirmation + /// Force removal without confirmation (also stops running VMs) #[clap(long, short = 'f')] pub force: bool, - /// Remove domain even if it's running + /// Stop domain if it's running (implied by --force) #[clap(long)] pub stop: bool, } +/// Core removal implementation that accepts pre-fetched domain state and info +/// +/// This private function performs the actual removal logic without fetching +/// domain information, allowing callers to optimize by reusing already-fetched data. +fn remove_vm_impl( + global_opts: &crate::libvirt::LibvirtOptions, + vm_name: &str, + state: &str, + domain_info: &crate::domain_list::PodmanBootcDomain, + stop_if_running: bool, +) -> Result<()> { + use color_eyre::eyre::Context; + + // Check if VM is running + if state == "running" { + if stop_if_running { + let output = global_opts + .virsh_command() + .args(&["destroy", vm_name]) + .output() + .with_context(|| "Failed to stop VM before removal")?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + return Err(color_eyre::eyre::eyre!( + "Failed to stop VM '{}' before removal: {}", + vm_name, + stderr + )); + } + } else { + return Err(color_eyre::eyre::eyre!( + "VM '{}' is running. Cannot remove without stopping.", + vm_name + )); + } + } + + // Remove disk manually if it exists (unmanaged storage) + if let Some(ref disk_path) = domain_info.disk_path { + if std::path::Path::new(disk_path).exists() { + std::fs::remove_file(disk_path) + .with_context(|| format!("Failed to remove disk file: {}", disk_path))?; + } + } + + // Remove libvirt domain with nvram and storage + let output = global_opts + .virsh_command() + .args(&["undefine", vm_name, "--nvram", "--remove-all-storage"]) + .output() + .with_context(|| "Failed to undefine libvirt domain")?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + return Err(color_eyre::eyre::eyre!( + "Failed to remove libvirt domain: {}", + stderr + )); + } + + Ok(()) +} + +/// Remove a VM without confirmation +/// +/// This is the core removal logic that can be reused by other commands. +/// It assumes the caller has already confirmed the operation. +pub fn remove_vm_forced( + global_opts: &crate::libvirt::LibvirtOptions, + vm_name: &str, + stop_if_running: bool, +) -> Result<()> { + use crate::domain_list::DomainLister; + use color_eyre::eyre::Context; + + let connect_uri = global_opts.connect.as_ref(); + let lister = match connect_uri { + Some(uri) => DomainLister::with_connection(uri.clone()), + None => DomainLister::new(), + }; + + // Check if domain exists and get its state + let state = lister + .get_domain_state(vm_name) + .map_err(|_| color_eyre::eyre::eyre!("VM '{}' not found", vm_name))?; + + // Get domain info for disk cleanup + let domain_info = lister + .get_domain_info(vm_name) + .with_context(|| format!("Failed to get info for VM '{}'", vm_name))?; + + remove_vm_impl(global_opts, vm_name, &state, &domain_info, stop_if_running) +} + /// Execute the libvirt rm command pub fn run(global_opts: &crate::libvirt::LibvirtOptions, opts: LibvirtRmOpts) -> Result<()> { use crate::domain_list::DomainLister; @@ -44,25 +139,12 @@ pub fn run(global_opts: &crate::libvirt::LibvirtOptions, opts: LibvirtRmOpts) -> // Check if VM is running if state == "running" { - if opts.stop { - println!("🛑 Stopping running VM '{}'...", opts.name); - let output = global_opts - .virsh_command() - .args(&["destroy", &opts.name]) - .output() - .with_context(|| "Failed to stop VM before removal")?; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - return Err(color_eyre::eyre::eyre!( - "Failed to stop VM '{}' before removal: {}", - opts.name, - stderr - )); - } + // --force implies --stop + if opts.stop || opts.force { + println!("Stopping running VM '{}'...", opts.name); } else { return Err(color_eyre::eyre::eyre!( - "VM '{}' is running. Use --stop to force removal or stop it first.", + "VM '{}' is running. Use --stop or --force to remove a running VM, or stop it first.", opts.name )); } @@ -88,30 +170,14 @@ pub fn run(global_opts: &crate::libvirt::LibvirtOptions, opts: LibvirtRmOpts) -> println!("Removing VM '{}'...", opts.name); - // Remove disk manually if it exists (unmanaged storage) - if let Some(ref disk_path) = domain_info.disk_path { - println!(" Removing disk image..."); - if std::path::Path::new(disk_path).exists() { - std::fs::remove_file(disk_path) - .with_context(|| format!("Failed to remove disk file: {}", disk_path))?; - } - } - - // Remove libvirt domain with nvram and storage - println!(" Removing libvirt domain..."); - let output = global_opts - .virsh_command() - .args(&["undefine", &opts.name, "--nvram", "--remove-all-storage"]) - .output() - .with_context(|| "Failed to undefine libvirt domain")?; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - return Err(color_eyre::eyre::eyre!( - "Failed to remove libvirt domain: {}", - stderr - )); - } + // Use the optimized removal implementation with already-fetched info + remove_vm_impl( + global_opts, + &opts.name, + &state, + &domain_info, + opts.stop || opts.force, + )?; println!("VM '{}' removed successfully", opts.name); Ok(()) diff --git a/crates/kit/src/libvirt/run.rs b/crates/kit/src/libvirt/run.rs index b6ee8fd..9f8321e 100644 --- a/crates/kit/src/libvirt/run.rs +++ b/crates/kit/src/libvirt/run.rs @@ -210,6 +210,10 @@ pub struct LibvirtRunOpts { #[clap(long)] pub name: Option, + /// Replace existing VM with same name (stop and remove if exists) + #[clap(long, short = 'R')] + pub replace: bool, + #[clap( long, help = "Instance type (e.g., u1.nano, u1.small, u1.medium). Overrides cpus/memory if specified." @@ -408,7 +412,21 @@ pub fn run(global_opts: &crate::libvirt::LibvirtOptions, mut opts: LibvirtRunOpt let vm_name = match &opts.name { Some(name) => { if existing_domains.contains(name) { - return Err(color_eyre::eyre::eyre!("VM '{}' already exists", name)); + if opts.replace { + // Replace mode: remove the existing VM + println!("Replacing existing VM '{}'...", name); + crate::libvirt::rm::remove_vm_forced( + global_opts, + name, + true, // stop if running + ) + .with_context(|| format!("Failed to remove existing VM '{}'", name))?; + } else { + return Err(color_eyre::eyre::eyre!( + "VM '{}' already exists. Use --replace to replace it.", + name + )); + } } name.clone() } diff --git a/docs/src/man/bcvk-libvirt-rm.md b/docs/src/man/bcvk-libvirt-rm.md index cae5367..244910b 100644 --- a/docs/src/man/bcvk-libvirt-rm.md +++ b/docs/src/man/bcvk-libvirt-rm.md @@ -21,11 +21,11 @@ Remove a libvirt domain and its resources **-f**, **--force** - Force removal without confirmation + Force removal without confirmation (also stops running VMs) **--stop** - Remove domain even if it's running + Stop domain if it's running (implied by --force) diff --git a/docs/src/man/bcvk-libvirt-run.md b/docs/src/man/bcvk-libvirt-run.md index 511266e..4778775 100644 --- a/docs/src/man/bcvk-libvirt-run.md +++ b/docs/src/man/bcvk-libvirt-run.md @@ -23,6 +23,10 @@ Run a bootable container as a persistent VM Name for the VM (auto-generated if not specified) +**-R**, **--replace** + + Replace existing VM with same name (stop and remove if exists) + **--itype**=*ITYPE* Instance type (e.g., u1.nano, u1.small, u1.medium). Overrides cpus/memory if specified.