From b2ff038a711a72b8578ec41f54cf378ef09c605c Mon Sep 17 00:00:00 2001 From: Babis Chalios Date: Wed, 29 Nov 2023 17:43:15 +0000 Subject: [PATCH 1/3] test(virtio-block): test also async engine So far, tests for virtio block devices only used the synchronous engine for testing. This commit, uses the "io_engine" fixture which returns both "Sync" and "Async" (for the kernels > 4.14). It also adds a test the we can mount devices after patching the backing file on the host. Signed-off-by: Babis Chalios --- .../integration_tests/functional/test_api.py | 21 ++++++-- .../functional/test_drive_virtio.py | 51 +++++++++++++------ 2 files changed, 51 insertions(+), 21 deletions(-) diff --git a/tests/integration_tests/functional/test_api.py b/tests/integration_tests/functional/test_api.py index fce3d86e295..3bfb27ba3ea 100644 --- a/tests/integration_tests/functional/test_api.py +++ b/tests/integration_tests/functional/test_api.py @@ -92,7 +92,7 @@ def test_drive_io_engine(test_microvm_with_api): assert test_microvm.api.vm_config.get().json()["drives"][0]["io_engine"] == "Sync" -def test_api_put_update_pre_boot(test_microvm_with_api): +def test_api_put_update_pre_boot(test_microvm_with_api, io_engine): """ Test that PUT updates are allowed before the microvm boots. @@ -111,6 +111,7 @@ def test_api_put_update_pre_boot(test_microvm_with_api): path_on_host=test_microvm.create_jailed_resource(fs1.path), is_root_device=False, is_read_only=False, + io_engine=io_engine, ) # Updates to `kernel_image_path` with an invalid path are not allowed. @@ -132,6 +133,7 @@ def test_api_put_update_pre_boot(test_microvm_with_api): path_on_host="foo.bar", is_read_only=True, is_root_device=True, + io_engine=io_engine, ) # Updates to `is_root_device` that result in two root block devices are not @@ -142,6 +144,7 @@ def test_api_put_update_pre_boot(test_microvm_with_api): path_on_host=test_microvm.get_jailed_resource(fs1.path), is_read_only=False, is_root_device=True, + io_engine=io_engine, ) # Valid updates to `path_on_host` and `is_read_only` are allowed. @@ -151,6 +154,7 @@ def test_api_put_update_pre_boot(test_microvm_with_api): path_on_host=test_microvm.create_jailed_resource(fs2.path), is_read_only=True, is_root_device=False, + io_engine=io_engine, ) # Valid updates to all fields in the machine configuration are allowed. @@ -473,7 +477,7 @@ def test_api_cpu_config(test_microvm_with_api, custom_cpu_template): test_microvm.api.cpu_config.put(**custom_cpu_template["template"]) -def test_api_put_update_post_boot(test_microvm_with_api): +def test_api_put_update_post_boot(test_microvm_with_api, io_engine): """ Test that PUT updates are rejected after the microvm boots. """ @@ -520,6 +524,7 @@ def test_api_put_update_post_boot(test_microvm_with_api): path_on_host=test_microvm.jailer.jailed_path(test_microvm.rootfs_file), is_read_only=False, is_root_device=True, + io_engine=io_engine, ) # MMDS config is not allowed post-boot. @@ -532,7 +537,7 @@ def test_api_put_update_post_boot(test_microvm_with_api): test_microvm.api.mmds_config.put(**mmds_config) -def test_rate_limiters_api_config(test_microvm_with_api): +def test_rate_limiters_api_config(test_microvm_with_api, io_engine): """ Test the IO rate limiter API config. """ @@ -549,6 +554,7 @@ def test_rate_limiters_api_config(test_microvm_with_api): is_read_only=False, is_root_device=False, rate_limiter={"bandwidth": {"size": 1000000, "refill_time": 100}}, + io_engine=io_engine, ) # Test drive with ops rate-limiting. @@ -559,6 +565,7 @@ def test_rate_limiters_api_config(test_microvm_with_api): is_read_only=False, is_root_device=False, rate_limiter={"ops": {"size": 1, "refill_time": 100}}, + io_engine=io_engine, ) # Test drive with bw and ops rate-limiting. @@ -572,6 +579,7 @@ def test_rate_limiters_api_config(test_microvm_with_api): "bandwidth": {"size": 1000000, "refill_time": 100}, "ops": {"size": 1, "refill_time": 100}, }, + io_engine=io_engine, ) # Test drive with 'empty' rate-limiting (same as not specifying the field) @@ -582,6 +590,7 @@ def test_rate_limiters_api_config(test_microvm_with_api): is_read_only=False, is_root_device=False, rate_limiter={}, + io_engine=io_engine, ) # Test the NET rate limiting API. @@ -636,7 +645,7 @@ def test_rate_limiters_api_config(test_microvm_with_api): ) -def test_api_patch_pre_boot(test_microvm_with_api): +def test_api_patch_pre_boot(test_microvm_with_api, io_engine): """ Test that PATCH updates are not allowed before the microvm boots. """ @@ -654,6 +663,7 @@ def test_api_patch_pre_boot(test_microvm_with_api): path_on_host=test_microvm.create_jailed_resource(fs1.path), is_root_device=False, is_read_only=False, + io_engine=io_engine, ) iface_id = "1" @@ -685,7 +695,7 @@ def test_api_patch_pre_boot(test_microvm_with_api): test_microvm.api.network.patch(iface_id=iface_id) -def test_negative_api_patch_post_boot(test_microvm_with_api): +def test_negative_api_patch_post_boot(test_microvm_with_api, io_engine): """ Test PATCH updates that are not allowed after the microvm boots. """ @@ -702,6 +712,7 @@ def test_negative_api_patch_post_boot(test_microvm_with_api): path_on_host=test_microvm.create_jailed_resource(fs1.path), is_root_device=False, is_read_only=False, + io_engine=io_engine, ) iface_id = "1" diff --git a/tests/integration_tests/functional/test_drive_virtio.py b/tests/integration_tests/functional/test_drive_virtio.py index dc11abb9b05..d978768e39e 100644 --- a/tests/integration_tests/functional/test_drive_virtio.py +++ b/tests/integration_tests/functional/test_drive_virtio.py @@ -23,7 +23,7 @@ def partuuid_and_disk_path_tmpfs(rootfs_ubuntu_22, tmp_path): disk_path.unlink() -def test_rescan_file(test_microvm_with_api): +def test_rescan_file(test_microvm_with_api, io_engine): """ Verify that rescan works with a file-backed virtio device. """ @@ -39,7 +39,7 @@ def test_rescan_file(test_microvm_with_api): fs = drive_tools.FilesystemFile( os.path.join(test_microvm.fsfiles, "scratch"), size=block_size ) - test_microvm.add_drive("scratch", fs.path) + test_microvm.add_drive("scratch", fs.path, io_engine=io_engine) test_microvm.start() @@ -64,7 +64,7 @@ def test_rescan_file(test_microvm_with_api): _check_block_size(test_microvm.ssh, "/dev/vdb", fs.size()) -def test_device_ordering(test_microvm_with_api): +def test_device_ordering(test_microvm_with_api, io_engine): """ Verify device ordering. @@ -78,7 +78,7 @@ def test_device_ordering(test_microvm_with_api): fs1 = drive_tools.FilesystemFile( os.path.join(test_microvm.fsfiles, "scratch1"), size=128 ) - test_microvm.add_drive("scratch1", fs1.path) + test_microvm.add_drive("scratch1", fs1.path, io_engine=io_engine) # Set up the microVM with 1 vCPUs, 256 MiB of RAM and a root file system # (this is the second block device added). @@ -89,7 +89,7 @@ def test_device_ordering(test_microvm_with_api): fs2 = drive_tools.FilesystemFile( os.path.join(test_microvm.fsfiles, "scratch2"), size=512 ) - test_microvm.add_drive("scratch2", fs2.path) + test_microvm.add_drive("scratch2", fs2.path, io_engine=io_engine) test_microvm.start() @@ -112,7 +112,7 @@ def test_device_ordering(test_microvm_with_api): _check_block_size(ssh_connection, "/dev/vdc", fs2.size()) -def test_rescan_dev(test_microvm_with_api): +def test_rescan_dev(test_microvm_with_api, io_engine): """ Verify that rescan works with a device-backed virtio device. """ @@ -125,7 +125,7 @@ def test_rescan_dev(test_microvm_with_api): # Add a scratch block device. fs1 = drive_tools.FilesystemFile(os.path.join(test_microvm.fsfiles, "fs1")) - test_microvm.add_drive("scratch", fs1.path) + test_microvm.add_drive("scratch", fs1.path, io_engine=io_engine) test_microvm.start() @@ -152,7 +152,7 @@ def test_rescan_dev(test_microvm_with_api): utils.run_cmd(["losetup", "--detach", loopback_device]) -def test_non_partuuid_boot(test_microvm_with_api): +def test_non_partuuid_boot(test_microvm_with_api, io_engine): """ Test the output reported by blockdev when booting from /dev/vda. """ @@ -165,7 +165,7 @@ def test_non_partuuid_boot(test_microvm_with_api): # Add another read-only block device. fs = drive_tools.FilesystemFile(os.path.join(test_microvm.fsfiles, "readonly")) - test_microvm.add_drive("scratch", fs.path, is_read_only=True) + test_microvm.add_drive("scratch", fs.path, is_read_only=True, io_engine=io_engine) test_microvm.start() @@ -183,7 +183,7 @@ def test_non_partuuid_boot(test_microvm_with_api): _check_drives(test_microvm, assert_dict, keys_array) -def test_partuuid_boot(test_microvm_with_api, partuuid_and_disk_path_tmpfs): +def test_partuuid_boot(test_microvm_with_api, partuuid_and_disk_path_tmpfs, io_engine): """ Test the output reported by blockdev when booting with PARTUUID. """ @@ -204,6 +204,7 @@ def test_partuuid_boot(test_microvm_with_api, partuuid_and_disk_path_tmpfs): disk_path, is_root_device=True, partuuid=partuuid, + io_engine=io_engine, ) test_microvm.start() @@ -216,7 +217,7 @@ def test_partuuid_boot(test_microvm_with_api, partuuid_and_disk_path_tmpfs): _check_drives(test_microvm, assert_dict, keys_array) -def test_partuuid_update(test_microvm_with_api): +def test_partuuid_update(test_microvm_with_api, io_engine): """ Test successful switching from PARTUUID boot to /dev/vda boot. """ @@ -229,7 +230,11 @@ def test_partuuid_update(test_microvm_with_api): # Add the root block device specified through PARTUUID. test_microvm.add_drive( - "rootfs", test_microvm.rootfs_file, is_root_device=True, partuuid="0eaa91a0-01" + "rootfs", + test_microvm.rootfs_file, + is_root_device=True, + partuuid="0eaa91a0-01", + io_engine=io_engine, ) # Update the root block device to boot from /dev/vda. @@ -237,6 +242,7 @@ def test_partuuid_update(test_microvm_with_api): "rootfs", test_microvm.rootfs_file, is_root_device=True, + io_engine=io_engine, ) test_microvm.start() @@ -249,7 +255,7 @@ def test_partuuid_update(test_microvm_with_api): _check_drives(test_microvm, assert_dict, keys_array) -def test_patch_drive(test_microvm_with_api): +def test_patch_drive(test_microvm_with_api, io_engine): """ Test replacing the backing filesystem after guest boot works. """ @@ -261,10 +267,12 @@ def test_patch_drive(test_microvm_with_api): test_microvm.add_net_iface() fs1 = drive_tools.FilesystemFile(os.path.join(test_microvm.fsfiles, "scratch")) - test_microvm.add_drive("scratch", fs1.path) + test_microvm.add_drive("scratch", fs1.path, io_engine=io_engine) test_microvm.start() + _check_mount(test_microvm.ssh, "/dev/vdb") + # Updates to `path_on_host` with a valid path are allowed. fs2 = drive_tools.FilesystemFile( os.path.join(test_microvm.fsfiles, "otherscratch"), size=512 @@ -273,6 +281,8 @@ def test_patch_drive(test_microvm_with_api): drive_id="scratch", path_on_host=test_microvm.create_jailed_resource(fs2.path) ) + _check_mount(test_microvm.ssh, "/dev/vdb") + # The `lsblk` command should output 2 lines to STDOUT: "SIZE" and the size # of the device, in bytes. blksize_cmd = "lsblk -b /dev/vdb --output SIZE" @@ -284,7 +294,7 @@ def test_patch_drive(test_microvm_with_api): assert lines[1].strip() == size_bytes_str -def test_no_flush(test_microvm_with_api): +def test_no_flush(test_microvm_with_api, io_engine): """ Verify default block ignores flush. """ @@ -299,6 +309,7 @@ def test_no_flush(test_microvm_with_api): "rootfs", test_microvm.rootfs_file, is_root_device=True, + io_engine=io_engine, ) test_microvm.start() @@ -317,7 +328,7 @@ def test_no_flush(test_microvm_with_api): assert fc_metrics["block"]["flush_count"] == 0 -def test_flush(uvm_plain_rw): +def test_flush(uvm_plain_rw, io_engine): """ Verify block with flush actually flushes. """ @@ -332,6 +343,7 @@ def test_flush(uvm_plain_rw): test_microvm.rootfs_file, is_root_device=True, cache_type="Writeback", + io_engine=io_engine, ) test_microvm.start() @@ -371,3 +383,10 @@ def _check_drives(test_microvm, assert_dict, keys_array): _, stdout, stderr = test_microvm.ssh.run("blockdev --report") assert stderr == "" _process_blockdev_output(stdout, assert_dict, keys_array) + + +def _check_mount(ssh_connection, dev_path): + _, _, stderr = ssh_connection.run(f"mount {dev_path} /tmp", timeout=30.0) + assert stderr == "" + _, _, stderr = ssh_connection.run("umount /tmp", timeout=30.0) + assert stderr == "" From 29a9aec19f1be243da9747bc054fd5263ef16efb Mon Sep 17 00:00:00 2001 From: Babis Chalios Date: Fri, 1 Dec 2023 10:21:05 +0000 Subject: [PATCH 2/3] fix(test): don't try to kill after CtrlAltDel In test test_send_ctrl_alt_del we send a CTRL+ALT+DEL to the microVM, which, in x86, makes the microVM to shutdown. Then we send a signal to the Firecracker process with `os.kill(firecracker_pid, 0)` and wait for it to fail. This works but logs an error in the test logs which can be confusing. Instead we can call os.waitpid() which waits for the Firecracker process to exit and returns immediately if the process has already exited. Signed-off-by: Babis Chalios --- .../integration_tests/functional/test_api.py | 20 +++++++------------ 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/tests/integration_tests/functional/test_api.py b/tests/integration_tests/functional/test_api.py index 3bfb27ba3ea..b4edc1e7223 100644 --- a/tests/integration_tests/functional/test_api.py +++ b/tests/integration_tests/functional/test_api.py @@ -795,19 +795,13 @@ def test_send_ctrl_alt_del(test_microvm_with_api): # If everything goes as expected, the guest OS will issue a reboot, # causing Firecracker to exit. - # We'll keep poking Firecracker for at most 30 seconds, waiting for it - # to die. - start_time = time.time() - shutdown_ok = False - while time.time() - start_time < 30: - try: - os.kill(firecracker_pid, 0) - time.sleep(0.01) - except OSError: - shutdown_ok = True - break - - assert shutdown_ok + # waitpid should block until the Firecracker process has exited. If + # it has already exited by the time we call waitpid, WNOHANG causes + # waitpid to raise a ChildProcessError exception. + try: + os.waitpid(firecracker_pid, os.WNOHANG) + except ChildProcessError: + pass def _drive_patch(test_microvm): From 0e290589e56d6b6e143e01f518c7fc313b0a6bbd Mon Sep 17 00:00:00 2001 From: Babis Chalios Date: Wed, 29 Nov 2023 17:16:44 +0000 Subject: [PATCH 3/3] fix: patching the path of Async block devices The asynchronous engine maintains an event file descriptor which passes to the IO uring interface when creating a new ring. IO uring uses this EventFd to notify us about completion of IO requests. When we PATCH an async block device, we create a new asynchronous engine, including a new EventFd. However, we still monitor the old EventFd. This breaks the use of async drives post PATCH requests, because we never get notified about the results of requests we submit to the IO uring engine. This commit changes the implementation along the PATCH code path, to reuse the previous EventFd for the asynchronous engine. Signed-off-by: Babis Chalios --- .../src/devices/virtio/virtio_block/device.rs | 51 +++++++++++++++---- .../virtio/virtio_block/io/async_io.rs | 32 ++++++++---- .../src/devices/virtio/virtio_block/io/mod.rs | 9 ++++ .../devices/virtio/virtio_block/io/sync_io.rs | 5 ++ 4 files changed, 77 insertions(+), 20 deletions(-) diff --git a/src/vmm/src/devices/virtio/virtio_block/device.rs b/src/vmm/src/devices/virtio/virtio_block/device.rs index 7a0ad7c9a42..81d3a54b661 100644 --- a/src/vmm/src/devices/virtio/virtio_block/device.rs +++ b/src/vmm/src/devices/virtio/virtio_block/device.rs @@ -71,19 +71,20 @@ pub struct DiskProperties { } impl DiskProperties { - pub fn new( - disk_image_path: String, - is_disk_read_only: bool, - file_engine_type: FileEngineType, - ) -> Result { - let mut disk_image = OpenOptions::new() + // Helper function that opens the file with the proper access permissions + fn open_file(disk_image_path: &str, is_disk_read_only: bool) -> Result { + OpenOptions::new() .read(true) .write(!is_disk_read_only) .open(PathBuf::from(&disk_image_path)) - .map_err(|x| VirtioBlockError::BackingFile(x, disk_image_path.clone()))?; + .map_err(|x| VirtioBlockError::BackingFile(x, disk_image_path.to_string())) + } + + // Helper function that gets the size of the file + fn file_size(disk_image_path: &str, disk_image: &mut File) -> Result { let disk_size = disk_image .seek(SeekFrom::End(0)) - .map_err(|x| VirtioBlockError::BackingFile(x, disk_image_path.clone()))?; + .map_err(|x| VirtioBlockError::BackingFile(x, disk_image_path.to_string()))?; // We only support disk size, which uses the first two words of the configuration space. // If the image is not a multiple of the sector size, the tail bits are not exposed. @@ -95,6 +96,17 @@ impl DiskProperties { ); } + Ok(disk_size) + } + + /// Create a new file for the block device using a FileEngine + pub fn new( + disk_image_path: String, + is_disk_read_only: bool, + file_engine_type: FileEngineType, + ) -> Result { + let mut disk_image = Self::open_file(&disk_image_path, is_disk_read_only)?; + let disk_size = Self::file_size(&disk_image_path, &mut disk_image)?; let image_id = Self::build_disk_image_id(&disk_image); Ok(Self { @@ -106,6 +118,25 @@ impl DiskProperties { }) } + /// Update the path to the file backing the block device + pub fn update( + &mut self, + disk_image_path: String, + is_disk_read_only: bool, + ) -> Result<(), VirtioBlockError> { + let mut disk_image = Self::open_file(&disk_image_path, is_disk_read_only)?; + let disk_size = Self::file_size(&disk_image_path, &mut disk_image)?; + + self.image_id = Self::build_disk_image_id(&disk_image); + self.file_engine + .update_file_path(disk_image) + .map_err(VirtioBlockError::FileEngine)?; + self.nsectors = disk_size >> SECTOR_SHIFT; + self.file_path = disk_image_path; + + Ok(()) + } + fn build_device_id(disk_file: &File) -> Result { let blk_metadata = disk_file .metadata() @@ -506,9 +537,7 @@ impl VirtioBlock { /// Update the backing file and the config space of the block device. pub fn update_disk_image(&mut self, disk_image_path: String) -> Result<(), VirtioBlockError> { - let disk_properties = - DiskProperties::new(disk_image_path, self.read_only, self.file_engine_type())?; - self.disk = disk_properties; + self.disk.update(disk_image_path, self.read_only)?; self.config_space = self.disk.virtio_block_config_space(); // Kick the driver to pick up the changes. diff --git a/src/vmm/src/devices/virtio/virtio_block/io/async_io.rs b/src/vmm/src/devices/virtio/virtio_block/io/async_io.rs index 2ac4a035e6b..b28460b24b7 100644 --- a/src/vmm/src/devices/virtio/virtio_block/io/async_io.rs +++ b/src/vmm/src/devices/virtio/virtio_block/io/async_io.rs @@ -4,6 +4,7 @@ use std::fmt::Debug; use std::fs::File; use std::marker::PhantomData; +use std::os::fd::RawFd; use std::os::unix::io::AsRawFd; use utils::eventfd::EventFd; @@ -13,7 +14,7 @@ use crate::devices::virtio::virtio_block::io::UserDataError; use crate::devices::virtio::virtio_block::IO_URING_NUM_ENTRIES; use crate::io_uring::operation::{Cqe, OpCode, Operation}; use crate::io_uring::restriction::Restriction; -use crate::io_uring::{IoUring, IoUringError}; +use crate::io_uring::{self, IoUring, IoUringError}; use crate::logger::log_dev_preview_warning; use crate::vstate::memory::{GuestAddress, GuestMemory, GuestMemoryExtension, GuestMemoryMmap}; @@ -66,13 +67,10 @@ impl WrappedUserData { } impl AsyncFileEngine { - pub fn from_file(file: File) -> Result, AsyncIoError> { - log_dev_preview_warning("Async file IO", Option::None); - - let completion_evt = EventFd::new(libc::EFD_NONBLOCK).map_err(AsyncIoError::EventFd)?; - let ring = IoUring::new( + fn new_ring(file: &File, completion_fd: RawFd) -> Result { + IoUring::new( u32::from(IO_URING_NUM_ENTRIES), - vec![&file], + vec![file], vec![ // Make sure we only allow operations on pre-registered fds. Restriction::RequireFixedFds, @@ -81,9 +79,16 @@ impl AsyncFileEngine { Restriction::AllowOpCode(OpCode::Write), Restriction::AllowOpCode(OpCode::Fsync), ], - Some(completion_evt.as_raw_fd()), + Some(completion_fd), ) - .map_err(AsyncIoError::IoUring)?; + } + + pub fn from_file(file: File) -> Result, AsyncIoError> { + log_dev_preview_warning("Async file IO", Option::None); + + let completion_evt = EventFd::new(libc::EFD_NONBLOCK).map_err(AsyncIoError::EventFd)?; + let ring = + Self::new_ring(&file, completion_evt.as_raw_fd()).map_err(AsyncIoError::IoUring)?; Ok(AsyncFileEngine { file, @@ -93,6 +98,15 @@ impl AsyncFileEngine { }) } + pub fn update_file(&mut self, file: File) -> Result<(), AsyncIoError> { + let ring = Self::new_ring(&file, self.completion_evt.as_raw_fd()) + .map_err(AsyncIoError::IoUring)?; + + self.file = file; + self.ring = ring; + Ok(()) + } + #[cfg(test)] pub fn file(&self) -> &File { &self.file diff --git a/src/vmm/src/devices/virtio/virtio_block/io/mod.rs b/src/vmm/src/devices/virtio/virtio_block/io/mod.rs index f174aece498..b87889d9cdf 100644 --- a/src/vmm/src/devices/virtio/virtio_block/io/mod.rs +++ b/src/vmm/src/devices/virtio/virtio_block/io/mod.rs @@ -74,6 +74,15 @@ impl FileEngine { } } + pub fn update_file_path(&mut self, file: File) -> Result<(), BlockIoError> { + match self { + FileEngine::Async(engine) => engine.update_file(file).map_err(BlockIoError::Async)?, + FileEngine::Sync(engine) => engine.update_file(file), + }; + + Ok(()) + } + #[cfg(test)] pub fn file(&self) -> &File { match self { diff --git a/src/vmm/src/devices/virtio/virtio_block/io/sync_io.rs b/src/vmm/src/devices/virtio/virtio_block/io/sync_io.rs index 9c2001c9e93..5ab90d700ec 100644 --- a/src/vmm/src/devices/virtio/virtio_block/io/sync_io.rs +++ b/src/vmm/src/devices/virtio/virtio_block/io/sync_io.rs @@ -34,6 +34,11 @@ impl SyncFileEngine { &self.file } + /// Update the backing file of the engine + pub fn update_file(&mut self, file: File) { + self.file = file + } + pub fn read( &mut self, offset: u64,