sled agent: index running VMMs by VMM ID, not instance ID (#6429)

Change sled agent's instance lookup tables so that Propolis jobs are
indexed by Propolis/VMM IDs instead of instance IDs.
This is a prerequisite to revisiting how the Failed instance state
works. See RFD 486 section 6.1 for all the details of why this is
needed, but very broadly: when an instance's VMM is Destroyed, we'd like
sled agent to tell Nexus that *before* the agent deregisters the
instance from the sled, for reasons described in the RFD; but if we do
that with no other changes, there's a race where Nexus may try to
restart the instance on the same sled before sled agent can update its
instance table, causing instance start to fail.

To achieve this:

- In sled agent, change the `InstanceManagerRunner`'s instance map to a
`BTreeMap<PropolisUuid, Instance>`, then clean up all the compilation
errors.
- In Nexus:
- Make callers of instance APIs furnish a Propolis ID instead of an
instance ID. This is generally very straightforward because they already
had to get a VMM record to figure out what sled to talk to.
- Change `cpapi_instances_put` to take a Propolis ID instead of an
instance ID. Regular sled agent still has both IDs, but with these
changes, simulated sled agents only have a Propolis ID to work with, and
plumbing an instance ID down to them requires significant code changes.
- Update test code:
- Unify the Nexus helper routines that let integration tests get sled
agent clients or sled IDs; now they get a single struct containing both
of those and the instance's Propolis IDs.
- Update users of the simulated agent's `poke` endpoints to use Propolis
IDs.
- Delete the "detach disks on instance stop" bits of simulated sled
agent. These don't appear to be load-bearing, they don't correspond to
any behavior in the actual sled agent (which doesn't manage disk
attachment or detachment), and it was a pain to rework them to work with
Propolis IDs.

Tests: cargo nextest.

Related: #4226 and #4872, among others.

Author: gjcolombo

clients/nexus-client/src/lib.rs

@@ -131,14 +131,11 @@ impl From<omicron_common::api::internal::nexus::VmmRuntimeState>
     }
 }
 
-impl From<omicron_common::api::internal::nexus::SledInstanceState>
-    for types::SledInstanceState
+impl From<omicron_common::api::internal::nexus::SledVmmState>
+    for types::SledVmmState
 {
-    fn from(
-        s: omicron_common::api::internal::nexus::SledInstanceState,
-    ) -> Self {
+    fn from(s: omicron_common::api::internal::nexus::SledVmmState) -> Self {
         Self {
-            propolis_id: s.propolis_id,
             vmm_state: s.vmm_state.into(),
             migration_in: s.migration_in.map(Into::into),
             migration_out: s.migration_out.map(Into::into),

clients/sled-agent-client/src/lib.rs

@@ -5,6 +5,7 @@
 //! Interface for making API requests to a Sled Agent
 
 use async_trait::async_trait;
+use omicron_uuid_kinds::PropolisUuid;
 use schemars::JsonSchema;
 use serde::Deserialize;
 use serde::Serialize;
@@ -161,12 +162,11 @@ impl From<types::VmmRuntimeState>
     }
 }
 
-impl From<types::SledInstanceState>
-    for omicron_common::api::internal::nexus::SledInstanceState
+impl From<types::SledVmmState>
+    for omicron_common::api::internal::nexus::SledVmmState
 {
-    fn from(s: types::SledInstanceState) -> Self {
+    fn from(s: types::SledVmmState) -> Self {
         Self {
-            propolis_id: s.propolis_id,
             vmm_state: s.vmm_state.into(),
             migration_in: s.migration_in.map(Into::into),
             migration_out: s.migration_out.map(Into::into),
@@ -448,33 +448,33 @@ impl From<types::SledIdentifiers>
 /// are bonus endpoints, not generated in the real client.
 #[async_trait]
 pub trait TestInterfaces {
-    async fn instance_single_step(&self, id: Uuid);
-    async fn instance_finish_transition(&self, id: Uuid);
-    async fn instance_simulate_migration_source(
+    async fn vmm_single_step(&self, id: PropolisUuid);
+    async fn vmm_finish_transition(&self, id: PropolisUuid);
+    async fn vmm_simulate_migration_source(
         &self,
-        id: Uuid,
+        id: PropolisUuid,
         params: SimulateMigrationSource,
     );
     async fn disk_finish_transition(&self, id: Uuid);
 }
 
 #[async_trait]
 impl TestInterfaces for Client {
-    async fn instance_single_step(&self, id: Uuid) {
+    async fn vmm_single_step(&self, id: PropolisUuid) {
         let baseurl = self.baseurl();
         let client = self.client();
-        let url = format!("{}/instances/{}/poke-single-step", baseurl, id);
+        let url = format!("{}/vmms/{}/poke-single-step", baseurl, id);
         client
             .post(url)
             .send()
             .await
             .expect("instance_single_step() failed unexpectedly");
     }
 
-    async fn instance_finish_transition(&self, id: Uuid) {
+    async fn vmm_finish_transition(&self, id: PropolisUuid) {
         let baseurl = self.baseurl();
         let client = self.client();
-        let url = format!("{}/instances/{}/poke", baseurl, id);
+        let url = format!("{}/vmms/{}/poke", baseurl, id);
         client
             .post(url)
             .send()
@@ -493,14 +493,14 @@ impl TestInterfaces for Client {
             .expect("disk_finish_transition() failed unexpectedly");
     }
 
-    async fn instance_simulate_migration_source(
+    async fn vmm_simulate_migration_source(
         &self,
-        id: Uuid,
+        id: PropolisUuid,
         params: SimulateMigrationSource,
     ) {
         let baseurl = self.baseurl();
         let client = self.client();
-        let url = format!("{baseurl}/instances/{id}/sim-migration-source");
+        let url = format!("{baseurl}/vmms/{id}/sim-migration-source");
         client
             .post(url)
             .json(&params)

common/src/api/internal/nexus.rs

@@ -113,13 +113,9 @@ pub struct VmmRuntimeState {
     pub time_updated: DateTime<Utc>,
 }
 
-/// A wrapper type containing a sled's total knowledge of the state of a
-/// specific VMM and the instance it incarnates.
+/// A wrapper type containing a sled's total knowledge of the state of a VMM.
 #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
-pub struct SledInstanceState {
-    /// The ID of the VMM whose state is being reported.
-    pub propolis_id: PropolisUuid,
-
+pub struct SledVmmState {
     /// The most recent state of the sled's VMM process.
     pub vmm_state: VmmRuntimeState,
 
@@ -142,7 +138,7 @@ impl Migrations<'_> {
     }
 }
 
-impl SledInstanceState {
+impl SledVmmState {
     pub fn migrations(&self) -> Migrations<'_> {
         Migrations {
             migration_in: self.migration_in.as_ref(),

nexus/db-queries/src/db/datastore/vmm.rs

@@ -5,7 +5,6 @@
 //! [`DataStore`] helpers for working with VMM records.
 
 use super::DataStore;
-use crate::authz;
 use crate::context::OpContext;
 use crate::db;
 use crate::db::error::public_error_from_diesel;
@@ -40,8 +39,13 @@ use uuid::Uuid;
 
 /// The result of an [`DataStore::vmm_and_migration_update_runtime`] call,
 /// indicating which records were updated.
-#[derive(Copy, Clone, Debug)]
+#[derive(Clone, Debug)]
 pub struct VmmStateUpdateResult {
+    /// The VMM record that the update query found and possibly updated.
+    ///
+    /// NOTE: This is the record prior to the update!
+    pub found_vmm: Vmm,
+
     /// `true` if the VMM record was updated, `false` otherwise.
     pub vmm_updated: bool,
 
@@ -108,14 +112,10 @@ impl DataStore {
     pub async fn vmm_fetch(
         &self,
         opctx: &OpContext,
-        authz_instance: &authz::Instance,
         vmm_id: &PropolisUuid,
     ) -> LookupResult<Vmm> {
-        opctx.authorize(authz::Action::Read, authz_instance).await?;
-
         let vmm = dsl::vmm
             .filter(dsl::id.eq(vmm_id.into_untyped_uuid()))
-            .filter(dsl::instance_id.eq(authz_instance.id()))
             .filter(dsl::time_deleted.is_null())
             .select(Vmm::as_select())
             .get_result_async(&*self.pool_connection_authorized(opctx).await?)
@@ -233,13 +233,21 @@ impl DataStore {
             .transaction(&conn, |conn| {
                 let err = err.clone();
                 async move {
-                let vmm_updated = self
+                let vmm_update_result = self
                     .vmm_update_runtime_on_connection(
                         &conn,
                         &vmm_id,
                         new_runtime,
                     )
-                    .await.map(|r| match r.status { UpdateStatus::Updated => true, UpdateStatus::NotUpdatedButExists => false })?;
+                    .await?;
+
+
+                let found_vmm = vmm_update_result.found;
+                let vmm_updated = match vmm_update_result.status {
+                     UpdateStatus::Updated => true,
+                     UpdateStatus::NotUpdatedButExists => false
+                };
+
                 let migration_out_updated = match migration_out {
                     Some(migration) => {
                         let r = self.migration_update_source_on_connection(
@@ -287,6 +295,7 @@ impl DataStore {
                     None => false,
                 };
                 Ok(VmmStateUpdateResult {
+                    found_vmm,
                     vmm_updated,
                     migration_in_updated,
                     migration_out_updated,

nexus/internal-api/src/lib.rs

@@ -33,14 +33,14 @@ use omicron_common::{
             DiskRuntimeState, DownstairsClientStopRequest,
             DownstairsClientStopped, ProducerEndpoint,
             ProducerRegistrationResponse, RepairFinishInfo, RepairProgress,
-            RepairStartInfo, SledInstanceState,
+            RepairStartInfo, SledVmmState,
         },
     },
     update::ArtifactId,
 };
 use omicron_uuid_kinds::{
-    DemoSagaUuid, DownstairsKind, SledUuid, TypedUuid, UpstairsKind,
-    UpstairsRepairKind,
+    DemoSagaUuid, DownstairsKind, PropolisUuid, SledUuid, TypedUuid,
+    UpstairsKind, UpstairsRepairKind,
 };
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
@@ -108,15 +108,15 @@ pub trait NexusInternalApi {
         body: TypedBody<SwitchPutRequest>,
     ) -> Result<HttpResponseOk<SwitchPutResponse>, HttpError>;
 
-    /// Report updated state for an instance.
+    /// Report updated state for a VMM.
     #[endpoint {
         method = PUT,
-        path = "/instances/{instance_id}",
+        path = "/vmms/{propolis_id}",
     }]
     async fn cpapi_instances_put(
         rqctx: RequestContext<Self::Context>,
-        path_params: Path<InstancePathParam>,
-        new_runtime_state: TypedBody<SledInstanceState>,
+        path_params: Path<VmmPathParam>,
+        new_runtime_state: TypedBody<SledVmmState>,
     ) -> Result<HttpResponseUpdatedNoContent, HttpError>;
 
     #[endpoint {
@@ -568,6 +568,12 @@ pub struct InstancePathParam {
     pub instance_id: Uuid,
 }
 
+/// Path parameters for VMM requests (internal API)
+#[derive(Deserialize, JsonSchema)]
+pub struct VmmPathParam {
+    pub propolis_id: PropolisUuid,
+}
+
 #[derive(Clone, Copy, Debug, Deserialize, JsonSchema, Serialize)]
 pub struct CollectorIdPathParams {
     /// The ID of the oximeter collector.

nexus/src/app/background/tasks/abandoned_vmm_reaper.rs

@@ -28,8 +28,8 @@
 //! remains alive and continues to own its virtual provisioning resources.
 //!
 //! Cleanup of instance resources when an instance's *active* VMM is destroyed
-//! is handled elsewhere, by `notify_instance_updated` and (eventually) the
-//! `instance-update` saga.
+//! is handled elsewhere, by `process_vmm_update` and the `instance-update`
+//! saga.
 
 use crate::app::background::BackgroundTask;
 use anyhow::Context;

nexus/src/app/background/tasks/instance_watcher.rs

@@ -19,9 +19,9 @@ use nexus_types::identity::Asset;
 use nexus_types::identity::Resource;
 use omicron_common::api::external::Error;
 use omicron_common::api::external::InstanceState;
-use omicron_common::api::internal::nexus::SledInstanceState;
+use omicron_common::api::internal::nexus::SledVmmState;
 use omicron_uuid_kinds::GenericUuid;
-use omicron_uuid_kinds::InstanceUuid;
+use omicron_uuid_kinds::PropolisUuid;
 use oximeter::types::ProducerRegistry;
 use sled_agent_client::Client as SledAgentClient;
 use std::borrow::Cow;
@@ -81,12 +81,12 @@ impl InstanceWatcher {
         let client = client.clone();
 
         async move {
-            slog::trace!(opctx.log, "checking on instance...");
-            let rsp = client
-                .instance_get_state(&InstanceUuid::from_untyped_uuid(
-                    target.instance_id,
-                ))
-                .await;
+            let vmm_id = PropolisUuid::from_untyped_uuid(target.vmm_id);
+            slog::trace!(
+                opctx.log, "checking on VMM"; "propolis_id" => %vmm_id
+            );
+
+            let rsp = client.vmm_get_state(&vmm_id).await;
             let mut check = Check {
                 target,
                 outcome: Default::default(),
@@ -151,18 +151,18 @@ impl InstanceWatcher {
                 }
             };
 
-            let new_runtime_state: SledInstanceState = state.into();
+            let new_runtime_state: SledVmmState = state.into();
             check.outcome =
                 CheckOutcome::Success(new_runtime_state.vmm_state.state.into());
             debug!(
                 opctx.log,
                 "updating instance state";
                 "state" => ?new_runtime_state.vmm_state.state,
             );
-            match crate::app::instance::notify_instance_updated(
+            match crate::app::instance::process_vmm_update(
                 &datastore,
                 &opctx,
-                InstanceUuid::from_untyped_uuid(target.instance_id),
+                PropolisUuid::from_untyped_uuid(target.vmm_id),
                 &new_runtime_state,
             )
             .await
@@ -176,7 +176,7 @@ impl InstanceWatcher {
                         _ => Err(Incomplete::UpdateFailed),
                     };
                 }
-                Ok(Some(saga)) => {
+                Ok(Some((_, saga))) => {
                     check.update_saga_queued = true;
                     if let Err(e) = sagas.saga_start(saga).await {
                         warn!(opctx.log, "update saga failed"; "error" => ?e);