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:
oxidecomputer/omicron#6726 (comment)

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

@@ -343,14 +343,7 @@ async fn instance_get_common(
     rqctx: &RequestContext<Arc<DropshotEndpointContext>>,
 ) -> Result<api::InstanceSpecGetResponse, HttpError> {
     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.

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<InstanceSpecGetResponse, VmError> {
+    ///
+    /// # Returns
+    ///
+    /// - `Some` if the VM has been created.
+    /// - `None` if no VM has ever been created.
+    pub(super) async fn get(&self) -> Option<InstanceSpecGetResponse> {
         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<InstanceStateRx, VmError> {
+    ///
+    /// # Returns
+    ///
+    /// - `Some` if the VM has been created.
+    /// - `None` if no VM has ever been created.
+    pub(super) async fn state_watcher(&self) -> Option<InstanceStateRx> {
         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())
+            }
         }
     }