From f164b676a788f27dd4b82ad5abcabbb8e598696c Mon Sep 17 00:00:00 2001 From: Diana Popa Date: Thu, 10 May 2018 12:56:08 -0500 Subject: [PATCH 1/7] refactoring: moving BootSourceBody to data model Signed-off-by: Diana Popa --- api_server/src/http_service.rs | 9 +- api_server/src/request/mod.rs | 2 +- api_server/src/request/sync/boot_source.rs | 102 +++++++----------- .../src/request/sync/machine_configuration.rs | 29 ++--- api_server/src/request/sync/mod.rs | 4 +- data_model/Cargo.toml | 3 + data_model/src/vm/boot_source.rs | 83 ++++++++++++++ ...ine_config.rs => machine_configuration.rs} | 12 +++ data_model/src/vm/mod.rs | 9 +- vmm/src/lib.rs | 56 +++++----- 10 files changed, 182 insertions(+), 127 deletions(-) create mode 100644 data_model/src/vm/boot_source.rs rename data_model/src/vm/{machine_config.rs => machine_configuration.rs} (83%) diff --git a/api_server/src/http_service.rs b/api_server/src/http_service.rs index 1c6b295cb06..110d8f1a524 100644 --- a/api_server/src/http_service.rs +++ b/api_server/src/http_service.rs @@ -13,6 +13,7 @@ use serde_json; use tokio_core::reactor::Handle; use super::{ActionMap, ActionMapValue}; +use data_model::vm::boot_source::BootSource; use data_model::vm::MachineConfiguration; use logger::{Metric, METRICS}; use request::instance_info::InstanceInfo; @@ -171,12 +172,12 @@ fn parse_boot_source_req<'a>( 0 if method == Method::Put => { METRICS.put_api_requests.boot_source_count.inc(); - Ok(serde_json::from_slice::(body) + Ok(serde_json::from_slice::(body) .map_err(|e| { METRICS.put_api_requests.boot_source_fails.inc(); Error::SerdeJson(e) })? - .into_parsed_request() + .into_parsed_request(Method::Put) .map_err(|s| { METRICS.put_api_requests.boot_source_fails.inc(); Error::Generic(StatusCode::BadRequest, s) @@ -934,9 +935,9 @@ mod tests { // PUT // Falling back to json deserialization for constructing the "correct" request because not - // all of BootSourceBody's members are accessible. Rather than making them all public just + // all of BootSource's members are accessible. Rather than making them all public just // for the purpose of unit tests, it's preferable to trust the deserialization. - let res_bsb = serde_json::from_slice::(&body); + let res_bsb = serde_json::from_slice::(&body); match res_bsb { Ok(boot_source_body) => { match parse_boot_source_req(&path_tokens, &path, Method::Put, &body) { diff --git a/api_server/src/request/mod.rs b/api_server/src/request/mod.rs index b2352b0d046..d02a4a40651 100644 --- a/api_server/src/request/mod.rs +++ b/api_server/src/request/mod.rs @@ -5,7 +5,7 @@ use std::result; pub use self::async::{AsyncOutcome, AsyncOutcomeReceiver, AsyncOutcomeSender, AsyncRequest, AsyncRequestBody}; -pub use self::sync::{APILoggerDescription, BootSourceBody, DriveDescription, NetworkInterfaceBody, +pub use self::sync::{APILoggerDescription, DriveDescription, NetworkInterfaceBody, SyncOutcomeReceiver, SyncOutcomeSender, SyncRequest}; use hyper::Method; diff --git a/api_server/src/request/sync/boot_source.rs b/api_server/src/request/sync/boot_source.rs index 28ab8ec33d3..f4d1cfef062 100644 --- a/api_server/src/request/sync/boot_source.rs +++ b/api_server/src/request/sync/boot_source.rs @@ -1,49 +1,21 @@ use std::result; use futures::sync::oneshot; -use hyper::{Response, StatusCode}; +use hyper::{Method, Response, StatusCode}; +use data_model::vm::boot_source::{BootSource, BootSourceError, PutBootSourceOutcome}; use http_service::{empty_response, json_fault_message, json_response}; use request::sync::GenerateResponse; -use request::{ParsedRequest, SyncRequest}; +use request::{IntoParsedRequest, ParsedRequest, SyncRequest}; -#[derive(Debug, Deserialize, PartialEq, Serialize)] -pub enum BootSourceType { - LocalImage, -} - -#[derive(Debug, Deserialize, PartialEq, Serialize)] -pub struct LocalImage { - pub kernel_image_path: String, -} - -#[derive(Debug, Deserialize, PartialEq, Serialize)] -pub struct BootSourceBody { - boot_source_id: String, - source_type: BootSourceType, - #[serde(skip_serializing_if = "Option::is_none")] - pub local_image: Option, - // drive_boot to be added later - // network_boot to be added later - #[serde(skip_serializing_if = "Option::is_none")] - pub boot_args: Option, -} - -#[derive(Debug)] -pub enum PutBootSourceConfigError { - InvalidKernelPath, - InvalidKernelCommandLine, -} - -impl GenerateResponse for PutBootSourceConfigError { +impl GenerateResponse for BootSourceError { fn generate_response(&self) -> Response { - use self::PutBootSourceConfigError::*; match *self { - InvalidKernelPath => json_response( + BootSourceError::InvalidKernelPath => json_response( StatusCode::BadRequest, json_fault_message("The kernel path is invalid!"), ), - InvalidKernelCommandLine => json_response( + BootSourceError::InvalidKernelCommandLine => json_response( StatusCode::BadRequest, json_fault_message("The kernel command line is invalid!"), ), @@ -51,12 +23,6 @@ impl GenerateResponse for PutBootSourceConfigError { } } -pub enum PutBootSourceOutcome { - Created, - Updated, - Error(PutBootSourceConfigError), -} - impl GenerateResponse for PutBootSourceOutcome { fn generate_response(&self) -> Response { use self::PutBootSourceOutcome::*; @@ -68,8 +34,8 @@ impl GenerateResponse for PutBootSourceOutcome { } } -impl BootSourceBody { - pub fn into_parsed_request(self) -> result::Result { +impl IntoParsedRequest for BootSource { + fn into_parsed_request(self, _method: Method) -> result::Result { let (sender, receiver) = oneshot::channel(); Ok(ParsedRequest::Sync( SyncRequest::PutBootSource(self, sender), @@ -81,17 +47,18 @@ impl BootSourceBody { #[cfg(test)] mod tests { use super::*; + use serde_json; #[test] fn test_generate_response_put_boot_source_config_error() { assert_eq!( - PutBootSourceConfigError::InvalidKernelPath + BootSourceError::InvalidKernelPath .generate_response() .status(), StatusCode::BadRequest ); assert_eq!( - PutBootSourceConfigError::InvalidKernelCommandLine + BootSourceError::InvalidKernelCommandLine .generate_response() .status(), StatusCode::BadRequest @@ -109,7 +76,7 @@ mod tests { StatusCode::NoContent ); assert_eq!( - PutBootSourceOutcome::Error(PutBootSourceConfigError::InvalidKernelPath) + PutBootSourceOutcome::Error(BootSourceError::InvalidKernelPath) .generate_response() .status(), StatusCode::BadRequest @@ -118,26 +85,31 @@ mod tests { #[test] fn test_into_parsed_request() { - let body = BootSourceBody { - boot_source_id: String::from("/foo/bar"), - source_type: BootSourceType::LocalImage, - local_image: Some(LocalImage { - kernel_image_path: String::from("/foo/bar"), - }), - boot_args: Some(String::from("foobar")), - }; - let same_body = BootSourceBody { - boot_source_id: String::from("/foo/bar"), - source_type: BootSourceType::LocalImage, - local_image: Some(LocalImage { - kernel_image_path: String::from("/foo/bar"), - }), - boot_args: Some(String::from("foobar")), - }; + let body = r#"{ + "boot_source_id": "/foo/bar", + "source_type": "LocalImage", + "local_image": { "kernel_image_path": "/foo/bar"} + }"#; + let result1: Result = serde_json::from_str(body); + assert!(result1.is_ok()); + + let body = r#"{ + "boot_source_id": "/foo/bar", + "source_type": "LocalImage", + "local_image": { "kernel_image_path": "/foo/bar"} + }"#; + let result2: Result = serde_json::from_str(body); + assert!(result2.is_ok()); + let (sender, receiver) = oneshot::channel(); - assert!(body.into_parsed_request().eq(&Ok(ParsedRequest::Sync( - SyncRequest::PutBootSource(same_body, sender), - receiver - )))) + assert!( + result1 + .unwrap() + .into_parsed_request(Method::Put) + .eq(&Ok(ParsedRequest::Sync( + SyncRequest::PutBootSource(result2.unwrap(), sender), + receiver + ))) + ) } } diff --git a/api_server/src/request/sync/machine_configuration.rs b/api_server/src/request/sync/machine_configuration.rs index ee6f0e495e0..0923d6915ec 100644 --- a/api_server/src/request/sync/machine_configuration.rs +++ b/api_server/src/request/sync/machine_configuration.rs @@ -3,30 +3,23 @@ use std::result; use futures::sync::oneshot; use hyper::{Method, Response, StatusCode}; -use data_model::vm::MachineConfiguration; +use data_model::vm::{MachineConfiguration, MachineConfigurationError, + PutMachineConfigurationOutcome}; use http_service::{empty_response, json_fault_message, json_response}; use request::sync::GenerateResponse; use request::{IntoParsedRequest, ParsedRequest, SyncRequest}; -#[derive(Debug, PartialEq)] -pub enum PutMachineConfigurationError { - InvalidVcpuCount, - InvalidMemorySize, -} - -impl GenerateResponse for PutMachineConfigurationError { +impl GenerateResponse for MachineConfigurationError { fn generate_response(&self) -> Response { - use self::PutMachineConfigurationError::*; - match self { - InvalidVcpuCount => json_response( + MachineConfigurationError::InvalidVcpuCount => json_response( StatusCode::BadRequest, json_fault_message( "The vCPU number is invalid! The vCPU number can only \ be 1 or an even number when hyperthreading is enabled.", ), ), - InvalidMemorySize => json_response( + MachineConfigurationError::InvalidMemorySize => json_response( StatusCode::BadRequest, json_fault_message("The memory size (MiB) is invalid."), ), @@ -34,12 +27,6 @@ impl GenerateResponse for PutMachineConfigurationError { } } -pub enum PutMachineConfigurationOutcome { - Created, - Updated, - Error(PutMachineConfigurationError), -} - impl GenerateResponse for PutMachineConfigurationOutcome { fn generate_response(&self) -> Response { use self::PutMachineConfigurationOutcome::*; @@ -112,13 +99,13 @@ mod tests { #[test] fn test_generate_response_put_machine_configuration_error() { assert_eq!( - PutMachineConfigurationError::InvalidVcpuCount + MachineConfigurationError::InvalidVcpuCount .generate_response() .status(), StatusCode::BadRequest ); assert_eq!( - PutMachineConfigurationError::InvalidMemorySize + MachineConfigurationError::InvalidMemorySize .generate_response() .status(), StatusCode::BadRequest @@ -140,7 +127,7 @@ mod tests { StatusCode::NoContent ); assert_eq!( - PutMachineConfigurationOutcome::Error(PutMachineConfigurationError::InvalidVcpuCount) + PutMachineConfigurationOutcome::Error(MachineConfigurationError::InvalidVcpuCount) .generate_response() .status(), StatusCode::BadRequest diff --git a/api_server/src/request/sync/mod.rs b/api_server/src/request/sync/mod.rs index 81369f254f9..76626291648 100644 --- a/api_server/src/request/sync/mod.rs +++ b/api_server/src/request/sync/mod.rs @@ -4,6 +4,7 @@ use std::result; use futures::sync::oneshot; use hyper::{self, StatusCode}; +use data_model::vm::boot_source::BootSource; use data_model::vm::MachineConfiguration; use http_service::{empty_response, json_fault_message, json_response}; use net_util::TapError; @@ -15,7 +16,6 @@ pub mod machine_configuration; mod net; mod rate_limiter; -pub use self::boot_source::{BootSourceBody, BootSourceType, LocalImage}; pub use self::drive::{DriveDescription, DriveError, DrivePermissions, PutDriveOutcome}; pub use self::logger::{APILoggerDescription, APILoggerError, APILoggerLevel, PutLoggerOutcome}; pub use self::net::NetworkInterfaceBody; @@ -57,7 +57,7 @@ pub enum DeviceState { // bits of information (ids, paths, etc.), together with an OutcomeSender, which is always present. pub enum SyncRequest { GetMachineConfiguration(SyncOutcomeSender), - PutBootSource(BootSourceBody, SyncOutcomeSender), + PutBootSource(BootSource, SyncOutcomeSender), PutDrive(DriveDescription, SyncOutcomeSender), PutLogger(APILoggerDescription, SyncOutcomeSender), PutMachineConfiguration(MachineConfiguration, SyncOutcomeSender), diff --git a/data_model/Cargo.toml b/data_model/Cargo.toml index c14df88f29b..4437626783c 100644 --- a/data_model/Cargo.toml +++ b/data_model/Cargo.toml @@ -6,3 +6,6 @@ authors = ["Amazon firecracker team "] [dependencies] serde = "=1.0.27" serde_derive = "=1.0.27" + +[dev-dependencies] +serde_json = "=1.0.9" \ No newline at end of file diff --git a/data_model/src/vm/boot_source.rs b/data_model/src/vm/boot_source.rs new file mode 100644 index 00000000000..a2650a2645b --- /dev/null +++ b/data_model/src/vm/boot_source.rs @@ -0,0 +1,83 @@ +#[derive(Debug, Deserialize, PartialEq, Serialize)] +enum BootSourceType { + LocalImage, +} + +#[derive(Debug, Deserialize, PartialEq, Serialize)] +pub struct LocalImage { + pub kernel_image_path: String, +} + +#[derive(Debug, Deserialize, PartialEq, Serialize)] +pub struct BootSource { + boot_source_id: String, + source_type: BootSourceType, + #[serde(skip_serializing_if = "Option::is_none")] + local_image: Option, + // drive_boot to be added later + // network_boot to be added later + #[serde(skip_serializing_if = "Option::is_none")] + boot_args: Option, +} + +impl BootSource { + pub fn get_kernel_image(&self) -> Option<&String> { + if let Some(ref image) = self.local_image { + Some(&image.kernel_image_path) + } else { + None + } + } + + pub fn get_boot_args(&self) -> Option<&String> { + if let Some(ref args) = self.boot_args { + Some(&args) + } else { + None + } + } +} + +pub enum BootSourceError { + InvalidKernelPath, + InvalidKernelCommandLine, +} + +pub enum PutBootSourceOutcome { + Created, + Updated, + Error(BootSourceError), +} + +#[cfg(test)] +mod tests { + extern crate serde_json; + + use super::*; + + #[test] + fn test_boot_source_getters() { + let body = r#"{ + "boot_source_id": "/foo/bar", + "source_type": "LocalImage", + "local_image": { "kernel_image_path": "/foo/bar"} + }"#; + let result: Result = serde_json::from_str(body); + assert!(result.is_ok()); + let boot_source = result.unwrap(); + assert!(boot_source.get_boot_args().is_none()); + assert_eq!( + boot_source.get_kernel_image(), + Some(&String::from("/foo/bar")) + ); + + let body = r#"{ + "boot_source_id": "/foo/bar", + "source_type": "LocalImage" + }"#; + let result: Result = serde_json::from_str(body); + assert!(result.is_ok()); + let boot_source = result.unwrap(); + assert!(boot_source.get_kernel_image().is_none()); + } +} diff --git a/data_model/src/vm/machine_config.rs b/data_model/src/vm/machine_configuration.rs similarity index 83% rename from data_model/src/vm/machine_config.rs rename to data_model/src/vm/machine_configuration.rs index ec53cf26e75..d3c4770104d 100644 --- a/data_model/src/vm/machine_config.rs +++ b/data_model/src/vm/machine_configuration.rs @@ -37,3 +37,15 @@ impl fmt::Display for CpuFeaturesTemplate { } } } + +#[derive(Debug, PartialEq)] +pub enum MachineConfigurationError { + InvalidVcpuCount, + InvalidMemorySize, +} + +pub enum PutMachineConfigurationOutcome { + Created, + Updated, + Error(MachineConfigurationError), +} diff --git a/data_model/src/vm/mod.rs b/data_model/src/vm/mod.rs index 0668301a42a..8170aa2b5c7 100644 --- a/data_model/src/vm/mod.rs +++ b/data_model/src/vm/mod.rs @@ -1,4 +1,7 @@ -pub mod machine_config; +pub mod boot_source; +pub mod machine_configuration; -pub use vm::machine_config::CpuFeaturesTemplate; -pub use vm::machine_config::MachineConfiguration; +pub use vm::boot_source::{BootSource, BootSourceError}; +pub use vm::machine_configuration::CpuFeaturesTemplate; +pub use vm::machine_configuration::{MachineConfiguration, MachineConfigurationError, + PutMachineConfigurationOutcome}; diff --git a/vmm/src/lib.rs b/vmm/src/lib.rs index a40155eab57..e8fa75c3e8e 100644 --- a/vmm/src/lib.rs +++ b/vmm/src/lib.rs @@ -35,17 +35,15 @@ use timerfd::{ClockId, SetTimeFlags, TimerFd, TimerState}; 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::{rate_limiter_description_into_implementation, - APILoggerDescription, BootSourceBody, DriveDescription, - DriveError, Error as SyncError, GenerateResponse, - NetworkInterfaceBody, OkStatus as SyncOkStatus, PutDriveOutcome, - PutLoggerOutcome, SyncOutcomeSender, SyncRequest}; - + APILoggerDescription, DriveDescription, DriveError, + Error as SyncError, GenerateResponse, NetworkInterfaceBody, + OkStatus as SyncOkStatus, PutDriveOutcome, PutLoggerOutcome, + SyncOutcomeSender, SyncRequest}; use api_server::ApiRequest; -use data_model::vm::MachineConfiguration; +use data_model::vm::boot_source::{BootSource, BootSourceError, PutBootSourceOutcome}; +use data_model::vm::{MachineConfiguration, MachineConfigurationError, + PutMachineConfigurationOutcome}; use device_config::*; use device_manager::legacy::LegacyDeviceManager; use device_manager::mmio::MMIODeviceManager; @@ -516,18 +514,18 @@ impl Vmm { pub fn put_virtual_machine_configuration( &mut self, machine_config: MachineConfiguration, - ) -> std::result::Result<(), PutMachineConfigurationError> { + ) -> std::result::Result<(), MachineConfigurationError> { if let Some(vcpu_count_value) = machine_config.vcpu_count { - // Check that the vcpu_count value is >=1 + // Check that the vcpu_count value is >=1. if vcpu_count_value <= 0 { - return Err(PutMachineConfigurationError::InvalidVcpuCount); + return Err(MachineConfigurationError::InvalidVcpuCount); } } if let Some(mem_size_mib_value) = machine_config.mem_size_mib { // TODO: add other memory checks if mem_size_mib_value <= 0 { - return Err(PutMachineConfigurationError::InvalidMemorySize); + return Err(MachineConfigurationError::InvalidMemorySize); } } @@ -541,10 +539,10 @@ impl Vmm { None => self.vm_config.vcpu_count.unwrap(), }; - // if hyperthreading is enabled or is to be enabled in this call - // only allow vcpu count to be 1 or even + // If hyperthreading is enabled or is to be enabled in this call + // only allow vcpu count to be 1 or even. if ht_enabled && vcpu_count_value > 1 && vcpu_count_value % 2 == 1 { - return Err(PutMachineConfigurationError::InvalidVcpuCount); + return Err(MachineConfigurationError::InvalidVcpuCount); } // Update all the fields that have a new value @@ -1143,11 +1141,7 @@ impl Vmm { } } - fn handle_put_boot_source( - &mut self, - boot_source_body: BootSourceBody, - sender: SyncOutcomeSender, - ) { + fn handle_put_boot_source(&mut self, boot_source_body: BootSource, sender: SyncOutcomeSender) { if self.is_instance_running() { sender .send(Box::new(SyncError::UpdateNotAllowedPostBoot)) @@ -1157,14 +1151,14 @@ impl Vmm { } // 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) { + let box_response: Box = match boot_source_body.get_kernel_image() { + Some(image) => match File::open(image) { 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)), + .get_boot_args() + .unwrap_or(&String::from(DEFAULT_KERNEL_CMDLINE)), ) { Ok(_) => { let kernel_config = KernelConfig { @@ -1181,12 +1175,12 @@ impl Vmm { self.configure_kernel(kernel_config); Box::new(outcome) } - Err(_) => Box::new(PutBootSourceConfigError::InvalidKernelCommandLine), + Err(_) => Box::new(BootSourceError::InvalidKernelCommandLine), } } - Err(_e) => Box::new(PutBootSourceConfigError::InvalidKernelPath), + Err(_e) => Box::new(BootSourceError::InvalidKernelPath), }, - None => Box::new(PutBootSourceConfigError::InvalidKernelPath), + None => Box::new(BootSourceError::InvalidKernelPath), }; sender .send(box_response) @@ -1380,7 +1374,7 @@ mod tests { assert_eq!( vmm.put_virtual_machine_configuration(machine_config) .unwrap_err(), - PutMachineConfigurationError::InvalidVcpuCount + MachineConfigurationError::InvalidVcpuCount ); assert_eq!(vmm.vm_config.vcpu_count, Some(3)); @@ -1395,7 +1389,7 @@ mod tests { assert_eq!( vmm.put_virtual_machine_configuration(machine_config) .unwrap_err(), - PutMachineConfigurationError::InvalidMemorySize + MachineConfigurationError::InvalidMemorySize ); assert_eq!(vmm.vm_config.vcpu_count, Some(3)); assert_eq!(vmm.vm_config.mem_size_mib, Some(256)); @@ -1414,7 +1408,7 @@ mod tests { assert_eq!( vmm.put_virtual_machine_configuration(machine_config) .unwrap_err(), - PutMachineConfigurationError::InvalidVcpuCount + MachineConfigurationError::InvalidVcpuCount ); assert_eq!(vmm.vm_config.ht_enabled, Some(false)); // Test that you can change the ht flag when you have a valid vcpu count From 6d4cdef84416e0dffacd22518e4544b9213b1f84 Mon Sep 17 00:00:00 2001 From: Diana Popa Date: Fri, 11 May 2018 09:24:41 -0500 Subject: [PATCH 2/7] refactoring: moving DriveDescription to data model Signed-off-by: Diana Popa --- api_server/Cargo.toml | 1 - api_server/src/http_service.rs | 34 +- api_server/src/request/mod.rs | 10 +- api_server/src/request/sync/boot_source.rs | 8 +- api_server/src/request/sync/drive.rs | 100 ++--- .../src/request/sync/machine_configuration.rs | 14 +- api_server/src/request/sync/mod.rs | 12 +- api_server/src/request/sync/net.rs | 11 +- data_model/Cargo.toml | 2 + data_model/src/device_config/drive.rs | 154 +++++++ data_model/src/device_config/mod.rs | 12 + data_model/src/device_config/net.rs | 1 + .../src/device_config}/rate_limiter.rs | 15 +- data_model/src/lib.rs | 2 + devices/src/virtio/block.rs | 2 +- devices/src/virtio/net.rs | 2 +- fc_util/src/lib.rs | 3 +- vmm/Cargo.toml | 2 + vmm/src/device_config/drive.rs | 418 ++++++++---------- vmm/src/device_config/mod.rs | 2 +- vmm/src/device_config/net.rs | 14 +- vmm/src/device_manager/mmio.rs | 4 +- vmm/src/lib.rs | 37 +- 23 files changed, 458 insertions(+), 402 deletions(-) create mode 100644 data_model/src/device_config/drive.rs create mode 100644 data_model/src/device_config/mod.rs create mode 100644 data_model/src/device_config/net.rs rename {api_server/src/request/sync => data_model/src/device_config}/rate_limiter.rs (84%) diff --git a/api_server/Cargo.toml b/api_server/Cargo.toml index 17361921fe4..c4d38145a55 100644 --- a/api_server/Cargo.toml +++ b/api_server/Cargo.toml @@ -11,7 +11,6 @@ serde_derive = "=1.0.27" serde_json = "=1.0.9" tokio-core = "=0.1.12" tokio-uds = "=0.1.7" -tokio-io = "=0.1.5" data_model = { path = "../data_model" } fc_util = { path = "../fc_util" } diff --git a/api_server/src/http_service.rs b/api_server/src/http_service.rs index 110d8f1a524..aa2e6b76dce 100644 --- a/api_server/src/http_service.rs +++ b/api_server/src/http_service.rs @@ -7,12 +7,13 @@ use std::sync::{Arc, RwLock}; use futures::future::{self, Either}; use futures::{Future, Stream}; - use hyper::{self, Chunk, Headers, Method, StatusCode}; use serde_json; use tokio_core::reactor::Handle; use super::{ActionMap, ActionMapValue}; + +use data_model::device_config::DriveConfig; use data_model::vm::boot_source::BootSource; use data_model::vm::MachineConfiguration; use logger::{Metric, METRICS}; @@ -177,7 +178,7 @@ fn parse_boot_source_req<'a>( METRICS.put_api_requests.boot_source_fails.inc(); Error::SerdeJson(e) })? - .into_parsed_request(Method::Put) + .into_parsed_request(Method::Put, None) .map_err(|s| { METRICS.put_api_requests.boot_source_fails.inc(); Error::Generic(StatusCode::BadRequest, s) @@ -203,12 +204,14 @@ fn parse_drives_req<'a>( 1 if method == Method::Put => { METRICS.put_api_requests.drive_count.inc(); - Ok(serde_json::from_slice::(body) + let unwrapped_id = id_from_path.ok_or(Error::InvalidID)?; + + Ok(serde_json::from_slice::(body) .map_err(|e| { METRICS.put_api_requests.drive_fails.inc(); Error::SerdeJson(e) })? - .into_parsed_request(id_from_path.unwrap()) + .into_parsed_request(method, Some(unwrapped_id)) .map_err(|s| { METRICS.put_api_requests.drive_fails.inc(); Error::Generic(StatusCode::BadRequest, s) @@ -264,7 +267,7 @@ fn parse_machine_config_req<'a>( cpu_template: None, }; Ok(empty_machine_config - .into_parsed_request(method) + .into_parsed_request(method, None) .map_err(|s| { METRICS.get_api_requests.machine_cfg_fails.inc(); Error::Generic(StatusCode::BadRequest, s) @@ -278,7 +281,7 @@ fn parse_machine_config_req<'a>( METRICS.put_api_requests.machine_cfg_fails.inc(); Error::SerdeJson(e) })? - .into_parsed_request(method) + .into_parsed_request(method, None) .map_err(|s| { METRICS.put_api_requests.machine_cfg_fails.inc(); Error::Generic(StatusCode::BadRequest, s) @@ -667,6 +670,7 @@ fn describe(sync: bool, method: &Method, path: &String, body: &String) -> String #[cfg(test)] mod tests { use super::*; + use data_model::device_config::DeviceState; use data_model::vm::CpuFeaturesTemplate; use fc_util::LriHashMap; use futures::sync::oneshot; @@ -674,8 +678,7 @@ mod tests { use hyper::Body; use net_util::MacAddr; use request::async::AsyncRequest; - use request::sync::{DeviceState, DriveDescription, DrivePermissions, NetworkInterfaceBody, - SyncRequest}; + use request::sync::{NetworkInterfaceBody, SyncRequest}; fn body_to_string(body: hyper::Body) -> String { let ret = body.fold(Vec::new(), |mut acc, chunk| { @@ -1001,16 +1004,11 @@ mod tests { } // PUT - let drive_desc = DriveDescription { - drive_id: String::from("bar"), - path_on_host: String::from("/foo/bar"), - state: DeviceState::Attached, - is_root_device: true, - permissions: DrivePermissions::ro, - rate_limiter: None, - }; + let result: result::Result = serde_json::from_str(json); + assert!(result.is_ok()); + let drive_desc = result.unwrap(); - match drive_desc.into_parsed_request("bar") { + match drive_desc.into_parsed_request(Method::Put, Some("bar")) { Ok(pr) => match parse_drives_req( &"/foo/bar"[1..].split_terminator('/').collect(), &"/foo/bar", @@ -1067,7 +1065,7 @@ mod tests { cpu_template: Some(CpuFeaturesTemplate::T2), }; - match mcb.into_parsed_request(Method::Put) { + match mcb.into_parsed_request(Method::Put, None) { Ok(pr) => match parse_machine_config_req(&path_tokens, &path, Method::Put, &body) { Ok(pr_mcb) => assert!(pr.eq(&pr_mcb)), _ => assert!(false), diff --git a/api_server/src/request/mod.rs b/api_server/src/request/mod.rs index d02a4a40651..3cb3dd37f66 100644 --- a/api_server/src/request/mod.rs +++ b/api_server/src/request/mod.rs @@ -5,8 +5,8 @@ use std::result; pub use self::async::{AsyncOutcome, AsyncOutcomeReceiver, AsyncOutcomeSender, AsyncRequest, AsyncRequestBody}; -pub use self::sync::{APILoggerDescription, DriveDescription, NetworkInterfaceBody, - SyncOutcomeReceiver, SyncOutcomeSender, SyncRequest}; +pub use self::sync::{APILoggerDescription, NetworkInterfaceBody, SyncOutcomeReceiver, + SyncOutcomeSender, SyncRequest}; use hyper::Method; pub mod instance_info; @@ -30,5 +30,9 @@ pub enum ApiRequest { } pub trait IntoParsedRequest { - fn into_parsed_request(self, method: Method) -> result::Result; + fn into_parsed_request( + self, + method: Method, + id_from_path: Option<&str>, + ) -> result::Result; } diff --git a/api_server/src/request/sync/boot_source.rs b/api_server/src/request/sync/boot_source.rs index f4d1cfef062..07776b65deb 100644 --- a/api_server/src/request/sync/boot_source.rs +++ b/api_server/src/request/sync/boot_source.rs @@ -35,7 +35,11 @@ impl GenerateResponse for PutBootSourceOutcome { } impl IntoParsedRequest for BootSource { - fn into_parsed_request(self, _method: Method) -> result::Result { + fn into_parsed_request( + self, + _method: Method, + _id_from_path: Option<&str>, + ) -> result::Result { let (sender, receiver) = oneshot::channel(); Ok(ParsedRequest::Sync( SyncRequest::PutBootSource(self, sender), @@ -105,7 +109,7 @@ mod tests { assert!( result1 .unwrap() - .into_parsed_request(Method::Put) + .into_parsed_request(Method::Put, None) .eq(&Ok(ParsedRequest::Sync( SyncRequest::PutBootSource(result2.unwrap(), sender), receiver diff --git a/api_server/src/request/sync/drive.rs b/api_server/src/request/sync/drive.rs index 24d7a55a262..f16370f9e06 100644 --- a/api_server/src/request/sync/drive.rs +++ b/api_server/src/request/sync/drive.rs @@ -1,48 +1,11 @@ -use std::result; - use futures::sync::oneshot; -use hyper::{Response, StatusCode}; +use hyper::{Method, Response, StatusCode}; +use std::result; -use super::rate_limiter::RateLimiterDescription; -use super::{DeviceState, GenerateResponse, SyncRequest}; +use data_model::device_config::{DriveConfig, DriveError, PutDriveOutcome}; use http_service::{empty_response, json_fault_message, json_response}; -use request::ParsedRequest; - -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] -#[allow(non_camel_case_types)] -pub enum DrivePermissions { - ro, - rw, -} - -// This struct represents the strongly typed equivalent of the json body from drive -// related requests. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] -pub struct DriveDescription { - pub drive_id: String, - pub path_on_host: String, - pub state: DeviceState, - pub is_root_device: bool, - pub permissions: DrivePermissions, - #[serde(skip_serializing_if = "Option::is_none")] - pub rate_limiter: Option, -} - -impl DriveDescription { - pub fn is_read_only(&self) -> bool { - self.permissions == DrivePermissions::ro - } -} - -#[derive(Debug)] -pub enum DriveError { - RootBlockDeviceAlreadyAdded, - InvalidBlockDevicePath, - BlockDevicePathAlreadyExists, - BlockDeviceUpdateFailed, - BlockDeviceUpdateNotAllowed, - NotImplemented, -} +use request::sync::GenerateResponse; +use request::{IntoParsedRequest, ParsedRequest, SyncRequest}; impl GenerateResponse for DriveError { fn generate_response(&self) -> Response { @@ -76,12 +39,6 @@ impl GenerateResponse for DriveError { } } -pub enum PutDriveOutcome { - Created, - Updated, - Error(DriveError), -} - impl GenerateResponse for PutDriveOutcome { fn generate_response(&self) -> Response { use self::PutDriveOutcome::*; @@ -93,9 +50,13 @@ impl GenerateResponse for PutDriveOutcome { } } -impl DriveDescription { - pub fn into_parsed_request(self, id_from_path: &str) -> result::Result { - if id_from_path != self.drive_id { +impl IntoParsedRequest for DriveConfig { + fn into_parsed_request( + self, + _method: Method, + id_from_path: Option<&str>, + ) -> result::Result { + if id_from_path.is_some() && id_from_path.unwrap() != self.get_id() { return Err(String::from( "The id from the path does not match the id from the body!", )); @@ -111,21 +72,9 @@ impl DriveDescription { #[cfg(test)] mod tests { - use super::*; + extern crate serde_json; - #[test] - fn test_is_read_only() { - assert!( - DriveDescription { - drive_id: String::from("foo"), - path_on_host: String::from("/foo/bar"), - state: DeviceState::Attached, - is_root_device: true, - permissions: DrivePermissions::ro, - rate_limiter: None, - }.is_read_only() - ); - } + use super::*; #[test] fn test_generate_response_drive_error() { @@ -185,19 +134,20 @@ mod tests { #[test] fn test_into_parsed_request() { - let desc = DriveDescription { - drive_id: String::from("foo"), - path_on_host: String::from("/foo/bar"), - state: DeviceState::Attached, - is_root_device: true, - permissions: DrivePermissions::ro, - rate_limiter: None, - }; + let json = "{ + \"drive_id\": \"foo\", + \"path_on_host\": \"/foo/bar\", + \"state\": \"Attached\", + \"is_root_device\": true, + \"permissions\": \"ro\" + }"; + let result: result::Result = serde_json::from_str(json); + assert!(result.is_ok()); + let desc = result.unwrap(); - assert!(&desc.clone().into_parsed_request("bar").is_err()); let (sender, receiver) = oneshot::channel(); assert!(&desc.clone() - .into_parsed_request("foo") + .into_parsed_request(Method::Put, Some("foo")) .eq(&Ok(ParsedRequest::Sync( SyncRequest::PutDrive(desc, sender), receiver diff --git a/api_server/src/request/sync/machine_configuration.rs b/api_server/src/request/sync/machine_configuration.rs index 0923d6915ec..b87e3dc8ea4 100644 --- a/api_server/src/request/sync/machine_configuration.rs +++ b/api_server/src/request/sync/machine_configuration.rs @@ -68,7 +68,11 @@ impl GenerateResponse for MachineConfiguration { } impl IntoParsedRequest for MachineConfiguration { - fn into_parsed_request(self, method: Method) -> result::Result { + fn into_parsed_request( + self, + method: Method, + _id_from_path: Option<&str>, + ) -> result::Result { let (sender, receiver) = oneshot::channel(); match method { Method::Get => Ok(ParsedRequest::Sync( @@ -145,7 +149,7 @@ mod tests { let (sender, receiver) = oneshot::channel(); assert!( body.clone() - .into_parsed_request(Method::Put) + .into_parsed_request(Method::Put, None) .eq(&Ok(ParsedRequest::Sync( SyncRequest::PutMachineConfiguration(body, sender), receiver @@ -160,17 +164,17 @@ mod tests { assert!( uninitialized .clone() - .into_parsed_request(Method::Get) + .into_parsed_request(Method::Get, None) .is_ok() ); assert!( uninitialized .clone() - .into_parsed_request(Method::Patch) + .into_parsed_request(Method::Patch, None) .eq(&Ok(ParsedRequest::Dummy)) ); - match uninitialized.into_parsed_request(Method::Put) { + match uninitialized.into_parsed_request(Method::Put, None) { Ok(_) => assert!(false), Err(e) => assert_eq!(e, String::from("Empty request.")), }; diff --git a/api_server/src/request/sync/mod.rs b/api_server/src/request/sync/mod.rs index 76626291648..6ccaaae7d1e 100644 --- a/api_server/src/request/sync/mod.rs +++ b/api_server/src/request/sync/mod.rs @@ -4,6 +4,7 @@ use std::result; use futures::sync::oneshot; use hyper::{self, StatusCode}; +use data_model::device_config::{DeviceState, DriveConfig}; use data_model::vm::boot_source::BootSource; use data_model::vm::MachineConfiguration; use http_service::{empty_response, json_fault_message, json_response}; @@ -14,13 +15,9 @@ mod drive; mod logger; pub mod machine_configuration; mod net; -mod rate_limiter; -pub use self::drive::{DriveDescription, DriveError, DrivePermissions, PutDriveOutcome}; pub use self::logger::{APILoggerDescription, APILoggerError, APILoggerLevel, PutLoggerOutcome}; pub use self::net::NetworkInterfaceBody; -pub use self::rate_limiter::description_into_implementation as rate_limiter_description_into_implementation; -pub use self::rate_limiter::RateLimiterDescription; // Unlike async requests, sync request have outcomes which implement this trait. The idea is for // each outcome to be a struct which is cheaply and quickly instantiated by the VMM thread, then @@ -48,17 +45,12 @@ where pub type SyncOutcomeSender = oneshot::Sender>; pub type SyncOutcomeReceiver = oneshot::Receiver>; -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] -pub enum DeviceState { - Attached, -} - // This enum contains messages for the VMM which represent sync requests. They each contain various // bits of information (ids, paths, etc.), together with an OutcomeSender, which is always present. pub enum SyncRequest { GetMachineConfiguration(SyncOutcomeSender), PutBootSource(BootSource, SyncOutcomeSender), - PutDrive(DriveDescription, SyncOutcomeSender), + PutDrive(DriveConfig, SyncOutcomeSender), PutLogger(APILoggerDescription, SyncOutcomeSender), PutMachineConfiguration(MachineConfiguration, SyncOutcomeSender), PutNetworkInterface(NetworkInterfaceBody, SyncOutcomeSender), diff --git a/api_server/src/request/sync/net.rs b/api_server/src/request/sync/net.rs index 544cfe6ead1..d7685183b93 100644 --- a/api_server/src/request/sync/net.rs +++ b/api_server/src/request/sync/net.rs @@ -2,7 +2,8 @@ use std::result; use futures::sync::oneshot; -use super::{DeviceState, RateLimiterDescription, SyncRequest}; +use super::{DeviceState, SyncRequest}; +use data_model::device_config::RateLimiterConfig; use net_util::MacAddr; use request::ParsedRequest; @@ -17,9 +18,9 @@ pub struct NetworkInterfaceBody { #[serde(skip_serializing_if = "Option::is_none")] pub guest_mac: Option, #[serde(skip_serializing_if = "Option::is_none")] - pub rx_rate_limiter: Option, + pub rx_rate_limiter: Option, #[serde(skip_serializing_if = "Option::is_none")] - pub tx_rate_limiter: Option, + pub tx_rate_limiter: Option, } impl NetworkInterfaceBody { @@ -74,8 +75,8 @@ mod tests { state: DeviceState::Attached, host_dev_name: String::from("bar"), guest_mac: Some(MacAddr::parse_str("12:34:56:78:9A:BC").unwrap()), - rx_rate_limiter: Some(RateLimiterDescription::default()), - tx_rate_limiter: Some(RateLimiterDescription::default()), + rx_rate_limiter: Some(RateLimiterConfig::default()), + tx_rate_limiter: Some(RateLimiterConfig::default()), }; // This is the json encoding of the netif variable. diff --git a/data_model/Cargo.toml b/data_model/Cargo.toml index 4437626783c..bd5a9674c68 100644 --- a/data_model/Cargo.toml +++ b/data_model/Cargo.toml @@ -7,5 +7,7 @@ authors = ["Amazon firecracker team "] serde = "=1.0.27" serde_derive = "=1.0.27" +fc_util = { path = "../fc_util" } + [dev-dependencies] serde_json = "=1.0.9" \ No newline at end of file diff --git a/data_model/src/device_config/drive.rs b/data_model/src/device_config/drive.rs new file mode 100644 index 00000000000..4de63133883 --- /dev/null +++ b/data_model/src/device_config/drive.rs @@ -0,0 +1,154 @@ +use std::path::PathBuf; + +use super::{DeviceState, RateLimiterConfig}; + +#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +#[allow(non_camel_case_types)] +pub enum DrivePermissions { + ro, + rw, +} + +// This struct represents the strongly typed equivalent of the json body from drive +// related requests. +#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +pub struct DriveConfig { + drive_id: String, + path_on_host: PathBuf, + state: DeviceState, + is_root_device: bool, + permissions: DrivePermissions, + #[serde(skip_serializing_if = "Option::is_none")] + rate_limiter: Option, +} + +impl DriveConfig { + pub fn get_id(&self) -> &str { + &self.drive_id + } + + pub fn get_path_on_host(&self) -> &PathBuf { + &self.path_on_host + } + + pub fn get_rate_limiter(&self) -> Option<&RateLimiterConfig> { + if let Some(ref rl) = self.rate_limiter { + Some(&rl) + } else { + None + } + } + pub fn is_read_only(&self) -> bool { + self.permissions == DrivePermissions::ro + } + + pub fn is_root_device(&self) -> bool { + self.is_root_device == true + } + + pub fn set_is_read_only(&mut self, is_read_only: bool) { + if is_read_only == true { + self.permissions = DrivePermissions::ro; + } else { + self.permissions = DrivePermissions::rw; + } + } + + pub fn set_is_root_device(&mut self, is_root_device: bool) { + self.is_root_device = is_root_device; + } + + pub fn set_path_on_host(&mut self, path_on_host: &PathBuf) { + self.path_on_host = path_on_host.clone(); + } +} + +#[derive(Debug)] +pub enum DriveError { + RootBlockDeviceAlreadyAdded, + InvalidBlockDevicePath, + BlockDevicePathAlreadyExists, + BlockDeviceUpdateFailed, + BlockDeviceUpdateNotAllowed, + NotImplemented, +} + +pub enum PutDriveOutcome { + Created, + Updated, + Error(DriveError), +} + +#[cfg(test)] +mod tests { + extern crate serde_json; + + use super::*; + + #[test] + fn test_block_device_config() { + let body = r#"{ + "drive_id": "1", + "path_on_host": "test_block_device_config", + "is_root_device": true, + "permissions": "ro", + "state": "Attached" + }"#; + + let result: Result = serde_json::from_str(body); + assert!(result.is_ok()); + let mut device1 = result.unwrap(); + + assert_eq!(device1.is_read_only(), true); + device1.set_is_read_only(false); + assert_eq!(device1.is_read_only(), false); + device1.set_is_read_only(true); + assert_eq!(device1.is_read_only(), true); + + assert_eq!(device1.is_root_device(), true); + device1.set_is_root_device(false); + assert_eq!(device1.is_root_device(), false); + + assert_eq!(device1.get_id(), "1"); + + assert_eq!( + device1.get_path_on_host().to_str().unwrap(), + "test_block_device_config" + ); + device1.set_path_on_host(&PathBuf::from("test_block_device_config_update")); + assert_eq!( + device1.get_path_on_host().to_str().unwrap(), + "test_block_device_config_update" + ); + + assert!(device1.get_rate_limiter().is_none()); + } + + #[test] + fn test_drive_error() { + assert_eq!( + format!("{:?}", DriveError::RootBlockDeviceAlreadyAdded), + "RootBlockDeviceAlreadyAdded" + ); + assert_eq!( + format!("{:?}", DriveError::InvalidBlockDevicePath), + "InvalidBlockDevicePath" + ); + assert_eq!( + format!("{:?}", DriveError::BlockDevicePathAlreadyExists), + "BlockDevicePathAlreadyExists" + ); + assert_eq!( + format!("{:?}", DriveError::BlockDeviceUpdateFailed), + "BlockDeviceUpdateFailed" + ); + assert_eq!( + format!("{:?}", DriveError::BlockDeviceUpdateNotAllowed), + "BlockDeviceUpdateNotAllowed" + ); + assert_eq!( + format!("{:?}", DriveError::NotImplemented), + "NotImplemented" + ); + } +} diff --git a/data_model/src/device_config/mod.rs b/data_model/src/device_config/mod.rs new file mode 100644 index 00000000000..076451beaa5 --- /dev/null +++ b/data_model/src/device_config/mod.rs @@ -0,0 +1,12 @@ +mod drive; +mod net; +mod rate_limiter; + +pub use device_config::drive::{DriveConfig, DriveError, DrivePermissions, PutDriveOutcome}; +pub use device_config::rate_limiter::{description_into_implementation as rate_limiter_description_into_implementation, + RateLimiterConfig}; + +#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +pub enum DeviceState { + Attached, +} diff --git a/data_model/src/device_config/net.rs b/data_model/src/device_config/net.rs new file mode 100644 index 00000000000..8b137891791 --- /dev/null +++ b/data_model/src/device_config/net.rs @@ -0,0 +1 @@ + diff --git a/api_server/src/request/sync/rate_limiter.rs b/data_model/src/device_config/rate_limiter.rs similarity index 84% rename from api_server/src/request/sync/rate_limiter.rs rename to data_model/src/device_config/rate_limiter.rs index 798b26046f9..dba0182424d 100644 --- a/api_server/src/request/sync/rate_limiter.rs +++ b/data_model/src/device_config/rate_limiter.rs @@ -1,7 +1,6 @@ use std::io; -use fc_util::ratelimiter::RateLimiter; - +use fc_util::RateLimiter; // This struct represents the strongly typed equivalent of the json body for TokenBucket #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] pub struct TokenBucketDescription { @@ -20,13 +19,13 @@ impl Default for TokenBucketDescription { // This struct represents the strongly typed equivalent of the json body for RateLimiter #[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize)] -pub struct RateLimiterDescription { +pub struct RateLimiterConfig { pub bandwidth: TokenBucketDescription, pub ops: TokenBucketDescription, } // TryFrom trait is sadly marked unstable, so make our own -impl RateLimiterDescription { +impl RateLimiterConfig { fn into_implementation(&self) -> io::Result { RateLimiter::new( self.bandwidth.size, @@ -38,7 +37,7 @@ impl RateLimiterDescription { } pub fn description_into_implementation( - rate_limiter_description: Option<&RateLimiterDescription>, + rate_limiter_description: Option<&RateLimiterConfig>, ) -> io::Result> { match rate_limiter_description { Some(rld) => Ok(Some(rld.into_implementation()?)), @@ -63,7 +62,7 @@ mod tests { #[test] fn test_rate_limiter_default() { - let l = RateLimiterDescription::default(); + let l = RateLimiterConfig::default(); assert_eq!(l.bandwidth.size, 0); assert_eq!(l.bandwidth.refill_time, 0); assert_eq!(l.ops.size, 0); @@ -72,8 +71,6 @@ mod tests { #[test] fn test_rate_limiter_into_impl() { - RateLimiterDescription::default() - .into_implementation() - .unwrap(); + RateLimiterConfig::default().into_implementation().unwrap(); } } diff --git a/data_model/src/lib.rs b/data_model/src/lib.rs index 92a389e80a3..2a7a049ea24 100644 --- a/data_model/src/lib.rs +++ b/data_model/src/lib.rs @@ -1,7 +1,9 @@ extern crate serde; #[macro_use] extern crate serde_derive; +extern crate fc_util; +pub mod device_config; pub mod vm; use std::sync::atomic::{AtomicBool, ATOMIC_BOOL_INIT}; diff --git a/devices/src/virtio/block.rs b/devices/src/virtio/block.rs index a275c6caa07..7f26fc9ea13 100644 --- a/devices/src/virtio/block.rs +++ b/devices/src/virtio/block.rs @@ -14,7 +14,7 @@ use std::sync::Arc; use super::{ActivateError, ActivateResult, DescriptorChain, Queue, VirtioDevice, TYPE_BLOCK, VIRTIO_MMIO_INT_VRING}; -use fc_util::ratelimiter::{RateLimiter, TokenType}; +use fc_util::{RateLimiter, TokenType}; use logger::{Metric, METRICS}; use sys_util::Result as SysResult; use sys_util::{EventFd, GuestAddress, GuestMemory, GuestMemoryError}; diff --git a/devices/src/virtio/net.rs b/devices/src/virtio/net.rs index fa1be6132b9..d56602dd1f6 100644 --- a/devices/src/virtio/net.rs +++ b/devices/src/virtio/net.rs @@ -16,7 +16,7 @@ use std::sync::mpsc; use std::sync::Arc; use super::{ActivateError, ActivateResult, Queue, VirtioDevice, TYPE_NET, VIRTIO_MMIO_INT_VRING}; -use fc_util::ratelimiter::{RateLimiter, TokenType}; +use fc_util::{RateLimiter, TokenType}; use logger::{Metric, METRICS}; use net_sys; use net_util::{MacAddr, Tap, TapError, MAC_ADDR_LEN}; diff --git a/fc_util/src/lib.rs b/fc_util/src/lib.rs index 5df6f533aff..b1d8b386355 100644 --- a/fc_util/src/lib.rs +++ b/fc_util/src/lib.rs @@ -2,6 +2,7 @@ extern crate logger; mod lri_hash_map; -pub mod ratelimiter; +mod ratelimiter; pub use lri_hash_map::LriHashMap; +pub use ratelimiter::{RateLimiter, TokenType}; diff --git a/vmm/Cargo.toml b/vmm/Cargo.toml index 389ebf6fdeb..f2a7850369f 100644 --- a/vmm/Cargo.toml +++ b/vmm/Cargo.toml @@ -20,3 +20,5 @@ net_util = { path = "../net_util"} sys_util = { path = "../sys_util" } x86_64 = { path = "../x86_64" } +[dev-dependencies] +serde_json = "=1.0.9" diff --git a/vmm/src/device_config/drive.rs b/vmm/src/device_config/drive.rs index 49abc665818..41c2554f500 100644 --- a/vmm/src/device_config/drive.rs +++ b/vmm/src/device_config/drive.rs @@ -1,46 +1,22 @@ use std::collections::LinkedList; use std::path::PathBuf; - use std::result; -use api_server::request::sync::{DriveDescription, DriveError, RateLimiterDescription}; +use data_model::device_config::{DriveConfig, DriveError}; type Result = result::Result; -/// Use this structure to set up the Block Device before booting the kernel -#[derive(PartialEq, Debug, Clone)] -pub struct BlockDeviceConfig { - pub drive_id: String, - pub path_on_host: PathBuf, - pub is_root_device: bool, - pub is_read_only: bool, - pub rate_limiter: Option, -} - -// Wrapper for the collection that holds all the Block Devices Configs +// Wrapper for the collection that holds all the Drive Configs. pub struct BlockDeviceConfigs { - pub config_list: LinkedList, + pub config_list: LinkedList, has_root_block: bool, read_only_root: bool, } -impl From for BlockDeviceConfig { - fn from(item: DriveDescription) -> Self { - let is_read_only = item.is_read_only(); - BlockDeviceConfig { - drive_id: item.drive_id, - path_on_host: PathBuf::from(item.path_on_host), - is_root_device: item.is_root_device, - is_read_only, - rate_limiter: item.rate_limiter, - } - } -} - impl BlockDeviceConfigs { pub fn new() -> BlockDeviceConfigs { BlockDeviceConfigs { - config_list: LinkedList::::new(), + config_list: LinkedList::::new(), has_root_block: false, read_only_root: false, } @@ -54,18 +30,18 @@ impl BlockDeviceConfigs { self.read_only_root } - pub fn contains_drive_path(&self, drive_path: PathBuf) -> bool { + fn contains_drive_path(&self, drive_path: &PathBuf) -> bool { for drive_config in self.config_list.iter() { - if drive_config.path_on_host == drive_path { + if drive_config.get_path_on_host() == drive_path { return true; } } return false; } - pub fn contains_drive_id(&self, drive_id: String) -> bool { + pub fn contains_drive_id(&self, drive_id: &str) -> bool { for drive_config in self.config_list.iter() { - if drive_config.drive_id == drive_id { + if drive_config.get_id() == drive_id { return true; } } @@ -74,24 +50,24 @@ impl BlockDeviceConfigs { /// This function adds a Block Device Config to the list. The root block device is always /// added to the beginning of the list. Only one root block device can be added. - pub fn add(&mut self, block_device_config: BlockDeviceConfig) -> Result<()> { + pub fn add(&mut self, block_device_config: DriveConfig) -> Result<()> { // check if the path exists - if !block_device_config.path_on_host.exists() { + if !block_device_config.get_path_on_host().exists() { return Err(DriveError::InvalidBlockDevicePath); } - if self.contains_drive_path(block_device_config.path_on_host.clone()) { + if self.contains_drive_path(block_device_config.get_path_on_host()) { return Err(DriveError::BlockDevicePathAlreadyExists); } // check whether the Device Config belongs to a root device // we need to satisfy the condition by which a VMM can only have on root device - if block_device_config.is_root_device { + if block_device_config.is_root_device() { if self.has_root_block { return Err(DriveError::RootBlockDeviceAlreadyAdded); } else { self.has_root_block = true; - self.read_only_root = block_device_config.is_read_only; + self.read_only_root = block_device_config.is_read_only(); // Root Device should be the first in the list self.config_list.push_front(block_device_config); } @@ -107,8 +83,8 @@ impl BlockDeviceConfigs { return None; } else { for cfg in self.config_list.iter() { - if cfg.is_root_device { - return Some(cfg.drive_id.clone()); + if cfg.is_root_device() { + return Some(cfg.get_id().to_string()); } } } @@ -117,33 +93,33 @@ impl BlockDeviceConfigs { /// 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<()> { + pub fn update(&mut self, block_device_config: &DriveConfig) -> Result<()> { // Check if the path exists - if !block_device_config.path_on_host.exists() { + if !block_device_config.get_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 { + if cfg.get_id() == block_device_config.get_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.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 { + 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; + 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; + cfg.set_is_root_device(block_device_config.is_root_device()); + cfg.set_is_read_only(block_device_config.is_read_only()); + cfg.set_path_on_host(block_device_config.get_path_on_host()); return Ok(()); } @@ -155,9 +131,11 @@ impl BlockDeviceConfigs { #[cfg(test)] mod tests { + extern crate serde_json; + use super::*; - use api_server::request::sync::{DeviceState, DrivePermissions}; use std::fs::{self, File}; + use std::result; // Helper function for creating a dummy file // The filename has to be unique among all tests because the tests are run on many threads @@ -181,141 +159,148 @@ mod tests { #[test] 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.clone(), - is_root_device: false, - is_read_only: false, - drive_id: dummy_id.clone(), - rate_limiter: None, - }; + let body = r#"{ + "drive_id": "1", + "path_on_host": "test_add_non_root_block_device", + "state": "Attached", + "is_root_device": false, + "permissions": "rw" + }"#; + + let result: result::Result = serde_json::from_str(body); + assert!(result.is_ok()); + let dummy_block_device = result.unwrap(); let mut block_devices_configs = BlockDeviceConfigs::new(); - let ret = block_devices_configs.add(dummy_block_device.clone()); - // clean up before the assert!s to make sure the temp files are deleted + create_dummy_path(dummy_filename.clone()); + let add_result = block_devices_configs.add(dummy_block_device.clone()); delete_dummy_path(dummy_filename); - assert!(ret.is_ok()); + assert!(add_result.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); - 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] - fn test_add_one_root_block_device() { - let dummy_filename = String::from("test_add_one_root_block_device"); - let dummy_path = create_dummy_path(dummy_filename.clone()); - - let dummy_block_device = BlockDeviceConfig { - path_on_host: dummy_path, - is_root_device: true, - is_read_only: true, - drive_id: String::from("1"), - rate_limiter: None, - }; + fn test_root_block_device_add() { let mut block_devices_configs = BlockDeviceConfigs::new(); - let ret = block_devices_configs.add(dummy_block_device.clone()); - delete_dummy_path(dummy_filename); - assert!(ret.is_ok()); + + let dummy_filename1 = String::from("test_root_block_device_add1"); + create_dummy_path(dummy_filename1.clone()); + + let body = r#"{ + "drive_id": "1", + "path_on_host": "test_root_block_device_add1", + "state": "Attached", + "is_root_device": true, + "permissions": "rw" + }"#; + + let result: result::Result = serde_json::from_str(body); + assert!(result.is_ok()); + let root_block_device1 = result.unwrap(); + let add_result = block_devices_configs.add(root_block_device1.clone()); + delete_dummy_path(dummy_filename1); + assert!(add_result.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); - assert_eq!(block_devices_configs.has_read_only_root(), true); - } - #[test] - fn test_add_two_root_block_devices_configs() { - let dummy_filename_1 = String::from("test_add_two_root_block_devices_configs_1"); - let dummy_path_1 = create_dummy_path(dummy_filename_1.clone()); - let root_block_device_1 = BlockDeviceConfig { - path_on_host: dummy_path_1, - is_root_device: true, - is_read_only: false, - drive_id: String::from("1"), - rate_limiter: None, - }; - - let dummy_filename_2 = String::from("test_add_two_root_block_devices_configs_2"); - let dummy_path_2 = create_dummy_path(dummy_filename_2.clone()); - let root_block_device_2 = BlockDeviceConfig { - path_on_host: dummy_path_2, - is_root_device: true, - is_read_only: false, - drive_id: String::from("2"), - rate_limiter: None, - }; + // test adding two block devices + let dummy_filename_2 = String::from("test_root_block_device_add2"); + create_dummy_path(dummy_filename_2.clone()); + + let body = r#"{ + "drive_id": "2", + "path_on_host": "test_root_block_device_add2", + "state": "Attached", + "is_root_device": true, + "permissions": "rw" + }"#; + + let result: result::Result = serde_json::from_str(body); + assert!(result.is_ok()); + let root_block_device_2 = result.unwrap(); - let mut block_devices_configs = BlockDeviceConfigs::new(); - 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); - delete_dummy_path(dummy_filename_1); delete_dummy_path(dummy_filename_2); - - assert!(ret.is_ok()); assert_eq!(expected_error, actual_error); + + let dev_config = block_devices_configs.config_list.iter().next().unwrap(); + assert_eq!(dev_config, &root_block_device1); } #[test] /// Test BlockDevicesConfigs::add when you first add the root device and then the other devices - fn test_add_root_block_device_first() { + fn test_add_ro_root_block_device_first() { let dummy_filename_1 = String::from("test_add_root_block_device_first_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"), - rate_limiter: None, - }; + + create_dummy_path(dummy_filename_1.clone()); + let body = r#"{ + "drive_id": "1", + "path_on_host": "test_add_root_block_device_first_1", + "state": "Attached", + "is_root_device": true, + "permissions": "ro" + }"#; + + let result: result::Result = serde_json::from_str(body); + assert!(result.is_ok()); + let root_block_device = result.unwrap(); let dummy_filename_2 = String::from("test_add_root_block_device_first_2"); - let dummy_path_2 = create_dummy_path(dummy_filename_2.clone()); - let dummy_block_device_2 = BlockDeviceConfig { - path_on_host: dummy_path_2, - is_root_device: false, - is_read_only: false, - drive_id: String::from("2"), - rate_limiter: None, - }; + create_dummy_path(dummy_filename_2.clone()); + + let body = r#"{ + "drive_id": "2", + "path_on_host": "test_add_root_block_device_first_2", + "state": "Attached", + "is_root_device": false, + "permissions": "rw" + }"#; + + let result: result::Result = serde_json::from_str(body); + assert!(result.is_ok()); + let dummy_block_device_2 = result.unwrap(); let dummy_filename_3 = String::from("test_add_root_block_device_first_3"); - let dummy_path_3 = create_dummy_path(dummy_filename_3.clone()); - let dummy_block_device_3 = BlockDeviceConfig { - path_on_host: dummy_path_3, - is_root_device: false, - is_read_only: false, - drive_id: String::from("3"), - rate_limiter: None, - }; + create_dummy_path(dummy_filename_3.clone()); + + let body = r#"{ + "drive_id": "3", + "path_on_host": "test_add_root_block_device_first_3", + "state": "Attached", + "is_root_device": false, + "permissions": "rw" + }"#; + + let result: result::Result = serde_json::from_str(body); + assert!(result.is_ok()); + let dummy_block_device_3 = result.unwrap(); let mut block_devices_configs = BlockDeviceConfigs::new(); - 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()); + let res_add1 = block_devices_configs.add(root_block_device.clone()); + let res_add2 = block_devices_configs.add(dummy_block_device_2.clone()); + let res_add3 = 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!(res_add1.is_ok()); + assert!(res_add2.is_ok()); + assert!(res_add3.is_ok()); assert_eq!(block_devices_configs.has_root_block_device(), true); + assert_eq!(block_devices_configs.has_read_only_root(), true); assert_eq!(block_devices_configs.config_list.len(), 3); let mut block_dev_iter = block_devices_configs.config_list.iter(); @@ -328,50 +313,68 @@ mod tests { /// Test BlockDevicesConfigs::add when you add other devices first and then the root device fn test_root_block_device_add_last() { let dummy_filename_1 = String::from("test_root_block_device_add_last_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"), - rate_limiter: None, - }; + + create_dummy_path(dummy_filename_1.clone()); + + let body = r#"{ + "drive_id": "1", + "path_on_host": "test_root_block_device_add_last_1", + "state": "Attached", + "is_root_device": true, + "permissions": "rw" + }"#; + + let result: result::Result = serde_json::from_str(body); + assert!(result.is_ok()); + let root_block_device = result.unwrap(); let dummy_filename_2 = String::from("test_root_block_device_add_last_2"); - let dummy_path_2 = create_dummy_path(dummy_filename_2.clone()); - let dummy_block_device_2 = BlockDeviceConfig { - path_on_host: dummy_path_2, - is_root_device: false, - is_read_only: false, - drive_id: String::from("2"), - rate_limiter: None, - }; + create_dummy_path(dummy_filename_2.clone()); + let body = r#"{ + "drive_id": "2", + "path_on_host": "test_root_block_device_add_last_2", + "state": "Attached", + "is_root_device": false, + "permissions": "rw" + }"#; + + let result: result::Result = serde_json::from_str(body); + assert!(result.is_ok()); + let dummy_block_device_2 = result.unwrap(); let dummy_filename_3 = String::from("test_root_block_device_add_last_3"); - let dummy_path_3 = create_dummy_path(dummy_filename_3.clone()); - let dummy_block_device_3 = BlockDeviceConfig { - path_on_host: dummy_path_3, - is_root_device: false, - is_read_only: false, - drive_id: String::from("3"), - rate_limiter: None, - }; + create_dummy_path(dummy_filename_3.clone()); + + let body = r#"{ + "drive_id": "3", + "path_on_host": "test_root_block_device_add_last_3", + "state": "Attached", + "is_root_device": false, + "permissions": "rw" + }"#; + + let result: result::Result = serde_json::from_str(body); + assert!(result.is_ok()); + let dummy_block_device_3 = result.unwrap(); let mut block_devices_configs = BlockDeviceConfigs::new(); - 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()); + + let res_add1 = block_devices_configs.add(dummy_block_device_2.clone()); + let res_add2 = block_devices_configs.add(dummy_block_device_3.clone()); + let res_add3 = 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!(res_add1.is_ok()); + assert!(res_add2.is_ok()); + assert!(res_add3.is_ok()); assert_eq!(block_devices_configs.has_root_block_device(), true); assert_eq!(block_devices_configs.config_list.len(), 3); + assert!(block_devices_configs.contains_drive_id("1")); + assert!(!block_devices_configs.contains_drive_id("4")); let mut block_dev_iter = block_devices_configs.config_list.iter(); // The root device should be first in the list no matter of the order in which the devices were added @@ -379,75 +382,4 @@ mod tests { 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, - rate_limiter: None, - }; - - 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"), - rate_limiter: None, - }; - - 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"), - rate_limiter: None, - }; - - 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); - - 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/mod.rs b/vmm/src/device_config/mod.rs index 2a899644e7f..c7fba653d1c 100644 --- a/vmm/src/device_config/mod.rs +++ b/vmm/src/device_config/mod.rs @@ -1,5 +1,5 @@ mod drive; mod net; -pub use self::drive::{BlockDeviceConfig, BlockDeviceConfigs}; +pub use self::drive::BlockDeviceConfigs; pub use self::net::{NetworkInterfaceConfig, NetworkInterfaceConfigs}; diff --git a/vmm/src/device_config/net.rs b/vmm/src/device_config/net.rs index f1afb8485ee..031e009533d 100644 --- a/vmm/src/device_config/net.rs +++ b/vmm/src/device_config/net.rs @@ -3,8 +3,8 @@ use std::mem; use std::rc::Rc; use std::result; -use api_server::request::sync::{Error as SyncError, NetworkInterfaceBody, - OkStatus as SyncOkStatus, RateLimiterDescription}; +use api_server::request::sync::{Error as SyncError, NetworkInterfaceBody, OkStatus as SyncOkStatus}; +use data_model::device_config::RateLimiterConfig; use net_util::{MacAddr, Tap, TapError}; pub struct NetworkInterfaceConfig { @@ -19,8 +19,8 @@ pub struct NetworkInterfaceConfig { // and if so, we want to report the failure back to the API caller immediately. This is an // option, because the inner value will be moved to the actual virtio net device before boot. pub tap: Option, - pub rx_rate_limiter: Option, - pub tx_rate_limiter: Option, + pub rx_rate_limiter: Option, + pub tx_rate_limiter: Option, } impl NetworkInterfaceConfig { @@ -92,7 +92,7 @@ impl NetworkInterfaceConfigs { #[cfg(test)] mod tests { use super::*; - use api_server::request::sync::DeviceState; + use data_model::device_config::DeviceState; use net_util::MacAddr; #[test] @@ -106,8 +106,8 @@ mod tests { state: DeviceState::Attached, host_dev_name: String::from("bar"), guest_mac: Some(mac.clone()), - rx_rate_limiter: Some(RateLimiterDescription::default()), - tx_rate_limiter: Some(RateLimiterDescription::default()), + rx_rate_limiter: Some(RateLimiterConfig::default()), + tx_rate_limiter: Some(RateLimiterConfig::default()), }; assert!(netif_configs.put(netif_body.clone()).is_ok()); assert_eq!(netif_configs.if_list.len(), 1); diff --git a/vmm/src/device_manager/mmio.rs b/vmm/src/device_manager/mmio.rs index 0a11dcd848e..feafa6d5a57 100644 --- a/vmm/src/device_manager/mmio.rs +++ b/vmm/src/device_manager/mmio.rs @@ -160,8 +160,8 @@ impl MMIODeviceManager { } /// Gets the address of the specified device on the bus. - pub fn get_address(&self, id: &String) -> Option<&u64> { - return self.id_to_addr_map.get(id.as_str()); + pub fn get_address(&self, id: &str) -> Option<&u64> { + return self.id_to_addr_map.get(id); } } diff --git a/vmm/src/lib.rs b/vmm/src/lib.rs index e8fa75c3e8e..06d88d63fa9 100644 --- a/vmm/src/lib.rs +++ b/vmm/src/lib.rs @@ -35,12 +35,12 @@ use timerfd::{ClockId, SetTimeFlags, TimerFd, TimerState}; use api_server::request::async::{AsyncOutcome, AsyncOutcomeSender, AsyncRequest}; use api_server::request::instance_info::{InstanceInfo, InstanceState}; -use api_server::request::sync::{rate_limiter_description_into_implementation, - APILoggerDescription, DriveDescription, DriveError, - Error as SyncError, GenerateResponse, NetworkInterfaceBody, - OkStatus as SyncOkStatus, PutDriveOutcome, PutLoggerOutcome, +use api_server::request::sync::{APILoggerDescription, Error as SyncError, GenerateResponse, + NetworkInterfaceBody, OkStatus as SyncOkStatus, PutLoggerOutcome, SyncOutcomeSender, SyncRequest}; use api_server::ApiRequest; +use data_model::device_config::{rate_limiter_description_into_implementation, DriveConfig, + DriveError, PutDriveOutcome}; use data_model::vm::boot_source::{BootSource, BootSourceError, PutBootSourceOutcome}; use data_model::vm::{MachineConfiguration, MachineConfigurationError, PutMachineConfigurationOutcome}; @@ -447,11 +447,11 @@ impl Vmm { /// Updating before the VM has started is not allowed. pub fn put_block_device( &mut self, - block_device_config: BlockDeviceConfig, + block_device_config: DriveConfig, ) -> result::Result { // if the id of the drive already exists in the list, the operation is update if self.block_device_configs - .contains_drive_id(block_device_config.drive_id.clone()) + .contains_drive_id(block_device_config.get_id()) { return self.update_block_device(&block_device_config); } else { @@ -464,7 +464,7 @@ impl Vmm { fn update_block_device( &mut self, - block_device_config: &BlockDeviceConfig, + block_device_config: &DriveConfig, ) -> result::Result { if self.mmio_device_manager.is_some() { self.live_update_block_device(block_device_config) @@ -477,17 +477,17 @@ impl Vmm { fn live_update_block_device( &mut self, - block_device_config: &BlockDeviceConfig, + block_device_config: &DriveConfig, ) -> result::Result { // Safe to unwrap() because mmio_device_manager is initialized in init_devices(), which is // called before the guest boots, and this function is called after boot. let device_manager = self.mmio_device_manager.as_ref().unwrap(); for drive_config in self.block_device_configs.config_list.iter() { - if drive_config.drive_id == block_device_config.drive_id { - match device_manager.get_address(&drive_config.drive_id) { + if drive_config.get_id() == block_device_config.get_id() { + match device_manager.get_address(drive_config.get_id()) { Some(&address) => { - let metadata = metadata(&block_device_config.path_on_host) + let metadata = metadata(block_device_config.get_path_on_host()) .map_err(|_| DriveError::BlockDeviceUpdateFailed)?; let new_size = metadata.len(); if new_size % virtio::block::SECTOR_SIZE != 0 { @@ -584,17 +584,18 @@ impl Vmm { // adding root blk device from file let root_image = OpenOptions::new() .read(true) - .write(!drive_config.is_read_only) - .open(&drive_config.path_on_host) + .write(!drive_config.is_read_only()) + .open(drive_config.get_path_on_host()) .map_err(Error::RootDiskImage)?; let epoll_config = epoll_context.allocate_virtio_block_tokens(); let rate_limiter = rate_limiter_description_into_implementation( - drive_config.rate_limiter.as_ref(), + drive_config.get_rate_limiter(), ).map_err(Error::RateLimiterNew)?; + let block_box = Box::new(devices::virtio::Block::new( root_image, - drive_config.is_read_only, + drive_config.is_read_only(), epoll_config, rate_limiter, ).map_err(Error::RootBlockDeviceNew)?); @@ -602,7 +603,7 @@ impl Vmm { .register_device( block_box, &mut kernel_config.cmdline, - Some(drive_config.drive_id.clone()), + Some(drive_config.get_id().to_string()), ) .map_err(Error::RegisterBlock)?; } @@ -1103,8 +1104,8 @@ impl Vmm { } } - fn handle_put_drive(&mut self, drive_description: DriveDescription, sender: SyncOutcomeSender) { - match self.put_block_device(BlockDeviceConfig::from(drive_description)) { + fn handle_put_drive(&mut self, drive_description: DriveConfig, sender: SyncOutcomeSender) { + match self.put_block_device(drive_description) { Ok(ret) => sender .send(Box::new(ret)) .map_err(|_| ()) From 77f589ee0658863920fd9fe32262a7d36bd29b3f Mon Sep 17 00:00:00 2001 From: Diana Popa Date: Fri, 11 May 2018 10:24:53 -0500 Subject: [PATCH 3/7] refactoring: moving NetworkInterfaceBody to data model Signed-off-by: Diana Popa --- Cargo.toml | 1 - api_server/Cargo.toml | 1 + api_server/src/http_service.rs | 46 ++++----- api_server/src/lib.rs | 4 +- api_server/src/request/mod.rs | 3 +- api_server/src/request/sync/mod.rs | 5 +- api_server/src/request/sync/net.rs | 89 ++++++---------- data_model/Cargo.toml | 5 +- data_model/src/device_config/mod.rs | 1 + data_model/src/device_config/net.rs | 145 ++++++++++++++++++++++++++ data_model/src/lib.rs | 7 +- jailer/src/lib.rs | 4 + kvm/src/lib.rs | 2 +- net_util/Cargo.toml | 1 - net_util/src/lib.rs | 1 - net_util/src/tap.rs | 12 +-- src/main.rs | 4 +- vmm/Cargo.toml | 1 - vmm/src/device_config/drive.rs | 2 + vmm/src/device_config/mod.rs | 2 +- vmm/src/device_config/net.rs | 155 +++++++++++----------------- vmm/src/lib.rs | 16 +-- 22 files changed, 292 insertions(+), 215 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index d3c1bcd9e9b..100be39d3be 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -7,7 +7,6 @@ authors = ["Amazon firecracker team "] clap = "=2.27.1" api_server = { path = "api_server" } -data_model = { path = "data_model" } jailer = { path = "jailer" } logger = { path = "logger"} seccomp_sys = { path = "seccomp_sys" } diff --git a/api_server/Cargo.toml b/api_server/Cargo.toml index c4d38145a55..ea9e68c2544 100644 --- a/api_server/Cargo.toml +++ b/api_server/Cargo.toml @@ -10,6 +10,7 @@ serde = "=1.0.27" serde_derive = "=1.0.27" serde_json = "=1.0.9" tokio-core = "=0.1.12" +tokio-io = "=0.1.5" tokio-uds = "=0.1.7" data_model = { path = "../data_model" } diff --git a/api_server/src/http_service.rs b/api_server/src/http_service.rs index aa2e6b76dce..09a3369dc25 100644 --- a/api_server/src/http_service.rs +++ b/api_server/src/http_service.rs @@ -12,8 +12,7 @@ use serde_json; use tokio_core::reactor::Handle; use super::{ActionMap, ActionMapValue}; - -use data_model::device_config::DriveConfig; +use data_model::device_config::{DriveConfig, NetworkInterfaceConfig}; use data_model::vm::boot_source::BootSource; use data_model::vm::MachineConfiguration; use logger::{Metric, METRICS}; @@ -308,18 +307,16 @@ fn parse_netif_req<'a>( let unwrapped_id = id_from_path.ok_or(Error::InvalidID)?; METRICS.put_api_requests.network_count.inc(); - Ok( - serde_json::from_slice::(body) - .map_err(|e| { - METRICS.put_api_requests.network_fails.inc(); - Error::SerdeJson(e) - })? - .into_parsed_request(unwrapped_id) - .map_err(|s| { - METRICS.put_api_requests.network_fails.inc(); - Error::Generic(StatusCode::BadRequest, s) - })?, - ) + Ok(serde_json::from_slice::(body) + .map_err(|e| { + METRICS.put_api_requests.network_fails.inc(); + Error::SerdeJson(e) + })? + .into_parsed_request(method, Some(unwrapped_id)) + .map_err(|s| { + METRICS.put_api_requests.network_fails.inc(); + Error::Generic(StatusCode::BadRequest, s) + })?) } _ => Err(Error::InvalidPathMethod(path, method)), } @@ -670,15 +667,14 @@ fn describe(sync: bool, method: &Method, path: &String, body: &String) -> String #[cfg(test)] mod tests { use super::*; - use data_model::device_config::DeviceState; use data_model::vm::CpuFeaturesTemplate; use fc_util::LriHashMap; use futures::sync::oneshot; use hyper::header::{ContentType, Headers}; use hyper::Body; - use net_util::MacAddr; use request::async::AsyncRequest; - use request::sync::{NetworkInterfaceBody, SyncRequest}; + use request::sync::SyncRequest; + use std::result; fn body_to_string(body: hyper::Body) -> String { let ret = body.fold(Vec::new(), |mut acc, chunk| { @@ -1129,16 +1125,14 @@ mod tests { } // PUT - let netif = NetworkInterfaceBody { - iface_id: String::from("bar"), - state: DeviceState::Attached, - host_dev_name: String::from("foo"), - guest_mac: Some(MacAddr::parse_str("12:34:56:78:9a:BC").unwrap()), - rx_rate_limiter: None, - tx_rate_limiter: None, - }; + let result: result::Result = + serde_json::from_str(json); + assert!(result.is_ok()); - match netif.into_parsed_request("bar") { + match result + .unwrap() + .into_parsed_request(Method::Put, Some("bar")) + { Ok(pr) => match parse_netif_req( &"/foo/bar"[1..].split_terminator('/').collect(), &"/foo/bar", diff --git a/api_server/src/lib.rs b/api_server/src/lib.rs index f3bb8b7e44f..407df057faa 100644 --- a/api_server/src/lib.rs +++ b/api_server/src/lib.rs @@ -86,9 +86,7 @@ impl ApiServer { let mut core = Core::new().map_err(Error::Io)?; let handle = Rc::new(core.handle()); - let listener = if data_model::FIRECRACKER_IS_JAILED - .load(std::sync::atomic::Ordering::Relaxed) - { + let listener = if jailer::FIRECRACKER_IS_JAILED.load(std::sync::atomic::Ordering::Relaxed) { // This is a UnixListener of the tokio_uds variety. Using fd inherited from the jailer. UnixListener::from_listener( unsafe { std::os::unix::net::UnixListener::from_raw_fd(jailer::LISTENER_FD) }, diff --git a/api_server/src/request/mod.rs b/api_server/src/request/mod.rs index 3cb3dd37f66..d31c54c789a 100644 --- a/api_server/src/request/mod.rs +++ b/api_server/src/request/mod.rs @@ -5,8 +5,7 @@ use std::result; pub use self::async::{AsyncOutcome, AsyncOutcomeReceiver, AsyncOutcomeSender, AsyncRequest, AsyncRequestBody}; -pub use self::sync::{APILoggerDescription, NetworkInterfaceBody, SyncOutcomeReceiver, - SyncOutcomeSender, SyncRequest}; +pub use self::sync::{APILoggerDescription, SyncOutcomeReceiver, SyncOutcomeSender, SyncRequest}; use hyper::Method; pub mod instance_info; diff --git a/api_server/src/request/sync/mod.rs b/api_server/src/request/sync/mod.rs index 6ccaaae7d1e..5c7eeeaeac9 100644 --- a/api_server/src/request/sync/mod.rs +++ b/api_server/src/request/sync/mod.rs @@ -4,7 +4,7 @@ use std::result; use futures::sync::oneshot; use hyper::{self, StatusCode}; -use data_model::device_config::{DeviceState, DriveConfig}; +use data_model::device_config::{DriveConfig, NetworkInterfaceConfig}; use data_model::vm::boot_source::BootSource; use data_model::vm::MachineConfiguration; use http_service::{empty_response, json_fault_message, json_response}; @@ -17,7 +17,6 @@ pub mod machine_configuration; mod net; pub use self::logger::{APILoggerDescription, APILoggerError, APILoggerLevel, PutLoggerOutcome}; -pub use self::net::NetworkInterfaceBody; // Unlike async requests, sync request have outcomes which implement this trait. The idea is for // each outcome to be a struct which is cheaply and quickly instantiated by the VMM thread, then @@ -53,7 +52,7 @@ pub enum SyncRequest { PutDrive(DriveConfig, SyncOutcomeSender), PutLogger(APILoggerDescription, SyncOutcomeSender), PutMachineConfiguration(MachineConfiguration, SyncOutcomeSender), - PutNetworkInterface(NetworkInterfaceBody, SyncOutcomeSender), + PutNetworkInterface(NetworkInterfaceConfig, SyncOutcomeSender), } // TODO: do we still need this? diff --git a/api_server/src/request/sync/net.rs b/api_server/src/request/sync/net.rs index d7685183b93..6e66da9a819 100644 --- a/api_server/src/request/sync/net.rs +++ b/api_server/src/request/sync/net.rs @@ -1,36 +1,22 @@ use std::result; use futures::sync::oneshot; +use hyper::Method; -use super::{DeviceState, SyncRequest}; -use data_model::device_config::RateLimiterConfig; +use data_model::device_config::NetworkInterfaceConfig; +use request::{IntoParsedRequest, ParsedRequest, SyncRequest}; -use net_util::MacAddr; -use request::ParsedRequest; - -// This struct represents the strongly typed equivalent of the json body from net iface -// related requests. -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] -pub struct NetworkInterfaceBody { - pub iface_id: String, - pub state: DeviceState, - pub host_dev_name: String, - #[serde(skip_serializing_if = "Option::is_none")] - pub guest_mac: Option, - #[serde(skip_serializing_if = "Option::is_none")] - pub rx_rate_limiter: Option, - #[serde(skip_serializing_if = "Option::is_none")] - pub tx_rate_limiter: Option, -} - -impl NetworkInterfaceBody { - pub fn into_parsed_request(self, id_from_path: &str) -> result::Result { - if id_from_path != self.iface_id { +impl IntoParsedRequest for NetworkInterfaceConfig { + fn into_parsed_request( + self, + _method: Method, + id_from_path: Option<&str>, + ) -> result::Result { + if id_from_path.is_some() && id_from_path.unwrap() != self.get_id() { return Err(String::from( "The id from the path does not match the id from the body!", )); } - let (sender, receiver) = oneshot::channel(); Ok(ParsedRequest::Sync( SyncRequest::PutNetworkInterface(self, sender), @@ -41,50 +27,39 @@ impl NetworkInterfaceBody { #[cfg(test)] mod tests { + extern crate serde_json; + use super::*; - use serde_json; #[test] - fn test_netif_into_parsed_request() { - let netif = NetworkInterfaceBody { - iface_id: String::from("foo"), - state: DeviceState::Attached, - host_dev_name: String::from("bar"), - guest_mac: Some(MacAddr::parse_str("12:34:56:78:9A:BC").unwrap()), - rx_rate_limiter: None, - tx_rate_limiter: None, - }; + fn test_network_config_into_parsed_request() { + let j = r#"{ + "iface_id": "foo", + "state": "Attached", + "host_dev_name": "bar", + "guest_mac": "12:34:56:78:9A:BC" + }"#; + let netif: NetworkInterfaceConfig = serde_json::from_str(j).unwrap(); + let netif2: NetworkInterfaceConfig = serde_json::from_str(j).unwrap(); - assert!(netif.clone().into_parsed_request("bar").is_err()); let (sender, receiver) = oneshot::channel(); assert!( netif - .clone() - .into_parsed_request("foo") + .into_parsed_request(Method::Put, Some("foo")) .eq(&Ok(ParsedRequest::Sync( - SyncRequest::PutNetworkInterface(netif, sender), + SyncRequest::PutNetworkInterface(netif2, sender), receiver ))) ); } #[test] - fn test_network_interface_body_serialization_and_deserialization() { - let netif = NetworkInterfaceBody { - iface_id: String::from("foo"), - state: DeviceState::Attached, - host_dev_name: String::from("bar"), - guest_mac: Some(MacAddr::parse_str("12:34:56:78:9A:BC").unwrap()), - rx_rate_limiter: Some(RateLimiterConfig::default()), - tx_rate_limiter: Some(RateLimiterConfig::default()), - }; - - // This is the json encoding of the netif variable. + fn test_network_config_serde() { let jstr = r#"{ "iface_id": "foo", - "host_dev_name": "bar", "state": "Attached", - "guest_mac": "12:34:56:78:9A:bc", + "host_dev_name": "bar", + "guest_mac": "12:34:56:78:9a:bc", "rx_rate_limiter": { "bandwidth": { "size": 0, "refill_time": 0 }, "ops": { "size": 0, "refill_time": 0 } @@ -95,12 +70,10 @@ mod tests { } }"#; - let x = serde_json::from_str(jstr).expect("deserialization failed."); - assert_eq!(netif, x); - - let y = serde_json::to_string(&netif).expect("serialization failed."); - let z = serde_json::from_str(y.as_ref()).expect("deserialization (2) failed."); - assert_eq!(x, z); + let x: NetworkInterfaceConfig = + serde_json::from_str(jstr).expect("deserialization failed."); + let y = serde_json::to_string(&x).expect("serialization failed."); + assert_eq!(y, String::from(jstr).replace("\n", "").replace(" ", "")); // Check that guest_mac and rate limiters are truly optional. let jstr_no_mac = r#"{ @@ -109,6 +82,6 @@ mod tests { "state": "Attached" }"#; - assert!(serde_json::from_str::(jstr_no_mac).is_ok()) + assert!(serde_json::from_str::(jstr_no_mac).is_ok()) } } diff --git a/data_model/Cargo.toml b/data_model/Cargo.toml index bd5a9674c68..e371560d58f 100644 --- a/data_model/Cargo.toml +++ b/data_model/Cargo.toml @@ -8,6 +8,9 @@ serde = "=1.0.27" serde_derive = "=1.0.27" fc_util = { path = "../fc_util" } +net_util = { path = "../net_util" } [dev-dependencies] -serde_json = "=1.0.9" \ No newline at end of file +serde_json = "=1.0.9" + + diff --git a/data_model/src/device_config/mod.rs b/data_model/src/device_config/mod.rs index 076451beaa5..95d6da2f37b 100644 --- a/data_model/src/device_config/mod.rs +++ b/data_model/src/device_config/mod.rs @@ -3,6 +3,7 @@ mod net; mod rate_limiter; pub use device_config::drive::{DriveConfig, DriveError, DrivePermissions, PutDriveOutcome}; +pub use device_config::net::NetworkInterfaceConfig; pub use device_config::rate_limiter::{description_into_implementation as rate_limiter_description_into_implementation, RateLimiterConfig}; diff --git a/data_model/src/device_config/net.rs b/data_model/src/device_config/net.rs index 8b137891791..f726b99a375 100644 --- a/data_model/src/device_config/net.rs +++ b/data_model/src/device_config/net.rs @@ -1 +1,146 @@ +use std::result; +use super::{DeviceState, RateLimiterConfig}; +use net_util::{MacAddr, Tap, TapError}; + +// This struct represents the strongly typed equivalent of the json body from net iface +// related requests. +#[derive(Debug, Deserialize, Serialize)] +pub struct NetworkInterfaceConfig { + iface_id: String, + state: DeviceState, + host_dev_name: String, + #[serde(skip_serializing_if = "Option::is_none")] + guest_mac: Option, + #[serde(skip_serializing_if = "Option::is_none")] + rx_rate_limiter: Option, + #[serde(skip_serializing_if = "Option::is_none")] + tx_rate_limiter: Option, + #[serde(skip_serializing)] + #[serde(skip_deserializing)] + tap: Option, +} + +impl NetworkInterfaceConfig { + pub fn get_id(&self) -> &String { + &self.iface_id + } + + pub fn open_tap(&mut self) -> result::Result<(), TapError> { + match Tap::open_named(&self.host_dev_name) { + Ok(t) => { + self.tap = Some(t); + Ok(()) + } + Err(e) => Err(e), + } + } + + pub fn take_tap(&mut self) -> Option { + self.tap.take() + } + + pub fn guest_mac(&self) -> Option<&MacAddr> { + self.guest_mac.as_ref() + } + + pub fn get_rx_rate_limiter(&self) -> Option<&RateLimiterConfig> { + self.rx_rate_limiter.as_ref() + } + + pub fn get_tx_rate_limiter(&self) -> Option<&RateLimiterConfig> { + self.tx_rate_limiter.as_ref() + } +} + +impl PartialEq for NetworkInterfaceConfig { + fn eq(&self, other: &NetworkInterfaceConfig) -> bool { + let mut is_mac_equal = false; + if self.guest_mac.is_some() && other.guest_mac.is_some() { + is_mac_equal = self.guest_mac == other.guest_mac; + } else if self.guest_mac.is_none() && other.guest_mac.is_none() { + is_mac_equal = true; + } + self.iface_id == other.iface_id && self.state == other.state + && self.host_dev_name == other.host_dev_name && is_mac_equal + } +} + +#[cfg(test)] +mod tests { + extern crate serde_json; + + use super::*; + + #[test] + fn test_mac_deserialize() { + let j = r#"{ + "iface_id": "bar", + "state": "Attached", + "host_dev_name": "foo", + "guest_mac": "12:34:56:78:9a:bc" + }"#; + let result: NetworkInterfaceConfig = serde_json::from_str(j).unwrap(); + + assert_eq!( + format!("{:?}", result), "NetworkInterfaceConfig { iface_id: \"bar\", state: Attached, \ + host_dev_name: \"foo\", guest_mac: Some(MacAddr { bytes: [18, 52, 86, 120, 154, 188] }), rx_rate_limiter: None, tx_rate_limiter: None, tap: None }", + ); + + let result = serde_json::to_string(&result); + assert!(result.is_ok()); + assert_eq!( + result.unwrap(), + String::from(j).replace("\n", "").replace(" ", "") + ); + + let j = r#"{ + "iface_id": "bar", + "state\": "Attached", + "host_dev_name": "foo", + "guest_mac": "12:34:56:78:9A:B" + }"#; + let result: Result = serde_json::from_str(j); + assert!(result.is_err()); + + let j = r#"{ + "iface_id": "bar", + "state": "Attached", + "host_dev_name": \"foo", + "guest_mac": "12:34:56:78:9A-BC" + }"#; + let result: Result = serde_json::from_str(j); + assert!(result.is_err()); + + let j = r#"{ + "iface_id": "bar", + "state": "Attached", + "host_dev_name": "foo", + "guest_mac": "12:34:56:78:9A" + }"#; + let result: Result = serde_json::from_str(j); + assert!(result.is_err()); + + let j = r#"{ + "iface_id": "bar", + "state": "Attached", + "host_dev_name": "foo", + "guest_mac": "12:34:56:78:9a:bc" + }"#; + let result: Result = serde_json::from_str(j); + assert!(result.is_ok()); + + // test serialization + let y = serde_json::to_string(&result.unwrap()).expect("serialization failed."); + assert_eq!(String::from(j).replace("\n", "").replace(" ", ""), y); + + // Check that guest_mac is truly optional. + let jstr_no_mac = r#"{ + "iface_id": "foo", + "host_dev_name": "bar", + "state": "Attached" + }"#; + + assert!(serde_json::from_str::(jstr_no_mac).is_ok()) + } +} diff --git a/data_model/src/lib.rs b/data_model/src/lib.rs index 2a7a049ea24..78dfdb07b16 100644 --- a/data_model/src/lib.rs +++ b/data_model/src/lib.rs @@ -1,12 +1,9 @@ extern crate serde; #[macro_use] extern crate serde_derive; + extern crate fc_util; +extern crate net_util; pub mod device_config; pub mod vm; - -use std::sync::atomic::{AtomicBool, ATOMIC_BOOL_INIT}; - -// ATOMIC_BOOL_INIT = false -pub static FIRECRACKER_IS_JAILED: AtomicBool = ATOMIC_BOOL_INIT; diff --git a/jailer/src/lib.rs b/jailer/src/lib.rs index 30d5377bcc5..460eea9acb2 100644 --- a/jailer/src/lib.rs +++ b/jailer/src/lib.rs @@ -13,6 +13,7 @@ use std::os::unix::io::AsRawFd; use std::os::unix::net::UnixListener; use std::path::PathBuf; use std::result; +use std::sync::atomic::{AtomicBool, ATOMIC_BOOL_INIT}; use env::Env; @@ -22,6 +23,9 @@ pub const LISTENER_FD: i32 = 5; const SOCKET_FILE_NAME: &str = "api.socket"; +// ATOMIC_BOOL_INIT = false +pub static FIRECRACKER_IS_JAILED: AtomicBool = ATOMIC_BOOL_INIT; + #[derive(Debug)] pub enum Error { Canonicalize(PathBuf, io::Error), diff --git a/kvm/src/lib.rs b/kvm/src/lib.rs index b7dacf5da1a..bb8f308f2ad 100644 --- a/kvm/src/lib.rs +++ b/kvm/src/lib.rs @@ -44,7 +44,7 @@ pub struct Kvm { impl Kvm { /// Opens `/dev/kvm/` and returns a Kvm object on success. pub fn new() -> Result { - let ret = if data_model::FIRECRACKER_IS_JAILED.load(std::sync::atomic::Ordering::Relaxed) { + let ret = if jailer::FIRECRACKER_IS_JAILED.load(std::sync::atomic::Ordering::Relaxed) { // /dev/kvm fd inherited from the jailer. jailer::KVM_FD } else { diff --git a/net_util/Cargo.toml b/net_util/Cargo.toml index 9d69f7bf1ea..c65bc7e87ca 100644 --- a/net_util/Cargo.toml +++ b/net_util/Cargo.toml @@ -7,7 +7,6 @@ authors = ["The Chromium OS Authors"] libc = ">=0.2.39" serde = "=1.0.27" -data_model = { path = "../data_model" } jailer = { path = "../jailer" } net_sys = { path = "../net_sys" } sys_util = { path = "../sys_util" } diff --git a/net_util/src/lib.rs b/net_util/src/lib.rs index ce0d0a5679e..728c7547cd4 100644 --- a/net_util/src/lib.rs +++ b/net_util/src/lib.rs @@ -10,7 +10,6 @@ extern crate lazy_static; extern crate libc; extern crate serde; -extern crate data_model; extern crate jailer; extern crate net_sys; extern crate sys_util; diff --git a/net_util/src/tap.rs b/net_util/src/tap.rs index 63d3416b583..974c1af0602 100644 --- a/net_util/src/tap.rs +++ b/net_util/src/tap.rs @@ -8,8 +8,9 @@ use std::net; use std::os::raw::*; use std::os::unix::io::{AsRawFd, FromRawFd, RawFd}; -use super::{create_sockaddr, create_socket, Error as NetUtilError}; use libc; + +use super::{create_sockaddr, create_socket, Error as NetUtilError}; use net_sys; use sys_util::{ioctl_with_mut_ref, ioctl_with_ref, ioctl_with_val}; @@ -26,7 +27,7 @@ pub enum Error { InvalidIfname, } -pub type Result = ::std::result::Result; +type Result = ::std::result::Result; /// Handle for a network tap interface. /// @@ -60,11 +61,10 @@ fn build_terminated_if_name(if_name: &str) -> Result> { } impl Tap { - pub fn open_named(if_name: &str) -> Result { + pub fn open_named(if_name: &String) -> Result { let terminated_if_name = build_terminated_if_name(if_name)?; - let fd = if ::data_model::FIRECRACKER_IS_JAILED.load(::std::sync::atomic::Ordering::Relaxed) - { + let fd = if ::jailer::FIRECRACKER_IS_JAILED.load(::std::sync::atomic::Ordering::Relaxed) { // This is the /dev/net/tun fd inherited from the jailer. ::jailer::DEV_NET_TUN_FD } else { @@ -114,7 +114,7 @@ impl Tap { /// Create a new tap interface. pub fn new() -> Result { - Self::open_named("vmtap%d") + Self::open_named(&String::from("vmtap%d")) } /// Set the host-side IP address for the tap interface. diff --git a/src/main.rs b/src/main.rs index ed38f1fdec0..a7501062446 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2,7 +2,7 @@ extern crate clap; extern crate api_server; -extern crate data_model; +extern crate jailer; #[macro_use] extern crate logger; extern crate seccomp_sys; @@ -63,7 +63,7 @@ fn main() { .unwrap(); if cmd_arguments.is_present("jailed") { - data_model::FIRECRACKER_IS_JAILED.store(true, std::sync::atomic::Ordering::Relaxed); + jailer::FIRECRACKER_IS_JAILED.store(true, std::sync::atomic::Ordering::Relaxed); } let shared_info = Arc::new(RwLock::new(InstanceInfo { diff --git a/vmm/Cargo.toml b/vmm/Cargo.toml index f2a7850369f..a5149ce64a6 100644 --- a/vmm/Cargo.toml +++ b/vmm/Cargo.toml @@ -16,7 +16,6 @@ kernel_loader = { path = "../kernel_loader" } kvm = { path = "../kvm" } kvm_sys = { path = "../kvm_sys" } logger = { path = "../logger" } -net_util = { path = "../net_util"} sys_util = { path = "../sys_util" } x86_64 = { path = "../x86_64" } diff --git a/vmm/src/device_config/drive.rs b/vmm/src/device_config/drive.rs index 41c2554f500..7dfd0b92fa7 100644 --- a/vmm/src/device_config/drive.rs +++ b/vmm/src/device_config/drive.rs @@ -174,6 +174,7 @@ mod tests { let mut block_devices_configs = BlockDeviceConfigs::new(); create_dummy_path(dummy_filename.clone()); + let add_result = block_devices_configs.add(dummy_block_device.clone()); delete_dummy_path(dummy_filename); assert!(add_result.is_ok()); @@ -202,6 +203,7 @@ mod tests { let result: result::Result = serde_json::from_str(body); assert!(result.is_ok()); let root_block_device1 = result.unwrap(); + let add_result = block_devices_configs.add(root_block_device1.clone()); delete_dummy_path(dummy_filename1); assert!(add_result.is_ok()); diff --git a/vmm/src/device_config/mod.rs b/vmm/src/device_config/mod.rs index c7fba653d1c..31fdcfb400d 100644 --- a/vmm/src/device_config/mod.rs +++ b/vmm/src/device_config/mod.rs @@ -2,4 +2,4 @@ mod drive; mod net; pub use self::drive::BlockDeviceConfigs; -pub use self::net::{NetworkInterfaceConfig, NetworkInterfaceConfigs}; +pub use self::net::NetworkInterfaceConfigs; diff --git a/vmm/src/device_config/net.rs b/vmm/src/device_config/net.rs index 031e009533d..de77b110a9d 100644 --- a/vmm/src/device_config/net.rs +++ b/vmm/src/device_config/net.rs @@ -1,59 +1,8 @@ use std::collections::linked_list::{self, LinkedList}; -use std::mem; -use std::rc::Rc; use std::result; -use api_server::request::sync::{Error as SyncError, NetworkInterfaceBody, OkStatus as SyncOkStatus}; -use data_model::device_config::RateLimiterConfig; -use net_util::{MacAddr, Tap, TapError}; - -pub struct NetworkInterfaceConfig { - // The request body received from the API side. - body: NetworkInterfaceBody, - // We extract the id from the body and hold it as a reference counted String. This should - // come in handy later on, when we'll need the id to appear in a number of data structures - // to implement efficient lookup, update, deletion, etc. - id: Rc, - // We open the tap that will be associated with the virtual device as soon as the PUT request - // arrives from the API. We want to see if there are any errors associated with the operation, - // and if so, we want to report the failure back to the API caller immediately. This is an - // option, because the inner value will be moved to the actual virtio net device before boot. - pub tap: Option, - pub rx_rate_limiter: Option, - pub tx_rate_limiter: Option, -} - -impl NetworkInterfaceConfig { - pub fn try_from_body(mut body: NetworkInterfaceBody) -> result::Result { - let id = Rc::new(mem::replace(&mut body.iface_id, String::new())); - - // TODO: rework net_util stuff such that references would suffice here, instead - // of having to move things around. - let tap = Tap::open_named(body.host_dev_name.as_str())?; - - let rx_rate_limiter = body.rx_rate_limiter.take(); - let tx_rate_limiter = body.tx_rate_limiter.take(); - Ok(NetworkInterfaceConfig { - body, - id, - tap: Some(tap), - rx_rate_limiter, - tx_rate_limiter, - }) - } - - pub fn id_as_str(&self) -> &str { - self.id.as_str() - } - - pub fn take_tap(&mut self) -> Option { - self.tap.take() - } - - pub fn guest_mac(&self) -> Option<&MacAddr> { - self.body.guest_mac.as_ref() - } -} +use api_server::request::sync::{Error as SyncError, OkStatus as SyncOkStatus}; +use data_model::device_config::NetworkInterfaceConfig; pub struct NetworkInterfaceConfigs { // We use just a list for now, since we only add interfaces as this point. @@ -67,20 +16,21 @@ impl NetworkInterfaceConfigs { } } - pub fn put(&mut self, body: NetworkInterfaceBody) -> result::Result { - let cfg = NetworkInterfaceConfig::try_from_body(body).map_err(SyncError::OpenTap)?; - 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); - } + pub fn put( + &mut self, + mut body: NetworkInterfaceConfig, + ) -> result::Result { + body.open_tap().map_err(SyncError::OpenTap)?; - if cfg.guest_mac().is_some() && device_config.guest_mac() == cfg.guest_mac() { + for x in self.if_list.iter() { + if x.get_id() == body.get_id() { + return Err(SyncError::UpdateNotImplemented); + } + if x.guest_mac() == body.guest_mac() { return Err(SyncError::GuestMacAddressInUse); } } - self.if_list.push_back(cfg); + self.if_list.push_back(body); Ok(SyncOkStatus::Created) } @@ -91,41 +41,56 @@ impl NetworkInterfaceConfigs { #[cfg(test)] mod tests { + extern crate serde_json; + use super::*; - use data_model::device_config::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()), - rx_rate_limiter: Some(RateLimiterConfig::default()), - tx_rate_limiter: Some(RateLimiterConfig::default()), - }; - 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()), - rx_rate_limiter: None, - tx_rate_limiter: None, - }; - assert!(netif_configs.put(other_netif_body).is_err()); - assert_eq!(netif_configs.if_list.len(), 1); + fn test_network_interface_configs() { + let mut net_cfgs = NetworkInterfaceConfigs::new(); + + let j = r#"{ + "iface_id": "foo", + "state": "Attached", + "host_dev_name": "bar", + "guest_mac": "12:34:56:78:9A:BC" + }"#; + let netif: NetworkInterfaceConfig = serde_json::from_str(j).unwrap(); + assert_eq!(netif.get_id(), &String::from("foo")); + assert_eq!(netif.guest_mac().unwrap().to_string(), "12:34:56:78:9a:bc"); + + assert!(net_cfgs.put(netif).is_ok()); + + let j = r#"{ + "iface_id": "foo", + "state": "Attached", + "host_dev_name": "bar1", + "guest_mac": "12:34:56:78:9A:BC" + }"#; + let netif: NetworkInterfaceConfig = serde_json::from_str(j).unwrap(); + + assert_eq!( + format!("{:?}", net_cfgs.put(netif).err().unwrap()), + "UpdateNotImplemented" + ); + + let j = r#"{ + "iface_id": "foo1", + "state": "Attached", + "host_dev_name": "bar2", + "guest_mac": "12:34:56:78:9A:BC" + }"#; + let netif: NetworkInterfaceConfig = serde_json::from_str(j).unwrap(); + assert_eq!( + format!("{:?}", net_cfgs.put(netif).err().unwrap()), + "GuestMacAddressInUse" + ); + + //testing iter mut and take_tap + assert_eq!(net_cfgs.iter_mut().len(), 1); + for cfg in net_cfgs.iter_mut() { + assert!(cfg.take_tap().is_some()); + assert_eq!(cfg.take_tap().is_some(), false); } } } diff --git a/vmm/src/lib.rs b/vmm/src/lib.rs index 06d88d63fa9..ff00b423311 100644 --- a/vmm/src/lib.rs +++ b/vmm/src/lib.rs @@ -10,7 +10,7 @@ extern crate kvm; extern crate kvm_sys; #[macro_use] extern crate logger; -extern crate net_util; +//extern crate net_util; extern crate sys_util; extern crate x86_64; @@ -36,11 +36,11 @@ use timerfd::{ClockId, SetTimeFlags, TimerFd, TimerState}; use api_server::request::async::{AsyncOutcome, AsyncOutcomeSender, AsyncRequest}; use api_server::request::instance_info::{InstanceInfo, InstanceState}; use api_server::request::sync::{APILoggerDescription, Error as SyncError, GenerateResponse, - NetworkInterfaceBody, OkStatus as SyncOkStatus, PutLoggerOutcome, - SyncOutcomeSender, SyncRequest}; + OkStatus as SyncOkStatus, PutLoggerOutcome, SyncOutcomeSender, + SyncRequest}; use api_server::ApiRequest; use data_model::device_config::{rate_limiter_description_into_implementation, DriveConfig, - DriveError, PutDriveOutcome}; + DriveError, NetworkInterfaceConfig, PutDriveOutcome}; use data_model::vm::boot_source::{BootSource, BootSourceError, PutBootSourceOutcome}; use data_model::vm::{MachineConfiguration, MachineConfigurationError, PutMachineConfigurationOutcome}; @@ -614,7 +614,7 @@ impl Vmm { pub fn put_net_device( &mut self, - body: NetworkInterfaceBody, + body: NetworkInterfaceConfig, ) -> result::Result { self.network_interface_configs.put(body) } @@ -629,10 +629,10 @@ impl Vmm { let epoll_config = self.epoll_context.allocate_virtio_net_tokens(); let rx_rate_limiter = rate_limiter_description_into_implementation( - cfg.rx_rate_limiter.as_ref(), + cfg.get_rx_rate_limiter(), ).map_err(Error::RateLimiterNew)?; let tx_rate_limiter = rate_limiter_description_into_implementation( - cfg.tx_rate_limiter.as_ref(), + cfg.get_tx_rate_limiter(), ).map_err(Error::RateLimiterNew)?; if let Some(tap) = cfg.take_tap() { @@ -1222,7 +1222,7 @@ impl Vmm { fn handle_put_network_interface( &mut self, - netif_body: NetworkInterfaceBody, + netif_body: NetworkInterfaceConfig, sender: SyncOutcomeSender, ) { if self.is_instance_running() { From 614c30a6cd6d6f7c72e8adfca8164da676a87471 Mon Sep 17 00:00:00 2001 From: Diana Popa Date: Mon, 14 May 2018 04:16:50 -0500 Subject: [PATCH 4/7] refactoring: moving LoggerDescription to data model Signed-off-by: Diana Popa --- api_server/src/http_service.rs | 25 ++++++----- api_server/src/request/mod.rs | 2 +- api_server/src/request/sync/logger.rs | 60 ++++++++------------------- api_server/src/request/sync/mod.rs | 5 +-- data_model/src/vm/logger.rs | 41 ++++++++++++++++++ data_model/src/vm/mod.rs | 2 + vmm/src/api_logger_config.rs | 43 ++++++++----------- vmm/src/lib.rs | 8 ++-- vmm/src/logger_config.rs | 42 ------------------- 9 files changed, 96 insertions(+), 132 deletions(-) create mode 100644 data_model/src/vm/logger.rs delete mode 100644 vmm/src/logger_config.rs diff --git a/api_server/src/http_service.rs b/api_server/src/http_service.rs index 09a3369dc25..d0c2390ea8b 100644 --- a/api_server/src/http_service.rs +++ b/api_server/src/http_service.rs @@ -14,10 +14,11 @@ use tokio_core::reactor::Handle; use super::{ActionMap, ActionMapValue}; use data_model::device_config::{DriveConfig, NetworkInterfaceConfig}; use data_model::vm::boot_source::BootSource; +use data_model::vm::LoggerDescription; use data_model::vm::MachineConfiguration; use logger::{Metric, METRICS}; use request::instance_info::InstanceInfo; -use request::{self, ApiRequest, AsyncOutcome, AsyncRequestBody, IntoParsedRequest, ParsedRequest}; +use request::{ApiRequest, AsyncOutcome, AsyncRequestBody, IntoParsedRequest, ParsedRequest}; use sys_util::EventFd; fn build_response_base>( @@ -232,18 +233,16 @@ fn parse_logger_req<'a>( 0 if method == Method::Put => { METRICS.put_api_requests.logger_count.inc(); - Ok( - serde_json::from_slice::(body) - .map_err(|e| { - METRICS.put_api_requests.logger_fails.inc(); - Error::SerdeJson(e) - })? - .into_parsed_request() - .map_err(|s| { - METRICS.put_api_requests.logger_fails.inc(); - Error::Generic(StatusCode::BadRequest, s) - })?, - ) + Ok(serde_json::from_slice::(body) + .map_err(|e| { + METRICS.put_api_requests.logger_fails.inc(); + Error::SerdeJson(e) + })? + .into_parsed_request(method, None) + .map_err(|s| { + METRICS.put_api_requests.logger_fails.inc(); + Error::Generic(StatusCode::BadRequest, s) + })?) } _ => Err(Error::InvalidPathMethod(path, method)), } diff --git a/api_server/src/request/mod.rs b/api_server/src/request/mod.rs index d31c54c789a..b8adc423f01 100644 --- a/api_server/src/request/mod.rs +++ b/api_server/src/request/mod.rs @@ -5,7 +5,7 @@ use std::result; pub use self::async::{AsyncOutcome, AsyncOutcomeReceiver, AsyncOutcomeSender, AsyncRequest, AsyncRequestBody}; -pub use self::sync::{APILoggerDescription, SyncOutcomeReceiver, SyncOutcomeSender, SyncRequest}; +pub use self::sync::{SyncOutcomeReceiver, SyncOutcomeSender, SyncRequest}; use hyper::Method; pub mod instance_info; diff --git a/api_server/src/request/sync/logger.rs b/api_server/src/request/sync/logger.rs index 3decf619158..fb03c9f06d5 100644 --- a/api_server/src/request/sync/logger.rs +++ b/api_server/src/request/sync/logger.rs @@ -1,39 +1,16 @@ use std::result; use futures::sync::oneshot; -use hyper::{Response, StatusCode}; +use hyper::{Method, Response, StatusCode}; +use data_model::vm::{LoggerDescription, LoggerError, PutLoggerOutcome}; use http_service::{empty_response, json_fault_message, json_response}; use request::sync::GenerateResponse; -use request::{ParsedRequest, SyncRequest}; +use request::{IntoParsedRequest, ParsedRequest, SyncRequest}; -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] -pub enum APILoggerLevel { - Error, - Warning, - Info, - Debug, -} - -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] -pub struct APILoggerDescription { - pub path: String, - #[serde(skip_serializing_if = "Option::is_none")] - pub level: Option, - #[serde(skip_serializing_if = "Option::is_none")] - pub show_level: Option, - #[serde(skip_serializing_if = "Option::is_none")] - pub show_log_origin: Option, -} - -#[derive(Debug)] -pub enum APILoggerError { - InitializationFailure(String), -} - -impl GenerateResponse for APILoggerError { +impl GenerateResponse for LoggerError { fn generate_response(&self) -> Response { - use self::APILoggerError::*; + use self::LoggerError::*; match *self { InitializationFailure(ref e) => json_response( StatusCode::BadRequest, @@ -43,11 +20,6 @@ impl GenerateResponse for APILoggerError { } } -pub enum PutLoggerOutcome { - Initialized, - Error(APILoggerError), -} - impl GenerateResponse for PutLoggerOutcome { fn generate_response(&self) -> Response { use self::PutLoggerOutcome::*; @@ -58,8 +30,12 @@ impl GenerateResponse for PutLoggerOutcome { } } -impl APILoggerDescription { - pub fn into_parsed_request(self) -> result::Result { +impl IntoParsedRequest for LoggerDescription { + fn into_parsed_request( + self, + _method: Method, + _id_from_path: Option<&str>, + ) -> result::Result { let (sender, receiver) = oneshot::channel(); Ok(ParsedRequest::Sync( SyncRequest::PutLogger(self, sender), @@ -75,7 +51,7 @@ mod tests { #[test] fn test_generate_response_logger_error() { assert_eq!( - APILoggerError::InitializationFailure("Could not initialize log system".to_string()) + LoggerError::InitializationFailure("Could not initialize log system".to_string()) .generate_response() .status(), StatusCode::BadRequest @@ -83,9 +59,7 @@ mod tests { assert!( format!( "{:?}", - APILoggerError::InitializationFailure( - "Could not initialize log system".to_string() - ) + LoggerError::InitializationFailure("Could not initialize log system".to_string()) ).contains("InitializationFailure") ); } @@ -97,7 +71,7 @@ mod tests { StatusCode::Created ); assert_eq!( - PutLoggerOutcome::Error(APILoggerError::InitializationFailure( + PutLoggerOutcome::Error(LoggerError::InitializationFailure( "Could not initialize log system".to_string() )).generate_response() .status(), @@ -107,17 +81,17 @@ mod tests { #[test] fn test_into_parsed_request() { - let desc = APILoggerDescription { + let desc = LoggerDescription { path: String::from(""), level: None, show_level: None, show_log_origin: None, }; format!("{:?}", desc); - assert!(&desc.clone().into_parsed_request().is_ok()); + assert!(&desc.clone().into_parsed_request(Method::Put, None).is_ok()); let (sender, receiver) = oneshot::channel(); assert!(&desc.clone() - .into_parsed_request() + .into_parsed_request(Method::Put, None) .eq(&Ok(ParsedRequest::Sync( SyncRequest::PutLogger(desc, sender), receiver diff --git a/api_server/src/request/sync/mod.rs b/api_server/src/request/sync/mod.rs index 5c7eeeaeac9..32d57cc84a2 100644 --- a/api_server/src/request/sync/mod.rs +++ b/api_server/src/request/sync/mod.rs @@ -6,6 +6,7 @@ use hyper::{self, StatusCode}; use data_model::device_config::{DriveConfig, NetworkInterfaceConfig}; use data_model::vm::boot_source::BootSource; +use data_model::vm::LoggerDescription; use data_model::vm::MachineConfiguration; use http_service::{empty_response, json_fault_message, json_response}; use net_util::TapError; @@ -16,8 +17,6 @@ mod logger; pub mod machine_configuration; mod net; -pub use self::logger::{APILoggerDescription, APILoggerError, APILoggerLevel, PutLoggerOutcome}; - // Unlike async requests, sync request have outcomes which implement this trait. The idea is for // each outcome to be a struct which is cheaply and quickly instantiated by the VMM thread, then // passed back the the API thread, and then unpacked into a http response using the implementation @@ -50,7 +49,7 @@ pub enum SyncRequest { GetMachineConfiguration(SyncOutcomeSender), PutBootSource(BootSource, SyncOutcomeSender), PutDrive(DriveConfig, SyncOutcomeSender), - PutLogger(APILoggerDescription, SyncOutcomeSender), + PutLogger(LoggerDescription, SyncOutcomeSender), PutMachineConfiguration(MachineConfiguration, SyncOutcomeSender), PutNetworkInterface(NetworkInterfaceConfig, SyncOutcomeSender), } diff --git a/data_model/src/vm/logger.rs b/data_model/src/vm/logger.rs new file mode 100644 index 00000000000..1cf557a52da --- /dev/null +++ b/data_model/src/vm/logger.rs @@ -0,0 +1,41 @@ +#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +pub enum LoggerLevel { + Error, + Warning, + Info, + Debug, +} + +#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +pub struct LoggerDescription { + pub path: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub level: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub show_level: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub show_log_origin: Option, +} + +#[derive(Debug)] +pub enum LoggerError { + InitializationFailure(String), +} + +pub enum PutLoggerOutcome { + Initialized, + Error(LoggerError), +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_logger_level_format() { + assert_eq!(format!("{:?}", LoggerLevel::Error), "Error"); + assert_eq!(format!("{:?}", LoggerLevel::Warning), "Warning"); + assert_eq!(format!("{:?}", LoggerLevel::Info), "Info"); + assert_eq!(format!("{:?}", LoggerLevel::Debug), "Debug"); + } +} diff --git a/data_model/src/vm/mod.rs b/data_model/src/vm/mod.rs index 8170aa2b5c7..f81b3f15a86 100644 --- a/data_model/src/vm/mod.rs +++ b/data_model/src/vm/mod.rs @@ -1,7 +1,9 @@ pub mod boot_source; +mod logger; pub mod machine_configuration; pub use vm::boot_source::{BootSource, BootSourceError}; +pub use vm::logger::{LoggerDescription, LoggerError, LoggerLevel, PutLoggerOutcome}; pub use vm::machine_configuration::CpuFeaturesTemplate; pub use vm::machine_configuration::{MachineConfiguration, MachineConfigurationError, PutMachineConfigurationOutcome}; diff --git a/vmm/src/api_logger_config.rs b/vmm/src/api_logger_config.rs index 8d9ffc577db..2621512da35 100644 --- a/vmm/src/api_logger_config.rs +++ b/vmm/src/api_logger_config.rs @@ -1,11 +1,12 @@ use std::result; -use api_server::request::sync::{APILoggerDescription, APILoggerError, APILoggerLevel}; +use data_model::vm::{LoggerDescription, LoggerError, LoggerLevel}; use logger::{Level, LOGGER}; -type Result = result::Result; +type Result = result::Result; -pub fn init_logger(api_logger: APILoggerDescription) -> Result<()> { +pub fn init_logger(api_logger: LoggerDescription) -> Result<()> { + //there are 3 things we need to get out: the level, whether to show it and whether to show the origin of the log let level = from_api_level(api_logger.level); if let Some(val) = level { @@ -21,19 +22,19 @@ pub fn init_logger(api_logger: APILoggerDescription) -> Result<()> { } if let Err(ref e) = LOGGER.init(Some(api_logger.path)) { - return Err(APILoggerError::InitializationFailure(e.to_string())); + return Err(LoggerError::InitializationFailure(e.to_string())); } else { Ok(()) } } -fn from_api_level(api_level: Option) -> Option { +fn from_api_level(api_level: Option) -> Option { if let Some(val) = api_level { match val { - APILoggerLevel::Error => Some(Level::Error), - APILoggerLevel::Warning => Some(Level::Warn), - APILoggerLevel::Info => Some(Level::Info), - APILoggerLevel::Debug => Some(Level::Debug), + LoggerLevel::Error => Some(Level::Error), + LoggerLevel::Warning => Some(Level::Warn), + LoggerLevel::Info => Some(Level::Info), + LoggerLevel::Debug => Some(Level::Debug), } } else { None @@ -43,7 +44,6 @@ fn from_api_level(api_level: Option) -> Option { #[cfg(test)] mod tests { use super::*; - use api_server::request::sync::{APILoggerDescription, APILoggerLevel}; use std::fs::{self, File}; use std::io::{BufRead, BufReader}; @@ -67,7 +67,7 @@ mod tests { #[test] fn test_init_logger_from_api() { - let desc = APILoggerDescription { + let desc = LoggerDescription { path: String::from(""), level: None, show_level: None, @@ -76,9 +76,9 @@ mod tests { assert!(init_logger(desc).is_err()); let filename = "tmp.log"; - let desc = APILoggerDescription { + let desc = LoggerDescription { path: String::from(filename), - level: Some(APILoggerLevel::Warning), + level: Some(LoggerLevel::Warning), show_level: Some(true), show_log_origin: Some(true), }; @@ -108,21 +108,12 @@ mod tests { #[test] fn test_from_api_level() { + assert_eq!(from_api_level(Some(LoggerLevel::Error)), Some(Level::Error)); assert_eq!( - from_api_level(Some(APILoggerLevel::Error)), - Some(Level::Error) - ); - assert_eq!( - from_api_level(Some(APILoggerLevel::Warning)), + from_api_level(Some(LoggerLevel::Warning)), Some(Level::Warn) ); - assert_eq!( - from_api_level(Some(APILoggerLevel::Info)), - Some(Level::Info) - ); - assert_eq!( - from_api_level(Some(APILoggerLevel::Debug)), - Some(Level::Debug) - ); + assert_eq!(from_api_level(Some(LoggerLevel::Info)), Some(Level::Info)); + assert_eq!(from_api_level(Some(LoggerLevel::Debug)), Some(Level::Debug)); } } diff --git a/vmm/src/lib.rs b/vmm/src/lib.rs index ff00b423311..7bb9d63bd40 100644 --- a/vmm/src/lib.rs +++ b/vmm/src/lib.rs @@ -35,13 +35,13 @@ use timerfd::{ClockId, SetTimeFlags, TimerFd, TimerState}; use api_server::request::async::{AsyncOutcome, AsyncOutcomeSender, AsyncRequest}; use api_server::request::instance_info::{InstanceInfo, InstanceState}; -use api_server::request::sync::{APILoggerDescription, Error as SyncError, GenerateResponse, - OkStatus as SyncOkStatus, PutLoggerOutcome, SyncOutcomeSender, - SyncRequest}; +use api_server::request::sync::{Error as SyncError, GenerateResponse, OkStatus as SyncOkStatus, + SyncOutcomeSender, SyncRequest}; use api_server::ApiRequest; use data_model::device_config::{rate_limiter_description_into_implementation, DriveConfig, DriveError, NetworkInterfaceConfig, PutDriveOutcome}; use data_model::vm::boot_source::{BootSource, BootSourceError, PutBootSourceOutcome}; +use data_model::vm::{LoggerDescription, PutLoggerOutcome}; use data_model::vm::{MachineConfiguration, MachineConfigurationError, PutMachineConfigurationOutcome}; use device_config::*; @@ -1119,7 +1119,7 @@ impl Vmm { fn handle_put_logger( &mut self, - logger_description: APILoggerDescription, + logger_description: LoggerDescription, sender: SyncOutcomeSender, ) { if self.is_instance_running() { diff --git a/vmm/src/logger_config.rs b/vmm/src/logger_config.rs deleted file mode 100644 index 4c7f7275eb6..00000000000 --- a/vmm/src/logger_config.rs +++ /dev/null @@ -1,42 +0,0 @@ -use std::result; -use api_server::request::sync::{APILoggerDescription, APILoggerError, APILoggerLevel}; -use logger::{Level, Logger}; - -type Result = result::Result; - -pub fn init_logger(api_logger: APILoggerDescription) -> Result<()> { - //there are 3 things we need to get out: the level, whether to show it and whether to show the origin of the log - let mut logger = Logger::new(); - let level = from_api_level(api_logger.level); - - if let Some(val) = level { - logger.set_level(val); - } - - if let Some(val) = api_logger.show_log_origin { - logger.set_include_origin(val, val); - } - - if let Some(val) = api_logger.show_level { - logger.set_include_level(val); - } - - if let Err(ref e) = logger.init(Some(api_logger.path)) { - return Err(APILoggerError::InitializationFailure(e.to_string())); - } else { - Ok(()) - } -} - -fn from_api_level(api_level: Option) -> Option { - if let Some(val) = api_level { - match val { - APILoggerLevel::Error => Some(Level::Error), - APILoggerLevel::Warning => Some(Level::Warn), - APILoggerLevel::Info => Some(Level::Info), - APILoggerLevel::Debug => Some(Level::Debug), - } - } else { - None - } -} From 3a4a54b4024781651fe6a4b36b6c66eea82e8609 Mon Sep 17 00:00:00 2001 From: Diana Popa Date: Mon, 14 May 2018 06:33:11 -0500 Subject: [PATCH 5/7] api_server: cleanup and additional unit tests Signed-off-by: Diana Popa --- api_server/src/lib.rs | 31 +++++++ api_server/src/request/async/mod.rs | 102 +++++++++++++++++++++++- api_server/src/request/instance_info.rs | 59 +++++++++++++- api_server/src/request/mod.rs | 1 - api_server/src/request/sync/mod.rs | 44 +++++----- 5 files changed, 210 insertions(+), 27 deletions(-) diff --git a/api_server/src/lib.rs b/api_server/src/lib.rs index 407df057faa..f9f84f4364d 100644 --- a/api_server/src/lib.rs +++ b/api_server/src/lib.rs @@ -133,3 +133,34 @@ impl ApiServer { self.efd.try_clone().map_err(Error::Eventfd) } } + +#[cfg(test)] +mod tests { + use super::*; + use request::instance_info::InstanceState; + use std::sync::mpsc::channel; + + #[test] + fn test_debug_traits_enums() { + assert!( + format!("{:?}", Error::Io(std::io::Error::from_raw_os_error(22))) + .contains("Io(Os { code: 22") + ); + assert_eq!( + format!("{:?}", Error::Eventfd(sys_util::Error::new(0))), + "Eventfd(Error(0))" + ); + } + + #[test] + fn test_api_server_init() { + let (to_vmm, _from_api) = channel(); + + let shared_info = Arc::new(RwLock::new(InstanceInfo { + state: InstanceState::Uninitialized, + })); + let server = ApiServer::new(shared_info.clone(), to_vmm, 0); + assert!(server.is_ok()); + assert!(server.unwrap().get_event_fd_clone().is_ok()); + } +} diff --git a/api_server/src/request/async/mod.rs b/api_server/src/request/async/mod.rs index 182c05e269b..45f907a53ab 100644 --- a/api_server/src/request/async/mod.rs +++ b/api_server/src/request/async/mod.rs @@ -13,7 +13,7 @@ pub enum AsyncOutcome { Error(String), } -// The halves of a request/reponse channel associated with each async request. +// The halves of a request/response channel associated with each async request. pub type AsyncOutcomeSender = oneshot::Sender; pub type AsyncOutcomeReceiver = oneshot::Receiver; @@ -124,6 +124,80 @@ mod tests { } } + #[test] + fn test_ser_deser() { + let j = "\"Drive\""; + let result: Result = serde_json::from_str(j); + assert!(result.is_ok()); + let result = serde_json::to_string(&result.unwrap()); + assert!(result.is_ok()); + assert_eq!( + result.unwrap(), + String::from(j).replace("\n", "").replace(" ", "") + ); + + let j = "{ + \"device_type\": \"Drive\", + \"device_resource_id\": \"dummy\", + \"force\": true\ + }"; + let result: InstanceDeviceDetachAction = serde_json::from_str(j).unwrap(); + assert_eq!( + format!("{:?}", result), + "InstanceDeviceDetachAction { device_type: Drive, device_resource_id: \"dummy\", force: true }" + ); + + let result = serde_json::to_string(&result); + assert!(result.is_ok()); + assert_eq!( + result.unwrap(), + String::from(j).replace("\n", "").replace(" ", "") + ); + + let j = "\"InstanceStart\""; + let result: Result = serde_json::from_str(j); + assert!(result.is_ok()); + let result = serde_json::to_string(&result.unwrap()); + assert!(result.is_ok()); + assert_eq!( + result.unwrap(), + String::from(j).replace("\n", "").replace(" ", "") + ); + + let j = "\"InstanceHalt\""; + let result: Result = serde_json::from_str(j); + assert!(result.is_ok()); + let result = serde_json::to_string(&result.unwrap()); + assert!(result.is_ok()); + assert_eq!( + result.unwrap(), + String::from(j).replace("\n", "").replace(" ", "") + ); + + let j = "{ + \"action_id\": \"dummy\", + \"action_type\": \"InstanceStart\", + \"instance_device_detach_action\": {\ + \"device_type\": \"Drive\", + \"device_resource_id\": \"dummy\", + \"force\": true}, + \"timestamp\": 1522850095\ + }"; + let result: AsyncRequestBody = serde_json::from_str(j).unwrap(); + assert_eq!( + format!("{:?}", result), + "AsyncRequestBody { action_id: \"dummy\", action_type: InstanceStart, \ + instance_device_detach_action: Some(InstanceDeviceDetachAction { device_type: Drive, \ + device_resource_id: \"dummy\", force: true }), timestamp: Some(1522850095) }" + ); + let result = serde_json::to_string(&result); + assert!(result.is_ok()); + assert_eq!( + result.unwrap(), + String::from(j).replace("\n", "").replace(" ", "") + ); + } + #[test] fn test_to_parsed_request() { let jsons = vec![ @@ -197,4 +271,30 @@ mod tests { assert_eq!(async_body.timestamp, Some(1522850096)); } + + #[test] + fn test_debug_traits_enums() { + assert_eq!(format!("{:?}", AsyncOutcome::Ok(0)), "Ok(0)"); + assert_eq!( + format!("{:?}", AsyncOutcome::Error(String::from("SomeError"))), + "Error(\"SomeError\")" + ); + let (sender, _receiver) = oneshot::channel(); + assert!( + format!("{:?}", AsyncRequest::StartInstance(sender)).contains("StartInstance(Sender") + ); + let (sender, _receiver) = oneshot::channel(); + assert!( + format!("{:?}", AsyncRequest::StopInstance(sender)).contains("StopInstance(Sender") + ); + assert_eq!(format!("{:?}", DeviceType::Drive), "Drive"); + assert_eq!( + format!("{:?}", AsyncActionType::InstanceStart), + "InstanceStart" + ); + assert_eq!( + format!("{:?}", AsyncActionType::InstanceHalt), + "InstanceHalt" + ); + } } diff --git a/api_server/src/request/instance_info.rs b/api_server/src/request/instance_info.rs index b2c74fc7b84..a5110da63f8 100644 --- a/api_server/src/request/instance_info.rs +++ b/api_server/src/request/instance_info.rs @@ -1,4 +1,4 @@ -#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +#[derive(Clone, Debug, Deserialize, Serialize)] pub enum InstanceState { Uninitialized, Starting, @@ -12,3 +12,60 @@ pub enum InstanceState { pub struct InstanceInfo { pub state: InstanceState, } + +#[cfg(test)] +mod tests { + use super::*; + use serde_json; + + fn test_ser_deser(j: &str) { + let result: Result = serde_json::from_str(j); + assert!(result.is_ok()); + let result = serde_json::to_string(&result.unwrap()); + assert!(result.is_ok()); + assert_eq!( + result.unwrap(), + String::from(j).replace("\n", "").replace(" ", "") + ); + } + + #[test] + fn test_instance_state_serde() { + test_ser_deser("\"Uninitialized\""); + test_ser_deser("\"Starting\""); + test_ser_deser("\"Running\""); + test_ser_deser("\"Halting\""); + test_ser_deser("\"Halted\""); + } + + #[test] + fn test_instance_state_debug_eq() { + assert_eq!( + format!("{:?}", InstanceState::Uninitialized), + "Uninitialized" + ); + assert_eq!(format!("{:?}", InstanceState::Starting), "Starting"); + assert_eq!(format!("{:?}", InstanceState::Running), "Running"); + assert_eq!(format!("{:?}", InstanceState::Halting), "Halting"); + assert_eq!(format!("{:?}", InstanceState::Halted), "Halted"); + } + + #[test] + fn test_instance_info() { + let j = "{\"state\": \"Uninitialized\"}"; + let result: InstanceInfo = serde_json::from_str(j).unwrap(); + assert_eq!( + format!("{:?}", result), + "InstanceInfo { state: Uninitialized }" + ); + + assert_eq!(format!("{:?}", result.state.clone()), "Uninitialized"); + + let result = serde_json::to_string(&result); + assert!(result.is_ok()); + assert_eq!( + result.unwrap(), + String::from(j).replace("\n", "").replace(" ", "") + ); + } +} diff --git a/api_server/src/request/mod.rs b/api_server/src/request/mod.rs index b8adc423f01..05d5e2c4415 100644 --- a/api_server/src/request/mod.rs +++ b/api_server/src/request/mod.rs @@ -22,7 +22,6 @@ pub enum ParsedRequest { // This enum represents a message which is passed to the VMM to request the execution // of a certain action. -#[derive(Debug)] pub enum ApiRequest { Async(AsyncRequest), Sync(SyncRequest), diff --git a/api_server/src/request/sync/mod.rs b/api_server/src/request/sync/mod.rs index 32d57cc84a2..eba633b0bac 100644 --- a/api_server/src/request/sync/mod.rs +++ b/api_server/src/request/sync/mod.rs @@ -1,4 +1,3 @@ -use std::fmt; use std::result; use futures::sync::oneshot; @@ -54,16 +53,8 @@ pub enum SyncRequest { PutNetworkInterface(NetworkInterfaceConfig, SyncOutcomeSender), } -// TODO: do we still need this? -impl fmt::Debug for SyncRequest { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "SyncRequest") - } -} - // TODO: we should move toward having both the ok status and various possible sync request errors // in this file, because there are many common sync outcomes. - pub enum OkStatus { Created, Updated, @@ -82,7 +73,6 @@ impl GenerateResponse for OkStatus { // Potential errors associated with sync requests. #[derive(Debug)] pub enum Error { - GuestCIDAlreadyInUse, GuestMacAddressInUse, OpenTap(TapError), UpdateNotAllowedPostBoot, @@ -93,17 +83,13 @@ impl GenerateResponse for Error { fn generate_response(&self) -> hyper::Response { use self::Error::*; match *self { - GuestCIDAlreadyInUse => json_response( - StatusCode::BadRequest, - json_fault_message("The specified guest CID is already in use."), - ), GuestMacAddressInUse => json_response( StatusCode::BadRequest, json_fault_message("The specified guest MAC address is already in use."), ), - OpenTap(_) => json_response( + OpenTap(ref e) => json_response( StatusCode::BadRequest, - json_fault_message("Could not open TAP device."), + json_fault_message(format!("Could not open TAP device. {:?}", e)), ), UpdateNotAllowedPostBoot => json_response( StatusCode::Forbidden, @@ -117,8 +103,6 @@ impl GenerateResponse for Error { } } -pub type Result = result::Result; - #[cfg(test)] mod tests { use super::*; @@ -165,17 +149,29 @@ mod tests { #[test] fn test_generate_response_error() { - let mut ret = Error::GuestCIDAlreadyInUse.generate_response(); - assert_eq!(ret.status(), StatusCode::BadRequest); - - ret = Error::GuestMacAddressInUse.generate_response(); + let ret = Error::GuestMacAddressInUse.generate_response(); + assert_eq!( + format!("{:?}", Error::GuestMacAddressInUse), + "GuestMacAddressInUse" + ); assert_eq!(ret.status(), StatusCode::BadRequest); - ret = Error::OpenTap(TapError::OpenTun(std::io::Error::from_raw_os_error(22))) + let ret = Error::OpenTap(TapError::OpenTun(std::io::Error::from_raw_os_error(22))) .generate_response(); + assert!( + format!( + "{:?}", + Error::OpenTap(TapError::OpenTun(std::io::Error::from_raw_os_error(22))) + ).contains("OpenTap(OpenTun(Os { code: 22"), + "OpenTap" + ); assert_eq!(ret.status(), StatusCode::BadRequest); - ret = Error::UpdateNotImplemented.generate_response(); + let ret = Error::UpdateNotImplemented.generate_response(); + assert_eq!( + format!("{:?}", Error::UpdateNotImplemented), + "UpdateNotImplemented" + ); assert_eq!(ret.status(), StatusCode::InternalServerError); } } From ba34b3edb35985bc89ea1054a20f77bbbe993bc8 Mon Sep 17 00:00:00 2001 From: Diana Popa Date: Mon, 14 May 2018 10:18:18 -0500 Subject: [PATCH 6/7] data_model: add unit tests for machine configuration Signed-off-by: Diana Popa --- api_server/src/request/sync/mod.rs | 2 +- data_model/src/vm/machine_config.rs | 30 +++++++++++++ data_model/src/vm/machine_configuration.rs | 49 ++++++++++++++++++++++ data_model/src/vm/mod.rs | 2 +- 4 files changed, 81 insertions(+), 2 deletions(-) create mode 100644 data_model/src/vm/machine_config.rs diff --git a/api_server/src/request/sync/mod.rs b/api_server/src/request/sync/mod.rs index eba633b0bac..9fe2a4d6ad9 100644 --- a/api_server/src/request/sync/mod.rs +++ b/api_server/src/request/sync/mod.rs @@ -13,7 +13,7 @@ use net_util::TapError; pub mod boot_source; mod drive; mod logger; -pub mod machine_configuration; +mod machine_configuration; mod net; // Unlike async requests, sync request have outcomes which implement this trait. The idea is for diff --git a/data_model/src/vm/machine_config.rs b/data_model/src/vm/machine_config.rs new file mode 100644 index 00000000000..d20309303bc --- /dev/null +++ b/data_model/src/vm/machine_config.rs @@ -0,0 +1,30 @@ +#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +pub struct MachineConfiguration { + #[serde(skip_serializing_if = "Option::is_none")] + pub vcpu_count: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub mem_size_mib: Option, +} + +impl Default for MachineConfiguration { + fn default() -> Self { + MachineConfiguration { + vcpu_count: Some(1), + mem_size_mib: Some(128), + } + } +} + +#[derive(Debug)] +pub enum PutMachineConfigurationError { + InvalidVcpuCount, + InvalidMemorySize, +} + +pub enum PutMachineConfigurationOutcome { + Created, + Updated, + Error(PutMachineConfigurationError), +} + + diff --git a/data_model/src/vm/machine_configuration.rs b/data_model/src/vm/machine_configuration.rs index d3c4770104d..87f76191b63 100644 --- a/data_model/src/vm/machine_configuration.rs +++ b/data_model/src/vm/machine_configuration.rs @@ -49,3 +49,52 @@ pub enum PutMachineConfigurationOutcome { Updated, Error(MachineConfigurationError), } + +#[cfg(test)] +mod tests { + extern crate serde_json; + + use super::*; + + #[test] + fn test_machine_config_default() { + let mcfg = MachineConfiguration::default(); + assert_eq!(mcfg.vcpu_count.unwrap(), 1); + assert_eq!(mcfg.mem_size_mib.unwrap(), 128); + assert_eq!(mcfg.ht_enabled.unwrap(), false); + + let j = r#"{ + "vcpu_count": 1, + "mem_size_mib": 128, + "ht_enabled": false + }"#; + let result: MachineConfiguration = serde_json::from_str(j).unwrap(); + + assert_eq!( + format!("{:?}", result.clone()), + "MachineConfiguration { vcpu_count: Some(1), mem_size_mib: Some(128), ht_enabled: Some(false), cpu_template: None }", + ); + + assert_eq!(result, mcfg); + + let result = serde_json::to_string(&result); + assert!(result.is_ok()); + assert_eq!( + result.unwrap(), + String::from(j).replace("\n", "").replace(" ", "") + ); + } + + #[test] + fn test_machine_config_error() { + assert_eq!( + format!("{:?}", MachineConfigurationError::InvalidVcpuCount), + "InvalidVcpuCount" + ); + assert_eq!( + format!("{:?}", MachineConfigurationError::InvalidMemorySize), + "InvalidMemorySize" + ); + } + +} diff --git a/data_model/src/vm/mod.rs b/data_model/src/vm/mod.rs index f81b3f15a86..5742978602c 100644 --- a/data_model/src/vm/mod.rs +++ b/data_model/src/vm/mod.rs @@ -1,6 +1,6 @@ pub mod boot_source; mod logger; -pub mod machine_configuration; +mod machine_configuration; pub use vm::boot_source::{BootSource, BootSourceError}; pub use vm::logger::{LoggerDescription, LoggerError, LoggerLevel, PutLoggerOutcome}; From 7e84470044fbd14a1200bd3ca8ab6d21ea3297a4 Mon Sep 17 00:00:00 2001 From: Diana Popa Date: Tue, 15 May 2018 06:10:36 -0500 Subject: [PATCH 7/7] ci: adding integration tests for... testing that specifying a different ids in the path and in the body triggers error. Signed-off-by: Diana Popa --- tests/functional/test_api.py | 56 ++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/tests/functional/test_api.py b/tests/functional/test_api.py index 761f5f43898..57ec3cfcfdc 100644 --- a/tests/functional/test_api.py +++ b/tests/functional/test_api.py @@ -155,6 +155,7 @@ def test_api_put_update_pre_boot(test_microvm_any): """ Valid updates to the network `host_dev_name` are allowed. """ assert(test_microvm.api_session.is_good_response(response.status_code)) + def test_api_put_machine_config(test_microvm_any): """Tests various scenarios for PUT on /machine_config that cannot be covered by the unit tests""" @@ -187,6 +188,7 @@ def test_api_put_machine_config(test_microvm_any): ) assert response.status_code == 400 + def test_api_put_update_post_boot(test_microvm_any): """ Tests that PUT updates are rejected after the microvm boots. """ @@ -289,3 +291,57 @@ def _test_default_block_devices(test_microvm): } ) assert(test_microvm.api_session.is_good_response(response.status_code)) + + +def test_api_id_mismatches(test_microvm_any): + """ Tests that the requests requiring an ID in their path is the same with the one from the body. """ + test_microvm = test_microvm_any + + # testing with same id first for net + response = test_microvm.api_session.put( + test_microvm.net_cfg_url + '/1', + json={ + 'iface_id': '1', + 'host_dev_name': test_microvm.slot.make_tap(name='mismatch_tap1'), + 'guest_mac': '06:00:00:00:00:01', + 'state': 'Attached' + } + ) + assert(test_microvm.api_session.is_good_response(response.status_code)) + + response = test_microvm.api_session.put( + test_microvm.net_cfg_url + '/2', + json={ + 'iface_id': '3', + 'host_dev_name': test_microvm.slot.make_tap(name='mismatch_tap2'), + 'guest_mac': '06:00:00:00:00:01', + 'state': 'Attached' + } + ) + assert(not test_microvm.api_session.is_good_response(response.status_code)) + + # testing with same id for drive + response = test_microvm.api_session.put( + test_microvm.blk_cfg_url + '/1', + json={ + 'drive_id': '1', + 'path_on_host': test_microvm.slot.make_fsfile(name='mismatch_drive_1'), + 'is_root_device': False, + 'permissions': 'rw', + 'state': 'Attached' + } + ) + assert(test_microvm.api_session.is_good_response(response.status_code)) + + # testing with different id for drive + response = test_microvm.api_session.put( + test_microvm.blk_cfg_url + '/2', + json={ + 'drive_id': '3', + 'path_on_host': test_microvm.slot.make_fsfile(name='mismatch_drive_2'), + 'is_root_device': False, + 'permissions': 'rw', + 'state': 'Attached' + } + ) + assert(not test_microvm.api_session.is_good_response(response.status_code))