diff --git a/crates/integration-tests/src/tests/libvirt_base_disks.rs b/crates/integration-tests/src/tests/libvirt_base_disks.rs index c7dd9dd..9f19c08 100644 --- a/crates/integration-tests/src/tests/libvirt_base_disks.rs +++ b/crates/integration-tests/src/tests/libvirt_base_disks.rs @@ -14,21 +14,15 @@ use crate::{get_bck_command, get_test_image, run_bcvk}; pub fn test_base_disk_creation_and_reuse() { let test_image = get_test_image(); - // Generate unique names for test VMs - let timestamp = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap() - .as_secs(); - let vm1_name = format!("test-base-disk-vm1-{}", timestamp); - let vm2_name = format!("test-base-disk-vm2-{}", timestamp); + // Generate unique names for test VMs using shortuuid pattern + let vm1_name_template = "test-base-disk-vm1-{shortuuid}"; + let vm2_name_template = "test-base-disk-vm2-{shortuuid}"; println!("Testing base disk creation and reuse"); - println!("VM1: {}", vm1_name); - println!("VM2: {}", vm2_name); - // Cleanup any existing test domains - cleanup_domain(&vm1_name); - cleanup_domain(&vm2_name); + // Create temp files for domain names + let vm1_id_file = tempfile::NamedTempFile::new().expect("Failed to create temp file"); + let vm1_id_path = vm1_id_file.path().to_str().expect("Invalid temp file path"); // Create first VM - this should create a new base disk println!("Creating first VM (should create base disk)..."); @@ -36,7 +30,9 @@ pub fn test_base_disk_creation_and_reuse() { "libvirt", "run", "--name", - &vm1_name, + vm1_name_template, + "--write-id-to", + vm1_id_path, "--filesystem", "ext4", &test_image, @@ -47,12 +43,19 @@ pub fn test_base_disk_creation_and_reuse() { println!("VM1 stderr: {}", vm1_output.stderr); if !vm1_output.success() { - cleanup_domain(&vm1_name); - cleanup_domain(&vm2_name); - + // Attempt cleanup before panicking + let _ = std::fs::read_to_string(vm1_id_path).map(|name| cleanup_domain(name.trim())); panic!("Failed to create first VM: {}", vm1_output.stderr); } + // Read the domain name from the file + let vm1_name = std::fs::read_to_string(vm1_id_path) + .expect("Failed to read VM1 domain name from file") + .trim() + .to_string(); + + println!("Created VM1: {}", vm1_name); + // Verify base disk was created assert!( vm1_output.stdout.contains("Using base disk") || vm1_output.stdout.contains("base disk"), @@ -61,11 +64,16 @@ pub fn test_base_disk_creation_and_reuse() { // Create second VM - this should reuse the base disk println!("Creating second VM (should reuse base disk)..."); + let vm2_id_file = tempfile::NamedTempFile::new().expect("Failed to create temp file"); + let vm2_id_path = vm2_id_file.path().to_str().expect("Invalid temp file path"); + let vm2_output = run_bcvk(&[ "libvirt", "run", "--name", - &vm2_name, + vm2_name_template, + "--write-id-to", + vm2_id_path, "--filesystem", "ext4", &test_image, @@ -75,14 +83,24 @@ pub fn test_base_disk_creation_and_reuse() { println!("VM2 stdout: {}", vm2_output.stdout); println!("VM2 stderr: {}", vm2_output.stderr); - // Cleanup before assertions - cleanup_domain(&vm1_name); - cleanup_domain(&vm2_name); - if !vm2_output.success() { + // Cleanup VM1 before panicking + cleanup_domain(&vm1_name); panic!("Failed to create second VM: {}", vm2_output.stderr); } + // Read the domain name from the file + let vm2_name = std::fs::read_to_string(vm2_id_path) + .expect("Failed to read VM2 domain name from file") + .trim() + .to_string(); + + println!("Created VM2: {}", vm2_name); + + // Cleanup before assertions + cleanup_domain(&vm1_name); + cleanup_domain(&vm2_name); + // Verify base disk was reused (should be faster and mention using existing) assert!( vm2_output.stdout.contains("Using base disk") || vm2_output.stdout.contains("base disk"), @@ -170,22 +188,22 @@ pub fn test_base_disks_prune_dry_run() { pub fn test_vm_disk_references_base() { let test_image = get_test_image(); - let timestamp = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap() - .as_secs(); - let vm_name = format!("test-disk-ref-{}", timestamp); + let vm_name_template = "test-disk-ref-{shortuuid}"; println!("Testing VM disk references base disk"); - cleanup_domain(&vm_name); + // Create temp file for domain name + let id_file = tempfile::NamedTempFile::new().expect("Failed to create temp file"); + let id_path = id_file.path().to_str().expect("Invalid temp file path"); // Create VM let output = run_bcvk(&[ "libvirt", "run", "--name", - &vm_name, + vm_name_template, + "--write-id-to", + id_path, "--filesystem", "ext4", &test_image, @@ -193,11 +211,19 @@ pub fn test_vm_disk_references_base() { .expect("Failed to create VM"); if !output.success() { - cleanup_domain(&vm_name); - + // Attempt cleanup before panicking + let _ = std::fs::read_to_string(id_path).map(|name| cleanup_domain(name.trim())); panic!("Failed to create VM: {}", output.stderr); } + // Read the domain name from the file + let vm_name = std::fs::read_to_string(id_path) + .expect("Failed to read domain name from file") + .trim() + .to_string(); + + println!("Created VM: {}", vm_name); + // Get VM disk path from domain XML let dumpxml_output = Command::new("virsh") .args(&["dumpxml", &vm_name]) @@ -238,7 +264,6 @@ pub fn test_vm_disk_references_base() { println!("✓ VM disk reference test passed"); } -/// Helper function to cleanup domain and its disk fn cleanup_domain(domain_name: &str) { println!("Cleaning up domain: {}", domain_name); diff --git a/crates/integration-tests/src/tests/libvirt_verb.rs b/crates/integration-tests/src/tests/libvirt_verb.rs index cf164ff..e8a447f 100644 --- a/crates/integration-tests/src/tests/libvirt_verb.rs +++ b/crates/integration-tests/src/tests/libvirt_verb.rs @@ -193,30 +193,24 @@ pub fn test_libvirt_ssh_integration() { pub fn test_libvirt_run_ssh_full_workflow() { let test_image = get_test_image(); - // Generate unique domain name for this test - let domain_name = format!( - "test-ssh-{}", - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap() - .as_secs() - ); + // Generate unique domain name for this test using shortuuid pattern + let domain_name_template = "test-ssh-{shortuuid}"; - println!( - "Testing full libvirt run + SSH workflow with domain: {}", - domain_name - ); + println!("Testing full libvirt run + SSH workflow"); - // Cleanup any existing domain with this name - cleanup_domain(&domain_name); + // Create temp file for domain name + let id_file = tempfile::NamedTempFile::new().expect("Failed to create temp file"); + let id_path = id_file.path().to_str().expect("Invalid temp file path"); - // Create domain with SSH key generation + // Create domain with SSH key generation (name will be auto-generated) println!("Creating libvirt domain with SSH key injection..."); let create_output = run_bcvk(&[ "libvirt", "run", "--name", - &domain_name, + domain_name_template, + "--write-id-to", + id_path, "--label", LIBVIRT_INTEGRATION_TEST_LABEL, "--filesystem", @@ -229,11 +223,17 @@ pub fn test_libvirt_run_ssh_full_workflow() { println!("Create stderr: {}", create_output.stderr); if !create_output.success() { - cleanup_domain(&domain_name); - + // Attempt cleanup before panicking + let _ = std::fs::read_to_string(id_path).map(|name| cleanup_domain(name.trim())); panic!("Failed to create domain with SSH: {}", create_output.stderr); } + // Read the domain name from the file + let domain_name = std::fs::read_to_string(id_path) + .expect("Failed to read domain name from file") + .trim() + .to_string(); + println!("Successfully created domain: {}", domain_name); // Wait for VM to boot and SSH to become available @@ -335,43 +335,15 @@ fn wait_for_ssh_available( /// Test VM startup and shutdown with libvirt run pub fn test_libvirt_vm_lifecycle() { let bck = get_bck_command().unwrap(); - let test_volume = "test-vm-lifecycle"; - let domain_name = format!("bootc-{}", test_volume); - - // Guard to ensure cleanup always runs - struct VmCleanupGuard { - domain_name: String, - bck: String, - } - impl Drop for VmCleanupGuard { - fn drop(&mut self) { - // Try to stop the VM first - let _ = std::process::Command::new("virsh") - .args(&["destroy", &self.domain_name]) - .output(); - // Use bcvk libvirt rm for cleanup - let cleanup_output = std::process::Command::new(&self.bck) - .args(&["libvirt", "rm", &self.domain_name, "--force", "--stop"]) - .output(); - if let Ok(output) = cleanup_output { - if output.status.success() { - println!("Cleaned up VM domain: {}", self.domain_name); - } - } - } - } - - // Cleanup any existing test domain - let _ = std::process::Command::new("virsh") - .args(&["destroy", &domain_name]) - .output(); - let _ = std::process::Command::new(&bck) - .args(&["libvirt", "rm", &domain_name, "--force", "--stop"]) - .output(); + let domain_name_template = "bootc-lifecycle-{shortuuid}"; // Create a minimal test volume (skip if no bootc container available) let test_image = &get_test_image(); + // Create temp file for domain name + let id_file = tempfile::NamedTempFile::new().expect("Failed to create temp file"); + let id_path = id_file.path().to_str().expect("Invalid temp file path"); + // First try to create a domain from container image let output = std::process::Command::new(&bck) .args(&[ @@ -380,7 +352,9 @@ pub fn test_libvirt_vm_lifecycle() { "--filesystem", "ext4", "--name", - &domain_name, + domain_name_template, + "--write-id-to", + id_path, "--label", LIBVIRT_INTEGRATION_TEST_LABEL, test_image, @@ -393,8 +367,37 @@ pub fn test_libvirt_vm_lifecycle() { panic!("Failed to create VM: {}", stderr); } + // Read the domain name from the file + let domain_name = std::fs::read_to_string(id_path) + .expect("Failed to read domain name from file") + .trim() + .to_string(); + println!("Created VM domain: {}", domain_name); + // Guard to ensure cleanup always runs + struct VmCleanupGuard { + domain_name: String, + bck: String, + } + impl Drop for VmCleanupGuard { + fn drop(&mut self) { + // Try to stop the VM first + let _ = std::process::Command::new("virsh") + .args(&["destroy", &self.domain_name]) + .output(); + // Use bcvk libvirt rm for cleanup + let cleanup_output = std::process::Command::new(&self.bck) + .args(&["libvirt", "rm", &self.domain_name, "--force", "--stop"]) + .output(); + if let Ok(output) = cleanup_output { + if output.status.success() { + println!("Cleaned up VM domain: {}", self.domain_name); + } + } + } + } + // Set up cleanup guard after successful creation let _guard = VmCleanupGuard { domain_name: domain_name.clone(), @@ -464,19 +467,14 @@ pub fn test_libvirt_bind_storage_ro() { return; } - // 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() - ); + // Generate unique domain name for this test using shortuuid pattern + let domain_name_template = "test-bind-storage-{shortuuid}"; - println!("Testing --bind-storage-ro with domain: {}", domain_name); + println!("Testing --bind-storage-ro"); - // Cleanup any existing domain with this name - cleanup_domain(&domain_name); + // Create temp file for domain name + let id_file = tempfile::NamedTempFile::new().expect("Failed to create temp file"); + let id_path = id_file.path().to_str().expect("Invalid temp file path"); // Create domain with --bind-storage-ro flag println!("Creating libvirt domain with --bind-storage-ro..."); @@ -484,7 +482,9 @@ pub fn test_libvirt_bind_storage_ro() { "libvirt", "run", "--name", - &domain_name, + domain_name_template, + "--write-id-to", + id_path, "--label", LIBVIRT_INTEGRATION_TEST_LABEL, "--bind-storage-ro", @@ -498,13 +498,20 @@ pub fn test_libvirt_bind_storage_ro() { println!("Create stderr: {}", create_output.stderr); if !create_output.success() { - cleanup_domain(&domain_name); + // Attempt cleanup before panicking + let _ = std::fs::read_to_string(id_path).map(|name| cleanup_domain(name.trim())); panic!( "Failed to create domain with --bind-storage-ro: {}", create_output.stderr ); } + // Read the domain name from the file + let domain_name = std::fs::read_to_string(id_path) + .expect("Failed to read domain name from file") + .trim() + .to_string(); + println!("Successfully created domain: {}", domain_name); // Check that the domain was created with virtiofs filesystem @@ -644,22 +651,14 @@ pub fn test_libvirt_label_functionality() { let bck = get_bck_command().unwrap(); let test_image = get_test_image(); - // Generate unique domain name for this test - let domain_name = format!( - "test-label-{}", - std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap() - .as_secs() - ); + // Generate unique domain name for this test using shortuuid pattern + let domain_name_template = "test-label-{shortuuid}"; - println!( - "Testing libvirt label functionality with domain: {}", - domain_name - ); + println!("Testing libvirt label functionality"); - // Cleanup any existing domain with this name - cleanup_domain(&domain_name); + // Create temp file for domain name + let id_file = tempfile::NamedTempFile::new().expect("Failed to create temp file"); + let id_path = id_file.path().to_str().expect("Invalid temp file path"); // Create domain with multiple labels println!("Creating libvirt domain with multiple labels..."); @@ -667,7 +666,9 @@ pub fn test_libvirt_label_functionality() { "libvirt", "run", "--name", - &domain_name, + domain_name_template, + "--write-id-to", + id_path, "--label", LIBVIRT_INTEGRATION_TEST_LABEL, "--label", @@ -684,13 +685,20 @@ pub fn test_libvirt_label_functionality() { println!("Create stderr: {}", create_output.stderr); if !create_output.success() { - cleanup_domain(&domain_name); + // Attempt cleanup before panicking + let _ = std::fs::read_to_string(id_path).map(|name| cleanup_domain(name.trim())); panic!( "Failed to create domain with labels: {}", create_output.stderr ); } + // Read the domain name from the file + let domain_name = std::fs::read_to_string(id_path) + .expect("Failed to read domain name from file") + .trim() + .to_string(); + println!("Successfully created domain with labels: {}", domain_name); // Verify labels are stored in domain XML diff --git a/crates/kit/src/libvirt/run.rs b/crates/kit/src/libvirt/run.rs index 3380258..96efde3 100644 --- a/crates/kit/src/libvirt/run.rs +++ b/crates/kit/src/libvirt/run.rs @@ -102,6 +102,10 @@ pub struct LibvirtRunOpts { /// User-defined labels for organizing VMs (comma not allowed in labels) #[clap(long)] pub label: Vec, + + /// Write the domain name to the specified file path + #[clap(long)] + pub write_id_to: Option, } impl LibvirtRunOpts { @@ -138,14 +142,25 @@ pub fn run(global_opts: &crate::libvirt::LibvirtOptions, opts: LibvirtRunOpts) - // Generate or validate VM name let vm_name = match &opts.name { Some(name) => { - if existing_domains.contains(name) { - return Err(color_eyre::eyre::eyre!("VM '{}' already exists", name)); + // Check if name contains {shortuuid} pattern + if name.contains("{shortuuid}") { + generate_name_with_shortuuid(name, &existing_domains) + } else { + if existing_domains.contains(name) { + return Err(color_eyre::eyre::eyre!("VM '{}' already exists", name)); + } + name.clone() } - name.clone() } None => generate_unique_vm_name(&opts.image, &existing_domains), }; + // Write domain name to file if requested + if let Some(id_file) = &opts.write_id_to { + std::fs::write(id_file, format!("{}\n", vm_name)) + .with_context(|| format!("Failed to write domain name to {}", id_file))?; + } + println!( "Creating libvirt domain '{}' (install source container image: {})", vm_name, opts.image @@ -374,6 +389,41 @@ pub fn get_libvirt_storage_pool_path(connect_uri: Option<&str>) -> Result String { + uuid::Uuid::new_v4().simple().to_string()[..16].to_string() +} + +/// Generate a VM name by replacing {shortuuid} pattern with a unique identifier +/// Retries until a unique name is found (extremely unlikely to need more than one attempt) +fn generate_name_with_shortuuid(template: &str, existing_domains: &[String]) -> String { + // Try up to 100 times to generate a unique name (should succeed on first try) + for _ in 0..100 { + let shortuuid = generate_shortuuid(); + let candidate = template.replace("{shortuuid}", &shortuuid); + + if !existing_domains.contains(&candidate) { + return candidate; + } + } + + // Fallback: append a counter if somehow we failed to find unique name + let shortuuid = generate_shortuuid(); + let base = template.replace("{shortuuid}", &shortuuid); + if !existing_domains.contains(&base) { + return base; + } + + let mut counter = 1; + loop { + let candidate = format!("{}-{}", base, counter); + if !existing_domains.contains(&candidate) { + return candidate; + } + counter += 1; + } +} + /// Generate a unique VM name from an image name fn generate_unique_vm_name(image: &str, existing_domains: &[String]) -> String { // Extract image name from full image path @@ -534,6 +584,71 @@ fn check_libvirt_readonly_support() -> Result<()> { mod tests { use super::*; + #[test] + fn test_generate_shortuuid_length() { + let shortuuid = generate_shortuuid(); + assert_eq!(shortuuid.len(), 16); + } + + #[test] + fn test_generate_shortuuid_alphanumeric() { + let shortuuid = generate_shortuuid(); + assert!(shortuuid.chars().all(|c| c.is_ascii_alphanumeric())); + } + + #[test] + fn test_generate_shortuuid_unique() { + let uuid1 = generate_shortuuid(); + let uuid2 = generate_shortuuid(); + // Extremely unlikely to be equal + assert_ne!(uuid1, uuid2); + } + + #[test] + fn test_generate_name_with_shortuuid_simple() { + let existing: Vec = vec![]; + let name = generate_name_with_shortuuid("test-{shortuuid}", &existing); + + assert!(name.starts_with("test-")); + assert_eq!(name.len(), 21); // "test-" (5) + shortuuid (16) + + // Verify the part after "test-" is alphanumeric + let uuid_part = &name[5..]; + assert!(uuid_part.chars().all(|c| c.is_ascii_alphanumeric())); + } + + #[test] + fn test_generate_name_with_shortuuid_multiple_patterns() { + let existing: Vec = vec![]; + let name = generate_name_with_shortuuid("foo-{shortuuid}-bar-{shortuuid}", &existing); + + assert!(name.starts_with("foo-")); + assert!(name.contains("-bar-")); + // Both {shortuuid} should be replaced with the same value + let parts: Vec<&str> = name.split("-bar-").collect(); + assert_eq!(parts.len(), 2); + let uuid_part1 = parts[0] + .strip_prefix("foo-") + .expect("name should start with 'foo-'"); + let uuid_part2 = parts[1]; + assert_eq!( + uuid_part1, uuid_part2, + "Both {{shortuuid}} placeholders should be replaced by the same value" + ); + } + + #[test] + fn test_generate_name_with_shortuuid_collision_retry() { + // Create an existing domain with a specific UUID pattern + let fake_uuid = "a".repeat(16); + let existing = vec![format!("test-{}", fake_uuid)]; + + // Generate should produce a different UUID + let name = generate_name_with_shortuuid("test-{shortuuid}", &existing); + assert!(name.starts_with("test-")); + assert!(!existing.contains(&name)); + } + #[test] fn test_parse_volume_mount_valid() { let result = parse_volume_mount("/tmp:mytag");