Skip to content

libvirt: Support generating unique names directly #78

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
cgwalters wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

libvirt: Support generating unique names directly #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 56 additions & 31 deletions crates/integration-tests/src/tests/libvirt_base_disks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,29 +14,25 @@ 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)...");
let vm1_output = run_bcvk(&[
"libvirt",
"run",
"--name",
&vm1_name,
vm1_name_template,
"--write-id-to",
vm1_id_path,
"--filesystem",
"ext4",
&test_image,
Expand All @@ -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);
}
Comment on lines 45 to 49
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If run_bcvk fails, the test panics without attempting to clean up the VM resources. Since the VM name is written to vm1_id_path before the domain creation is attempted, you can read this file to get the name and attempt a cleanup. This will make the tests more robust and prevent leaving orphaned VMs on test runners.

Suggested change
if !vm1_output.success() {
cleanup_domain(&vm1_name);
cleanup_domain(&vm2_name);
panic!("Failed to create first VM: {}", vm1_output.stderr);
}
if !vm1_output.success() {
// 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"),
Expand All @@ -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,
Expand All @@ -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"),
Expand Down Expand Up @@ -170,34 +188,42 @@ 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,
])
.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);
}
Comment on lines 213 to 217
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Similar to the other test, if run_bcvk fails here, the test panics without cleaning up resources. It's good practice to attempt cleanup in failure paths to ensure a clean test environment and prevent resource leaks.

Suggested change
if !output.success() {
cleanup_domain(&vm_name);
panic!("Failed to create VM: {}", output.stderr);
}
if !output.success() {
// 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])
Expand Down Expand Up @@ -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);

Expand Down
Loading
Loading