[nexus] Make instance deletion actions idempotent

nexus/src/app/sagas/instance_create.rs

@@ -960,7 +960,7 @@ async fn sic_instance_ensure(
 }
 
 #[cfg(test)]
-mod test {
+pub mod test {
     use crate::{
         app::saga::create_saga_dag, app::sagas::instance_create::Params,
         app::sagas::instance_create::SagaInstanceCreate,
@@ -1124,7 +1124,9 @@ mod test {
             && sled_agent.disk_count().await == 0
     }
 
-    async fn verify_clean_clate(cptestctx: &ControlPlaneTestContext) {
+    pub(crate) async fn verify_clean_slate(
+        cptestctx: &ControlPlaneTestContext,
+    ) {
         let sled_agent = &cptestctx.sled_agent.sled_agent;
         let datastore = cptestctx.server.apictx.nexus.datastore();
 
@@ -1178,7 +1180,7 @@ mod test {
                 .await
                 .expect_err("Saga should have failed");
 
-            verify_clean_clate(&cptestctx).await;
+            verify_clean_slate(&cptestctx).await;
         }
     }
 
@@ -1245,7 +1247,7 @@ mod test {
                 .await
                 .expect_err("Saga should have failed");
 
-            verify_clean_clate(&cptestctx).await;
+            verify_clean_slate(&cptestctx).await;
         }
     }
 
@@ -1308,6 +1310,6 @@ mod test {
         // This is important to ensure that our original saga didn't
         // double-allocate during repeated actions.
         destroy_instance(&cptestctx).await;
-        verify_clean_clate(&cptestctx).await;
+        verify_clean_slate(&cptestctx).await;
     }
 }

nexus/src/app/sagas/instance_delete.rs

@@ -8,6 +8,7 @@ use super::NexusSaga;
 use crate::app::sagas::declare_saga_actions;
 use crate::context::OpContext;
 use crate::{authn, authz};
+use omicron_common::api::external::{Error, ResourceType};
 use serde::Deserialize;
 use serde::Serialize;
 use steno::ActionError;
@@ -70,6 +71,16 @@ async fn sid_delete_instance_record(
         .datastore()
         .project_delete_instance(&opctx, &params.authz_instance)
         .await
+        .or_else(|err| {
+            // Necessary for idempotency
+            match err {
+                Error::ObjectNotFound {
+                    type_name: ResourceType::Instance,
+                    lookup_type: _,
+                } => Ok(()),
+                _ => Err(err),
+            }
+        })
         .map_err(ActionError::action_failed)?;
     Ok(())
 }
@@ -104,3 +115,193 @@ async fn sid_deallocate_external_ip(
         .map_err(ActionError::action_failed)?;
     Ok(())
 }
+
+#[cfg(test)]
+mod test {
+    use crate::{
+        app::saga::create_saga_dag,
+        app::sagas::instance_create::test::verify_clean_slate,
+        app::sagas::instance_delete::Params,
+        app::sagas::instance_delete::SagaInstanceDelete,
+        authn::saga::Serialized, context::OpContext, db,
+        db::lookup::LookupPath, external_api::params,
+    };
+    use dropshot::test_util::ClientTestContext;
+    use nexus_test_utils::resource_helpers::create_disk;
+    use nexus_test_utils::resource_helpers::create_organization;
+    use nexus_test_utils::resource_helpers::create_project;
+    use nexus_test_utils::resource_helpers::populate_ip_pool;
+    use nexus_test_utils::resource_helpers::DiskTest;
+    use nexus_test_utils_macros::nexus_test;
+    use nexus_types::identity::Resource;
+    use omicron_common::api::external::{
+        ByteCount, IdentityMetadataCreateParams, InstanceCpuCount, Name,
+    };
+    use std::num::NonZeroU32;
+    use uuid::Uuid;
+
+    type ControlPlaneTestContext =
+        nexus_test_utils::ControlPlaneTestContext<crate::Server>;
+
+    const INSTANCE_NAME: &str = "my-instance";
+    const ORG_NAME: &str = "test-org";
+    const PROJECT_NAME: &str = "springfield-squidport";
+    const DISK_NAME: &str = "my-disk";
+
+    async fn create_org_project_and_disk(client: &ClientTestContext) -> Uuid {
+        populate_ip_pool(&client, "default", None).await;
+        create_organization(&client, ORG_NAME).await;
+        let project = create_project(client, ORG_NAME, PROJECT_NAME).await;
+        create_disk(&client, ORG_NAME, PROJECT_NAME, DISK_NAME).await;
+        project.identity.id
+    }
+
+    async fn new_test_params(
+        cptestctx: &ControlPlaneTestContext,
+        instance_id: Uuid,
+    ) -> Params {
+        let opctx = test_opctx(&cptestctx);
+        let datastore = cptestctx.server.apictx.nexus.datastore();
+
+        let (.., authz_instance, _instance) =
+            LookupPath::new(&opctx, &datastore)
+                .instance_id(instance_id)
+                .fetch()
+                .await
+                .expect("Failed to lookup instance");
+        Params {
+            serialized_authn: Serialized::for_opctx(&opctx),
+            authz_instance,
+        }
+    }
+
+    // Helper for creating instance create parameters
+    fn new_instance_create_params() -> params::InstanceCreate {
+        params::InstanceCreate {
+            identity: IdentityMetadataCreateParams {
+                name: INSTANCE_NAME.parse().unwrap(),
+                description: "My instance".to_string(),
+            },
+            ncpus: InstanceCpuCount::try_from(2).unwrap(),
+            memory: ByteCount::from_gibibytes_u32(4),
+            hostname: String::from("inst"),
+            user_data: vec![],
+            network_interfaces:
+                params::InstanceNetworkInterfaceAttachment::Default,
+            external_ips: vec![params::ExternalIpCreate::Ephemeral {
+                pool_name: None,
+            }],
+            disks: vec![params::InstanceDiskAttachment::Attach(
+                params::InstanceDiskAttach { name: DISK_NAME.parse().unwrap() },
+            )],
+            start: false,
+        }
+    }
+
+    pub fn test_opctx(cptestctx: &ControlPlaneTestContext) -> OpContext {
+        OpContext::for_tests(
+            cptestctx.logctx.log.new(o!()),
+            cptestctx.server.apictx.nexus.datastore().clone(),
+        )
+    }
+
+    #[nexus_test(server = crate::Server)]
+    async fn test_saga_basic_usage_succeeds(
+        cptestctx: &ControlPlaneTestContext,
+    ) {
+        DiskTest::new(cptestctx).await;
+        let client = &cptestctx.external_client;
+        let nexus = &cptestctx.server.apictx.nexus;
+        create_org_project_and_disk(&client).await;
+
+        // Build the saga DAG with the provided test parameters
+        let dag = create_saga_dag::<SagaInstanceDelete>(
+            new_test_params(
+                &cptestctx,
+                create_instance(&cptestctx, new_instance_create_params())
+                    .await
+                    .id(),
+            )
+            .await,
+        )
+        .unwrap();
+        let runnable_saga = nexus.create_runnable_saga(dag).await.unwrap();
+
+        // Actually run the saga
+        nexus
+            .run_saga(runnable_saga)
+            .await
+            .expect("Saga should have succeeded");
+    }
+
+    async fn create_instance(
+        cptestctx: &ControlPlaneTestContext,
+        params: params::InstanceCreate,
+    ) -> db::model::Instance {
+        let nexus = &cptestctx.server.apictx.nexus;
+        let opctx = test_opctx(&cptestctx);
+
+        let project_selector = params::ProjectSelector {
+            organization_selector: Some(
+                Name::try_from(ORG_NAME.to_string()).unwrap().into(),
+            ),
+            project: PROJECT_NAME.to_string().try_into().unwrap(),
+        };
+        let project_lookup =
+            nexus.project_lookup(&opctx, &project_selector).unwrap();
+        nexus
+            .project_create_instance(&opctx, &project_lookup, &params)
+            .await
+            .unwrap()
+    }
+
+    #[nexus_test(server = crate::Server)]
+    async fn test_actions_succeed_idempotently(
+        cptestctx: &ControlPlaneTestContext,
+    ) {
+        DiskTest::new(cptestctx).await;
+
+        let client = &cptestctx.external_client;
+        let nexus = &cptestctx.server.apictx.nexus;
+        create_org_project_and_disk(&client).await;
+
+        // Build the saga DAG with the provided test parameters
+        let dag = create_saga_dag::<SagaInstanceDelete>(
+            new_test_params(
+                &cptestctx,
+                create_instance(&cptestctx, new_instance_create_params())
+                    .await
+                    .id(),
+            )
+            .await,
+        )
+        .unwrap();
+
+        let runnable_saga =
+            nexus.create_runnable_saga(dag.clone()).await.unwrap();
+
+        // Cause all actions to run twice. The saga should succeed regardless!
+        for node in dag.get_nodes() {
+            nexus
+                .sec()
+                .saga_inject_repeat(
+                    runnable_saga.id(),
+                    node.index(),
+                    steno::RepeatInjected {
+                        action: NonZeroU32::new(2).unwrap(),
+                        undo: NonZeroU32::new(1).unwrap(),
+                    },
+                )
+                .await
+                .unwrap();
+        }
+
+        // Verify that the saga's execution succeeded.
+        nexus
+            .run_saga(runnable_saga)
+            .await
+            .expect("Saga should have succeeded");
+
+        verify_clean_slate(&cptestctx).await;
+    }
+}