From b8119579a246727805a03c5a8e60fb44109410f6 Mon Sep 17 00:00:00 2001 From: Lars Wirzenius Date: Sat, 13 Mar 2021 14:51:19 +0200 Subject: fix: VMs can be restarted Previously, the temporary file for the cloud-init configuration ISO was left attached to the VM. This meant the VM couldn't be turned off and back on again: the temporary no longer existed. Now we detach the ISO file after the VM has booted. As a side effect, vmadm has gained start and shutdown subcommands, so that the fix can be tested. --- src/bin/vmadm.rs | 34 ++++++++++++++++++++---- src/cloudinit.rs | 2 +- src/cmd/mod.rs | 6 +++++ src/cmd/new.rs | 32 ++++++++++++---------- src/cmd/shutdown.rs | 19 +++++++++++++ src/cmd/start.rs | 31 ++++++++++++++++++++++ src/install.rs | 8 +++--- src/lib.rs | Bin 667 -> 681 bytes src/libvirt.rs | 75 ++++++++++++++++++++++++++++++++++++++++++++++++---- src/spec.rs | 4 +-- src/sshkeys.rs | 5 +--- src/util.rs | 15 +++++++++++ subplot/vmadm.py | 16 +++++++++++ subplot/vmadm.yaml | 6 +++++ vmadm.md | 3 +++ 15 files changed, 220 insertions(+), 36 deletions(-) create mode 100644 src/cmd/shutdown.rs create mode 100644 src/cmd/start.rs create mode 100644 src/util.rs diff --git a/src/bin/vmadm.rs b/src/bin/vmadm.rs index 9b16966..711bc98 100644 --- a/src/bin/vmadm.rs +++ b/src/bin/vmadm.rs @@ -40,6 +40,22 @@ enum Command { spec: PathBuf, }, + Start { + #[structopt(flatten)] + common: CommonOptions, + + #[structopt(parse(from_os_str))] + spec: PathBuf, + }, + + Shutdown { + #[structopt(flatten)] + common: CommonOptions, + + #[structopt(parse(from_os_str))] + spec: PathBuf, + }, + CloudInit { #[structopt(flatten)] common: CommonOptions, @@ -79,6 +95,16 @@ fn main() -> anyhow::Result<()> { cmd::delete(&specs)?; } + Command::Start { common, spec } => { + let specs = get_specs(&common, &spec)?; + cmd::start(&specs)?; + } + + Command::Shutdown { common, spec } => { + let specs = get_specs(&common, &spec)?; + cmd::shutdown(&specs)?; + } + Command::CloudInit { common, spec, @@ -107,11 +133,9 @@ fn config(common: &CommonOptions) -> anyhow::Result { fn config_filename(common: &CommonOptions) -> PathBuf { if let Some(ref filename) = common.config { filename.to_path_buf() + } else if let Some(dirs) = ProjectDirs::from(QUALIFIER, ORG, APP) { + dirs.config_dir().join("config.yaml") } else { - if let Some(dirs) = ProjectDirs::from(QUALIFIER, ORG, APP) { - dirs.config_dir().join("config.yaml") - } else { - PathBuf::from("xxx") - } + PathBuf::from("xxx") } } diff --git a/src/cloudinit.rs b/src/cloudinit.rs index ec4efd3..169b13f 100644 --- a/src/cloudinit.rs +++ b/src/cloudinit.rs @@ -254,7 +254,7 @@ impl Hostkeys { debug!("generated Ed25519 host certificate {:?}", cert); Ok(Some(Self { ed25519_private: Some(pair.private().to_string()), - ed25519_certificate: Some(cert.to_string()), + ed25519_certificate: Some(cert), ..Self::default() })) } else { diff --git a/src/cmd/mod.rs b/src/cmd/mod.rs index 606e326..8ae8f70 100644 --- a/src/cmd/mod.rs +++ b/src/cmd/mod.rs @@ -12,5 +12,11 @@ pub use list::list; pub mod delete; pub use delete::delete; +pub mod start; +pub use start::start; + +pub mod shutdown; +pub use shutdown::shutdown; + pub mod cloud_init; pub use cloud_init::cloud_init; diff --git a/src/cmd/new.rs b/src/cmd/new.rs index 57f69ae..5484c08 100644 --- a/src/cmd/new.rs +++ b/src/cmd/new.rs @@ -3,13 +3,13 @@ use crate::cloudinit::{CloudInitConfig, CloudInitError}; use crate::image::{ImageError, VirtualMachineImage}; use crate::install::{virt_install, VirtInstallArgs, VirtInstallError}; +use crate::libvirt::{Libvirt, VirtError}; use crate::spec::Specification; +use crate::util::wait_for_ssh; use bytesize::GIB; use log::info; -use std::net::TcpStream; - -const SSH_PORT: i32 = 22; +use tempfile::tempdir; /// Errors returned by this module. #[derive(Debug, thiserror::Error)] @@ -25,6 +25,14 @@ pub enum NewError { /// Problem with libvirt. #[error(transparent)] VirtInstallError(#[from] VirtInstallError), + + /// Problem with virsh. + #[error(transparent)] + VirshError(#[from] std::io::Error), + + /// Problem from libvirt server. + #[error(transparent)] + VirtError(#[from] VirtError), } /// The `new` sub-command. @@ -32,6 +40,7 @@ pub enum NewError { /// Create all the new virtual machines specified by the caller. Wait /// until each VM's SSH port listens for connections. pub fn new(specs: &[Specification]) -> Result<(), NewError> { + let libvirt = Libvirt::connect("qemu:///system")?; for spec in specs { info!("creating new VM {}", spec.name); @@ -49,23 +58,18 @@ pub fn new(specs: &[Specification]) -> Result<(), NewError> { image.resize(spec.image_size_gib * GIB)?; info!("creating VM"); + let dir = tempdir()?; + let iso = dir.path().join("cloudinit.iso"); let mut args = VirtInstallArgs::new(&spec.name, &image, &init); args.set_memory(spec.memory_mib); args.set_vcpus(spec.cpus); - virt_install(&args)?; + virt_install(&args, &iso)?; info!("waiting for {} to open its SSH port", spec.name); - wait_for_port(&spec.name, SSH_PORT)?; + wait_for_ssh(&spec.name); + + libvirt.detach_cloud_init_iso(&spec.name)?; } Ok(()) } - -fn wait_for_port(name: &str, port: i32) -> Result<(), NewError> { - let addr = format!("{}:{}", name, port); - loop { - if TcpStream::connect(&addr).is_ok() { - return Ok(()); - } - } -} diff --git a/src/cmd/shutdown.rs b/src/cmd/shutdown.rs new file mode 100644 index 0000000..b53ebd3 --- /dev/null +++ b/src/cmd/shutdown.rs @@ -0,0 +1,19 @@ +//! The `shutdown` sub-command. + +use crate::libvirt::{Libvirt, VirtError}; +use crate::spec::Specification; +use log::{debug, info}; + +/// Shut down VMs corresponding to specifications. +pub fn shutdown(specs: &[Specification]) -> Result<(), VirtError> { + let libvirt = Libvirt::connect("qemu:///system")?; + for spec in specs { + info!("shutting down virtual machine {}", spec.name); + libvirt.shutdown(&spec.name)?; + } + for spec in specs { + debug!("waiting for {} to become inactive", spec.name); + libvirt.wait_for_inactive(&spec.name)?; + } + Ok(()) +} diff --git a/src/cmd/start.rs b/src/cmd/start.rs new file mode 100644 index 0000000..a69f54c --- /dev/null +++ b/src/cmd/start.rs @@ -0,0 +1,31 @@ +//! The `start` sub-command. + +use crate::libvirt::{Libvirt, VirtError}; +use crate::spec::Specification; +use crate::util::wait_for_ssh; +use log::info; + +/// Errors from this module. +#[derive(Debug, thiserror::Error)] +pub enum StartError { + /// Error from libvirt. + #[error(transparent)] + VirtError(#[from] VirtError), + + /// Error doing I/O. + #[error(transparent)] + IoError(#[from] std::io::Error), +} + +/// Start existing VMs corresponding to specifications. +pub fn start(specs: &[Specification]) -> Result<(), StartError> { + let libvirt = Libvirt::connect("qemu:///system")?; + for spec in specs { + info!("starting virtual machine {}", spec.name); + libvirt.start(&spec.name)?; + } + for spec in specs { + wait_for_ssh(&spec.name); + } + Ok(()) +} diff --git a/src/install.rs b/src/install.rs index 7506e0d..dce0fea 100644 --- a/src/install.rs +++ b/src/install.rs @@ -6,9 +6,9 @@ use crate::cloudinit::{CloudInitConfig, CloudInitError}; use crate::image::VirtualMachineImage; +use std::path::{Path, PathBuf}; use std::process::Command; use std::result::Result; -use tempfile::tempdir; /// Errors from this module #[derive(Debug, thiserror::Error)] @@ -91,9 +91,7 @@ impl VirtInstallArgs { } /// Create new VM with virt-install. -pub fn virt_install(args: &VirtInstallArgs) -> Result<(), VirtInstallError> { - let dir = tempdir()?; - let iso = dir.path().join("cloudinit.iso"); +pub fn virt_install(args: &VirtInstallArgs, iso: &Path) -> Result { args.init().create_iso(&iso)?; let r = Command::new("virt-install") @@ -122,5 +120,5 @@ pub fn virt_install(args: &VirtInstallArgs) -> Result<(), VirtInstallError> { let stderr = String::from_utf8(r.stderr)?; return Err(VirtInstallError::VirtInstallFailed(stderr)); } - Ok(()) + Ok(iso.to_path_buf()) } diff --git a/src/lib.rs b/src/lib.rs index f734bc8..3d94461 100644 Binary files a/src/lib.rs and b/src/lib.rs differ diff --git a/src/libvirt.rs b/src/libvirt.rs index 0dbc4e3..f0ad2d0 100644 --- a/src/libvirt.rs +++ b/src/libvirt.rs @@ -1,11 +1,14 @@ //! An abstraction on top of the libvirt bindings. +use crate::util::wait_for_ssh; use log::debug; use std::path::Path; use std::thread; use std::time::Duration; use virt::connect::Connect; -use virt::domain::Domain; +use virt::domain::{ + Domain, VIR_DOMAIN_AFFECT_CONFIG, VIR_DOMAIN_AFFECT_CURRENT, VIR_DOMAIN_AFFECT_LIVE, +}; /// Errors from this module. #[derive(Debug, thiserror::Error)] @@ -61,9 +64,33 @@ impl Libvirt { } } + pub fn wait_for_inactive(&self, name: &str) -> Result<(), VirtError> { + loop { + if !self.is_active(name)? { + break; + } + } + Ok(()) + } + + 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 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)?; + } + } + Ok(()) + } + pub fn start(&self, name: &str) -> Result<(), VirtError> { if let Some(domain) = self.get_domain(name)? { domain.create()?; + wait_for_ssh(name); } Ok(()) } @@ -71,6 +98,7 @@ impl Libvirt { pub fn shutdown(&self, name: &str) -> Result<(), VirtError> { if let Some(domain) = self.get_domain(name)? { domain.shutdown()?; + wait_until_inactive(&domain, name); } Ok(()) } @@ -78,9 +106,9 @@ impl Libvirt { pub fn delete(&self, name: &str, image: &Path) -> Result<(), VirtError> { if let Some(domain) = self.get_domain(name)? { debug!("shutting down {}", name); - domain.shutdown()?; + domain.shutdown().ok(); - wait_until_inactive(&domain, name)?; + wait_until_inactive(&domain, name); debug!("undefine {}", name); domain.undefine()?; @@ -93,7 +121,7 @@ impl Libvirt { } } -fn wait_until_inactive(domain: &Domain, name: &str) -> Result<(), VirtError> { +fn wait_until_inactive(domain: &Domain, name: &str) { debug!("waiting for domain {} to become inactive", name); let briefly = Duration::from_millis(1000); loop { @@ -107,5 +135,42 @@ fn wait_until_inactive(domain: &Domain, name: &str) -> Result<(), VirtError> { } debug!("domain {} is still running", name); } - Ok(()) +} + +// This is a HACK. The XML description of a domain contains +// descriptions of attached virtual disks. We find one that contains +// ".iso", and return that. +// +// +// +// +// +// +// +// +//
+// + +fn find_iso_xml(xml: &str) -> String { + let mut xml = xml; + loop { + let start = xml.find(""); + if end.is_none() { + break; + } + let end = end.unwrap(); + let disk = &xml[..end + 7]; + if let Some(_) = disk.find(".iso") { + return disk.to_string(); + } + xml = &xml[end..]; + } + "".to_string() } diff --git a/src/spec.rs b/src/spec.rs index fcf6eab..4bd0bb9 100644 --- a/src/spec.rs +++ b/src/spec.rs @@ -269,7 +269,7 @@ impl Specification { let spec = Specification { name: name.to_string(), - ssh_keys: ssh_keys, + ssh_keys, rsa_host_key: input.rsa_host_key.clone(), rsa_host_cert: input.rsa_host_cert.clone(), dsa_host_key: input.dsa_host_key.clone(), @@ -284,7 +284,7 @@ impl Specification { memory_mib: input.memory_mib(config, name)?, cpus: input.cpus(config, name)?, generate_host_certificate: gen_cert, - ca_key: ca_key, + ca_key, }; debug!("specification as with defaults applied: {:#?}", spec); diff --git a/src/sshkeys.rs b/src/sshkeys.rs index 813a26d..1425cb3 100644 --- a/src/sshkeys.rs +++ b/src/sshkeys.rs @@ -85,10 +85,7 @@ pub struct KeyPair { impl KeyPair { /// Create pair from string representation. pub fn from_str(public: String, private: String) -> Self { - Self { - private: private, - public: public, - } + Self { private, public } } /// Generate a new key pair of the desired kind. diff --git a/src/util.rs b/src/util.rs new file mode 100644 index 0000000..9933f03 --- /dev/null +++ b/src/util.rs @@ -0,0 +1,15 @@ +//! Utilities. + +use std::net::TcpStream; + +const SSH_PORT: i32 = 22; + +// Wait for a virtual machine to have opened its SSH port. +pub fn wait_for_ssh(name: &str) { + let addr = format!("{}:{}", name, SSH_PORT); + loop { + if TcpStream::connect(&addr).is_ok() { + return; + } + } +} diff --git a/subplot/vmadm.py b/subplot/vmadm.py index 6997c5b..dcdcbb6 100644 --- a/subplot/vmadm.py +++ b/subplot/vmadm.py @@ -81,6 +81,22 @@ def delete_vm(ctx, config=None, filename=None): runcmd_exit_code_is_zero(ctx) +def shutdown_vm(ctx, config=None, filename=None): + runcmd_run = globals()["runcmd_run"] + runcmd_exit_code_is_zero = globals()["runcmd_exit_code_is_zero"] + + runcmd_run(ctx, ["vmadm", "shutdown", "--config", config, filename]) + runcmd_exit_code_is_zero(ctx) + + +def start_vm(ctx, config=None, filename=None): + runcmd_run = globals()["runcmd_run"] + runcmd_exit_code_is_zero = globals()["runcmd_exit_code_is_zero"] + + runcmd_run(ctx, ["vmadm", "start", "--config", config, filename]) + runcmd_exit_code_is_zero(ctx) + + def run_hostname_over_ssh(ctx, config=None, target=None, args=None): runcmd_run = globals()["runcmd_run"] runcmd_exit_code_is_zero = globals()["runcmd_exit_code_is_zero"] diff --git a/subplot/vmadm.yaml b/subplot/vmadm.yaml index 21662b6..76881e1 100644 --- a/subplot/vmadm.yaml +++ b/subplot/vmadm.yaml @@ -11,6 +11,12 @@ function: create_vm cleanup: delete_vm +- when: "I invoke vmadm shutdown --config {config} {filename}" + function: shutdown_vm + +- when: "I invoke vmadm start --config {config} {filename}" + function: start_vm + - when: "I invoke ssh -F {config} {target} {args:text}" function: run_hostname_over_ssh diff --git a/vmadm.md b/vmadm.md index 0ba4f12..a3e62d7 100644 --- a/vmadm.md +++ b/vmadm.md @@ -158,6 +158,9 @@ when I invoke ssh -F .ssh/config debian@smoke df -h / then stdout contains "4.9G" when I invoke ssh -F .ssh/config debian@smoke free -m then stdout contains "1997" +when I invoke vmadm shutdown --config config.yaml smoke.yaml +when I invoke vmadm start --config config.yaml smoke.yaml +when I invoke ssh -F .ssh/config debian@smoke hostname ~~~ -- cgit v1.2.1