From 6497e12ae4df4c7cb98992f5cf1948b3eebc486d Mon Sep 17 00:00:00 2001 From: Lars Wirzenius Date: Thu, 25 Mar 2021 10:17:42 +0200 Subject: fix: put back most of the VM creation scenario Oops. --- subplot/vmadm.yaml | 3 +++ vmadm.md | 8 ++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/subplot/vmadm.yaml b/subplot/vmadm.yaml index 76881e1..fb778dd 100644 --- a/subplot/vmadm.yaml +++ b/subplot/vmadm.yaml @@ -11,6 +11,9 @@ function: create_vm cleanup: delete_vm +- when: "I invoke vmadm delete --config {config} {filename}" + function: delete_vm + - when: "I invoke vmadm shutdown --config {config} {filename}" function: shutdown_vm diff --git a/vmadm.md b/vmadm.md index bfd09b4..24ffe5c 100644 --- a/vmadm.md +++ b/vmadm.md @@ -158,7 +158,7 @@ given file .ssh/known_hosts from known_hosts Then we create the VM, and verify that we can log in. -~~~ +~~~scenario when I invoke vmadm new --config config.yaml smoke.yaml when I invoke ssh -F .ssh/config debian@smoke hostname then stdout contains "smoke" @@ -171,7 +171,7 @@ then stdout contains "1997" Then we shut it down, twice. The second time is to verify shutting down works even if the VM is already shut down. -~~~ +~~~scenario when I invoke vmadm shutdown --config config.yaml smoke.yaml when I invoke vmadm shutdown --config config.yaml smoke.yaml ~~~ @@ -179,7 +179,7 @@ when I invoke vmadm shutdown --config config.yaml smoke.yaml Then we start it back up again and verify we can log in. Then we start it again, while it's already running, to verify that that works. -~~~ +~~~scenario when I invoke vmadm start --config config.yaml smoke.yaml when I invoke ssh -F .ssh/config debian@smoke hostname when I invoke vmadm start --config config.yaml smoke.yaml @@ -187,7 +187,7 @@ when I invoke vmadm start --config config.yaml smoke.yaml Finally, we delete it twice. -~~~ +~~~scenario when I invoke vmadm delete --config config.yaml smoke.yaml when I invoke vmadm delete --config config.yaml smoke.yaml ~~~ -- cgit v1.2.1 From 23138e5bdb8ca751834009d99d85b37e5e4ae5ae Mon Sep 17 00:00:00 2001 From: Lars Wirzenius Date: Thu, 25 Mar 2021 09:57:21 +0200 Subject: feat: give more useful and specific error messages --- src/cloudinit.rs | 33 +++++++++++++----- src/config.rs | 9 ++--- src/errors.rs | 4 --- src/image.rs | 17 +++++++--- src/install.rs | 9 ++--- src/libvirt.rs | 100 ++++++++++++++++++++++++++++++++++++++++++++++--------- src/spec.rs | 11 +++--- src/sshkeys.rs | 49 ++++++++++++++++++++------- 8 files changed, 174 insertions(+), 58 deletions(-) delete mode 100644 src/errors.rs diff --git a/src/cloudinit.rs b/src/cloudinit.rs index 169b13f..e6d2587 100644 --- a/src/cloudinit.rs +++ b/src/cloudinit.rs @@ -13,7 +13,7 @@ use shell_words::quote; use std::default::Default; use std::fs::write; // use std::os::unix::fs::PermissionsExt; -use std::path::Path; +use std::path::{Path, PathBuf}; use std::process::Command; use tempfile::tempdir; @@ -119,8 +119,8 @@ pub enum CloudInitError { KeyError(#[from] KeyError), /// Something went wrong doing I/O. - #[error(transparent)] - IoError(#[from] std::io::Error), + #[error("could not write to {0}: {1}")] + WriteError(PathBuf, #[source] std::io::Error), /// Something went wrong parsing a string as UTF8. #[error(transparent)] @@ -129,6 +129,14 @@ pub enum CloudInitError { /// Something went wrong parsing or serializing to YAML. #[error(transparent)] SerdeError(#[from] serde_yaml::Error), + + /// Could not create a temporary directory. + #[error("couldn't create a temporary directory: {0}")] + TempDirError(#[source] std::io::Error), + + /// Could not execute command. + #[error("couldn't execute {0}: {1}")] + CommandError(String, #[source] std::io::Error), } #[derive(Clone, Debug, Serialize)] @@ -304,11 +312,13 @@ impl CloudInitConfig { pub fn create_dir(&self, path: &Path) -> Result<(), CloudInitError> { let metadata = path.join("meta-data"); debug!("writing metadata to {}", metadata.display()); - write(&metadata, self.metadata()?)?; + write(&metadata, self.metadata()?) + .map_err(|err| CloudInitError::WriteError(metadata, err))?; let userdata = path.join("user-data"); debug!("writing userdata to {}", userdata.display()); - write(&userdata, self.userdata()?)?; + write(&userdata, self.userdata()?) + .map_err(|err| CloudInitError::WriteError(userdata, err))?; // let scriptsdir = path.join("scripts/per-once"); // let script = scriptsdir.join("vmadm-configure-sshd"); @@ -328,10 +338,13 @@ impl CloudInitConfig { /// The image will be attached to the VM when it starts. /// cloud-init finds it via the volume ID (file system label). pub fn create_iso(&self, filename: &Path) -> Result<(), CloudInitError> { - let dir = tempdir()?; + let dir = match tempdir() { + Ok(path) => path, + Err(err) => return Err(CloudInitError::TempDirError(err)), + }; self.create_dir(dir.path())?; - let r = Command::new("genisoimage") + let r = match Command::new("genisoimage") .arg("-quiet") .arg("-volid") .arg("cidata") @@ -340,7 +353,11 @@ impl CloudInitConfig { .arg("-output") .arg(filename) .arg(dir.path()) - .output()?; + .output() + { + Ok(r) => r, + Err(err) => return Err(CloudInitError::CommandError("genisoimage".to_string(), err)), + }; if !r.status.success() { let stderr = String::from_utf8(r.stderr)?; return Err(CloudInitError::IsoFailed(stderr)); diff --git a/src/config.rs b/src/config.rs index 88c186b..1866971 100644 --- a/src/config.rs +++ b/src/config.rs @@ -41,9 +41,9 @@ pub struct Configuration { /// Errors from this module. #[derive(Debug, thiserror::Error)] pub enum ConfigurationError { - /// I/O error. - #[error(transparent)] - IoError(#[from] std::io::Error), + /// Error reading configuration file. + #[error("couldn't read configuration file {0}")] + ReadError(PathBuf, #[source] std::io::Error), /// YAML parsing error. #[error(transparent)] @@ -55,7 +55,8 @@ impl Configuration { pub fn from_file(filename: &Path) -> Result { if filename.exists() { debug!("reading configuration file {}", filename.display()); - let config = fs::read(filename)?; + let config = fs::read(filename) + .map_err(|err| ConfigurationError::ReadError(filename.to_path_buf(), err))?; let config: Configuration = serde_yaml::from_slice(&config)?; debug!("config: {:#?}", config); Ok(config) diff --git a/src/errors.rs b/src/errors.rs deleted file mode 100644 index ba7875a..0000000 --- a/src/errors.rs +++ /dev/null @@ -1,4 +0,0 @@ -use thiserror::Error; - -#[derive(Debug, Error)] -pub enum VmadminError {} diff --git a/src/image.rs b/src/image.rs index 62f80f7..7b7c2ce 100644 --- a/src/image.rs +++ b/src/image.rs @@ -12,9 +12,13 @@ pub enum ImageError { #[error("qemu-img resize failed: {0}")] ResizeError(String), - /// I/O error. - #[error(transparent)] - IoError(#[from] std::io::Error), + /// Base image copy error. + #[error("could not copy base image {0} to {1}")] + BaseImageCopy(PathBuf, PathBuf, #[source] std::io::Error), + + /// Could not execute command. + #[error("couldn't execute {0}: {1}")] + CommandError(String, #[source] std::io::Error), /// Error parsing a string as UTF8. #[error(transparent)] @@ -30,7 +34,9 @@ pub struct VirtualMachineImage { impl VirtualMachineImage { /// Create new image from a base image. pub fn new_from_base(base_image: &Path, image: &Path) -> Result { - copy(base_image, image)?; + copy(base_image, image).map_err(|err| { + ImageError::BaseImageCopy(base_image.to_path_buf(), image.to_path_buf(), err) + })?; Ok(Self { filename: image.to_path_buf(), }) @@ -47,7 +53,8 @@ impl VirtualMachineImage { .arg("resize") .arg(self.filename()) .arg(format!("{}", new_size)) - .output()?; + .output() + .map_err(|err| ImageError::CommandError("qemu-img resize".to_string(), err))?; if !r.status.success() { let stderr = String::from_utf8(r.stderr)?; return Err(ImageError::ResizeError(stderr)); diff --git a/src/install.rs b/src/install.rs index dce0fea..e74e6ba 100644 --- a/src/install.rs +++ b/src/install.rs @@ -17,9 +17,9 @@ pub enum VirtInstallError { #[error("virt-install failed: {0}")] VirtInstallFailed(String), - /// I/O error. - #[error(transparent)] - IoError(#[from] std::io::Error), + /// Failed to run virt-install. + #[error("couldn't run virt-install")] + Run(#[source] std::io::Error), /// Error parsing a string as UTF8. #[error(transparent)] @@ -115,7 +115,8 @@ pub fn virt_install(args: &VirtInstallArgs, iso: &Path) -> Result Result { debug!("connecting to libvirtd {}", url); - let conn = Connect::open(url)?; + let conn = Connect::open(url).map_err(|err| VirtError::Connect(err))?; Ok(Self { conn }) } fn get_domains(&self) -> Result, VirtError> { debug!("listing all domains"); - Ok(self.conn.list_all_domains(0)?) + Ok(self + .conn + .list_all_domains(0) + .map_err(|err| VirtError::Domains(err))?) } fn get_domain(&self, name: &str) -> Result, VirtError> { for domain in self.get_domains()? { - if domain.get_name()? == name { + if get_name(&domain)? == name { return Ok(Some(domain)); } } @@ -51,14 +90,14 @@ impl Libvirt { pub fn names(&self) -> Result, VirtError> { let mut ret = vec![]; for domain in self.get_domains()? { - ret.push(domain.get_name()?); + ret.push(get_name(&domain)?); } Ok(ret) } pub fn is_active(&self, name: &str) -> Result { if let Some(domain) = self.get_domain(name)? { - Ok(domain.is_active()?) + Ok(is_active(&domain, name)?) } else { Ok(false) } @@ -76,12 +115,14 @@ impl Libvirt { pub fn detach_cloud_init_iso(&self, name: &str) -> Result<(), VirtError> { if let Some(domain) = self.get_domain(name)? { debug!("detaching cloud-init ISO from {}", name); - let xml = domain.get_xml_desc(0)?; + let xml = get_xml(&domain, name)?; let disk = find_iso_xml(&xml); let flags = VIR_DOMAIN_AFFECT_CONFIG | VIR_DOMAIN_AFFECT_CURRENT | VIR_DOMAIN_AFFECT_LIVE; if disk.len() > 0 { - domain.detach_device_flags(&disk, flags)?; + domain + .detach_device_flags(&disk, flags) + .map_err(|err| VirtError::DetachIso(name.to_string(), err))?; } } Ok(()) @@ -89,9 +130,11 @@ impl Libvirt { pub fn trigger_start(&self, name: &str) -> Result<(), VirtError> { if let Some(domain) = self.get_domain(name)? { - if !domain.is_active()? { + if !is_active(&domain, name)? { debug!("starting {}", name); - domain.create()?; + domain + .create() + .map_err(|err| VirtError::Create(name.to_string(), err))?; } } Ok(()) @@ -108,8 +151,10 @@ impl Libvirt { pub fn trigger_shutdown(&self, name: &str) -> Result<(), VirtError> { if let Some(domain) = self.get_domain(name)? { debug!("asking {} to shut down", name); - if domain.is_active()? { - domain.shutdown()?; + if is_active(&domain, name)? { + domain + .shutdown() + .map_err(|err| VirtError::Shutdown(name.to_string(), err))?; } } Ok(()) @@ -129,7 +174,9 @@ impl Libvirt { self.shutdown(name)?; debug!("undefine {}", name); - domain.undefine()?; + domain + .undefine() + .map_err(|err| VirtError::Undefine(name.to_string(), err))?; debug!("removing image file {}", image.display()); std::fs::remove_file(image)?; @@ -140,12 +187,33 @@ impl Libvirt { pub fn set_autostart(&self, name: &str, autostart: bool) -> Result<(), VirtError> { if let Some(domain) = self.get_domain(name)? { - domain.set_autostart(autostart)?; + domain + .set_autostart(autostart) + .map_err(|err| VirtError::Autostart(name.to_string(), err))?; } Ok(()) } } +fn get_name(domain: &Domain) -> Result { + let name = domain.get_name().map_err(|err| VirtError::GetName(err))?; + Ok(name) +} + +fn is_active(domain: &Domain, name: &str) -> Result { + let is = domain + .is_active() + .map_err(|err| VirtError::IsActive(name.to_string(), err))?; + Ok(is) +} + +fn get_xml(domain: &Domain, name: &str) -> Result { + let is = domain + .get_xml_desc(0) + .map_err(|err| VirtError::GetXml(name.to_string(), err))?; + Ok(is) +} + fn wait_until_inactive(domain: &Domain, name: &str) { debug!("waiting for domain {} to become inactive", name); let briefly = Duration::from_millis(1000); diff --git a/src/spec.rs b/src/spec.rs index b9828e9..a1d1c2f 100644 --- a/src/spec.rs +++ b/src/spec.rs @@ -216,14 +216,14 @@ pub enum SpecificationError { #[error("No SSH authorized keys specified for {0} and no default configured")] NoAuthorizedKeys(String), + /// Error reading specification file. + #[error("Couldn't read specification file {0}")] + Read(PathBuf, #[source] std::io::Error), + /// Error reading SSH public key. #[error("Failed to read SSH public key file {0}")] SshKeyRead(PathBuf, #[source] std::io::Error), - /// I/O error. - #[error(transparent)] - IoError(#[from] std::io::Error), - /// Error parsing string as UTF8. #[error(transparent)] FromUtf8Error(#[from] std::string::FromUtf8Error), @@ -247,7 +247,8 @@ impl Specification { filename: &Path, ) -> Result, SpecificationError> { debug!("reading specification from {}", filename.display()); - let spec = fs::read(filename)?; + let spec = fs::read(filename) + .map_err(|err| SpecificationError::Read(filename.to_path_buf(), err))?; let input: HashMap = serde_yaml::from_slice(&spec)?; debug!("specification as read from file: {:#?}", input); diff --git a/src/sshkeys.rs b/src/sshkeys.rs index 1425cb3..349cabe 100644 --- a/src/sshkeys.rs +++ b/src/sshkeys.rs @@ -3,7 +3,7 @@ use std::fs::{read, File, Permissions}; use std::io::Write; use std::os::unix::fs::PermissionsExt; -use std::path::Path; +use std::path::{Path, PathBuf}; use std::process::Command; use tempfile::tempdir; @@ -18,9 +18,29 @@ pub enum KeyError { #[error("ssh-keygen failed to certify a key: {0}")] CertError(String), - /// I/O error. - #[error(transparent)] - IoError(#[from] std::io::Error), + /// Error creating a temporary directory. + #[error("Couldn't create temporary directory")] + TempDir(#[source] std::io::Error), + + /// Error running ssh-keygen. + #[error("Couldn't run ssh-keygen")] + Run(#[source] std::io::Error), + + /// Error reading a file. + #[error("Couldn't read file {0}")] + Read(PathBuf, #[source] std::io::Error), + + /// Error writing a file. + #[error("Couldn't write file {0}")] + Write(PathBuf, #[source] std::io::Error), + + /// Error creating a file. + #[error("Couldn't create file {0}")] + Create(PathBuf, #[source] std::io::Error), + + /// Error setting file permissions. + #[error("Couldn't set permissions for file {0}")] + SetPerm(PathBuf, #[source] std::io::Error), /// Error parsing a string as UTF8. #[error(transparent)] @@ -90,7 +110,7 @@ impl KeyPair { /// Generate a new key pair of the desired kind. pub fn generate(kind: KeyKind) -> Result { - let dirname = tempdir()?; + let dirname = tempdir().map_err(|err| KeyError::TempDir(err))?; let private_key = dirname.path().join("key"); let output = Command::new("ssh-keygen") .arg("-f") @@ -101,7 +121,8 @@ impl KeyPair { .arg(format!("{}", kind.bits())) .arg("-N") .arg("") - .output()?; + .output() + .map_err(|err| KeyError::Run(err))?; if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); @@ -128,7 +149,7 @@ impl KeyPair { } fn read_string(filename: &Path) -> Result { - let bytes = read(filename)?; + let bytes = read(filename).map_err(|err| KeyError::Read(filename.to_path_buf(), err))?; Ok(String::from_utf8(bytes)?) } @@ -157,7 +178,7 @@ impl CaKey { /// /// Return as a string. pub fn certify_host(&self, host_key: &KeyPair, hostname: &str) -> Result { - let dirname = tempdir()?; + let dirname = tempdir().map_err(|err| KeyError::TempDir(err))?; let ca_key = dirname.path().join("ca"); let host_key_pub = dirname.path().join("host.pub"); let cert = dirname.path().join("host-cert.pub"); @@ -174,7 +195,8 @@ impl CaKey { .arg("-I") .arg(format!("host key for {}", hostname)) .arg(&host_key_pub) - .output()?; + .output() + .map_err(|err| KeyError::Run(err))?; if !output.status.success() { let stderr = String::from_utf8_lossy(&output.stderr).into_owned(); @@ -186,10 +208,13 @@ impl CaKey { } fn write_string(filename: &Path, s: &str) -> Result<(), KeyError> { - let mut file = File::create(filename)?; + let mut file = + File::create(filename).map_err(|err| KeyError::Create(filename.to_path_buf(), err))?; let ro_user = Permissions::from_mode(0o600); - file.set_permissions(ro_user)?; - file.write_all(s.as_bytes())?; + file.set_permissions(ro_user) + .map_err(|err| KeyError::SetPerm(filename.to_path_buf(), err))?; + file.write_all(s.as_bytes()) + .map_err(|err| KeyError::Write(filename.to_path_buf(), err))?; Ok(()) } -- cgit v1.2.1