diff --git a/bin/propolis-server/src/lib/server.rs b/bin/propolis-server/src/lib/server.rs index 81bb6cf65..39dcf2d34 100644 --- a/bin/propolis-server/src/lib/server.rs +++ b/bin/propolis-server/src/lib/server.rs @@ -296,12 +296,7 @@ impl DropshotEndpointContext { self.services.vm.lock().await, VmControllerState::as_controller, ) - .map_err(|_| { - HttpError::for_not_found( - None, - "Server not initialized (no instance)".to_string(), - ) - }) + .map_err(|_| not_created_error()) } } @@ -576,19 +571,21 @@ async fn instance_ensure_common( { let existing_properties = existing.properties(); if existing_properties.id != properties.id { - return Err(HttpError::for_status( - Some(format!( + return Err(HttpError::for_client_error( + Some(api::ErrorCode::AlreadyInitialized.to_string()), + http::status::StatusCode::CONFLICT, + format!( "Server already initialized with ID {}", existing_properties.id - )), - http::status::StatusCode::CONFLICT, + ), )); } if *existing_properties != properties { - return Err(HttpError::for_status( - Some("Cannot update running server".to_string()), + return Err(HttpError::for_client_error( + Some(api::ErrorCode::AlreadyRunning.to_string()), http::status::StatusCode::CONFLICT, + "Cannot update running server".to_string(), )); } @@ -658,10 +655,11 @@ async fn instance_ensure_common( vm_hdl.await.unwrap() } .map_err(|e| { - HttpError::for_internal_error(format!( - "failed to create instance: {}", - e - )) + HttpError::for_client_error( + Some(api::ErrorCode::CreateFailed.to_string()), + http::status::StatusCode::INTERNAL_SERVER_ERROR, + format!("failed to create instance: {e}"), + ) })?; if let Some(ramfb) = vm.framebuffer() { @@ -785,10 +783,7 @@ async fn instance_get_common( ) -> Result<(api::Instance, VersionedInstanceSpec), HttpError> { let ctx = rqctx.context(); match &*ctx.services.vm.lock().await { - VmControllerState::NotCreated => Err(HttpError::for_not_found( - None, - "Server not initialized (no instance)".to_string(), - )), + VmControllerState::NotCreated => Err(not_created_error()), VmControllerState::Created(vm) => { Ok(( api::Instance { @@ -862,10 +857,7 @@ async fn instance_state_monitor( let vm_state = ctx.services.vm.lock().await; match &*vm_state { VmControllerState::NotCreated => { - return Err(HttpError::for_not_found( - None, - "Server not initialized (no instance)".to_string(), - )); + return Err(not_created_error()); } VmControllerState::Created(vm) => vm.state_watcher().clone(), VmControllerState::Destroyed { state_watcher, .. } => { @@ -885,12 +877,13 @@ async fn instance_state_monitor( // means it will never reach the number the client wants it to reach. // Inform the client of this condition so it doesn't wait forever. state_watcher.changed().await.map_err(|_| { - HttpError::for_status( - Some(format!( + HttpError::for_client_error( + Some(api::ErrorCode::NoInstance.to_string()), + http::status::StatusCode::GONE, + format!( "No instance present; will never reach generation {}", gen - )), - http::status::StatusCode::GONE, + ), ) })?; } @@ -1046,10 +1039,7 @@ async fn instance_migrate_status( let migration_id = path_params.into_inner().migration_id; let ctx = rqctx.context(); match &*ctx.services.vm.lock().await { - VmControllerState::NotCreated => Err(HttpError::for_not_found( - None, - "Server not initialized (no instance)".to_string(), - )), + VmControllerState::NotCreated => Err(not_created_error()), VmControllerState::Created(vm) => { vm.migrate_status(migration_id).map_err(Into::into).map(|state| { HttpResponseOk(api::InstanceMigrateStatusResponse { @@ -1206,6 +1196,14 @@ pub fn api() -> ApiDescription> { api } +fn not_created_error() -> HttpError { + HttpError::for_client_error( + Some(api::ErrorCode::NoInstance.to_string()), + http::StatusCode::FAILED_DEPENDENCY, + "Server not initialized (no instance)".to_string(), + ) +} + #[cfg(test)] mod tests { #[test] diff --git a/crates/propolis-api-types/src/lib.rs b/crates/propolis-api-types/src/lib.rs index 3785a0845..3725f9754 100644 --- a/crates/propolis-api-types/src/lib.rs +++ b/crates/propolis-api-types/src/lib.rs @@ -4,7 +4,7 @@ //! Definitions for types exposed by the propolis-server API -use std::net::SocketAddr; +use std::{fmt, net::SocketAddr}; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; @@ -395,3 +395,47 @@ pub struct SnapshotRequestPathParams { pub struct VCRRequestPathParams { pub id: Uuid, } + +/// Error codes used to populate the `error_code` field of Dropshot API responses. +#[derive( + Clone, Copy, Debug, Deserialize, PartialEq, Eq, Serialize, JsonSchema, +)] +pub enum ErrorCode { + /// This `propolis-server` process has not received an `InstanceEnsure` + /// request yet. + NoInstance, + /// This `propolis-server` process has already received an `InstanceEnsure` + /// request with a different ID. + AlreadyInitialized, + /// Cannot update a running server. + AlreadyRunning, + /// Instance creation failed + CreateFailed, +} + +impl fmt::Display for ErrorCode { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + fmt::Debug::fmt(self, f) + } +} + +impl std::str::FromStr for ErrorCode { + type Err = &'static str; + fn from_str(s: &str) -> Result { + match s.trim() { + s if s.eq_ignore_ascii_case("NoInstance") => Ok(Self::NoInstance), + s if s.eq_ignore_ascii_case("AlreadyInitialized") => { + Ok(ErrorCode::AlreadyInitialized) + } + s if s.eq_ignore_ascii_case("AlreadyRunning") => { + Ok(ErrorCode::AlreadyRunning) + } + s if s.eq_ignore_ascii_case("CreateFailed") => { + Ok(ErrorCode::CreateFailed) + } + _ => Err("unknown error code, expected one of: \ + 'NoInstance', 'AlreadyInitialized', 'AlreadyRunning', \ + 'CreateFailed'"), + } + } +} diff --git a/phd-tests/tests/src/server_state_machine.rs b/phd-tests/tests/src/server_state_machine.rs index b1ee13382..260e87f1c 100644 --- a/phd-tests/tests/src/server_state_machine.rs +++ b/phd-tests/tests/src/server_state_machine.rs @@ -37,15 +37,15 @@ async fn instance_stop_causes_destroy_test(ctx: &Framework) { assert!(matches!( vm.run().await.unwrap_err().status().unwrap(), - http::status::StatusCode::NOT_FOUND + http::status::StatusCode::FAILED_DEPENDENCY )); assert!(matches!( vm.stop().await.unwrap_err().status().unwrap(), - http::status::StatusCode::NOT_FOUND + http::status::StatusCode::FAILED_DEPENDENCY )); assert!(matches!( vm.reset().await.unwrap_err().status().unwrap(), - http::status::StatusCode::NOT_FOUND + http::status::StatusCode::FAILED_DEPENDENCY )); }