From b61cd1e9f5e539f6d8887903b52d2af964ca169e Mon Sep 17 00:00:00 2001 From: Alexandra Ghecenco Date: Tue, 15 May 2018 16:45:42 +0300 Subject: [PATCH 1/4] vmm: split the run_api_cmd function Split run_api_cmd into smaller functions handling each match branch to improve readability and avoid duplicate code. Signed-off-by: Alexandra Ghecenco --- vmm/src/lib.rs | 307 +++++++++++++++++++++++++++---------------------- 1 file changed, 170 insertions(+), 137 deletions(-) diff --git a/vmm/src/lib.rs b/vmm/src/lib.rs index eb0ac117e17..1b31ea72517 100644 --- a/vmm/src/lib.rs +++ b/vmm/src/lib.rs @@ -31,14 +31,15 @@ use std::sync::{Arc, Barrier, RwLock}; use std::thread; use api_server::ApiRequest; -use api_server::request::async::{AsyncOutcome, AsyncRequest}; +use api_server::request::async::{AsyncOutcome, AsyncOutcomeSender, AsyncRequest}; use api_server::request::instance_info::{InstanceInfo, InstanceState}; use api_server::request::sync::boot_source::{PutBootSourceConfigError, PutBootSourceOutcome}; use api_server::request::sync::machine_configuration::{PutMachineConfigurationError, PutMachineConfigurationOutcome}; -use api_server::request::sync::{DriveError, Error as SyncError, GenerateResponse, +use api_server::request::sync::{APILoggerDescription, BootSourceBody, DriveDescription, + DriveError, Error as SyncError, GenerateResponse, NetworkInterfaceBody, OkStatus as SyncOkStatus, PutDriveOutcome, - PutLoggerOutcome, SyncRequest}; + PutLoggerOutcome, SyncOutcomeSender, SyncRequest}; use data_model::vm::MachineConfiguration; use device_config::*; use device_manager::legacy::LegacyDeviceManager; @@ -863,6 +864,149 @@ impl Vmm { Ok(()) } + fn handle_start_instance(&mut self, sender: AsyncOutcomeSender) { + let instance_state = { + // unwrap() to crash if the other thread poisoned this lock + let shared_info = self.shared_info.read().unwrap(); + shared_info.state.clone() + }; + let result = match instance_state { + InstanceState::Starting | InstanceState::Running | InstanceState::Halting => { + AsyncOutcome::Error("Guest Instance already running.".to_string()) + } + _ => match self.start_instance() { + Ok(_) => AsyncOutcome::Ok(0), + Err(e) => { + let _ = self.stop(); + AsyncOutcome::Error(format!("cannot boot kernel: {:?}", e)) + } + }, + }; + // doing expect() to crash this thread as well if the other thread crashed + sender.send(result).expect("one-shot channel closed"); + } + + fn handle_stop_instance(&mut self, sender: AsyncOutcomeSender) { + let result = match self.stop() { + Ok(_) => AsyncOutcome::Ok(0), + Err(e) => AsyncOutcome::Error(format!( + "Errors detected during instance stop()! err: {:?}", + e + )), + }; + sender.send(result).expect("one-shot channel closed"); + } + + fn handle_put_drive(&mut self, drive_description: DriveDescription, sender: SyncOutcomeSender) { + match self.put_block_device(BlockDeviceConfig::from(drive_description)) { + Ok(outcome) => sender + .send(Box::new(outcome)) + .map_err(|_| ()) + .expect("one-shot channel closed"), + Err(e) => sender + .send(Box::new(e)) + .map_err(|_| ()) + .expect("one-shot channel closed"), + } + } + + fn handle_put_logger( + &mut self, + logger_description: APILoggerDescription, + sender: SyncOutcomeSender, + ) { + match api_logger_config::init_logger(logger_description) { + Ok(_) => sender + .send(Box::new(PutLoggerOutcome::Initialized)) + .map_err(|_| ()) + .expect("one-shot channel closed"), + Err(e) => sender + .send(Box::new(e)) + .map_err(|_| ()) + .expect("one-shot channel closed"), + } + } + + fn handle_put_boot_source( + &mut self, + boot_source_body: BootSourceBody, + sender: SyncOutcomeSender, + ) { + // check that the kernel path exists and it is valid + let box_response: Box = match boot_source_body.local_image { + Some(image) => match File::open(image.kernel_image_path) { + Ok(kernel_file) => { + let mut cmdline = kernel_cmdline::Cmdline::new(CMDLINE_MAX_SIZE); + match cmdline.insert_str( + boot_source_body + .boot_args + .unwrap_or(String::from(DEFAULT_KERNEL_CMDLINE)), + ) { + Ok(_) => { + let kernel_config = KernelConfig { + kernel_file, + cmdline, + kernel_start_addr: GuestAddress(KERNEL_START_OFFSET), + cmdline_addr: GuestAddress(CMDLINE_OFFSET), + }; + // if the kernel was already configure, we have an update operation + let outcome = match self.kernel_config { + Some(_) => PutBootSourceOutcome::Updated, + None => PutBootSourceOutcome::Created, + }; + self.configure_kernel(kernel_config); + Box::new(outcome) + } + Err(_) => Box::new(PutBootSourceConfigError::InvalidKernelCommandLine), + } + } + Err(_e) => Box::new(PutBootSourceConfigError::InvalidKernelPath), + }, + None => Box::new(PutBootSourceConfigError::InvalidKernelPath), + }; + sender + .send(box_response) + .map_err(|_| ()) + .expect("one-shot channel closed"); + } + + fn handle_get_machine_configuration(&self, sender: SyncOutcomeSender) { + sender + .send(Box::new(self.vm_config.clone())) + .map_err(|_| ()) + .expect("one-shot channel closed"); + } + + fn handle_put_machine_configuration( + &mut self, + machine_config: MachineConfiguration, + sender: SyncOutcomeSender, + ) { + let boxed_response = match self.put_virtual_machine_configuration( + machine_config.vcpu_count, + machine_config.mem_size_mib, + ) { + Ok(_) => Box::new(PutMachineConfigurationOutcome::Updated), + Err(e) => Box::new(PutMachineConfigurationOutcome::Error(e)), + }; + + sender + .send(boxed_response) + .map_err(|_| ()) + .expect("one-shot channel closed"); + } + + fn handle_put_network_interface( + &mut self, + netif_body: NetworkInterfaceBody, + sender: SyncOutcomeSender, + ) { + sender + .send(Box::new(self.put_net_device(netif_body))) + .map_err(|_| ()) + .expect("one-shot channel closed"); + } + fn run_api_cmd(&mut self) -> Result<()> { let request = match self.from_api.try_recv() { Ok(t) => t, @@ -874,141 +1018,30 @@ impl Vmm { } }; match *request { - ApiRequest::Async(req) => { - match req { - AsyncRequest::StartInstance(sender) => { - let instance_state = { - // unwrap() to crash if the other thread poisoned this lock - let shared_info = self.shared_info.read().unwrap(); - shared_info.state.clone() - }; - let result = match instance_state { - InstanceState::Starting - | InstanceState::Running - | InstanceState::Halting => { - AsyncOutcome::Error("Guest Instance already running.".to_string()) - } - _ => match self.start_instance() { - Ok(_) => AsyncOutcome::Ok(0), - Err(e) => { - let _ = self.stop(); - AsyncOutcome::Error(format!("cannot boot kernel: {:?}", e)) - } - }, - }; - // doing expect() to crash this thread as well if the other thread crashed - sender.send(result).expect("one-shot channel closed"); - } - AsyncRequest::StopInstance(sender) => { - let result = match self.stop() { - Ok(_) => AsyncOutcome::Ok(0), - Err(e) => AsyncOutcome::Error(format!( - "Errors detected during instance stop()! err: {:?}", - e - )), - }; - // doing expect() to crash this thread as well if the other thread crashed - sender.send(result).expect("one-shot channel closed"); - } - }; - } - ApiRequest::Sync(req) => { - match req { - SyncRequest::GetMachineConfiguration(sender) => { - sender - .send(Box::new(self.vm_config.clone())) - .map_err(|_| ()) - .expect("one-shot channel closed"); - } - SyncRequest::PutBootSource(boot_source_body, sender) => { - // check that the kernel path exists and it is valid - let box_response: Box = match boot_source_body - .local_image - { - Some(image) => match File::open(image.kernel_image_path) { - Ok(kernel_file) => { - let mut cmdline = - kernel_cmdline::Cmdline::new(CMDLINE_MAX_SIZE); - match cmdline.insert_str( - boot_source_body - .boot_args - .unwrap_or(String::from(DEFAULT_KERNEL_CMDLINE)), - ) { - Ok(_) => { - let kernel_config = KernelConfig { - kernel_file, - cmdline, - kernel_start_addr: GuestAddress( - KERNEL_START_OFFSET, - ), - cmdline_addr: GuestAddress(CMDLINE_OFFSET), - }; - // if the kernel was already configure, we have an update operation - let outcome = match self.kernel_config { - Some(_) => PutBootSourceOutcome::Updated, - None => PutBootSourceOutcome::Created, - }; - self.configure_kernel(kernel_config); - Box::new(outcome) - } - Err(_) => Box::new( - PutBootSourceConfigError::InvalidKernelCommandLine, - ), - } - } - Err(_e) => Box::new(PutBootSourceConfigError::InvalidKernelPath), - }, - None => Box::new(PutBootSourceConfigError::InvalidKernelPath), - }; - sender - .send(box_response) - .map_err(|_| ()) - .expect("one-shot channel closed"); - } - SyncRequest::PutDrive(drive_description, sender) => { - match self.put_block_device(BlockDeviceConfig::from(drive_description)) { - Ok(outcome) => sender - .send(Box::new(outcome)) - .map_err(|_| ()) - .expect("one-shot channel closed"), - Err(e) => sender - .send(Box::new(e)) - .map_err(|_| ()) - .expect("one-shot channel closed"), - } - } - SyncRequest::PutLogger(logger_description, sender) => { - match api_logger_config::init_logger(logger_description) { - Ok(_) => sender - .send(Box::new(PutLoggerOutcome::Initialized)) - .map_err(|_| ()) - .expect("one-shot channel closed"), - Err(e) => sender - .send(Box::new(e)) - .map_err(|_| ()) - .expect("one-shot channel closed"), - } - } - SyncRequest::PutMachineConfiguration(machine_config_body, sender) => { - let boxed_response = match self.put_virtual_machine_configuration( - machine_config_body.vcpu_count, - machine_config_body.mem_size_mib, - ) { - Ok(_) => Box::new(PutMachineConfigurationOutcome::Updated), - Err(e) => Box::new(PutMachineConfigurationOutcome::Error(e)), - }; - - sender - .send(boxed_response) - .map_err(|_| ()) - .expect("one-shot channel closed");; - } - SyncRequest::PutNetworkInterface(body, outcome_sender) => outcome_sender - .send(Box::new(self.put_net_device(body))) - .map_err(|_| ()) - .expect("one-shot channel closed"), + ApiRequest::Async(req) => match req { + AsyncRequest::StartInstance(sender) => self.handle_start_instance(sender), + AsyncRequest::StopInstance(sender) => self.handle_stop_instance(sender), + }, + ApiRequest::Sync(req) => match req { + SyncRequest::GetMachineConfiguration(sender) => { + self.handle_get_machine_configuration(sender) } - } + SyncRequest::PutBootSource(boot_source_body, sender) => { + self.handle_put_boot_source(boot_source_body, sender) + } + SyncRequest::PutDrive(drive_description, sender) => { + self.handle_put_drive(drive_description, sender) + } + SyncRequest::PutLogger(logger_description, sender) => { + self.handle_put_logger(logger_description, sender) + } + SyncRequest::PutMachineConfiguration(machine_config_body, sender) => { + self.handle_put_machine_configuration(machine_config_body, sender) + } + SyncRequest::PutNetworkInterface(netif_body, sender) => { + self.handle_put_network_interface(netif_body, sender) + } + }, } Ok(()) From 5b20b7d7a69d116a17878429274d6c8e6e664ce9 Mon Sep 17 00:00:00 2001 From: Alexandra Ghecenco Date: Tue, 15 May 2018 16:47:06 +0300 Subject: [PATCH 2/4] api: forbid PUT operations after guest boot vmm returns 403 Forbidden to PUT operations issued after the guest has booted, except for block devices where live resize is supported. Signed-off-by: Alexandra Ghecenco --- api_server/src/request/sync/mod.rs | 5 +++++ vmm/src/lib.rs | 36 ++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/api_server/src/request/sync/mod.rs b/api_server/src/request/sync/mod.rs index 9fc9e54e6a8..b71d4270219 100644 --- a/api_server/src/request/sync/mod.rs +++ b/api_server/src/request/sync/mod.rs @@ -90,6 +90,7 @@ pub enum Error { GuestCIDAlreadyInUse, GuestMacAddressInUse, OpenTap(TapError), + UpdateNotAllowed, UpdateNotImplemented, } @@ -109,6 +110,10 @@ impl GenerateResponse for Error { StatusCode::BadRequest, json_fault_message("Could not open TAP device."), ), + UpdateNotAllowed => json_response( + StatusCode::Forbidden, + json_fault_message("The update operation is not allowed"), + ), UpdateNotImplemented => json_response( StatusCode::InternalServerError, json_fault_message("Update operation is not implemented yet."), diff --git a/vmm/src/lib.rs b/vmm/src/lib.rs index 1b31ea72517..53580f3c4a7 100644 --- a/vmm/src/lib.rs +++ b/vmm/src/lib.rs @@ -897,6 +897,18 @@ impl Vmm { sender.send(result).expect("one-shot channel closed"); } + fn check_instance_running(&self) -> bool { + let instance_state = { + // unwrap() to crash if the other thread poisoned this lock + let shared_info = self.shared_info.read().unwrap(); + shared_info.state.clone() + }; + match instance_state { + InstanceState::Uninitialized => false, + _ => true, + } + } + fn handle_put_drive(&mut self, drive_description: DriveDescription, sender: SyncOutcomeSender) { match self.put_block_device(BlockDeviceConfig::from(drive_description)) { Ok(outcome) => sender @@ -932,6 +944,14 @@ impl Vmm { boot_source_body: BootSourceBody, sender: SyncOutcomeSender, ) { + if self.check_instance_running() { + sender + .send(Box::new(SyncError::UpdateNotAllowed)) + .map_err(|_| ()) + .expect("one-shot channel closed"); + return; + } + // check that the kernel path exists and it is valid let box_response: Box = match boot_source_body.local_image { Some(image) => match File::open(image.kernel_image_path) { @@ -982,6 +1002,14 @@ impl Vmm { machine_config: MachineConfiguration, sender: SyncOutcomeSender, ) { + if self.check_instance_running() { + sender + .send(Box::new(SyncError::UpdateNotAllowed)) + .map_err(|_| ()) + .expect("one-shot channel closed"); + return; + } + let boxed_response = match self.put_virtual_machine_configuration( machine_config.vcpu_count, machine_config.mem_size_mib, @@ -1001,6 +1029,14 @@ impl Vmm { netif_body: NetworkInterfaceBody, sender: SyncOutcomeSender, ) { + if self.check_instance_running() { + sender + .send(Box::new(SyncError::UpdateNotAllowed)) + .map_err(|_| ()) + .expect("one-shot channel closed"); + return; + } + sender .send(Box::new(self.put_net_device(netif_body))) .map_err(|_| ()) From 675d6384a79ad495397c19e966f740159525ca14 Mon Sep 17 00:00:00 2001 From: Alexandra Ghecenco Date: Mon, 7 May 2018 20:23:40 +0300 Subject: [PATCH 3/4] api: add support for update operations pre-boot * implemented update with PUT for /boot-source, /drives, /network-interfaces and /vsock * added unit tests in the modified files to improve coverage * the updates are meant to be *full* updates, not partial ones; partial updates will be implemented as PATCH operations in the future * block device live resize is still a PUT; it will become a PUT /drives/operations in the future Signed-off-by: Alexandra Ghecenco --- api_server/src/request/sync/mod.rs | 11 +- vmm/src/device_config/drive.rs | 206 ++++++++++++++++++++++------- vmm/src/device_config/net.rs | 48 ++++++- vmm/src/lib.rs | 49 ++++--- 4 files changed, 245 insertions(+), 69 deletions(-) diff --git a/api_server/src/request/sync/mod.rs b/api_server/src/request/sync/mod.rs index b71d4270219..2f232a31805 100644 --- a/api_server/src/request/sync/mod.rs +++ b/api_server/src/request/sync/mod.rs @@ -73,6 +73,7 @@ impl fmt::Debug for SyncRequest { pub enum OkStatus { Created, + Updated, } impl GenerateResponse for OkStatus { @@ -80,6 +81,7 @@ impl GenerateResponse for OkStatus { use self::OkStatus::*; match *self { Created => empty_response(StatusCode::Created), + Updated => empty_response(StatusCode::NoContent), } } } @@ -90,7 +92,7 @@ pub enum Error { GuestCIDAlreadyInUse, GuestMacAddressInUse, OpenTap(TapError), - UpdateNotAllowed, + UpdateNotAllowedPostBoot, UpdateNotImplemented, } @@ -110,7 +112,7 @@ impl GenerateResponse for Error { StatusCode::BadRequest, json_fault_message("Could not open TAP device."), ), - UpdateNotAllowed => json_response( + UpdateNotAllowedPostBoot => json_response( StatusCode::Forbidden, json_fault_message("The update operation is not allowed"), ), @@ -161,8 +163,11 @@ mod tests { #[test] fn test_generate_response_okstatus() { - let ret = OkStatus::Created.generate_response(); + let mut ret = OkStatus::Created.generate_response(); assert_eq!(ret.status(), StatusCode::Created); + + ret = OkStatus::Updated.generate_response(); + assert_eq!(ret.status(), StatusCode::NoContent); } #[test] diff --git a/vmm/src/device_config/drive.rs b/vmm/src/device_config/drive.rs index 1f6b5dc1c60..8728a420983 100644 --- a/vmm/src/device_config/drive.rs +++ b/vmm/src/device_config/drive.rs @@ -99,11 +99,62 @@ impl BlockDeviceConfigs { Ok(()) } + + fn get_root_id(&self) -> Option { + if !self.has_root_block { + return None; + } else { + for cfg in self.config_list.iter() { + if cfg.is_root_device { + return Some(cfg.drive_id.clone()); + } + } + } + None + } + + /// This function updates a Block Device Config prior to the guest boot. The update fails if it + /// would result in two root block devices. + pub fn update(&mut self, block_device_config: &BlockDeviceConfig) -> Result<()> { + // Check if the path exists + if !block_device_config.path_on_host.exists() { + return Err(DriveError::InvalidBlockDevicePath); + } + + let root_id = self.get_root_id(); + for cfg in self.config_list.iter_mut() { + if cfg.drive_id == block_device_config.drive_id { + if cfg.is_root_device { + // Check if the root block device is being updated + self.has_root_block = block_device_config.is_root_device; + self.read_only_root = + block_device_config.is_root_device && block_device_config.is_read_only; + } else if block_device_config.is_root_device { + // Check if a second root block device is being added + if root_id.is_some() { + return Err(DriveError::RootBlockDeviceAlreadyAdded); + } else { + // One of the non-root blocks is becoming root + self.has_root_block = true; + self.read_only_root = block_device_config.is_read_only; + } + } + cfg.is_root_device = block_device_config.is_root_device; + cfg.path_on_host = block_device_config.path_on_host.clone(); + cfg.is_read_only = block_device_config.is_read_only; + + return Ok(()); + } + } + + Err(DriveError::BlockDeviceUpdateFailed) + } } #[cfg(test)] mod tests { use super::*; + use api_server::request::sync::{DeviceState, DrivePermissions}; use std::fs::{self, File}; // Helper function for creating a dummy file @@ -129,26 +180,29 @@ mod tests { fn test_add_non_root_block_device() { let dummy_filename = String::from("test_add_non_root_block_device"); let dummy_path = create_dummy_path(dummy_filename.clone()); + let dummy_id = String::from("1"); let dummy_block_device = BlockDeviceConfig { - path_on_host: dummy_path, + path_on_host: dummy_path.clone(), is_root_device: false, is_read_only: false, - drive_id: String::from("1"), + drive_id: dummy_id.clone(), }; let mut block_devices_configs = BlockDeviceConfigs::new(); - assert!( - block_devices_configs - .add(dummy_block_device.clone()) - .is_ok() - ); + let ret = block_devices_configs.add(dummy_block_device.clone()); + // clean up before the assert!s to make sure the temp files are deleted + delete_dummy_path(dummy_filename); + assert!(ret.is_ok()); + assert_eq!(block_devices_configs.has_root_block, false); assert_eq!(block_devices_configs.config_list.len(), 1); let dev_config = block_devices_configs.config_list.iter().next().unwrap(); assert_eq!(dev_config, &dummy_block_device); - - delete_dummy_path(dummy_filename); + assert!(block_devices_configs.contains_drive_path(dummy_path)); + assert!(!block_devices_configs.contains_drive_path(PathBuf::from("/foo/bar"))); + assert!(block_devices_configs.contains_drive_id(dummy_id.clone())); + assert!(!block_devices_configs.contains_drive_id(String::from("foo"))); } #[test] @@ -159,21 +213,19 @@ mod tests { let dummy_block_device = BlockDeviceConfig { path_on_host: dummy_path, is_root_device: true, - is_read_only: false, + is_read_only: true, drive_id: String::from("1"), }; let mut block_devices_configs = BlockDeviceConfigs::new(); - assert!( - block_devices_configs - .add(dummy_block_device.clone()) - .is_ok() - ); + let ret = block_devices_configs.add(dummy_block_device.clone()); + delete_dummy_path(dummy_filename); + assert!(ret.is_ok()); + assert_eq!(block_devices_configs.has_root_block, true); assert_eq!(block_devices_configs.config_list.len(), 1); let dev_config = block_devices_configs.config_list.iter().next().unwrap(); assert_eq!(dev_config, &dummy_block_device); - - delete_dummy_path(dummy_filename); + assert_eq!(block_devices_configs.has_read_only_root(), true); } #[test] @@ -197,16 +249,18 @@ mod tests { }; let mut block_devices_configs = BlockDeviceConfigs::new(); - assert!(block_devices_configs.add(root_block_device_1).is_ok()); + let ret = block_devices_configs.add(root_block_device_1); let actual_error = format!( "{:?}", block_devices_configs.add(root_block_device_2).unwrap_err() ); let expected_error = format!("{:?}", DriveError::RootBlockDeviceAlreadyAdded); - assert_eq!(expected_error, actual_error); delete_dummy_path(dummy_filename_1); delete_dummy_path(dummy_filename_2); + + assert!(ret.is_ok()); + assert_eq!(expected_error, actual_error); } #[test] @@ -240,17 +294,17 @@ mod tests { }; let mut block_devices_configs = BlockDeviceConfigs::new(); - assert!(block_devices_configs.add(root_block_device.clone()).is_ok()); - assert!( - block_devices_configs - .add(dummy_block_device_2.clone()) - .is_ok() - ); - assert!( - block_devices_configs - .add(dummy_block_device_3.clone()) - .is_ok() - ); + let ret1 = block_devices_configs.add(root_block_device.clone()); + let ret2 = block_devices_configs.add(dummy_block_device_2.clone()); + let ret3 = block_devices_configs.add(dummy_block_device_3.clone()); + + delete_dummy_path(dummy_filename_1); + delete_dummy_path(dummy_filename_2); + delete_dummy_path(dummy_filename_3); + + assert!(ret1.is_ok()); + assert!(ret2.is_ok()); + assert!(ret3.is_ok()); assert_eq!(block_devices_configs.has_root_block_device(), true); assert_eq!(block_devices_configs.config_list.len(), 3); @@ -259,10 +313,6 @@ mod tests { assert_eq!(block_dev_iter.next().unwrap(), &root_block_device); assert_eq!(block_dev_iter.next().unwrap(), &dummy_block_device_2); assert_eq!(block_dev_iter.next().unwrap(), &dummy_block_device_3); - - delete_dummy_path(dummy_filename_1); - delete_dummy_path(dummy_filename_2); - delete_dummy_path(dummy_filename_3); } #[test] @@ -296,17 +346,17 @@ mod tests { }; let mut block_devices_configs = BlockDeviceConfigs::new(); - assert!( - block_devices_configs - .add(dummy_block_device_2.clone()) - .is_ok() - ); - assert!( - block_devices_configs - .add(dummy_block_device_3.clone()) - .is_ok() - ); - assert!(block_devices_configs.add(root_block_device.clone()).is_ok()); + let ret2 = block_devices_configs.add(dummy_block_device_2.clone()); + let ret3 = block_devices_configs.add(dummy_block_device_3.clone()); + let ret1 = block_devices_configs.add(root_block_device.clone()); + + delete_dummy_path(dummy_filename_1); + delete_dummy_path(dummy_filename_2); + delete_dummy_path(dummy_filename_3); + + assert!(ret2.is_ok()); + assert!(ret3.is_ok()); + assert!(ret1.is_ok()); assert_eq!(block_devices_configs.has_root_block_device(), true); assert_eq!(block_devices_configs.config_list.len(), 3); @@ -316,9 +366,73 @@ mod tests { assert_eq!(block_dev_iter.next().unwrap(), &root_block_device); assert_eq!(block_dev_iter.next().unwrap(), &dummy_block_device_2); assert_eq!(block_dev_iter.next().unwrap(), &dummy_block_device_3); + } + + #[test] + fn test_from_drive_description() { + let dd = DriveDescription { + is_root_device: true, + path_on_host: String::from("/foo/bar"), + drive_id: String::from("foo"), + state: DeviceState::Attached, + permissions: DrivePermissions::ro, + }; + + let drive = BlockDeviceConfig::from(dd); + assert_eq!(drive.drive_id, String::from("foo")); + assert_eq!(drive.path_on_host, PathBuf::from(String::from("/foo/bar"))); + assert_eq!(drive.is_root_device, true); + assert_eq!(drive.is_read_only, true); + } + + #[test] + fn test_update() { + let dummy_filename_1 = String::from("test_update_1"); + let dummy_path_1 = create_dummy_path(dummy_filename_1.clone()); + let root_block_device = BlockDeviceConfig { + path_on_host: dummy_path_1, + is_root_device: true, + is_read_only: false, + drive_id: String::from("1"), + }; + + let dummy_filename_2 = String::from("test_update_2"); + let dummy_path_2 = create_dummy_path(dummy_filename_2.clone()); + let mut dummy_block_device_2 = BlockDeviceConfig { + path_on_host: dummy_path_2.clone(), + is_root_device: false, + is_read_only: false, + drive_id: String::from("2"), + }; + + let mut block_devices_configs = BlockDeviceConfigs::new(); + + // Add 2 block devices + let ret1 = block_devices_configs.add(root_block_device.clone()); + let ret2 = block_devices_configs.add(dummy_block_device_2.clone()); + + // Update OK + dummy_block_device_2.is_read_only = true; + let ret_update_ok = block_devices_configs.update(&dummy_block_device_2); + + // Update with invalid path + let dummy_filename_3 = String::from("test_update_3"); + let dummy_path_3 = PathBuf::from(dummy_filename_3.clone()); + dummy_block_device_2.path_on_host = dummy_path_3; + let ret_inv_path = block_devices_configs.update(&dummy_block_device_2); + + // Update with 2 root block devices + dummy_block_device_2.path_on_host = dummy_path_2; + dummy_block_device_2.is_root_device = true; + let ret_2roots = block_devices_configs.update(&dummy_block_device_2); delete_dummy_path(dummy_filename_1); delete_dummy_path(dummy_filename_2); - delete_dummy_path(dummy_filename_3); + + assert!(ret1.is_ok()); + assert!(ret2.is_ok()); + assert!(ret_update_ok.is_ok()); + assert!(ret_inv_path.is_err()); + assert!(ret_2roots.is_err()); } } diff --git a/vmm/src/device_config/net.rs b/vmm/src/device_config/net.rs index 1b9923817f7..e06cf979185 100644 --- a/vmm/src/device_config/net.rs +++ b/vmm/src/device_config/net.rs @@ -62,11 +62,14 @@ impl NetworkInterfaceConfigs { pub fn put(&mut self, body: NetworkInterfaceBody) -> result::Result { let cfg = NetworkInterfaceConfig::try_from_body(body).map_err(SyncError::OpenTap)?; - for x in self.if_list.iter() { - if x.id_as_str() == cfg.id_as_str() { - return Err(SyncError::UpdateNotImplemented); + for device_config in self.if_list.iter_mut() { + if device_config.id_as_str() == cfg.id_as_str() { + device_config.tap = cfg.tap; + device_config.body = cfg.body.clone(); + return Ok(SyncOkStatus::Updated); } - if x.guest_mac() == cfg.guest_mac() { + + if device_config.guest_mac() == cfg.guest_mac() { return Err(SyncError::GuestMacAddressInUse); } } @@ -78,3 +81,40 @@ impl NetworkInterfaceConfigs { self.if_list.iter_mut() } } + +#[cfg(test)] +mod tests { + use super::*; + use api_server::request::sync::DeviceState; + use net_util::MacAddr; + + #[test] + fn test_put() { + let mut netif_configs = NetworkInterfaceConfigs::new(); + assert!(netif_configs.if_list.is_empty()); + + if let Ok(mac) = MacAddr::parse_str("01:23:45:67:89:0A") { + let mut netif_body = NetworkInterfaceBody { + iface_id: String::from("foo"), + state: DeviceState::Attached, + host_dev_name: String::from("bar"), + guest_mac: Some(mac.clone()), + }; + assert!(netif_configs.put(netif_body.clone()).is_ok()); + assert_eq!(netif_configs.if_list.len(), 1); + + netif_body.host_dev_name = String::from("baz"); + assert!(netif_configs.put(netif_body).is_ok()); + assert_eq!(netif_configs.if_list.len(), 1); + + let other_netif_body = NetworkInterfaceBody { + iface_id: String::from("bar"), + state: DeviceState::Attached, + host_dev_name: String::from("foo"), + guest_mac: Some(mac.clone()), + }; + assert!(netif_configs.put(other_netif_body).is_err()); + assert_eq!(netif_configs.if_list.len(), 1); + } + } +} diff --git a/vmm/src/lib.rs b/vmm/src/lib.rs index 53580f3c4a7..0e9d77a2409 100644 --- a/vmm/src/lib.rs +++ b/vmm/src/lib.rs @@ -363,11 +363,7 @@ impl Vmm { if self.block_device_configs .contains_drive_id(block_device_config.drive_id.clone()) { - if self.mmio_device_manager.is_some() { - return self.update_block_device(&block_device_config); - } else { - Err(DriveError::BlockDeviceUpdateNotAllowed) - } + return self.update_block_device(&block_device_config); } else { match self.block_device_configs.add(block_device_config) { Ok(_) => Ok(PutDriveOutcome::Created), @@ -376,7 +372,20 @@ impl Vmm { } } - pub fn update_block_device( + fn update_block_device( + &mut self, + block_device_config: &BlockDeviceConfig, + ) -> result::Result { + if self.mmio_device_manager.is_some() { + self.live_update_block_device(block_device_config) + } else { + self.block_device_configs + .update(block_device_config) + .map(|_| PutDriveOutcome::Updated) + } + } + + fn live_update_block_device( &mut self, block_device_config: &BlockDeviceConfig, ) -> result::Result { @@ -897,7 +906,7 @@ impl Vmm { sender.send(result).expect("one-shot channel closed"); } - fn check_instance_running(&self) -> bool { + fn is_instance_running(&self) -> bool { let instance_state = { // unwrap() to crash if the other thread poisoned this lock let shared_info = self.shared_info.read().unwrap(); @@ -911,8 +920,8 @@ impl Vmm { fn handle_put_drive(&mut self, drive_description: DriveDescription, sender: SyncOutcomeSender) { match self.put_block_device(BlockDeviceConfig::from(drive_description)) { - Ok(outcome) => sender - .send(Box::new(outcome)) + Ok(ret) => sender + .send(Box::new(ret)) .map_err(|_| ()) .expect("one-shot channel closed"), Err(e) => sender @@ -927,6 +936,14 @@ impl Vmm { logger_description: APILoggerDescription, sender: SyncOutcomeSender, ) { + if self.is_instance_running() { + sender + .send(Box::new(SyncError::UpdateNotAllowedPostBoot)) + .map_err(|_| ()) + .expect("one-shot channel closed"); + return; + } + match api_logger_config::init_logger(logger_description) { Ok(_) => sender .send(Box::new(PutLoggerOutcome::Initialized)) @@ -944,9 +961,9 @@ impl Vmm { boot_source_body: BootSourceBody, sender: SyncOutcomeSender, ) { - if self.check_instance_running() { + if self.is_instance_running() { sender - .send(Box::new(SyncError::UpdateNotAllowed)) + .send(Box::new(SyncError::UpdateNotAllowedPostBoot)) .map_err(|_| ()) .expect("one-shot channel closed"); return; @@ -969,7 +986,7 @@ impl Vmm { kernel_start_addr: GuestAddress(KERNEL_START_OFFSET), cmdline_addr: GuestAddress(CMDLINE_OFFSET), }; - // if the kernel was already configure, we have an update operation + // if the kernel was already configured, we have an update operation let outcome = match self.kernel_config { Some(_) => PutBootSourceOutcome::Updated, None => PutBootSourceOutcome::Created, @@ -1002,9 +1019,9 @@ impl Vmm { machine_config: MachineConfiguration, sender: SyncOutcomeSender, ) { - if self.check_instance_running() { + if self.is_instance_running() { sender - .send(Box::new(SyncError::UpdateNotAllowed)) + .send(Box::new(SyncError::UpdateNotAllowedPostBoot)) .map_err(|_| ()) .expect("one-shot channel closed"); return; @@ -1029,9 +1046,9 @@ impl Vmm { netif_body: NetworkInterfaceBody, sender: SyncOutcomeSender, ) { - if self.check_instance_running() { + if self.is_instance_running() { sender - .send(Box::new(SyncError::UpdateNotAllowed)) + .send(Box::new(SyncError::UpdateNotAllowedPostBoot)) .map_err(|_| ()) .expect("one-shot channel closed"); return; From 41e42152f241ec47b08a1b1bb5b747affa03ed78 Mon Sep 17 00:00:00 2001 From: Alexandra Ghecenco Date: Thu, 10 May 2018 12:30:11 +0300 Subject: [PATCH 4/4] api: add integration tests for updates * only covering PUT updates Signed-off-by: Alexandra Ghecenco --- tests/functional/test_api.py | 197 ++++++++++++++++++++++++++++++++++- 1 file changed, 195 insertions(+), 2 deletions(-) diff --git a/tests/functional/test_api.py b/tests/functional/test_api.py index b055ad2bcd4..e7b4eed0bb7 100644 --- a/tests/functional/test_api.py +++ b/tests/functional/test_api.py @@ -1,5 +1,5 @@ """ -Tests that ensure the corectness of the Firecracker API. +Tests that ensure the correctness of the Firecracker API. # TODO @@ -7,7 +7,7 @@ """ import pytest - +import shutil @pytest.mark.timeout(240) def test_api_happy_start(test_microvm_any, uhttp): @@ -91,3 +91,196 @@ def test_api_happy_start(test_microvm_any, uhttp): ) """ Issues a power-on command to the microvm. """ assert(uhttp.is_good_response(response.status_code)) + + +def test_api_put_update_pre_boot(test_microvm_any, uhttp): + """ Tests that PUT updates are allowed before the microvm boots. """ + + test_microvm = test_microvm_any + + _setup_microvm(test_microvm, uhttp) + + response = uhttp.put( + test_microvm.boot_cfg_url, + json={ + 'boot_source_id': '1', + 'source_type': 'LocalImage', + 'local_image': {'kernel_image_path': 'foo.bar'} + } + ) + """ Updating the kernel with an invalid path is not allowed. """ + assert(not uhttp.is_good_response(response.status_code)) + + kernel_copy = test_microvm.slot.kernel_file + '.copy' + # The copy will be cleaned up by the microvm fixture's teardown() function. + shutil.copy(test_microvm.slot.kernel_file, kernel_copy) + response = uhttp.put( + test_microvm.boot_cfg_url, + json={ + 'boot_source_id': '1', + 'source_type': 'LocalImage', + 'local_image': {'kernel_image_path': kernel_copy} + } + ) + """ Updates the kernel. """ + assert(uhttp.is_good_response(response.status_code)) + + response = uhttp.put( + test_microvm.blk_cfg_url + '/root', + json={ + 'drive_id': 'root', + 'path_on_host': 'foo.bar', + 'is_root_device': True, + 'permissions': 'ro', + 'state': 'Attached' + } + ) + """ Updating a block device with an invalid path is not allowed. """ + assert(not uhttp.is_good_response(response.status_code)) + + response = uhttp.put( + test_microvm.blk_cfg_url + '/scratch', + json={ + 'drive_id': 'scratch', + 'path_on_host': test_microvm.slot.make_fsfile(name='scratch'), + 'is_root_device': True, + 'permissions': 'rw', + 'state': 'Attached' + } + ) + """ An update that would result in two root block devices is not allowed.""" + assert(not uhttp.is_good_response(response.status_code)) + + response = uhttp.put( + test_microvm.blk_cfg_url + '/scratch', + json={ + 'drive_id': 'scratch', + 'path_on_host': test_microvm.slot.make_fsfile(name='scratch'), + 'is_root_device': False, + 'permissions': 'ro', + 'state': 'Attached' + } + ) + """ Updates a block device.""" + assert(uhttp.is_good_response(response.status_code)) + + response = uhttp.put(test_microvm.microvm_cfg_url, json={'vcpu_count': 2}) + """ Updates the vcpu count in the machine configuration. + The machine configuration has a default value, so all PUTs are updates. + """ + assert(uhttp.is_good_response(response.status_code)) + + response = uhttp.put( + test_microvm.net_cfg_url + '/1', + json={ + 'iface_id': '1', + 'host_dev_name': test_microvm.slot.make_tap(name='dummytap'), + 'guest_mac': '06:00:00:00:00:01', + 'state': 'Attached' + } + ) + """ Updates the network interface.""" + assert(uhttp.is_good_response(response.status_code)) + + +def test_api_put_update_post_boot(test_microvm_any, uhttp): + """ Tests that PUT updates are rejected after the microvm boots. """ + + test_microvm = test_microvm_any + + _setup_microvm(test_microvm, uhttp) + + uhttp.put( + test_microvm.actions_url + '/1', + json={'action_id': '1', 'action_type': 'InstanceStart'} + ) + + response = uhttp.put( + test_microvm.boot_cfg_url, + json={ + 'boot_source_id': '1', + 'source_type': 'LocalImage', + 'local_image': {'kernel_image_path': test_microvm.slot.kernel_file} + } + ) + """ Kernel update is not allowed after boot. """ + assert(not uhttp.is_good_response(response.status_code)) + + """ TODO + Uncomment this block after the block device update is implemented properly. Until then, the PUT triggers a rescan. + """ + # response = uhttp.put( + # test_microvm.blk_cfg_url + '/scratch', + # json={ + # 'drive_id': 'scratch', + # 'path_on_host': test_microvm.slot.make_fsfile(name='scratch'), + # 'is_root_device': False, + # 'permissions': 'ro', + # 'state': 'Attached' + # } + # ) + # """ Block device updates are not allowed via PUT after boot.""" + # assert(not uhttp.is_good_response(response.status_code)) + + response = uhttp.put(test_microvm.microvm_cfg_url, json={'vcpu_count': 2}) + """ Machine configuration update is not allowed after boot.""" + assert(not uhttp.is_good_response(response.status_code)) + + response = uhttp.put( + test_microvm.net_cfg_url + '/1', + json={ + 'iface_id': '1', + 'host_dev_name': test_microvm.slot.make_tap(name='dummytap'), + 'guest_mac': '06:00:00:00:00:01', + 'state': 'Attached' + } + ) + """ Network interface update is not allowed after boot.""" + assert(not uhttp.is_good_response(response.status_code)) + + +def _setup_microvm(test_microvm_any, uhttp): + """ Sets up a microvm with a kernel, 2 block devices and a network interface. """ + + test_microvm = test_microvm_any + + uhttp.put( + test_microvm.boot_cfg_url, + json={ + 'boot_source_id': '1', + 'source_type': 'LocalImage', + 'local_image': {'kernel_image_path': test_microvm.slot.kernel_file} + } + ) + + uhttp.put( + test_microvm.blk_cfg_url + '/root', + json={ + 'drive_id': 'root', + 'path_on_host': test_microvm.slot.rootfs_file, + 'is_root_device': True, + 'permissions': 'ro', + 'state': 'Attached' + } + ) + + uhttp.put( + test_microvm.blk_cfg_url + '/scratch', + json={ + 'drive_id': 'scratch', + 'path_on_host': test_microvm.slot.make_fsfile(name='scratch'), + 'is_root_device': False, + 'permissions': 'rw', + 'state': 'Attached' + } + ) + + uhttp.put( + test_microvm.net_cfg_url + '/1', + json={ + 'iface_id': '1', + 'host_dev_name': test_microvm.slot.make_tap(), + 'guest_mac': '06:00:00:00:00:01', + 'state': 'Attached' + } + )