Remove simulated disk stuff from sim sled-agent (#9914)

As part of looking into how expunge will affect disks backed by local
storage, I'm a few yaks deep and am looking at the work required to
change `DiskState`. Problem is that `DiskState` is in common and used by
Nexus' external and internal apis, as well as sled-agent.

Wait a minute you ask, why is `DiskState` used by the sled-agent api?
That type refers to Nexus' Disk type, which is an abstraction layer
higher than sled-agent should be aware of.

All this code is left-over from very early days of the simulated
sled-agent, and it's time it goes. This commit removes all the simulated
disk machinery from the simulated sled-agent.

So next, what part of Nexus' internal api uses `DiskState`? It's from
`cpapi_disks_put`. What calls this? The simulated disk machinery in the
simulated sled-agent notifies Nexus of state changes. This is incorrect:
Nexus should completely own the state of a Disk, and having the
simulated sled-agent poke into Nexus this way could hide real bugs.

It would be best to completely remove `cpapi_disks_put`, but until the
nexus-internal API trait can have new versions we cannot. This commit
instead replaces the implementation of that endpoint with
a 400. No part of the actual product uses this.

Happily, with all that taken out there were no failing tests!

Author: jmpesp

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

@@ -316,7 +316,6 @@ pub trait TestInterfaces {
         id: PropolisUuid,
         params: SimulateMigrationSource,
     );
-    async fn disk_finish_transition(&self, id: Uuid);
 }
 
 #[async_trait]
@@ -350,18 +349,6 @@ impl TestInterfaces for Client {
         Ok(())
     }
 
-    async fn disk_finish_transition(&self, id: Uuid) {
-        let baseurl = self.baseurl();
-        let client = self.client();
-        let url = format!("{}/disks/{}/poke", baseurl, id);
-        client
-            .post(url)
-            .api_version_header(self.api_version())
-            .send()
-            .await
-            .expect("disk_finish_transition() failed unexpectedly");
-    }
-
     async fn vmm_simulate_migration_source(
         &self,
         id: PropolisUuid,

nexus/src/app/disk.rs

@@ -27,7 +27,6 @@ use omicron_common::api::external::LookupResult;
 use omicron_common::api::external::NameOrId;
 use omicron_common::api::external::UpdateResult;
 use omicron_common::api::external::http_pagination::PaginatedBy;
-use omicron_common::api::internal::nexus::DiskRuntimeState;
 use std::sync::Arc;
 use uuid::Uuid;
 
@@ -359,63 +358,6 @@ impl super::Nexus {
         Ok(disks.into_iter().map(Into::into).collect())
     }
 
-    pub(crate) async fn notify_disk_updated(
-        &self,
-        opctx: &OpContext,
-        id: Uuid,
-        new_state: &DiskRuntimeState,
-    ) -> Result<(), Error> {
-        let log = &self.log;
-        let (.., authz_disk) = LookupPath::new(&opctx, &self.db_datastore)
-            .disk_id(id)
-            .lookup_for(authz::Action::Modify)
-            .await?;
-
-        let result = self
-            .db_datastore
-            .disk_update_runtime(opctx, &authz_disk, &new_state.clone().into())
-            .await;
-
-        // TODO-cleanup commonize with notify_instance_updated()
-        match result {
-            Ok(true) => {
-                info!(log, "disk updated by sled agent";
-                    "disk_id" => %id,
-                    "new_state" => ?new_state);
-                Ok(())
-            }
-
-            Ok(false) => {
-                info!(log, "disk update from sled agent ignored (old)";
-                    "disk_id" => %id);
-                Ok(())
-            }
-
-            // If the disk doesn't exist, swallow the error -- there's
-            // nothing to do here.
-            // TODO-robustness This could only be possible if we've removed a
-            // disk from the datastore altogether.  When would we do that?
-            // We don't want to do it as soon as something's destroyed, I think,
-            // and in that case, we'd need some async task for cleaning these
-            // up.
-            Err(Error::ObjectNotFound { .. }) => {
-                warn!(log, "non-existent disk updated by sled agent";
-                    "instance_id" => %id,
-                    "new_state" => ?new_state);
-                Ok(())
-            }
-
-            // If the datastore is unavailable, propagate that to the caller.
-            Err(error) => {
-                warn!(log, "failed to update disk from sled agent";
-                    "disk_id" => %id,
-                    "new_state" => ?new_state,
-                    "error" => ?error);
-                Err(error)
-            }
-        }
-    }
-
     pub(crate) async fn project_delete_disk(
         self: &Arc<Self>,
         opctx: &OpContext,

nexus/src/app/sagas/instance_create.rs

@@ -1642,8 +1642,8 @@ pub mod test {
             == "detached"
     }
 
-    async fn no_instances_or_disks_on_sled(sled_agent: &SledAgent) -> bool {
-        sled_agent.vmm_count().await == 0 && sled_agent.disk_count().await == 0
+    async fn no_instances_on_sled(sled_agent: &SledAgent) -> bool {
+        sled_agent.vmm_count().await == 0
     }
 
     pub(crate) async fn verify_clean_slate(
@@ -1672,7 +1672,7 @@ pub mod test {
             .await
         );
         assert!(disk_is_detached(datastore).await);
-        assert!(no_instances_or_disks_on_sled(&sled_agent).await);
+        assert!(no_instances_on_sled(&sled_agent).await);
 
         let v2p_mappings = &*sled_agent.v2p_mappings.lock().unwrap();
         assert!(v2p_mappings.is_empty());

nexus/src/internal_api/http_entrypoints.rs

@@ -131,24 +131,16 @@ impl NexusInternalApi for NexusInternalApiImpl {
     }
 
     async fn cpapi_disks_put(
-        rqctx: RequestContext<Self::Context>,
-        path_params: Path<DiskPathParam>,
-        new_runtime_state: TypedBody<DiskRuntimeState>,
+        _rqctx: RequestContext<Self::Context>,
+        _path_params: Path<DiskPathParam>,
+        _new_runtime_state: TypedBody<DiskRuntimeState>,
     ) -> Result<HttpResponseUpdatedNoContent, HttpError> {
-        let apictx = &rqctx.context().context;
-        let nexus = &apictx.nexus;
-        let path = path_params.into_inner();
-        let new_state = new_runtime_state.into_inner();
-        let handler = async {
-            let opctx =
-                crate::context::op_context_for_internal_api(&rqctx).await;
-            nexus.notify_disk_updated(&opctx, path.disk_id, &new_state).await?;
-            Ok(HttpResponseUpdatedNoContent())
-        };
-        apictx
-            .internal_latencies
-            .instrument_dropshot_handler(&rqctx, handler)
-            .await
+        // TODO: remove from the nexus-internal api trait when we can make new
+        // versions of it
+        Err(HttpError::for_bad_request(
+            None,
+            String::from("removed due to incorrect abstraction"),
+        ))
     }
 
     async fn cpapi_volume_remove_read_only_parent(

nexus/tests/integration_tests/instances.rs

@@ -5008,12 +5008,12 @@ async fn test_disks_detached_when_instance_destroyed(
     let instance_id = InstanceUuid::from_untyped_uuid(instance.identity.id);
     let apictx = &cptestctx.server.server_context();
     let nexus = &apictx.nexus;
-    let sa = nexus
+
+    nexus
         .active_instance_info(&instance_id, None)
         .await
         .unwrap()
-        .expect("instance should be on a sled while it's running")
-        .sled_client;
+        .expect("instance should be on a sled while it's running");
 
     // Stop and delete instance
     instance_post(&client, instance_name, InstanceOp::Stop).await;
@@ -5037,9 +5037,6 @@ async fn test_disks_detached_when_instance_destroyed(
     assert_eq!(disks.len(), 8);
     for disk in &disks {
         assert_eq!(disk.state, DiskState::Detached);
-
-        // Simulate each one of the disks to move from "Detaching" to "Detached"
-        sa.disk_finish_transition(disk.identity.id).await;
     }
 
     // Ensure that the disks can be attached to another instance