Make use of check_and_update for attach/detach

Should close #4628.
oxidecomputer · Dec 18, 2023 · 81c7d25 · 81c7d25
1 parent 7d6118f
commit 81c7d25
Show file tree

Hide file tree

Showing 5 changed files with 72 additions and 101 deletions.
diff --git a/nexus/db-queries/src/db/datastore/external_ip.rs b/nexus/db-queries/src/db/datastore/external_ip.rs
@@ -436,26 +436,19 @@ impl DataStore {
         &self,
         opctx: &OpContext,
         authz_fip: &authz::FloatingIp,
-        db_fip: &FloatingIp,
     ) -> DeleteResult {
         use db::schema::external_ip::dsl;
 
-        // Verify this FIP is not attached to any instances/services.
-        if db_fip.parent_id.is_some() {
-            return Err(Error::invalid_request(
-                "Floating IP cannot be deleted while attached to an instance",
-            ));
-        }
-
         opctx.authorize(authz::Action::Delete, authz_fip).await?;
 
         let now = Utc::now();
-        let updated_rows = diesel::update(dsl::external_ip)
-            .filter(dsl::id.eq(db_fip.id()))
+        let result = diesel::update(dsl::external_ip)
+            .filter(dsl::id.eq(authz_fip.id()))
             .filter(dsl::time_deleted.is_null())
             .filter(dsl::parent_id.is_null())
             .set(dsl::time_deleted.eq(now))
-            .execute_async(&*self.pool_connection_authorized(opctx).await?)
+            .check_if_exists::<FloatingIp>(authz_fip.id())
+            .execute_and_check(&*self.pool_connection_authorized(opctx).await?)
             .await
             .map_err(|e| {
                 public_error_from_diesel(
@@ -464,31 +457,26 @@ impl DataStore {
                 )
             })?;
 
-        if updated_rows == 0 {
-            return Err(Error::invalid_request(
-                "deletion failed due to concurrent modification",
-            ));
+        match result.status {
+            // Verify this FIP is not attached to any instances/services.
+            UpdateStatus::NotUpdatedButExists if result.found.parent_id.is_some() => Err(Error::invalid_request(
+                "Floating IP cannot be deleted while attached to an instance",
+            )),
+            // Only remaining cause of `NotUpdated` is earlier soft-deletion.
+            // Return success in this case to maintain idempotency.
+            UpdateStatus::Updated | UpdateStatus::NotUpdatedButExists => Ok(()),
         }
-        Ok(())
     }
 
     /// Attaches a Floating IP address to an instance.
     pub async fn floating_ip_attach(
         &self,
         opctx: &OpContext,
         authz_fip: &authz::FloatingIp,
-        db_fip: &FloatingIp,
         instance_id: Uuid,
     ) -> UpdateResult<FloatingIp> {
         use db::schema::external_ip::dsl;
 
-        // Verify this FIP is not attached to any instances/services.
-        if db_fip.parent_id.is_some() {
-            return Err(Error::invalid_request(
-                "Floating IP cannot be attached to one instance while still attached to another",
-            ));
-        }
-
         let (.., authz_instance, _db_instance) = LookupPath::new(&opctx, self)
             .instance_id(instance_id)
             .fetch_for(authz::Action::Modify)
@@ -497,85 +485,89 @@ impl DataStore {
         opctx.authorize(authz::Action::Modify, authz_fip).await?;
         opctx.authorize(authz::Action::Modify, &authz_instance).await?;
 
+        let fip_id = authz_fip.id();
+
         let out = diesel::update(dsl::external_ip)
-            .filter(dsl::id.eq(db_fip.id()))
+            .filter(dsl::id.eq(fip_id))
             .filter(dsl::kind.eq(IpKind::Floating))
             .filter(dsl::time_deleted.is_null())
             .filter(dsl::parent_id.is_null())
             .set((
                 dsl::parent_id.eq(Some(instance_id)),
                 dsl::time_modified.eq(Utc::now()),
             ))
-            .returning(ExternalIp::as_returning())
-            .get_result_async(&*self.pool_connection_authorized(opctx).await?)
+            .check_if_exists::<FloatingIp>(fip_id)
+            .execute_and_check(&*self.pool_connection_authorized(opctx).await?)
             .await
             .map_err(|e| {
                 public_error_from_diesel(
                     e,
                     ErrorHandler::NotFoundByResource(authz_fip),
                 )
-            })
-            .and_then(|r| FloatingIp::try_from(r))
-            .map_err(|e| Error::internal_error(&format!("{e}")))?;
+            })?;
 
-        Ok(out)
+        match (out.status, out.found.parent_id) {
+            (UpdateStatus::NotUpdatedButExists, Some(_)) => Err(Error::invalid_request(
+                "Floating IP cannot be attached to one instance while still attached to another",
+            )),
+            (UpdateStatus::Updated, _) => Ok(out.found.into()),
+            _ => unreachable!(),
+        }
     }
 
     /// Detaches a Floating IP address from an instance.
     pub async fn floating_ip_detach(
         &self,
         opctx: &OpContext,
         authz_fip: &authz::FloatingIp,
-        db_fip: &FloatingIp,
-        target_instance_id: Option<Uuid>,
-    ) -> UpdateResult<(FloatingIp, Option<Uuid>)> {
+        instance_id: Uuid,
+    ) -> UpdateResult<FloatingIp> {
         use db::schema::external_ip::dsl;
 
-        let Some(instance_id) = db_fip.parent_id else {
-            return Err(Error::invalid_request(
-                "Floating IP is not attached to an instance",
-            ));
-        };
-
-        if let Some(target_instance_id) = target_instance_id {
-            if target_instance_id != instance_id {
-                return Err(Error::invalid_request(
-                    "Floating IP is not attached to the target instance",
-                ));
-            }
-        }
-
-        let (.., authz_instance, _db_instance) = LookupPath::new(&opctx, self)
+        let (.., authz_instance) = LookupPath::new(&opctx, self)
             .instance_id(instance_id)
-            .fetch_for(authz::Action::Modify)
+            .lookup_for(authz::Action::Modify)
             .await?;
 
         opctx.authorize(authz::Action::Modify, authz_fip).await?;
         opctx.authorize(authz::Action::Modify, &authz_instance).await?;
 
-        let i = self.instance_fetch_with_vmm(opctx, &authz_instance).await?;
+        let fip_id = authz_fip.id();
 
         let out = diesel::update(dsl::external_ip)
-            .filter(dsl::id.eq(db_fip.id()))
+            .filter(dsl::id.eq(fip_id))
             .filter(dsl::kind.eq(IpKind::Floating))
             .filter(dsl::time_deleted.is_null())
             .filter(dsl::parent_id.eq(instance_id))
             .set((
                 dsl::parent_id.eq(Option::<Uuid>::None),
                 dsl::time_modified.eq(Utc::now()),
             ))
-            .returning(ExternalIp::as_returning())
-            .get_result_async(&*self.pool_connection_authorized(opctx).await?)
+            .check_if_exists::<FloatingIp>(fip_id)
+            .execute_and_check(&*self.pool_connection_authorized(opctx).await?)
             .await
             .map_err(|e| {
                 public_error_from_diesel(
                     e,
                     ErrorHandler::NotFoundByResource(authz_fip),
                 )
-            })
-            .and_then(|r| FloatingIp::try_from(r))
-            .map_err(|e| Error::internal_error(&format!("{e}")))?;
+            })?;
 
-        Ok((out, i.sled_id()))
+        match (out.status, out.found.parent_id) {
+            (UpdateStatus::NotUpdatedButExists, Some(id))
+                if id != instance_id =>
+            {
+                Err(Error::invalid_request(
+                    "Floating IP is not attached to the target instance",
+                ))
+            }
+            (UpdateStatus::NotUpdatedButExists, None) => {
+                Err(Error::invalid_request(
+                    "Floating IP is not attached to an instance",
+                ))
+            }
+            (UpdateStatus::Updated, _) => Ok(out.found.into()),
+            _ => unreachable!(),
+        }
     }
 }
diff --git a/nexus/src/app/external_ip.rs b/nexus/src/app/external_ip.rs
@@ -115,9 +115,9 @@ impl super::Nexus {
         opctx: &OpContext,
         ip_lookup: lookup::FloatingIp<'_>,
     ) -> DeleteResult {
-        let (.., authz_fip, db_fip) =
-            ip_lookup.fetch_for(authz::Action::Delete).await?;
+        let (.., authz_fip) =
+            ip_lookup.lookup_for(authz::Action::Delete).await?;
 
-        self.db_datastore.floating_ip_delete(opctx, &authz_fip, &db_fip).await
+        self.db_datastore.floating_ip_delete(opctx, &authz_fip).await
     }
 }
diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs
@@ -634,15 +634,15 @@ async fn sic_allocate_instance_external_ip(
         // Set the parent of an existing floating IP to the new instance's ID.
         params::ExternalIpCreate::Floating { ref floating_ip_name } => {
             let floating_ip_name = db::model::Name(floating_ip_name.clone());
-            let (.., authz_fip, db_fip) = LookupPath::new(&opctx, &datastore)
+            let (.., authz_fip) = LookupPath::new(&opctx, &datastore)
                 .project_id(saga_params.project_id)
                 .floating_ip_name(&floating_ip_name)
-                .fetch_for(authz::Action::Modify)
+                .lookup_for(authz::Action::Modify)
                 .await
                 .map_err(ActionError::action_failed)?;
 
             datastore
-                .floating_ip_attach(&opctx, &authz_fip, &db_fip, instance_id)
+                .floating_ip_attach(&opctx, &authz_fip, instance_id)
                 .await
                 .map_err(ActionError::action_failed)?;
         }
@@ -674,18 +674,17 @@ async fn sic_allocate_instance_external_ip_undo(
         }
         params::ExternalIpCreate::Floating { floating_ip_name } => {
             let floating_ip_name = db::model::Name(floating_ip_name.clone());
-            let (.., authz_fip, db_fip) = LookupPath::new(&opctx, &datastore)
+            let (.., authz_fip) = LookupPath::new(&opctx, &datastore)
                 .project_id(saga_params.project_id)
                 .floating_ip_name(&floating_ip_name)
-                .fetch_for(authz::Action::Modify)
+                .lookup_for(authz::Action::Modify)
                 .await?;
 
             datastore
                 .floating_ip_detach(
                     &opctx,
                     &authz_fip,
-                    &db_fip,
-                    Some(repeat_saga_params.instance_id),
+                    repeat_saga_params.instance_id,
                 )
                 .await?;
         }

diff --git a/nexus/src/app/sagas/instance_ip_attach.rs b/nexus/src/app/sagas/instance_ip_attach.rs
@@ -247,23 +247,18 @@ async fn siia_attach_ip(
         }
         // Set the parent of an existing floating IP to the new instance's ID.
         params::ExternalIpCreate::Floating { .. } => {
-            let (.., authz_fip, db_fip) = LookupPath::new(&opctx, &datastore)
+            let (.., authz_fip) = LookupPath::new(&opctx, &datastore)
                 .floating_ip_id(new_ip_uuid)
-                .fetch_for(authz::Action::Modify)
+                .lookup_for(authz::Action::Modify)
                 .await
                 .map_err(ActionError::action_failed)?;
 
             let eip = datastore
-                .floating_ip_attach(
-                    &opctx,
-                    &authz_fip,
-                    &db_fip,
-                    params.instance.id(),
-                )
+                .floating_ip_attach(&opctx, &authz_fip, params.instance.id())
                 .await
                 .map_err(ActionError::action_failed)?;
 
-            Ok(ExternalIp::Floating(eip.ip.ip(), db_fip.id()))
+            Ok(ExternalIp::Floating(eip.ip.ip(), authz_fip.id()))
         }
     }
 }
@@ -286,18 +281,13 @@ async fn siia_attach_ip_undo(
             datastore.deallocate_external_ip(&opctx, new_ip_uuid).await?;
         }
         params::ExternalIpCreate::Floating { .. } => {
-            let (.., authz_fip, db_fip) = LookupPath::new(&opctx, &datastore)
+            let (.., authz_fip) = LookupPath::new(&opctx, &datastore)
                 .floating_ip_id(new_ip_uuid)
-                .fetch_for(authz::Action::Modify)
+                .lookup_for(authz::Action::Modify)
                 .await?;
 
             datastore
-                .floating_ip_detach(
-                    &opctx,
-                    &authz_fip,
-                    &db_fip,
-                    Some(params.instance.id()),
-                )
+                .floating_ip_detach(&opctx, &authz_fip, params.instance.id())
                 .await?;
         }
     }

diff --git a/nexus/src/app/sagas/instance_ip_detach.rs b/nexus/src/app/sagas/instance_ip_detach.rs
@@ -332,19 +332,14 @@ async fn siid_detach_ip(
                 .map_err(ActionError::action_failed)?;
         }
         params::ExternalIpDelete::Floating { .. } => {
-            let (.., authz_fip, db_fip) = LookupPath::new(&opctx, &datastore)
+            let (.., authz_fip) = LookupPath::new(&opctx, &datastore)
                 .floating_ip_id(new_ip_uuid)
-                .fetch_for(authz::Action::Modify)
+                .lookup_for(authz::Action::Modify)
                 .await
                 .map_err(ActionError::action_failed)?;
 
             datastore
-                .floating_ip_detach(
-                    &opctx,
-                    &authz_fip,
-                    &db_fip,
-                    Some(params.instance.id()),
-                )
+                .floating_ip_detach(&opctx, &authz_fip, params.instance.id())
                 .await
                 .map_err(ActionError::action_failed)?;
         }
@@ -391,19 +386,14 @@ async fn siid_detach_ip_undo(
         }
         // Set the parent of an existing floating IP to the new instance's ID.
         params::ExternalIpDelete::Floating { .. } => {
-            let (.., authz_fip, db_fip) = LookupPath::new(&opctx, &datastore)
+            let (.., authz_fip) = LookupPath::new(&opctx, &datastore)
                 .floating_ip_id(new_ip_uuid)
-                .fetch_for(authz::Action::Modify)
+                .lookup_for(authz::Action::Modify)
                 .await
                 .map_err(ActionError::action_failed)?;
 
             let _eip = datastore
-                .floating_ip_attach(
-                    &opctx,
-                    &authz_fip,
-                    &db_fip,
-                    params.instance.id(),
-                )
+                .floating_ip_attach(&opctx, &authz_fip, params.instance.id())
                 .await
                 .map_err(ActionError::action_failed)?;