Review feedback: nits and error messages

oxidecomputer · Jan 10, 2024 · 725efa4 · 725efa4
1 parent 5a916ca
commit 725efa4
Show file tree

Hide file tree

Showing 5 changed files with 77 additions and 23 deletions.
diff --git a/nexus/db-queries/src/db/datastore/external_ip.rs b/nexus/db-queries/src/db/datastore/external_ip.rs
@@ -460,7 +460,9 @@ impl DataStore {
                     // a UniqueViolation.
                     IpAttachState::Attaching | IpAttachState::Detaching
                         => return Err(Error::unavail(&format!(
-                        "tried to attach {kind} IP mid-attach/detach"
+                        "tried to attach {kind} IP mid-attach/detach: \
+                         attach will be safe to retry once operation on \
+                         same IP resource completes"
                     ))),
 
                     IpAttachState::Detached => {},
@@ -469,7 +471,8 @@ impl DataStore {
                 Err(match &collection.runtime_state.nexus_state {
                     state if SAFE_TRANSIENT_INSTANCE_STATES.contains(&state)
                         => Error::unavail(&format!(
-                        "tried to attach {kind} IP while instance was changing state"
+                        "tried to attach {kind} IP while instance was changing state: \
+                         attach will be safe to retry once start/stop/migrate completes"
                     )),
                     state if SAFE_TO_ATTACH_INSTANCE_STATES.contains(&state) => {
                         if attached_count >= MAX_EXTERNAL_IPS_PLUS_SNAT as i64 {
@@ -577,14 +580,17 @@ impl DataStore {
                     // User can reattempt depending on how the current saga unfolds.
                     IpAttachState::Attaching
                         | IpAttachState::Detaching => return Err(Error::unavail(&format!(
-                        "tried to detach {kind} IP mid-attach/detach"
+                        "tried to detach {kind} IP mid-attach/detach: \
+                         attach will be safe to retry once operation on \
+                         same IP resource completes"
                     ))),
                     IpAttachState::Attached => {},
                 }
 
                 match collection.runtime_state.nexus_state {
                     state if SAFE_TRANSIENT_INSTANCE_STATES.contains(&state) => Error::unavail(&format!(
-                        "tried to detach {kind} IP while instance was changing state"
+                        "tried to attach {kind} IP while instance was changing state: \
+                         attach will be safe to retry once start/stop/migrate completes"
                     )),
                     state if SAFE_TO_ATTACH_INSTANCE_STATES.contains(&state) => {
                         Error::internal_error(&format!("failed to detach {kind} IP"))

diff --git a/nexus/preprocessed_configs/config.xml b/nexus/preprocessed_configs/config.xml
diff --git a/nexus/src/app/sagas/instance_common.rs b/nexus/src/app/sagas/instance_common.rs
@@ -168,18 +168,17 @@ pub async fn instance_ip_move_state(
     serialized_authn: &authn::saga::Serialized,
     from: IpAttachState,
     to: IpAttachState,
+    new_ip: &ModifyStateForExternalIp,
 ) -> Result<bool, ActionError> {
     let osagactx = sagactx.user_data();
     let datastore = osagactx.datastore();
     let opctx =
         crate::context::op_context_for_saga_action(&sagactx, serialized_authn);
 
-    let new_ip = sagactx.lookup::<ModifyStateForExternalIp>("target_ip")?;
-
     if !new_ip.do_saga {
         return Ok(true);
     }
-    let Some(new_ip) = new_ip.external_ip else {
+    let Some(new_ip) = new_ip.external_ip.as_ref() else {
         return Err(ActionError::action_failed(Error::internal_error(
             "tried to `do_saga` without valid external IP",
         )));
@@ -266,19 +265,19 @@ pub async fn instance_ip_add_nat(
     sagactx: &NexusActionContext,
     serialized_authn: &authn::saga::Serialized,
     authz_instance: &authz::Instance,
+    sled_uuid: Option<Uuid>,
+    target_ip: ModifyStateForExternalIp,
 ) -> Result<(), ActionError> {
     let osagactx = sagactx.user_data();
     let datastore = osagactx.datastore();
     let opctx =
         crate::context::op_context_for_saga_action(&sagactx, serialized_authn);
 
     // No physical sled? Don't push NAT.
-    let Some(sled_uuid) = sagactx.lookup::<Option<Uuid>>("instance_state")?
-    else {
+    let Some(sled_uuid) = sled_uuid else {
         return Ok(());
     };
 
-    let target_ip = sagactx.lookup::<ModifyStateForExternalIp>("target_ip")?;
     if !target_ip.do_saga {
         return Ok(());
     }
@@ -314,17 +313,18 @@ pub async fn instance_ip_remove_nat(
     sagactx: &NexusActionContext,
     serialized_authn: &authn::saga::Serialized,
     authz_instance: &authz::Instance,
+    sled_uuid: Option<Uuid>,
+    target_ip: ModifyStateForExternalIp,
 ) -> Result<(), ActionError> {
     let osagactx = sagactx.user_data();
     let opctx =
         crate::context::op_context_for_saga_action(&sagactx, serialized_authn);
 
     // No physical sled? Don't push NAT.
-    let Some(_) = sagactx.lookup::<Option<Uuid>>("instance_state")? else {
+    if sled_uuid.is_none() {
         return Ok(());
     };
 
-    let target_ip = sagactx.lookup::<ModifyStateForExternalIp>("target_ip")?;
     if !target_ip.do_saga {
         return Ok(());
     }
@@ -346,16 +346,16 @@ pub async fn instance_ip_remove_nat(
 pub async fn instance_ip_add_opte(
     sagactx: &NexusActionContext,
     authz_instance: &authz::Instance,
+    sled_uuid: Option<Uuid>,
+    target_ip: ModifyStateForExternalIp,
 ) -> Result<(), ActionError> {
     let osagactx = sagactx.user_data();
 
     // No physical sled? Don't inform OPTE.
-    let Some(sled_uuid) = sagactx.lookup::<Option<Uuid>>("instance_state")?
-    else {
+    let Some(sled_uuid) = sled_uuid else {
         return Ok(());
     };
 
-    let target_ip = sagactx.lookup::<ModifyStateForExternalIp>("target_ip")?;
     if !target_ip.do_saga {
         return Ok(());
     }
@@ -396,16 +396,16 @@ pub async fn instance_ip_add_opte(
 pub async fn instance_ip_remove_opte(
     sagactx: &NexusActionContext,
     authz_instance: &authz::Instance,
+    sled_uuid: Option<Uuid>,
+    target_ip: ModifyStateForExternalIp,
 ) -> Result<(), ActionError> {
     let osagactx = sagactx.user_data();
 
     // No physical sled? Don't inform OPTE.
-    let Some(sled_uuid) = sagactx.lookup::<Option<Uuid>>("instance_state")?
-    else {
+    let Some(sled_uuid) = sled_uuid else {
         return Ok(());
     };
 
-    let target_ip = sagactx.lookup::<ModifyStateForExternalIp>("target_ip")?;
     if !target_ip.do_saga {
         return Ok(());
     }

diff --git a/nexus/src/app/sagas/instance_ip_attach.rs b/nexus/src/app/sagas/instance_ip_attach.rs
@@ -160,11 +160,13 @@ async fn siia_begin_attach_ip_undo(
     let log = sagactx.user_data().log();
     warn!(log, "siia_begin_attach_ip_undo: Reverting detached->attaching");
     let params = sagactx.saga_params::<Params>()?;
+    let new_ip = sagactx.lookup::<ModifyStateForExternalIp>("target_ip")?;
     if !instance_ip_move_state(
         &sagactx,
         &params.serialized_authn,
         IpAttachState::Attaching,
         IpAttachState::Detached,
+        &new_ip,
     )
     .await?
     {
@@ -189,10 +191,14 @@ async fn siia_get_instance_state(
 
 async fn siia_nat(sagactx: NexusActionContext) -> Result<(), ActionError> {
     let params = sagactx.saga_params::<Params>()?;
+    let sled_id = sagactx.lookup::<Option<Uuid>>("instance_state")?;
+    let target_ip = sagactx.lookup::<ModifyStateForExternalIp>("target_ip")?;
     instance_ip_add_nat(
         &sagactx,
         &params.serialized_authn,
         &params.authz_instance,
+        sled_id,
+        target_ip,
     )
     .await
 }
@@ -202,10 +208,14 @@ async fn siia_nat_undo(
 ) -> Result<(), anyhow::Error> {
     let log = sagactx.user_data().log();
     let params = sagactx.saga_params::<Params>()?;
+    let sled_id = sagactx.lookup::<Option<Uuid>>("instance_state")?;
+    let target_ip = sagactx.lookup::<ModifyStateForExternalIp>("target_ip")?;
     if let Err(e) = instance_ip_remove_nat(
         &sagactx,
         &params.serialized_authn,
         &params.authz_instance,
+        sled_id,
+        target_ip,
     )
     .await
     {
@@ -219,16 +229,26 @@ async fn siia_update_opte(
     sagactx: NexusActionContext,
 ) -> Result<(), ActionError> {
     let params = sagactx.saga_params::<Params>()?;
-    instance_ip_add_opte(&sagactx, &params.authz_instance).await
+    let sled_id = sagactx.lookup::<Option<Uuid>>("instance_state")?;
+    let target_ip = sagactx.lookup::<ModifyStateForExternalIp>("target_ip")?;
+    instance_ip_add_opte(&sagactx, &params.authz_instance, sled_id, target_ip)
+        .await
 }
 
 async fn siia_update_opte_undo(
     sagactx: NexusActionContext,
 ) -> Result<(), anyhow::Error> {
     let log = sagactx.user_data().log();
     let params = sagactx.saga_params::<Params>()?;
-    if let Err(e) =
-        instance_ip_remove_opte(&sagactx, &params.authz_instance).await
+    let sled_id = sagactx.lookup::<Option<Uuid>>("instance_state")?;
+    let target_ip = sagactx.lookup::<ModifyStateForExternalIp>("target_ip")?;
+    if let Err(e) = instance_ip_remove_opte(
+        &sagactx,
+        &params.authz_instance,
+        sled_id,
+        target_ip,
+    )
+    .await
     {
         error!(log, "siia_update_opte_undo: failed to notify sled-agent: {e}");
     }
@@ -247,6 +267,7 @@ async fn siia_complete_attach(
         &params.serialized_authn,
         IpAttachState::Attaching,
         IpAttachState::Attached,
+        &target_ip,
     )
     .await?
     {

diff --git a/nexus/src/app/sagas/instance_ip_detach.rs b/nexus/src/app/sagas/instance_ip_detach.rs
@@ -137,11 +137,13 @@ async fn siid_begin_detach_ip_undo(
     let log = sagactx.user_data().log();
     warn!(log, "siid_begin_detach_ip_undo: Reverting attached->detaching");
     let params = sagactx.saga_params::<Params>()?;
+    let new_ip = sagactx.lookup::<ModifyStateForExternalIp>("target_ip")?;
     if !instance_ip_move_state(
         &sagactx,
         &params.serialized_authn,
         IpAttachState::Detaching,
         IpAttachState::Attached,
+        &new_ip,
     )
     .await?
     {
@@ -166,10 +168,14 @@ async fn siid_get_instance_state(
 
 async fn siid_nat(sagactx: NexusActionContext) -> Result<(), ActionError> {
     let params = sagactx.saga_params::<Params>()?;
+    let sled_id = sagactx.lookup::<Option<Uuid>>("instance_state")?;
+    let target_ip = sagactx.lookup::<ModifyStateForExternalIp>("target_ip")?;
     instance_ip_remove_nat(
         &sagactx,
         &params.serialized_authn,
         &params.authz_instance,
+        sled_id,
+        target_ip,
     )
     .await
 }
@@ -179,10 +185,14 @@ async fn siid_nat_undo(
 ) -> Result<(), anyhow::Error> {
     let log = sagactx.user_data().log();
     let params = sagactx.saga_params::<Params>()?;
+    let sled_id = sagactx.lookup::<Option<Uuid>>("instance_state")?;
+    let target_ip = sagactx.lookup::<ModifyStateForExternalIp>("target_ip")?;
     if let Err(e) = instance_ip_add_nat(
         &sagactx,
         &params.serialized_authn,
         &params.authz_instance,
+        sled_id,
+        target_ip,
     )
     .await
     {
@@ -196,15 +206,31 @@ async fn siid_update_opte(
     sagactx: NexusActionContext,
 ) -> Result<(), ActionError> {
     let params = sagactx.saga_params::<Params>()?;
-    instance_ip_remove_opte(&sagactx, &params.authz_instance).await
+    let sled_id = sagactx.lookup::<Option<Uuid>>("instance_state")?;
+    let target_ip = sagactx.lookup::<ModifyStateForExternalIp>("target_ip")?;
+    instance_ip_remove_opte(
+        &sagactx,
+        &params.authz_instance,
+        sled_id,
+        target_ip,
+    )
+    .await
 }
 
 async fn siid_update_opte_undo(
     sagactx: NexusActionContext,
 ) -> Result<(), anyhow::Error> {
     let log = sagactx.user_data().log();
     let params = sagactx.saga_params::<Params>()?;
-    if let Err(e) = instance_ip_add_opte(&sagactx, &params.authz_instance).await
+    let sled_id = sagactx.lookup::<Option<Uuid>>("instance_state")?;
+    let target_ip = sagactx.lookup::<ModifyStateForExternalIp>("target_ip")?;
+    if let Err(e) = instance_ip_add_opte(
+        &sagactx,
+        &params.authz_instance,
+        sled_id,
+        target_ip,
+    )
+    .await
     {
         error!(log, "siid_update_opte_undo: failed to notify sled-agent: {e}");
     }
@@ -223,6 +249,7 @@ async fn siid_complete_detach(
         &params.serialized_authn,
         IpAttachState::Detaching,
         IpAttachState::Detached,
+        &target_ip,
     )
     .await?
     {