diff --git a/bin/propolis-server/src/lib/initializer.rs b/bin/propolis-server/src/lib/initializer.rs index 666e0f549..92e5a94f8 100644 --- a/bin/propolis-server/src/lib/initializer.rs +++ b/bin/propolis-server/src/lib/initializer.rs @@ -468,7 +468,7 @@ impl<'a> MachineInitializer<'a> { } async fn create_storage_backend_from_spec( - &self, + &mut self, backend_spec: &StorageBackend, backend_id: &SpecKey, nexus_client: &Option, @@ -573,6 +573,10 @@ impl<'a> MachineInitializer<'a> { ) .context("failed to create in-memory storage backend")?; + // In-memory backends need to be registered for lifecycle + // notifications so that they can export/import changes to the + // backing disk across migrations. + self.devices.insert(backend_id.clone(), be.clone()); Ok(StorageBackendInstance { be, crucible: None }) } } diff --git a/bin/propolis-server/src/lib/stats/virtual_machine.rs b/bin/propolis-server/src/lib/stats/virtual_machine.rs index 75c5ba0ac..2574f29e7 100644 --- a/bin/propolis-server/src/lib/stats/virtual_machine.rs +++ b/bin/propolis-server/src/lib/stats/virtual_machine.rs @@ -190,7 +190,7 @@ impl KstatTarget for VirtualMachine { .find_map(|(_, kstat, data)| { kstat_instance_from_instance_id(kstat, data, &self.vm_name) }) - .ok_or_else(|| Error::NoSuchKstat)?; + .ok_or(Error::NoSuchKstat)?; // Armed with the kstat instance, find all relevant metrics related to // this particular VM. For now, we produce only vCPU usage metrics, but diff --git a/lib/propolis/src/block/in_memory.rs b/lib/propolis/src/block/in_memory.rs index 3636bc8fa..0eb6c2dad 100644 --- a/lib/propolis/src/block/in_memory.rs +++ b/lib/propolis/src/block/in_memory.rs @@ -8,6 +8,11 @@ use std::sync::{Arc, Mutex}; use crate::accessors::MemAccessor; use crate::block; +use crate::common::Lifecycle; +use crate::migrate::{ + MigrateCtx, MigrateSingle, MigrateStateError, Migrator, PayloadOffer, + PayloadOutput, +}; use crate::tasks::ThreadGroup; use crate::vmm::{MemCtx, SubMapping}; @@ -232,3 +237,68 @@ fn process_write_request( Ok(()) } + +impl Lifecycle for InMemoryBackend { + fn type_name(&self) -> &'static str { + "in-memory-storage" + } + + fn migrate(&self) -> Migrator { + Migrator::Single(self) + } +} + +impl MigrateSingle for InMemoryBackend { + fn export( + &self, + _ctx: &MigrateCtx, + ) -> std::result::Result { + let bytes = self.state.bytes.lock().unwrap(); + Ok(migrate::InMemoryBlockBackendV1 { bytes: bytes.clone() }.into()) + } + + fn import( + &self, + mut offer: PayloadOffer, + _ctx: &MigrateCtx, + ) -> std::result::Result<(), MigrateStateError> { + let data: migrate::InMemoryBlockBackendV1 = offer.parse()?; + let mut guard = self.state.bytes.lock().unwrap(); + if guard.len() != data.bytes.len() { + return Err(MigrateStateError::ImportFailed(format!( + "imported in-memory block backend data has length {}, \ + but backend's original length was {}", + data.bytes.len(), + guard.len() + ))); + } + + *guard = data.bytes; + Ok(()) + } +} + +mod migrate { + use serde::{Deserialize, Serialize}; + + use crate::migrate::{Schema, SchemaId}; + + #[derive(Serialize, Deserialize)] + pub struct InMemoryBlockBackendV1 { + pub(super) bytes: Vec, + } + + impl std::fmt::Debug for InMemoryBlockBackendV1 { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("InMemoryBlockBackendV1") + .field("bytes", &"".to_string()) + .finish() + } + } + + impl Schema<'_> for InMemoryBlockBackendV1 { + fn id() -> SchemaId { + ("in-memory-block-backend", 1) + } + } +} diff --git a/phd-tests/framework/src/artifacts/store.rs b/phd-tests/framework/src/artifacts/store.rs index 08fc84b58..65def357d 100644 --- a/phd-tests/framework/src/artifacts/store.rs +++ b/phd-tests/framework/src/artifacts/store.rs @@ -487,7 +487,10 @@ fn file_hash_equals( path: impl AsRef, expected_digest: &str, ) -> anyhow::Result<()> { - let file = File::open(path)?; + let file = File::open(&path).with_context(|| { + format!("checking hash for file {}", path.as_ref().display()) + })?; + let mut reader = BufReader::new(file); hash_equals(&mut reader, expected_digest) } diff --git a/phd-tests/framework/src/disk/fat.rs b/phd-tests/framework/src/disk/fat.rs index d31583d6c..e845e86e8 100644 --- a/phd-tests/framework/src/disk/fat.rs +++ b/phd-tests/framework/src/disk/fat.rs @@ -143,13 +143,26 @@ impl FatFilesystem { Self { files: vec![], sectors_remaining: total_usable_sectors() } } + /// Converts the supplied `contents` string slice to bytes and adds it to + /// the filesystem using [`Self::add_file_from_bytes`]. + /// + /// The supplied `filename` must not contain any path separators (the `/` + /// character). + pub fn add_file_from_str( + &mut self, + filename: &str, + contents: &str, + ) -> Result<(), Error> { + self.add_file_from_bytes(filename, contents.as_bytes()) + } + /// Adds a file with the supplied `contents` that will appear in the root /// directory of the generated file system. The given `filename` must not /// contain any path separators (the `/` character). - pub fn add_file_from_str( + pub fn add_file_from_bytes( &mut self, filename: &str, - contents: &str, + contents: &[u8], ) -> Result<(), Error> { // The `fatfs` crate will break paths containing separators into their // component directories before trying to create the requested file in @@ -163,8 +176,7 @@ impl FatFilesystem { return Err(Error::PathSeparatorInFilename(filename.to_owned())); } - let bytes = contents.as_bytes(); - let sectors_needed = Sectors::needed_for_bytes(bytes.len()); + let sectors_needed = Sectors::needed_for_bytes(contents.len()); if sectors_needed > self.sectors_remaining { Err(Error::NoSpace { required: sectors_needed.0, @@ -173,7 +185,7 @@ impl FatFilesystem { } else { self.files.push(File { name: filename.to_owned(), - contents: bytes.to_vec(), + contents: contents.to_vec(), }); self.sectors_remaining -= sectors_needed; diff --git a/phd-tests/framework/src/guest_os/mod.rs b/phd-tests/framework/src/guest_os/mod.rs index 18051328c..5892f5efa 100644 --- a/phd-tests/framework/src/guest_os/mod.rs +++ b/phd-tests/framework/src/guest_os/mod.rs @@ -14,7 +14,7 @@ mod debian11_nocloud; mod linux; mod shell_commands; mod ubuntu22_04; -mod windows; +pub mod windows; mod windows_server_2016; mod windows_server_2019; mod windows_server_2022; diff --git a/phd-tests/framework/src/guest_os/ubuntu22_04.rs b/phd-tests/framework/src/guest_os/ubuntu22_04.rs index 85dcece44..077669158 100644 --- a/phd-tests/framework/src/guest_os/ubuntu22_04.rs +++ b/phd-tests/framework/src/guest_os/ubuntu22_04.rs @@ -16,13 +16,17 @@ impl GuestOs for Ubuntu2204 { CommandSequenceEntry::write_str("ubuntu"), CommandSequenceEntry::wait_for("Password: "), CommandSequenceEntry::write_str("1!Passw0rd"), + CommandSequenceEntry::wait_for("ubuntu@ubuntu:~$"), + CommandSequenceEntry::write_str("sudo bash\n"), + CommandSequenceEntry::wait_for("root@ubuntu:/home/ubuntu#"), + CommandSequenceEntry::write_str("cd ~\n"), CommandSequenceEntry::wait_for(self.get_shell_prompt()), ]) .extend(super::linux::stty_enable_long_lines(self)) } fn get_shell_prompt(&self) -> &'static str { - "ubuntu@ubuntu:~$" + "root@ubuntu:~#" } fn read_only_fs(&self) -> bool { diff --git a/phd-tests/framework/src/guest_os/windows.rs b/phd-tests/framework/src/guest_os/windows.rs index 8da922532..8180550e7 100644 --- a/phd-tests/framework/src/guest_os/windows.rs +++ b/phd-tests/framework/src/guest_os/windows.rs @@ -2,10 +2,56 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -//! Helper functions for generating Windows guest OS adaptations. +//! Functionality common to all Windows guests. + +use crate::TestVm; use super::{CommandSequence, CommandSequenceEntry, GuestOsKind}; +use tracing::info; + +/// A wrapper that provides Windows-specific extensions to the core `TestVm` +/// implementation. +pub struct WindowsVm<'a> { + /// The VM being extended by this structure. The framework is required to + /// ensure that the VM is actually configured to run a Windows guest OS. + pub(crate) vm: &'a TestVm, +} + +impl WindowsVm<'_> { + /// Runs `cmd` as a Powershell command. + pub async fn run_powershell_command( + &self, + cmd: &str, + ) -> anyhow::Result { + assert!(self.vm.guest_os_kind().is_windows()); + + info!(cmd, "executing Powershell command"); + + // Use Powershell's -encodedCommand switch to keep important Powershell + // sigils in the command (like "$") from being interpreted by whatever + // shell is being used to invoke Powershell. This switch expects that + // the encoded string will decode into a UTF-16 string; `str`s are, of + // course, UTF-8, so switch encodings before converting to base64. + let utf16 = cmd.encode_utf16().collect::>(); + let base64 = base64::Engine::encode( + &base64::engine::general_purpose::STANDARD, + unsafe { utf16.align_to::().1 }, + ); + + let cmd = format!("powershell -encodedCommand {base64}"); + self.vm.run_shell_command(&cmd).await + } +} + +impl std::ops::Deref for WindowsVm<'_> { + type Target = TestVm; + + fn deref(&self) -> &Self::Target { + self.vm + } +} + const CYGWIN_CMD: &str = "C:\\cygwin\\cygwin.bat\r"; /// Prepends a `reset` command to the shell command supplied in `cmd`. Windows diff --git a/phd-tests/framework/src/test_vm/mod.rs b/phd-tests/framework/src/test_vm/mod.rs index 872856592..caa17360e 100644 --- a/phd-tests/framework/src/test_vm/mod.rs +++ b/phd-tests/framework/src/test_vm/mod.rs @@ -12,7 +12,8 @@ use std::{ use crate::{ guest_os::{ - self, CommandSequence, CommandSequenceEntry, GuestOs, GuestOsKind, + self, windows::WindowsVm, CommandSequence, CommandSequenceEntry, + GuestOs, GuestOsKind, }, serial::{BufferKind, SerialConsole}, test_vm::{ @@ -387,6 +388,12 @@ impl TestVm { self.spec.guest_os_kind } + /// If this VM is running a Windows guest, returns a wrapper that provides + /// Windows-specific VM functions. + pub fn get_windows_vm(&self) -> Option { + self.guest_os_kind().is_windows().then_some(WindowsVm { vm: self }) + } + /// Sets the VM to the running state. If the VM has not yet been launched /// (by sending a Propolis instance-ensure request to it), send that request /// first. diff --git a/phd-tests/tests/src/disk.rs b/phd-tests/tests/src/disk.rs index aac22a3f3..bcf5f9238 100644 --- a/phd-tests/tests/src/disk.rs +++ b/phd-tests/tests/src/disk.rs @@ -4,60 +4,136 @@ use phd_framework::{ disk::{fat::FatFilesystem, DiskSource}, - test_vm::{DiskBackend, DiskInterface}, + guest_os::GuestOsKind, + test_vm::{DiskBackend, DiskInterface, MigrationTimeout}, + TestVm, }; use phd_testcase::*; -use tracing::{info, warn}; +use uuid::Uuid; -#[phd_testcase] -async fn in_memory_backend_smoke_test(ctx: &Framework) { - const HELLO_MSG: &str = "hello oxide!"; - - let mut cfg = ctx.vm_config_builder("in_memory_backend_test"); - let mut data = FatFilesystem::new(); - data.add_file_from_str("hello_oxide.txt", HELLO_MSG)?; +/// Creates a VM with an in-memory disk backed by the supplied `data`, waits for +/// it to boot, and issues some shell commands to find the +/// +/// Returns a tuple containing the created VM and the path to the guest disk +/// device representing the in-memory disk. +async fn launch_vm_and_find_in_memory_disk( + ctx: &Framework, + vm_name: &str, + data: DiskSource<'_>, + readonly: bool, +) -> anyhow::Result<(TestVm, String)> { + let mut cfg = ctx.vm_config_builder(vm_name); cfg.data_disk( "data-disk-0", - DiskSource::FatFilesystem(data), + data, DiskInterface::Virtio, - DiskBackend::InMemory { readonly: true }, + DiskBackend::InMemory { readonly }, 24, ); - let mut vm = ctx.spawn_vm(&cfg, None).await?; - if vm.guest_os_kind().is_windows() { - phd_skip!("this test uses mount options not supported by Cygwin"); - } vm.launch().await?; vm.wait_to_boot().await?; - // Some guests expose a /dev/disk/by-path directory that contains symlinks - // mapping PCI paths to the underlying device nodes under /dev. If this is - // present, check the mapping for a virtio disk at 0.24.0. - // - // The commands after this try to mount the in-memory disk and assume that - // its device is located at /dev/vda. If the by-path directory is present, - // try to check that the disk is located there and fail the test early if - // it's not. If the by-path directory is missing, put up a warning and hope - // for the best. - let dev_disk = vm.run_shell_command("ls /dev/disk").await?; - if dev_disk.contains("by-path") { - let ls = vm.run_shell_command("ls -la /dev/disk/by-path").await?; - info!(%ls, "guest disk device paths"); - assert!(ls.contains("virtio-pci-0000:00:18.0 -> ../../vda")); + let device_path = if let Some(vm) = vm.get_windows_vm() { + // Cygwin documents that \Device\HardDisk devices in the NT device + // namespace map to /dev/sd devices in the emulated POSIX namespace: + // disk 0 is /dev/sda, disk 1 is /dev/sdb, and so on. Get the NT device + // number of the attached in-memory disk. + let cmd = "(Get-PhysicalDisk | Where {$_.BusType -ne 'NVMe'}).DeviceId"; + let num = vm.run_powershell_command(cmd).await?.parse::()?; + + // If the test requires the disk to be writable, run diskpart to ensure + // that its readonly attribute is cleared. + if !readonly { + vm.run_shell_command(&format!( + "echo 'select disk {num}' >> diskpart.txt" + )) + .await?; + vm.run_shell_command( + "echo 'attributes disk clear readonly' >> diskpart.txt", + ) + .await?; + vm.run_shell_command("diskpart /s diskpart.txt").await?; + } + + // Crudely map from the drive number to the appropriate letter suffix. + // Cygwin supports more than 26 drives (up to /dev/sddx), but the data + // disk shouldn't map into that range unless Windows does something + // unexpected with its drive number assignments. + assert!( + num < 26, + "physical drive number must be less than 26 to map to a Cygwin dev" + ); + + format!("/dev/sd{}", (b'a' + num) as char) } else { - warn!( - "guest doesn't support /dev/disk/by-path, did not verify device \ - path" + let ls = vm + .run_shell_command( + "ls /sys/devices/pci0000:00/0000:00:18.0/virtio0/block", + ) + .await?; + + format!("/dev/{ls}") + }; + + Ok((vm, device_path)) +} + +async fn mount_in_memory_disk( + vm: &mut TestVm, + device_path: &str, + readonly: bool, +) -> anyhow::Result<()> { + if vm.guest_os_kind().is_windows() { + phd_skip!( + "in-memory disk tests use mount options not supported by Cygwin" ); } vm.run_shell_command("mkdir /phd").await?; - // The disk is read-only, so pass the `ro` option to `mount` so that it - // doesn't complain about not being able to mount for writing. - let mount = vm.run_shell_command("mount -o ro /dev/vda /phd").await?; - assert_eq!(mount, ""); + // If the disk is read-only, add the `ro` qualifier to the mount command + // so that it doesn't complain about being unable to mount for writing. + if readonly { + let mount = vm + .run_shell_command(&format!("mount -o ro {device_path} /phd")) + .await?; + assert_eq!(mount, ""); + } else { + vm.run_shell_command(&format!( + "echo '{device_path} /phd vfat defaults 0 2' >> /etc/fstab" + )) + .await?; + + let mount = vm.run_shell_command("mount -a").await?; + assert_eq!(mount, ""); + } + + Ok(()) +} + +#[phd_testcase] +async fn in_memory_backend_smoke_test(ctx: &Framework) { + if ctx.default_guest_os_kind().await?.is_windows() { + phd_skip!( + "in-memory disk tests use mount options not supported by Cygwin" + ); + } + + const HELLO_MSG: &str = "hello oxide!"; + + let readonly = true; + let mut data = FatFilesystem::new(); + data.add_file_from_str("hello_oxide.txt", HELLO_MSG)?; + let (mut vm, device_path) = launch_vm_and_find_in_memory_disk( + ctx, + "in_memory_backend_test", + DiskSource::FatFilesystem(data), + readonly, + ) + .await?; + + mount_in_memory_disk(&mut vm, &device_path, readonly).await?; // The file should be there and have the expected contents. let ls = vm.run_shell_command("ls /phd").await?; @@ -66,3 +142,75 @@ async fn in_memory_backend_smoke_test(ctx: &Framework) { let cat = vm.run_shell_command("cat /phd/hello_oxide.txt").await?; assert_eq!(cat, HELLO_MSG); } + +#[phd_testcase] +async fn in_memory_backend_migration_test(ctx: &Framework) { + // A blank disk is fine for this test: the rest of the test will address the + // disk device directly instead of assuming it has a file system. This works + // around #824 for Windows guests (which may not recognize the FAT + // filesystems PHD produces). + let (vm, device_path) = launch_vm_and_find_in_memory_disk( + ctx, + "in_memory_backend_migration_test_source", + DiskSource::Blank(16 * 1024), + false, + ) + .await?; + + // Scribble random data into the first kilobyte of the data disk, passing + // the appropriate flags to ensure that the guest actually writes the data + // to the disk (instead of just holding it in memory). + let force_sync = if let GuestOsKind::Alpine = vm.guest_os_kind() { + "conv=sync" + } else { + "oflag=sync" + }; + + vm.run_shell_command(&format!( + "dd if=/dev/random of={device_path} {force_sync} bs=1K count=1" + )) + .await?; + + // Read the scribbled data out to a file on the main OS disk. + vm.run_shell_command(&format!( + "dd if={device_path} of=/tmp/before iflag=direct bs=1K" + )) + .await?; + + // Migrate the VM. + let mut target = ctx + .spawn_successor_vm( + "in_memory_backend_migration_test_target", + &vm, + None, + ) + .await?; + + target + .migrate_from(&vm, Uuid::new_v4(), MigrationTimeout::default()) + .await?; + + // Read the scribbled data back from the disk. On most guests, adding + // `iflag=direct` to the `dd` invocation is sufficient to bypass the guest's + // caches and read from the underlying disk. Alpine guests appear also to + // need a procfs poke to drop page caches before they'll read from the disk. + if let GuestOsKind::Alpine = vm.guest_os_kind() { + target.run_shell_command("sync").await?; + target.run_shell_command("echo 3 > /proc/sys/vm/drop_caches").await?; + } + + target + .run_shell_command(&format!( + "dd if={device_path} of=/tmp/after iflag=direct bs=1K" + )) + .await?; + + // The data that was scribbled before migrating should match what was read + // back from the disk. If it doesn't, migration restored the original + // (blank) disk contents, which is incorrect. + let out = target + .run_shell_command("diff --report-identical /tmp/before /tmp/after") + .await?; + + assert_eq!(out, "Files /tmp/before and /tmp/after are identical"); +}