From 0ce53445e898829b0e93dbfe3f411dcad3e2caaa Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Thu, 23 Nov 2023 12:30:13 +0000 Subject: [PATCH] fix: Give build_microvm_from_requests a proper error type Using FcExitCode as an error type is undesirable, as it allows us to construct Err(FcExitCode::Ok), e.g. an object that says "error: everything's okay!". This is confusing and has caused problems in different contexts before, so replace FcExitCode with a proper error type here. Signed-off-by: Patrick Roy --- src/firecracker/src/api_server_adapter.rs | 9 +++-- src/vmm/src/rpc_interface.rs | 40 +++++++++++++---------- 2 files changed, 28 insertions(+), 21 deletions(-) diff --git a/src/firecracker/src/api_server_adapter.rs b/src/firecracker/src/api_server_adapter.rs index 3f008658a229..f529bb60038b 100644 --- a/src/firecracker/src/api_server_adapter.rs +++ b/src/firecracker/src/api_server_adapter.rs @@ -15,14 +15,17 @@ use utils::eventfd::EventFd; use vmm::logger::{error, warn, ProcessTimeReporter}; use vmm::resources::VmResources; use vmm::rpc_interface::{ - ApiRequest, ApiResponse, PrebootApiController, RuntimeApiController, VmmAction, + ApiRequest, ApiResponse, BuildMicrovmFromRequestsError, PrebootApiController, + RuntimeApiController, VmmAction, }; use vmm::vmm_config::instance_info::InstanceInfo; use vmm::{EventManager, FcExitCode, Vmm}; #[derive(Debug, thiserror::Error, displaydoc::Display)] pub enum ApiServerError { - /// MicroVMStopped with an error: {0:?} + /// Failed to build MicroVM: {0}. + BuildMicroVmError(BuildMicrovmFromRequestsError), + /// MicroVM stopped with an error: {0:?} MicroVMStoppedWithError(FcExitCode), /// Failed to open the API socket at: {0}. Check that it is not already used. FailedToBindSocket(String), @@ -222,7 +225,7 @@ pub(crate) fn run_with_api( mmds_size_limit, metadata_json, ) - .map_err(ApiServerError::MicroVMStoppedWithError), + .map_err(ApiServerError::BuildMicroVmError), }; let result = build_result.and_then(|(vm_resources, vmm)| { diff --git a/src/vmm/src/rpc_interface.rs b/src/vmm/src/rpc_interface.rs index d0f5da8a444a..9d343b4ee490 100644 --- a/src/vmm/src/rpc_interface.rs +++ b/src/vmm/src/rpc_interface.rs @@ -20,7 +20,7 @@ use super::{ }; use crate::builder::StartMicrovmError; use crate::cpu_config::templates::{CustomCpuTemplate, GuestConfigError}; -use crate::logger::{error, info, warn, LoggerConfig, *}; +use crate::logger::{info, warn, LoggerConfig, *}; use crate::mmds::data_store::{self, Mmds}; use crate::persist::{CreateSnapshotError, RestoreFromSnapshotError, VmInfo}; use crate::resources::VmmConfig; @@ -42,7 +42,7 @@ use crate::vmm_config::net::{ use crate::vmm_config::snapshot::{CreateSnapshotParams, LoadSnapshotParams, SnapshotType}; use crate::vmm_config::vsock::{VsockConfigError, VsockDeviceConfig}; use crate::vmm_config::{self, RateLimiterUpdate}; -use crate::{EventManager, FcExitCode}; +use crate::EventManager; /// This enum represents the public interface of the VMM. Each action contains various /// bits of information (ids, paths, etc.). @@ -247,7 +247,7 @@ pub struct PrebootApiController<'a> { boot_path: bool, // Some PrebootApiRequest errors are irrecoverable and Firecracker // should cleanly teardown if they occur. - fatal_error: Option, + fatal_error: Option, } // TODO Remove when `EventManager` implements `std::fmt::Debug`. @@ -287,6 +287,17 @@ pub type ApiRequest = Box; /// Shorthand type for a response containing a boxed Result. pub type ApiResponse = Box>; +/// Error type for `PrebootApiController::build_microvm_from_requests`. +#[derive(Debug, thiserror::Error, displaydoc::Display, derive_more::From)] +pub enum BuildMicrovmFromRequestsError { + /// Populating MMDS from file failed: {0}. + Mmds(data_store::Error), + /// Loading snapshot failed. + Restore, + /// Resuming MicroVM after loading snapshot failed. + Resume, +} + impl<'a> PrebootApiController<'a> { /// Constructor for the PrebootApiController. pub fn new( @@ -320,7 +331,7 @@ impl<'a> PrebootApiController<'a> { boot_timer_enabled: bool, mmds_size_limit: usize, metadata_json: Option<&str>, - ) -> Result<(VmResources, Arc>), FcExitCode> { + ) -> Result<(VmResources, Arc>), BuildMicrovmFromRequestsError> { let mut vm_resources = VmResources::default(); // Silence false clippy warning. Clippy suggests using // VmResources { boot_timer: boot_timer_enabled, ..Default::default() }; but this will @@ -333,16 +344,9 @@ impl<'a> PrebootApiController<'a> { // Init the data store from file, if present. if let Some(data) = metadata_json { - vm_resources - .locked_mmds_or_default() - .put_data( - serde_json::from_str(data) - .expect("MMDS error: metadata provided not valid json"), - ) - .map_err(|err| { - error!("Populating MMDS from file failed: {:?}", err); - crate::FcExitCode::GenericError - })?; + vm_resources.locked_mmds_or_default().put_data( + serde_json::from_str(data).expect("MMDS error: metadata provided not valid json"), + )?; info!("Successfully added metadata to mmds from file"); } @@ -376,8 +380,8 @@ impl<'a> PrebootApiController<'a> { to_api.send(Box::new(res)).expect("one-shot channel closed"); // If any fatal errors were encountered, break the loop. - if let Some(exit_code) = preboot_controller.fatal_error { - return Err(exit_code); + if let Some(preboot_error) = preboot_controller.fatal_error { + return Err(preboot_error); } } @@ -577,7 +581,7 @@ impl<'a> PrebootApiController<'a> { ) .map_err(|err| { // If restore fails, we consider the process is too dirty to recover. - self.fatal_error = Some(FcExitCode::BadConfiguration); + self.fatal_error = Some(BuildMicrovmFromRequestsError::Restore); err })?; // Resume VM @@ -587,7 +591,7 @@ impl<'a> PrebootApiController<'a> { .resume_vm() .map_err(|err| { // If resume fails, we consider the process is too dirty to recover. - self.fatal_error = Some(FcExitCode::BadConfiguration); + self.fatal_error = Some(BuildMicrovmFromRequestsError::Resume); err })?; }