libvirt: Support generating unique names directly #78
Conversation
Summary of ChangesHello @cgwalters, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a convenient way to generate unique VM names directly from the CLI using a {shortuuid} template, which is a nice improvement for scripting and testing. The implementation is generally solid and includes new unit tests. My review focuses on a few areas to improve robustness and maintainability:
- Test Robustness: The integration tests currently panic on VM creation failure without cleaning up potentially created resources. I've suggested adding cleanup logic to these failure paths to prevent leaving orphaned VMs.
- Performance: The current method for checking name uniqueness involves a linear scan, which could be slow with many domains. I've recommended using a
HashSetfor more efficient lookups. - Test Correctness: One of the new unit tests doesn't fully validate its intended behavior. I've suggested an improvement to make it more accurate.
- Maintainability: I've also proposed a small refactoring in the name generation logic to improve its clarity.
Overall, these are great changes that improve the usability of the tool.
| if !vm1_output.success() { | ||
| cleanup_domain(&vm1_name); | ||
| cleanup_domain(&vm2_name); | ||
|
|
||
| panic!("Failed to create first VM: {}", vm1_output.stderr); | ||
| } |
There was a problem hiding this comment.
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.
| 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); | |
| } |
| if !output.success() { | ||
| cleanup_domain(&vm_name); | ||
|
|
||
| panic!("Failed to create VM: {}", output.stderr); | ||
| } |
There was a problem hiding this comment.
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.
| 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); | |
| } |
| if !create_output.success() { | ||
| cleanup_domain(&domain_name); | ||
|
|
||
| panic!("Failed to create domain with SSH: {}", create_output.stderr); | ||
| } |
There was a problem hiding this comment.
If run_bcvk fails, the test panics without attempting to clean up the VM resources. Since the VM name is written to 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.
| if !create_output.success() { | |
| cleanup_domain(&domain_name); | |
| panic!("Failed to create domain with SSH: {}", create_output.stderr); | |
| } | |
| if !create_output.success() { | |
| // 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); | |
| } |
crates/kit/src/libvirt/run.rs
Outdated
| // Fallback: append a counter if somehow we failed to find unique name | ||
| let shortuuid = generate_shortuuid(); | ||
| let base = template.replace("{shortuuid}", &shortuuid); | ||
| let mut counter = 1; | ||
| let mut candidate = base.clone(); | ||
|
|
||
| while existing_domains.contains(&candidate) { | ||
| counter += 1; | ||
| candidate = format!("{}-{}", base, counter); | ||
| } | ||
|
|
||
| candidate |
There was a problem hiding this comment.
The fallback logic for generating a unique name is a bit complex and has an unusual behavior of starting its counter suffix from -2 if the base name collides. This can be simplified for better readability and more predictable behavior.
// 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;
}| let parts: Vec<&str> = name.split("-bar-").collect(); | ||
| assert_eq!(parts.len(), 2); |
There was a problem hiding this comment.
The test comment states that it verifies that both {shortuuid} placeholders are replaced with the same value, but the assertion only checks that splitting the string results in two parts. The test should be strengthened to actually compare the generated UUID parts to ensure the implementation is correct.
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");
This is more convenient from the CLI and in theory we can make it race-free. Motivated by supporting tests. Signed-off-by: Colin Walters <walters@verbum.org>
cgwalters
force-pushed
the
libvirt-genname
branch
from
0acaec8 to
9063992
Compare
1 participant
This is more convenient from the CLI and in theory we can make it race-free.
Motivated by supporting tests.