From 81de4226686733eb5ba01de1a71bd04de1b20207 Mon Sep 17 00:00:00 2001 From: Eliza Weisman Date: Mon, 30 Sep 2024 13:29:26 -0700 Subject: [PATCH] Less anxiety-inducing `Vm::{get, state_watcher}` Currently, the `Vm::get()` and `Vm::state_watcher()` methods return a `Result<{something}, VmError>`. This means that they can return _any_ `VmError` variant, but the current implementation of these methods will only ever return `VmError::NotCreated`. This gave me a bit of anxiety when I noticed that we were presently handling the `VmError::WaitingForInit` variant identically to `NotCreated`, which seemed wrong as it implied that we could, potentially, return an error code indicating that no instance has ever been ensured when in fact, one _has_ been ensured but is still being initialized. I freaked out about this a bit here: https://github.com/oxidecomputer/omicron/pull/6726#discussion_r1781676019 It turns out the current behavior is actually fine, since if the VM is still initializing, we still allow accessing it and just return a `Starting` state, which is correct. But the type signatures of these functions allow them to return any of these errors, and forces their callers to handle those errors, and that's the part we were doing somewhat incorrectly (although, again, in practice it doesn't matter). This commit changes those functions to return an `Option` rather than a `Result`. Now, we return `None` if no VM has ever been created, making that the _only_ error case that callers have to handle. There's no longer any risk of the HTTP API handlers accidentally returning a "VM never ensured" error when the VM is ensured-but-initializing. Also, we can remove the `NotCreated` variant, since it's now no longer returned anywhere. --- bin/propolis-server/src/lib/server.rs | 31 ++++--------------------- bin/propolis-server/src/lib/vm/mod.rs | 33 ++++++++++++++++----------- 2 files changed, 24 insertions(+), 40 deletions(-) diff --git a/bin/propolis-server/src/lib/server.rs b/bin/propolis-server/src/lib/server.rs index ae44f8248..48b889f44 100644 --- a/bin/propolis-server/src/lib/server.rs +++ b/bin/propolis-server/src/lib/server.rs @@ -343,14 +343,7 @@ async fn instance_get_common( rqctx: &RequestContext>, ) -> Result { let ctx = rqctx.context(); - ctx.vm.get().await.map_err(|e| match e { - VmError::NotCreated | VmError::WaitingToInitialize => { - not_created_error() - } - _ => HttpError::for_internal_error(format!( - "unexpected error from VM controller: {e}" - )), - }) + ctx.vm.get().await.ok_or_else(not_created_error) } #[endpoint { @@ -393,14 +386,7 @@ async fn instance_state_monitor( let ctx = rqctx.context(); let gen = request.into_inner().gen; let mut state_watcher = - ctx.vm.state_watcher().await.map_err(|e| match e { - VmError::NotCreated | VmError::WaitingToInitialize => { - not_created_error() - } - _ => HttpError::for_internal_error(format!( - "unexpected error from VM controller: {e}" - )), - })?; + ctx.vm.state_watcher().await.ok_or_else(not_created_error)?; loop { let last = state_watcher.borrow().clone(); @@ -440,9 +426,7 @@ async fn instance_state_put( .put_state(requested_state) .map(|_| HttpResponseUpdatedNoContent {}) .map_err(|e| match e { - VmError::NotCreated | VmError::WaitingToInitialize => { - not_created_error() - } + VmError::WaitingToInitialize => not_created_error(), VmError::ForbiddenStateChange(reason) => HttpError::for_status( Some(format!("instance state change not allowed: {}", reason)), hyper::StatusCode::FORBIDDEN, @@ -614,14 +598,7 @@ async fn instance_migrate_status( .state_watcher() .await .map(|rx| HttpResponseOk(rx.borrow().migration.clone())) - .map_err(|e| match e { - VmError::NotCreated | VmError::WaitingToInitialize => { - not_created_error() - } - _ => HttpError::for_internal_error(format!( - "unexpected error from VM controller: {e}" - )), - }) + .ok_or_else(not_created_error) } /// Issues a snapshot request to a crucible backend. diff --git a/bin/propolis-server/src/lib/vm/mod.rs b/bin/propolis-server/src/lib/vm/mod.rs index 87364fa36..fea20119f 100644 --- a/bin/propolis-server/src/lib/vm/mod.rs +++ b/bin/propolis-server/src/lib/vm/mod.rs @@ -165,9 +165,6 @@ pub(crate) enum VmError { #[error("VM operation result channel unexpectedly closed")] ResultChannelClosed, - #[error("VM not created")] - NotCreated, - #[error("VM is currently initializing")] WaitingToInitialize, @@ -317,18 +314,23 @@ impl Vm { /// Returns the state, properties, and instance spec for the instance most /// recently wrapped by this `Vm`. - pub(super) async fn get(&self) -> Result { + /// + /// # Returns + /// + /// - `Some` if the VM has been created. + /// - `None` if no VM has ever been created. + pub(super) async fn get(&self) -> Option { let guard = self.inner.read().await; match &guard.state { // If no VM has ever been created, there's nothing to get. - VmState::NoVm => Err(VmError::NotCreated), + VmState::NoVm => None, // If the VM is active, pull the required data out of its objects. VmState::Active(vm) => { let spec = vm.objects().lock_shared().await.instance_spec().clone(); let state = vm.external_state_rx.borrow().clone(); - Ok(InstanceSpecGetResponse { + Some(InstanceSpecGetResponse { properties: vm.properties.clone(), spec: VersionedInstanceSpec::V0(spec.into()), state: state.state, @@ -340,7 +342,7 @@ impl Vm { // machine. VmState::WaitingForInit(vm) | VmState::Rundown(vm) - | VmState::RundownComplete(vm) => Ok(InstanceSpecGetResponse { + | VmState::RundownComplete(vm) => Some(InstanceSpecGetResponse { properties: vm.properties.clone(), state: vm.external_state_rx.borrow().state, spec: VersionedInstanceSpec::V0(vm.spec.clone().into()), @@ -350,16 +352,21 @@ impl Vm { /// Yields a handle to the most recent instance state receiver wrapped by /// this `Vm`. - pub(super) async fn state_watcher( - &self, - ) -> Result { + /// + /// # Returns + /// + /// - `Some` if the VM has been created. + /// - `None` if no VM has ever been created. + pub(super) async fn state_watcher(&self) -> Option { let guard = self.inner.read().await; match &guard.state { - VmState::NoVm => Err(VmError::NotCreated), - VmState::Active(vm) => Ok(vm.external_state_rx.clone()), + VmState::NoVm => None, + VmState::Active(vm) => Some(vm.external_state_rx.clone()), VmState::WaitingForInit(vm) | VmState::Rundown(vm) - | VmState::RundownComplete(vm) => Ok(vm.external_state_rx.clone()), + | VmState::RundownComplete(vm) => { + Some(vm.external_state_rx.clone()) + } } }