From 2fff978ab6977b1a3f9bab893e30a5dda56bf416 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Mon, 9 Dec 2024 18:49:25 +0000 Subject: [PATCH 1/4] phd: add new test for in-memory disk backend migration This test launches a VM with a disk backed by an in-memory backend, modifies the disk, migrates the VM, and ensures that the changes are still present on the migration target. Tune a few things up along the way: - Add a missing `with_context` in the artifact store. - Add an affordance to set the minimum size of a PHD FAT volume (previously the size was determined solely by the number of files a test added to the volume). - Tweak the Ubuntu 22.04 guest adapter's startup sequence so that tests run as root (so that they don't need to elevate with `sudo` to run commands that Just Work on other guest OSes that run as a superuser). --- phd-tests/framework/src/artifacts/store.rs | 5 +- phd-tests/framework/src/disk/fat.rs | 22 +- phd-tests/framework/src/guest_os/mod.rs | 2 +- .../framework/src/guest_os/ubuntu22_04.rs | 6 +- phd-tests/framework/src/guest_os/windows.rs | 40 +++- phd-tests/framework/src/test_vm/mod.rs | 9 +- phd-tests/tests/src/disk.rs | 218 +++++++++++++++--- 7 files changed, 256 insertions(+), 46 deletions(-) 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..58f0a5458 100644 --- a/phd-tests/framework/src/guest_os/windows.rs +++ b/phd-tests/framework/src/guest_os/windows.rs @@ -2,10 +2,48 @@ // 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 + } +} + 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..6273e9b79 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(win_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 = win_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,73 @@ 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) { + // This test verifies that live-migrating a disk with an in-memory backend + // copies the backend's data from the source to the target. To do that, it + // uses `dd` flags to (try to) force writes to be synchronized to disk + // immediately and to force reads to bypass the guest OS's caches. + // + // Dynamic tracing of block backend activity shows that Alpine guests don't + // reliably read from underlying storage even when `iflag=direct` is used. + // This can cause this test to pass incorrectly. Unless and until a more + // reliable way to bypass caches is found, skip this test on Alpine. + if let GuestOsKind::Alpine = ctx.default_guest_os_kind().await? { + phd_skip!("iflag=direct doesn't work as required on Alpine"); + } + + // 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. Use + // `oflag=sync` to force the guest OS to ensure this data is actually + // persisted to the device (and not just held in an in-memory cache). + vm.run_shell_command(&format!( + "dd if=/dev/random of={device_path} oflag=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 again. Use `iflag=direct` to try to get the guest + // to read this data back from the physical device instead of its disk + // cache. + target + .run_shell_command(&format!( + "dd if={device_path} of=/tmp/after iflag=direct bs=1K" + )) + .await?; + + 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"); +} From 22abc8b5437995eed2738832b8ac04790d9141a9 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Tue, 10 Dec 2024 21:13:54 +0000 Subject: [PATCH 2/4] implement live migration for in-memory backends --- bin/propolis-server/src/lib/initializer.rs | 6 +- lib/propolis/src/block/in_memory.rs | 71 ++++++++++++++++++++++ 2 files changed, 76 insertions(+), 1 deletion(-) diff --git a/bin/propolis-server/src/lib/initializer.rs b/bin/propolis-server/src/lib/initializer.rs index 281be81fe..2fcdf1b1a 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/lib/propolis/src/block/in_memory.rs b/lib/propolis/src/block/in_memory.rs index 3636bc8fa..25ad8d704 100644 --- a/lib/propolis/src/block/in_memory.rs +++ b/lib/propolis/src/block/in_memory.rs @@ -8,11 +8,17 @@ use std::sync::{Arc, Mutex}; use crate::accessors::MemAccessor; use crate::block; +use crate::migrate::{ + MigrateCtx, MigrateSingle, MigrateStateError, Migrator, PayloadOffer, + PayloadOutput, +}; use crate::tasks::ThreadGroup; use crate::vmm::{MemCtx, SubMapping}; use anyhow::Context; +use super::Lifecycle; + pub struct InMemoryBackend { state: Arc, @@ -232,3 +238,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) + } + } +} From ae0f618d16b9ec3ca4339d0e56dd1662945680b5 Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Wed, 11 Dec 2024 21:20:08 +0000 Subject: [PATCH 3/4] PR feedback --- lib/propolis/src/block/in_memory.rs | 3 +-- phd-tests/framework/src/guest_os/windows.rs | 8 ++++++++ phd-tests/tests/src/disk.rs | 4 ++-- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/lib/propolis/src/block/in_memory.rs b/lib/propolis/src/block/in_memory.rs index 25ad8d704..0eb6c2dad 100644 --- a/lib/propolis/src/block/in_memory.rs +++ b/lib/propolis/src/block/in_memory.rs @@ -8,6 +8,7 @@ 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, @@ -17,8 +18,6 @@ use crate::vmm::{MemCtx, SubMapping}; use anyhow::Context; -use super::Lifecycle; - pub struct InMemoryBackend { state: Arc, diff --git a/phd-tests/framework/src/guest_os/windows.rs b/phd-tests/framework/src/guest_os/windows.rs index 58f0a5458..bc55724d2 100644 --- a/phd-tests/framework/src/guest_os/windows.rs +++ b/phd-tests/framework/src/guest_os/windows.rs @@ -44,6 +44,14 @@ impl WindowsVm<'_> { } } +impl<'a> std::ops::Deref for WindowsVm<'a> { + 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/tests/src/disk.rs b/phd-tests/tests/src/disk.rs index 6273e9b79..cb72797ed 100644 --- a/phd-tests/tests/src/disk.rs +++ b/phd-tests/tests/src/disk.rs @@ -34,13 +34,13 @@ async fn launch_vm_and_find_in_memory_disk( vm.launch().await?; vm.wait_to_boot().await?; - let device_path = if let Some(win_vm) = vm.get_windows_vm() { + 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 = win_vm.run_powershell_command(cmd).await?.parse::()?; + 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. From ef7998c73536a5f1b2f579d910b57d12c9454e5f Mon Sep 17 00:00:00 2001 From: Greg Colombo Date: Thu, 12 Dec 2024 22:02:29 +0000 Subject: [PATCH 4/4] phd: make in-memory backend test work for Alpine --- phd-tests/tests/src/disk.rs | 42 +++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/phd-tests/tests/src/disk.rs b/phd-tests/tests/src/disk.rs index cb72797ed..bcf5f9238 100644 --- a/phd-tests/tests/src/disk.rs +++ b/phd-tests/tests/src/disk.rs @@ -145,19 +145,6 @@ async fn in_memory_backend_smoke_test(ctx: &Framework) { #[phd_testcase] async fn in_memory_backend_migration_test(ctx: &Framework) { - // This test verifies that live-migrating a disk with an in-memory backend - // copies the backend's data from the source to the target. To do that, it - // uses `dd` flags to (try to) force writes to be synchronized to disk - // immediately and to force reads to bypass the guest OS's caches. - // - // Dynamic tracing of block backend activity shows that Alpine guests don't - // reliably read from underlying storage even when `iflag=direct` is used. - // This can cause this test to pass incorrectly. Unless and until a more - // reliable way to bypass caches is found, skip this test on Alpine. - if let GuestOsKind::Alpine = ctx.default_guest_os_kind().await? { - phd_skip!("iflag=direct doesn't work as required on Alpine"); - } - // 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 @@ -170,11 +157,17 @@ async fn in_memory_backend_migration_test(ctx: &Framework) { ) .await?; - // Scribble random data into the first kilobyte of the data disk. Use - // `oflag=sync` to force the guest OS to ensure this data is actually - // persisted to the device (and not just held in an in-memory cache). + // 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} oflag=sync bs=1K count=1" + "dd if=/dev/random of={device_path} {force_sync} bs=1K count=1" )) .await?; @@ -197,15 +190,24 @@ async fn in_memory_backend_migration_test(ctx: &Framework) { .migrate_from(&vm, Uuid::new_v4(), MigrationTimeout::default()) .await?; - // Read the scribbled data again. Use `iflag=direct` to try to get the guest - // to read this data back from the physical device instead of its disk - // cache. + // 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?;