feat: introduce machine pool with integrated network setup

[jjl9807]

This PR brings the following changes:

• Added MachinePool for managing multiple machines concurrently.
• Introduced network setup with virsh to ensure the "qlean" network exists.
• Updated error handling to use bail! for cleaner code.
• Added tests for machine pool functionality and concurrency.
• Included random MAC address generation for machines.

The above modifications do not introduce any breaking changes to existing interfaces, but they do add new interfaces and require manual configuration of networking functionality. Therefore, in accordance with Semantic Versioning, Qlean should be released as version 0.2.0 after merging this PR.

[github-actions]

Claude finished @jjl9807's task in 3m 40s —— View job

---

Code Review Complete

I've completed a comprehensive review of the MachinePool feature PR. Here's my detailed analysis:

Tasks

• Check for repository-specific instructions (CLAUDE.md)
• Review core implementation files
• Analyze code quality and best practices
• Check for security issues
• Evaluate performance implications
• Assess test coverage
• Review documentation updates
• Provide comprehensive feedback

---

Summary

This PR introduces a well-designed MachinePool feature for concurrent machine management. The code quality is generally good with consistent error handling improvements using bail!. However, there are several critical security and operational issues that must be addressed before merging.

Critical Issues 🔴

1. Privilege Escalation Risk (README.md lines 990-1001)

The documentation instructs users to set the setuid bit on qemu-bridge-helper:

sudo chmod u+s /usr/lib/qemu/qemu-bridge-helper

Security Concerns:

• Setuid root binaries are high-value targets for privilege escalation attacks
• This grants elevated network configuration privileges to all users on the system
• Expands the attack surface unnecessarily

Recommended Solutions:

1. Preferred: Use virsh with qemu:///session and user-owned networks (no root required)
2. Use libvirt's built-in network management (you're already using virsh)
3. Add users to the libvirt group instead of requiring setuid
4. Use network namespaces with user namespaces (rootless approach)

At minimum: Add prominent security warnings in documentation explaining the risks and provide alternative setup methods.

2. Missing Permission Validation (src/utils.rs:735-785)

The ensure_network() function will fail for non-root users without proper libvirt permissions, but provides no guidance:

Issues:

• No check for required permissions before attempting network operations
• Error messages don't guide users on how to fix permission issues
• No detection of existing network state

Fixes Needed:

// Check permissions first
let check_perms = tokio::process::Command::new("virsh")
    .arg("-c").arg("qemu:///system")
    .arg("net-list")
    .output()
    .await?;

if !check_perms.status.success() {
    bail!("Insufficient libvirt permissions. Add your user to the 'libvirt' group: sudo usermod -aG libvirt $USER");
}

3. SSH Timeout Regression (src/machine.rs:587)

The SSH connection timeout was increased from 20s to 60s without explanation:

-Duration::from_secs(20),
+Duration::from_secs(60),

Concerns:

• 3x increase hides potential boot/network issues
• No justification in commit message or comments
• Makes tests slower when they fail

Recommendation: Revert to 20s unless there's a specific reason (which should be documented). If needed, make it configurable.

4. Hardcoded Network Configuration (src/utils.rs:747-757)

The network configuration uses hardcoded values with no validation:

<ip address='192.168.221.1' netmask='255.255.255.0'>
  <dhcp>
    <range start='192.168.221.2' end='192.168.221.254'/>
  </dhcp>
</ip>

Issues:

• No check for IP conflicts with existing networks
• No validation if subnet is already in use
• Could break existing network configurations

Recommendations:

• Check for subnet conflicts before creating network
• Allow configuration via environment variable
• Document network requirements clearly

Major Issues 🟠

5. Weak Random MAC Generation (src/utils.rs:788-794)

let mut rng = rand::rng();
let bytes: [u8; 6] = [0x52, 0x54, 0x00, rng.random(), rng.random(), rng.random()];

Issues:

• rand::rng() is not cryptographically secure
• No collision detection between machines
• Could lead to MAC conflicts in concurrent tests

Fix:

use rand::rngs::OsRng;
let mut rng = OsRng;
let bytes: [u8; 6] = [0x52, 0x54, 0x00, rng.gen(), rng.gen(), rng.gen()];
// TODO: Add collision detection by checking existing MACs

6. Incomplete MachinePool API (src/pool.rs)

The MachinePool is missing essential methods:

Missing:

• remove(name) - Remove a specific machine from pool
• list() - Get list of all machine names
• len() - Get number of machines in pool
• is_empty() - Check if pool is empty
• Individual machine control (init/spawn/shutdown by name)

Recommendation: Add these methods for a complete API.

7. No Error Recovery in Pool Operations (src/pool.rs:364-391)

init_all() and spawn_all() use try_join_all which short-circuits on first error, leaving other machines in unknown states.

Issue: Partial initialization leaves the pool in an inconsistent state with no way to query which machines succeeded.

Recommendation: Collect all errors and return detailed status for each machine, or add partial recovery logic.

Minor Issues 🟡

8. Missing IP Caching Logic (src/machine.rs:259-266)

The get_ip() method checks self.ip but nothing ever sets it:

pub async fn get_ip(&mut self) -> Result<String> {
    if let Some(ip) = &self.ip {
        Ok(ip.to_owned())
    } else {
        let (ssh, _) = self.get_ssh()?;
        ssh.get_remote_ip().await  // <-- Should cache the result!
    }
}

Fix: Cache the IP after fetching:

let ip = ssh.get_remote_ip().await?;
self.ip = Some(ip.clone());
Ok(ip)

9. Shell Command Injection Risk (src/ssh.rs:676)

.call_with_output("hostname -I | awk '{print $1}'", CancellationToken::new())

While this specific command is safe, using shell pipes creates a pattern that could be dangerous if extended. Consider using safer alternatives or documenting the security implications.

10. Test Coverage Gaps

The new tests are good but missing:

• Error handling tests (what happens when virsh fails?)
• Network isolation tests (can machines reach external network?)
• MAC address collision scenarios
• Pool with >254 machines (DHCP limit)
• Concurrent pool operations from multiple processes

Positive Aspects ✅

1. Consistent Error Handling - Good use of bail! macro throughout
2. Well-Structured Code - Clear separation of concerns with MachinePool
3. Good Documentation - README updates are comprehensive
4. Concurrent Operations - Proper use of try_join_all for parallel machine management
5. Test Coverage - New tests demonstrate core functionality
6. RAII Pattern - with_pool ensures proper cleanup

Performance Notes

• Network bridge creation happens on every ensure_prerequisites() call - consider caching this check
• DHCP lease management might become bottleneck with many machines
• Consider adding connection pooling for libvirt connections

Documentation Issues

1. Missing: Minimum libvirt version requirements
2. Missing: Troubleshooting section for common network issues
3. Missing: Security best practices section
4. Missing: How to clean up the network after use

Recommendations

Before Merging (Required):

1. ✅ Fix the setuid security issue - provide safer alternatives
2. ✅ Add permission validation with helpful error messages
3. ✅ Justify or revert SSH timeout increase
4. ✅ Add network conflict detection
5. ✅ Fix IP caching in get_ip()
6. ✅ Improve random MAC generation security

Nice to Have:

• Complete MachinePool API with remove/list methods
• Better error recovery in pool operations
• Additional test coverage
• Security documentation section
• Network cleanup documentation

---

Semantic Versioning

The version bump to 0.2.0 is correct per semver - this adds new public APIs (MachinePool, with_pool) without breaking existing interfaces.

Overall Assessment

This is a well-designed feature with clean implementation, but has critical security and operational issues that must be addressed. The core architecture is sound, and the changes are additive rather than breaking. With the security issues fixed and better permission handling, this will be a valuable addition to qlean.

Recommendation: Request Changes - Address critical security issues before merging.

---

[genedna]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: eaa5ccfcc6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Copilot]

Pull request overview

This PR introduces a machine pool feature for managing multiple virtual machines concurrently, along with networking infrastructure changes. The implementation switches from QEMU's user-mode networking to bridge networking, enabling inter-VM communication.

Changes:

• Added MachinePool API with methods for concurrent machine lifecycle management (init_all, spawn_all, shutdown_all)
• Implemented automatic network setup using libvirt/virsh with a dedicated "qlean" bridge network
• Refactored error handling throughout the codebase to use bail! for consistency
• Added IP address retrieval functionality and running state checks for machines
• Included comprehensive tests for multi-machine scenarios

Reviewed changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
tests/single_machine.rs	Refactored to use shared tracing initialization utility
tests/machine_pool.rs	New integration tests for machine pool functionality including ping tests and concurrency
tests/common.rs	Extracted shared tracing subscriber initialization for test reuse
src/utils.rs	Added network setup automation, MAC address generation, and base directory tracking
src/ssh.rs	Migrated error handling to use bail! macro; added remote IP retrieval method
src/qemu.rs	Restructured launch parameters into QemuLaunchParams struct; updated network config to use bridge mode
src/pool.rs	New MachinePool implementation with concurrent operations support
src/machine.rs	Added IP caching, running state checks, and increased SSH timeout to 60s
src/lib.rs	Added with_pool function for pool-based workflows; capitalized "Qlean" in error messages
src/image.rs	Migrated error handling to use bail! macro
README.md	Updated documentation with bridge networking setup instructions and multi-machine examples
Cargo.toml	Version bump to 0.2.0; added rand dependency
Cargo.lock	Updated to reflect new rand dependency

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The ensure_network function only checks if the network exists but doesn't verify if it's active. A network can exist but be in an inactive state. Consider checking the network status with "virsh net-list --all" and comparing against "virsh net-list" (active networks only), then starting the network if it exists but is inactive. This would prevent failures when the network exists but isn't running.

[Copilot]

If the closure returns an error, the error is captured in 'result' but then shutdown_all is called unconditionally. If shutdown_all also fails, its error will override the original error from the closure. The original with_machine function at line 35 has the same pattern and should also be fixed. Consider using a pattern that preserves the first error while still attempting cleanup, or document that shutdown errors take precedence.

[Copilot]

Setting the setuid bit on qemu-bridge-helper (line 51) is a security-sensitive operation. While this is documented, consider adding a security warning that explains: 1) why this is necessary, 2) what the security implications are, and 3) alternative approaches if users are concerned about setuid binaries. This is especially important since it requires sudo privileges and affects system-wide security.

[Copilot]

The network XML configuration uses a fixed IP range (192.168.221.0/24). If users have existing networks in this range, there could be conflicts. Consider documenting this IP range in the README or providing a way for users to customize it. Additionally, the network is not set to autostart, which means it won't survive host reboots. Consider adding a virsh net-autostart command after creation.

[github-actions]

Claude finished @jjl9807's task in 3m 36s —— View job

---

Code Review Complete

I've performed a comprehensive review of the MachinePool feature PR, building on the previous reviews and analyzing the implementation in detail.

Tasks

• Check for repository-specific instructions (CLAUDE.md)
• Review core implementation files
• Analyze code quality and best practices
• Check for security issues
• Evaluate performance implications
• Assess test coverage
• Review documentation updates
• Provide comprehensive feedback

---

Summary

This PR introduces a well-structured MachinePool feature for managing multiple VMs concurrently. The implementation is generally solid with good concurrent operation support and clean error handling using bail!. However, there are several critical issues and areas for improvement that should be addressed.

Critical Issues 🔴

1. IP Caching Not Implemented (src/machine.rs:259-266)

The get_ip() method checks self.ip but never sets it, causing unnecessary SSH commands on every call:

pub async fn get_ip(&mut self) -> Result<String> {
    if let Some(ip) = &self.ip {
        Ok(ip.to_owned())
    } else {
        let (ssh, _) = self.get_ssh()?;
        ssh.get_remote_ip().await  // Should cache this result!
    }
}

Impact: Performance degradation - every get_ip() call executes a remote command.

Fix: Add self.ip = Some(ip.clone()); before returning.

Fix this →

2. SSH Timeout Increased Without Justification (src/machine.rs:568)

The SSH connection timeout was increased from 20s to 60s (3x increase) with no explanation:

connect_ssh(
    self.cid,
    Duration::from_secs(60),  // Was 20s before
    ...
)

Concerns:

• Makes tests 3x slower when they fail
• Masks potential boot/network configuration issues
• No documentation explaining why this is needed

Recommendation: Revert to 20s unless there's a specific reason (which should be documented as a comment).

3. Duplicate Pool Names Silently Overwrite (src/pool.rs:33)

MachinePool::add() uses HashMap::insert() which silently replaces existing entries:

pub async fn add(&mut self, name: String, image: &Image, config: &MachineConfig) -> Result<()> {
    let machine = Machine::new(image, config).await?;
    self.pool.insert(name, Mutex::new(machine));  // Silently overwrites!
    Ok(())
}

Impact: If a user accidentally reuses a name, the old VM keeps running with no handle, leaking resources. shutdown_all() can no longer reach the orphaned VM.

Fix: Return an error if the name already exists:

if self.pool.contains_key(&name) {
    bail!("Machine '{}' already exists in pool", name);
}

Fix this →

4. Process Existence Check Has False Positives (src/machine.rs:577-582)

The is_running() method only checks if /proc/{pid} exists, which is Linux-specific and can give false positives if the PID is reused:

pub(crate) async fn is_running(&self) -> Result<bool> {
    if let Some(pid) = self.pid {
        let process_exists = std::path::Path::new(&format!("/proc/{}", pid)).exists();
        Ok(process_exists)
    } else {
        Ok(false)
    }
}

Issues:

• PID can be reused by another process
• No verification it's actually a QEMU process

Improvement: Read /proc/{pid}/cmdline to verify it's a QEMU process.

Major Issues 🟠

5. Network Configuration Not Validated (src/utils.rs:168-190)

The ensure_network() function uses hardcoded IP range without checking for conflicts:

let xml = r#"
<network>
  <name>qlean</name>
  <bridge name='qlbr0'/>
  <forward mode="nat"/>
  <ip address='192.168.221.1' netmask='255.255.255.0'>
    <dhcp>
      <range start='192.168.221.2' end='192.168.221.254'/>
    </dhcp>
  </ip>
</network>
"#;

Problems:

• No check if 192.168.221.0/24 conflicts with existing networks
• No validation if network is active (only checks if it exists)
• Could break user's existing network setup

Recommendations:

1. Check network status and start if inactive
2. Document the IP range clearly in README
3. Consider making it configurable via environment variable

6. Weak MAC Address Generation (src/utils.rs:193-200)

The MAC generation uses non-cryptographic RNG with no collision detection:

pub fn gen_random_mac() -> String {
    let mut rng = rand::rng();  // Not cryptographically secure
    let bytes: [u8; 6] = [0x52, 0x54, 0x00, rng.random(), rng.random(), rng.random()];
    // ...
}

Issues:

• rand::rng() is not cryptographically secure
• No collision detection between machines
• Could cause MAC conflicts in concurrent tests

Fix: Use OsRng for better randomness:

use rand::rngs::OsRng;
let mut rng = OsRng;

7. Incomplete MachinePool API (src/pool.rs)

The MachinePool is missing essential methods for a complete API:

Missing:

• remove(name) - Remove a specific machine from pool
• list() / keys() - Get list of all machine names
• len() - Get number of machines in pool
• is_empty() - Check if pool is empty
• Individual machine control (init/spawn/shutdown by name)

Recommendation: Add these methods for API completeness and usability.

8. Error Recovery in Pool Operations (src/pool.rs:51-63)

init_all() and spawn_all() use try_join_all which short-circuits on first error:

pub async fn init_all(&mut self) -> Result<()> {
    let tasks = self.pool.values().map(|machine| async {
        let mut m = machine.lock().await;
        if !m.is_running().await? {
            m.init().await?;
        }
        Ok::<(), anyhow::Error>(())
    });

    futures::future::try_join_all(tasks).await?;  // Short-circuits on first error
    Ok(())
}

Problem: Partial initialization leaves pool in inconsistent state with no way to query which machines succeeded.

Options:

1. Collect all errors and return detailed status
2. Add partial recovery logic
3. At minimum, document this behavior clearly

9. Shell Command Injection Pattern (src/ssh.rs:676)

The get_remote_ip() method uses shell piping which creates a risky pattern:

.call_with_output("hostname -I | awk '{print $1}'", CancellationToken::new())

While this specific command is safe, it establishes a pattern that could be dangerous if extended. Consider using safer alternatives or documenting security implications.

Minor Issues 🟡

10. Unnecessary Import (src/utils.rs:3)

use anyhow::{Context, Ok, Result, bail};

The Ok import from anyhow is unnecessary - standard Result::Ok is sufficient. This could cause confusion.

Fix: Remove Ok from imports.

11. QemuLaunchParams Refactoring (src/qemu.rs:25-32)

The conversion from individual parameters to a struct is good, but QemuLaunchParams should derive Debug for better debugging:

#[derive(Debug)]  // Add this
pub struct QemuLaunchParams {
    // ...
}

12. Missing Documentation

README gaps:

• No minimum libvirt version requirements
• Missing troubleshooting section for network issues
• No security best practices section
• No cleanup instructions for the "qlean" network
• Missing explanation of permissions requirements

13. Test Coverage Gaps

The new tests are good but missing:

• Error handling tests (network failure, virsh unavailable)
• Network isolation verification
• MAC address collision scenarios
• Pool with >254 machines (DHCP limit)
• Concurrent pool operations from multiple processes
• Empty pool edge cases

Positive Aspects ✅

1. Consistent Error Handling - Excellent use of bail! macro throughout
2. Well-Structured Code - Clear separation of concerns with MachinePool
3. Concurrent Operations - Proper use of try_join_all for parallel management
4. RAII Pattern - with_pool ensures proper cleanup
5. Test Demonstration - New tests effectively demonstrate core functionality
6. Good Abstraction - QemuLaunchParams struct improves code organization

Performance Notes

• Network creation happens on every ensure_prerequisites() call - should cache this check
• DHCP lease management might become bottleneck with many machines (limit: 253)
• Connection pooling for libvirt could improve performance
• Random MAC generation is fast but collision checking would add overhead

Security Considerations

1. Libvirt permissions: Code will fail for non-root users without proper group membership - needs documentation
2. Network configuration: Modifies system-wide network settings - document required permissions
3. MAC randomness: Should use cryptographically secure RNG
4. Shell command patterns: Document security implications of piped commands

Architecture Decision Notes

Network Switch (User-mode → Bridge):

• Pros: Enables inter-VM communication, more realistic networking
• Cons: Requires system-wide network configuration, elevated permissions
• Assessment: Good choice for the use case, but needs better documentation

Semantic Versioning:
The version bump to 0.2.0 is correct per semver - adds new public APIs (MachinePool, with_pool, get_ip, etc.) without breaking existing interfaces.

Recommendations

Before Merging (Required):

1. ✅ Fix IP caching in get_ip() (Critical)
2. ✅ Add duplicate name check in MachinePool::add() (Critical)
3. ✅ Justify or revert SSH timeout change (Critical)
4. ✅ Improve is_running() reliability (Major)
5. ✅ Add network status checking and activation (Major)
6. ✅ Use cryptographically secure RNG for MACs (Major)

Nice to Have:

• Complete MachinePool API (remove, list, len, is_empty)
• Better error recovery in pool operations
• Additional test coverage
• Enhanced documentation (permissions, troubleshooting, cleanup)
• Remove unnecessary Ok import

Overall Assessment

This is a well-designed and valuable feature with solid concurrent operation support. The core architecture is sound and the implementation demonstrates good Rust practices. However, there are critical bugs (IP caching, duplicate names) and operational concerns (network validation, timeout changes) that must be addressed before merging.

The semantic versioning is appropriate, and the additive nature of changes minimizes risk to existing users. With the critical issues fixed and better documentation around network setup, this will be a strong addition to qlean.

Recommendation: Request Changes - Address critical issues, particularly IP caching and duplicate name handling, before merging.

---