diff --git a/crates/kit/src/domain_list.rs b/crates/kit/src/domain_list.rs index a3aa5bd2a..7de8c0c40 100644 --- a/crates/kit/src/domain_list.rs +++ b/crates/kit/src/domain_list.rs @@ -3,11 +3,11 @@ //! This module provides functionality to list libvirt domains created by bcvk libvirt, //! using libvirt as the source of truth instead of the VmRegistry cache. +use crate::libvirt::virsh::VirshCommand; use crate::xml_utils; use base64::Engine; use color_eyre::{eyre::Context, Result}; use serde::{Deserialize, Serialize}; -use std::process::Command; use std::time::SystemTime; /// Information about a podman-bootc domain from libvirt @@ -87,13 +87,8 @@ impl DomainLister { } /// Build a virsh command with optional connection URI - fn virsh_command(&self) -> Command { - let mut cmd = Command::new("virsh"); - cmd.env("LC_ALL", "C"); - if let Some(ref uri) = self.connect_uri { - cmd.arg("-c").arg(uri); - } - cmd + fn virsh_command(&self) -> VirshCommand { + VirshCommand::new(self.connect_uri.as_deref()) } /// List all domains (running and inactive) diff --git a/crates/kit/src/libvirt/list_volumes.rs b/crates/kit/src/libvirt/list_volumes.rs index cb98a61c4..15c9ae673 100644 --- a/crates/kit/src/libvirt/list_volumes.rs +++ b/crates/kit/src/libvirt/list_volumes.rs @@ -3,11 +3,11 @@ //! This module provides functionality to discover and list bootc volumes //! with their container image metadata and creation information. +use crate::libvirt::virsh::VirshCommand; use clap::Parser; use color_eyre::{eyre::eyre, Result}; use comfy_table::{presets::UTF8_FULL, Table}; use serde_json::{json, Value}; -use std::process::Command; use tracing::{debug, warn}; /// Configuration options for listing bootc volumes @@ -71,7 +71,7 @@ impl BootcVolume { impl LibvirtListVolumesOpts { /// Build a virsh command with optional connection URI - fn virsh_command(&self, global_opts: &crate::libvirt::LibvirtOptions) -> Command { + fn virsh_command(&self, global_opts: &crate::libvirt::LibvirtOptions) -> VirshCommand { global_opts.virsh_command() } diff --git a/crates/kit/src/libvirt/mod.rs b/crates/kit/src/libvirt/mod.rs index 2a5d5e57a..f40521f9e 100644 --- a/crates/kit/src/libvirt/mod.rs +++ b/crates/kit/src/libvirt/mod.rs @@ -40,6 +40,7 @@ pub mod start; pub mod status; pub mod stop; pub mod upload; +pub mod virsh; /// Global options for libvirt operations #[derive(Debug, Clone, Default)] @@ -50,13 +51,8 @@ pub struct LibvirtOptions { impl LibvirtOptions { /// Create a virsh Command with the appropriate connection URI - pub fn virsh_command(&self) -> std::process::Command { - let mut cmd = std::process::Command::new("virsh"); - cmd.env("LC_ALL", "C"); - if let Some(ref uri) = self.connect { - cmd.arg("-c").arg(uri); - } - cmd + pub fn virsh_command(&self) -> virsh::VirshCommand { + virsh::VirshCommand::new(self.connect.as_deref()) } } diff --git a/crates/kit/src/libvirt/run.rs b/crates/kit/src/libvirt/run.rs index 67eaa645e..0dc848f48 100644 --- a/crates/kit/src/libvirt/run.rs +++ b/crates/kit/src/libvirt/run.rs @@ -27,13 +27,8 @@ const SSH_WAIT_TIMEOUT_SECONDS: u64 = 180; const UPDATE_FROM_HOST_TRANSPORT: &str = "containers-storage"; /// Create a virsh command with optional connection URI -pub(super) fn virsh_command(connect_uri: Option<&str>) -> Result { - let mut cmd = std::process::Command::new("virsh"); - cmd.env("LC_ALL", "C"); - if let Some(uri) = connect_uri { - cmd.arg("-c").arg(uri); - } - Ok(cmd) +pub(super) fn virsh_command(connect_uri: Option<&str>) -> Result { + Ok(super::virsh::VirshCommand::new(connect_uri)) } /// Run a virsh command and handle errors consistently diff --git a/crates/kit/src/libvirt/status.rs b/crates/kit/src/libvirt/status.rs index c09bdbeb6..78ad6f034 100644 --- a/crates/kit/src/libvirt/status.rs +++ b/crates/kit/src/libvirt/status.rs @@ -3,10 +3,10 @@ //! This module provides a status command that outputs JSON metadata about //! the libvirt environment, including version information and domain count. +use crate::libvirt::virsh::VirshCommand; use clap::Parser; use color_eyre::{eyre::Context, Result}; use serde::{Deserialize, Serialize}; -use std::process::Command; use crate::domain_list::DomainLister; @@ -98,7 +98,7 @@ fn parse_libvirt_version_from_output(version_output: &str) -> Option Result> { - let output = Command::new("virsh") + let output = VirshCommand::new(None) .args(&["version"]) .output() .with_context(|| "Failed to check libvirt version")?; diff --git a/crates/kit/src/libvirt/upload.rs b/crates/kit/src/libvirt/upload.rs index e2960c688..e1896e433 100644 --- a/crates/kit/src/libvirt/upload.rs +++ b/crates/kit/src/libvirt/upload.rs @@ -5,13 +5,13 @@ use crate::common_opts::MemoryOpts; use crate::install_options::InstallOptions; +use crate::libvirt::virsh::VirshCommand; use crate::to_disk::{run as to_disk, ToDiskAdditionalOpts, ToDiskOpts}; use crate::{images, utils}; use camino::Utf8PathBuf; use clap::Parser; use color_eyre::{eyre::eyre, Result}; use std::path::Path; -use std::process::Command; use tracing::debug; /// Configuration options for uploading a bootc disk image to libvirt @@ -46,7 +46,7 @@ pub struct LibvirtUploadOpts { impl LibvirtUploadOpts { /// Build a virsh command with optional connection URI - fn virsh_command(&self, global_opts: &crate::libvirt::LibvirtOptions) -> Command { + fn virsh_command(&self, global_opts: &crate::libvirt::LibvirtOptions) -> VirshCommand { global_opts.virsh_command() } diff --git a/crates/kit/src/libvirt/virsh.rs b/crates/kit/src/libvirt/virsh.rs new file mode 100644 index 000000000..1c62ac366 --- /dev/null +++ b/crates/kit/src/libvirt/virsh.rs @@ -0,0 +1,273 @@ +//! Running `virsh`, working around a libvirt session daemon restart race +//! +//! This works around a libvirt bug, present from at least v10.0.0 through +//! v12.8.0 (current master as of 2026-10). For `qemu:///session`, the libvirt +//! client auto-spawns a per-user daemon (`libvirtd` or `virtqemud`) with +//! `--timeout=120`, which exits after two idle minutes. On exit the daemon +//! removes its socket first (`virNetDaemonRun()` closes its servers) and +//! releases its pidfile only once its drivers have shut down, at the end of +//! `main()` in `src/remote/remote_daemon.c`. A client connecting in between +//! gets ENOENT and, in `virNetSocketNewConnectUNIX()` +//! (`src/rpc/virnetsocket.c`), spawns a new daemon once; that daemon exits +//! immediately because the pidfile is still held, and the client then waits +//! five seconds for a socket nobody will create and fails with +//! `Failed to connect socket to '.../libvirt-sock': No such file or directory`. +//! See . +//! +//! Upstream bug not yet filed; draft report with a reproducer: +//! . +//! Remove this retry once the libvirt versions bcvk supports include a fix +//! (link it here when one lands). +//! +//! When virsh fails to connect it has not run its command, so retrying is +//! always safe. Every retry spawns the daemon again, which succeeds once the +//! old one has gone. + +use std::ffi::OsStr; +use std::io; +use std::process::{Command, Output}; +use std::time::{Duration, Instant}; + +/// How long to keep retrying a failed connection. libvirt gives an exiting +/// daemon up to 30 seconds to shut down, and each virsh attempt itself waits +/// up to 5 seconds for the socket of the daemon it spawned. +const CONNECT_RETRY_TIMEOUT: Duration = Duration::from_secs(60); + +/// First delay between attempts; it doubles up to [`MAX_BACKOFF`]. +const INITIAL_BACKOFF: Duration = Duration::from_millis(250); + +/// Longest delay between attempts. +const MAX_BACKOFF: Duration = Duration::from_secs(4); + +/// What virsh prints when opening the connection failed. +const CONNECT_FAILED: &str = "failed to connect to the hypervisor"; + +/// What libvirt prints when the daemon's socket can't be connected to. +const SOCKET_CONNECT_FAILED: &str = "Failed to connect socket to"; + +/// Socket errors that mean the session daemon is exiting or not listening +/// yet, as opposed to e.g. EACCES. +const TRANSIENT_SOCKET_ERRORS: &[&str] = &["No such file or directory", "Connection refused"]; + +/// Errors from a daemon that accepted the connection while it was exiting. +const TRANSIENT_STREAM_ERRORS: &[&str] = + &["End of file while reading data", "Connection reset by peer"]; + +/// A `virsh` invocation with an optional connection URI. +/// +/// Unlike a plain [`Command`], [`VirshCommand::output`] retries when virsh +/// could not connect to a session daemon that is restarting. It deliberately +/// doesn't deref to [`Command`], so a call chain can't skip the retry. +#[derive(Debug)] +pub struct VirshCommand { + cmd: Command, + retry_connect: bool, +} + +impl VirshCommand { + /// Create a virsh command, connecting to `connect_uri` or virsh's default. + pub fn new(connect_uri: Option<&str>) -> Self { + let mut cmd = Command::new("virsh"); + // We match on virsh's error messages, so they must not be translated. + cmd.env("LC_ALL", "C"); + if let Some(uri) = connect_uri { + cmd.arg("-c").arg(uri); + } + let default_uri = std::env::var("LIBVIRT_DEFAULT_URI").ok(); + let retry_connect = uses_session_daemon( + connect_uri.or(default_uri.as_deref()), + rustix::process::geteuid().is_root(), + ); + Self { cmd, retry_connect } + } + + /// Add arguments, like [`Command::args`]. + pub fn args(&mut self, args: I) -> &mut Self + where + I: IntoIterator, + S: AsRef, + { + self.cmd.args(args); + self + } + + /// Run virsh and collect its output, like [`Command::output`], retrying + /// while the session daemon can't be reached. + pub fn output(&mut self) -> io::Result { + if !self.retry_connect { + return self.cmd.output(); + } + let cmd = &mut self.cmd; + output_with_retry(|| cmd.output(), CONNECT_RETRY_TIMEOUT, INITIAL_BACKOFF) + } +} + +/// Whether connecting to `uri` (`None` meaning libvirt's built-in default) +/// goes through an auto-spawned per-user daemon. +fn uses_session_daemon(uri: Option<&str>, is_root: bool) -> bool { + match uri { + Some(uri) => { + let without_query = uri.split_once('?').map_or(uri, |(base, _)| base); + without_query.ends_with("/session") + } + // Without a URI, libvirt picks qemu:///session for unprivileged users. + None => !is_root, + } +} + +/// Whether virsh failed because the session daemon was exiting or not up yet. +fn is_transient_connect_failure(output: &Output) -> bool { + if output.status.success() { + return false; + } + let stderr = String::from_utf8_lossy(&output.stderr); + stderr.contains(CONNECT_FAILED) + && stderr.lines().any(|line| { + let socket_gone = line.contains(SOCKET_CONNECT_FAILED) + && TRANSIENT_SOCKET_ERRORS.iter().any(|e| line.ends_with(e)); + socket_gone || TRANSIENT_STREAM_ERRORS.iter().any(|e| line.contains(e)) + }) +} + +/// Call `run` until it succeeds, fails for another reason than a transient +/// connection failure, or `timeout` has passed; the last output is returned. +fn output_with_retry( + mut run: impl FnMut() -> io::Result, + timeout: Duration, + initial_backoff: Duration, +) -> io::Result { + let deadline = Instant::now() + timeout; + let mut backoff = initial_backoff; + let mut attempt = 1u32; + loop { + let output = run()?; + if !is_transient_connect_failure(&output) || Instant::now() >= deadline { + return Ok(output); + } + tracing::debug!( + "virsh could not connect to the libvirt session daemon (attempt {attempt}), retrying in {backoff:?}: {}", + String::from_utf8_lossy(&output.stderr).trim() + ); + std::thread::sleep(backoff); + backoff = (backoff * 2).min(MAX_BACKOFF); + attempt += 1; + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::os::unix::process::ExitStatusExt; + + const ENOENT_STDERR: &str = "error: failed to connect to the hypervisor\nerror: Failed to connect socket to '/run/user/1001/libvirt/libvirt-sock': No such file or directory\n"; + + fn output(code: i32, stderr: &str) -> Output { + Output { + status: std::process::ExitStatus::from_raw(code << 8), + stdout: Vec::new(), + stderr: stderr.as_bytes().to_vec(), + } + } + + #[test] + fn test_uses_session_daemon() { + let cases = [ + (Some("qemu:///session"), false, true), + (Some("qemu:///session?socket=/tmp/sock"), false, true), + (Some("qemu:///system"), false, false), + (Some("qemu+ssh://host/system"), false, false), + (None, false, true), + (None, true, false), + ]; + for (uri, is_root, expected) in cases { + assert_eq!( + uses_session_daemon(uri, is_root), + expected, + "uri={uri:?} is_root={is_root}" + ); + } + } + + #[test] + fn test_is_transient_connect_failure() { + let cases = [ + (1, ENOENT_STDERR, true), + ( + 1, + "error: failed to connect to the hypervisor\nerror: Failed to connect socket to '/run/user/1000/libvirt/virtqemud-sock': Connection refused\n", + true, + ), + ( + 1, + "error: failed to connect to the hypervisor\nerror: End of file while reading data: Input/output error\n", + true, + ), + // Not a connection problem: the command itself failed + (1, "error: failed to get domain 'foo'\n", false), + // A missing daemon binary won't fix itself + ( + 1, + "error: failed to connect to the hypervisor\nerror: binary 'virtqemud' does not exist in $PATH: No such file or directory\n", + false, + ), + ( + 1, + "error: failed to connect to the hypervisor\nerror: Failed to connect socket to '/run/user/1000/libvirt/libvirt-sock': Permission denied\n", + false, + ), + (0, ENOENT_STDERR, false), + ]; + for (code, stderr, expected) in cases { + assert_eq!( + is_transient_connect_failure(&output(code, stderr)), + expected, + "code={code} stderr={stderr:?}" + ); + } + } + + #[test] + fn test_output_with_retry() { + // (failures before success, timeout, expected attempts, expected success) + let cases = [ + (0, Duration::from_secs(60), 1, true), + (3, Duration::from_secs(60), 4, true), + // Once the deadline has passed the last failure is returned + (u32::MAX, Duration::ZERO, 1, false), + ]; + for (failures, timeout, expected_attempts, expected_success) in cases { + let mut attempts = 0u32; + let out = output_with_retry( + || { + attempts += 1; + Ok(if attempts > failures { + output(0, "") + } else { + output(1, ENOENT_STDERR) + }) + }, + timeout, + Duration::ZERO, + ) + .unwrap(); + assert_eq!(attempts, expected_attempts, "failures={failures}"); + assert_eq!(out.status.success(), expected_success); + } + } + + #[test] + fn test_output_with_retry_other_failure() { + let mut attempts = 0; + let out = output_with_retry( + || { + attempts += 1; + Ok(output(1, "error: failed to get domain 'foo'\n")) + }, + Duration::from_secs(60), + Duration::ZERO, + ) + .unwrap(); + assert_eq!(attempts, 1); + assert!(!out.status.success()); + } +} diff --git a/crates/kit/src/libvirt_upload_disk.rs b/crates/kit/src/libvirt_upload_disk.rs index b2fda0017..9c0db4e14 100644 --- a/crates/kit/src/libvirt_upload_disk.rs +++ b/crates/kit/src/libvirt_upload_disk.rs @@ -5,6 +5,7 @@ use crate::common_opts::MemoryOpts; use crate::install_options::InstallOptions; +use crate::libvirt::virsh::VirshCommand; use crate::to_disk::{run as to_disk, ToDiskAdditionalOpts, ToDiskOpts}; use crate::xml_utils::{self, XmlWriter}; use crate::{images, utils}; @@ -12,7 +13,6 @@ use camino::Utf8Path; use clap::Parser; use color_eyre::{eyre::eyre, Result}; use std::path::Path; -use std::process::Command; use tracing::debug; /// Configuration options for uploading a bootc disk image to libvirt @@ -77,7 +77,7 @@ impl LibvirtUploadDiskOpts { /// Check if libvirt storage pool exists fn check_pool_exists(&self) -> Result<()> { - let output = Command::new("virsh") + let output = VirshCommand::new(None) .args(&["pool-info", &self.pool]) .output()?; @@ -102,12 +102,12 @@ impl LibvirtUploadDiskOpts { let volume_path = format!("{}.raw", volume_name); // Delete existing volume if it exists - let _ = Command::new("virsh") + let _ = VirshCommand::new(None) .args(&["vol-delete", &volume_path, "--pool", &self.pool]) .output(); // Use the provided disk size - let output = Command::new("virsh") + let output = VirshCommand::new(None) .args(&[ "vol-create-as", &self.pool, @@ -125,7 +125,7 @@ impl LibvirtUploadDiskOpts { // Upload the disk image to the volume debug!("Uploading disk image to volume '{}'", volume_path); - let output = Command::new("virsh") + let output = VirshCommand::new(None) .args(&[ "vol-upload", &volume_path, @@ -173,7 +173,7 @@ impl LibvirtUploadDiskOpts { std::fs::write(&temp_metadata, metadata_xml)?; // Set the metadata on the volume - let _output = Command::new("virsh") + let _output = VirshCommand::new(None) .args(&[ "vol-desc", &volume_path, @@ -188,7 +188,7 @@ impl LibvirtUploadDiskOpts { // This is more reliable than vol-desc which might not support metadata // Get current volume XML - let output = Command::new("virsh") + let output = VirshCommand::new(None) .args(&["vol-dumpxml", &volume_path, "--pool", &self.pool]) .output()?;