diff --git a/nexus/src/app/background/tasks/instance_watcher.rs b/nexus/src/app/background/tasks/instance_watcher.rs index 31bd501a980..772da50651b 100644 --- a/nexus/src/app/background/tasks/instance_watcher.rs +++ b/nexus/src/app/background/tasks/instance_watcher.rs @@ -139,7 +139,10 @@ impl InstanceWatcher { }; let Some(state) = check - .run(&opctx, inv_rx, &instance, &vmm, &sled, gateways, &client) + .run( + &opctx, &datastore, inv_rx, &instance, &vmm, &sled, + gateways, &client, + ) .await else { // Check did not result in an updated state, nothing else to @@ -201,6 +204,7 @@ impl Check { async fn run( &mut self, opctx: &OpContext, + datastore: &DataStore, inv_rx: watch::Receiver>>, instance: &Instance, vmm: &Vmm, @@ -234,16 +238,16 @@ impl Check { } // Ask the sled-agent what it has to say for itself. - let rsp = client + let result = client .vmm_get_state(&PropolisUuid::from_untyped_uuid(vmm.id)) - .await - .map_err(SledAgentInstanceError); - match rsp { - // We received a response from the sled-agent. We shall update the - // instance's state to match. - // - // Note that this does not always mean that the *VMM* is healthy, - // but only that we successfully got its state from the sled-agent. + .await; + + // We received a response from the sled-agent. We shall update the + // instance's state to match. + // + // Note that this does not always mean that the *VMM* is healthy, + // but only that we successfully got its state from the sled-agent. + let error = match result { Ok(rsp) => { let vmm_state = SledVmmState::from(rsp.into_inner()); let instance_state = { @@ -261,28 +265,46 @@ impl Check { ) }; self.outcome = CheckOutcome::Success(instance_state); - Some(vmm_state) + return Some(vmm_state); + } + Err(e) => { + SledAgentInstanceError::new(opctx, datastore, sled.id(), e) + .await } + }; + + match error { // Oh, this error indicates that the VMM should transition to // `Failed`. Let's synthesize a `SledInstanceState` that does // that. - Err(e) if e.vmm_gone() => { - let error = InlineErrorChain::new(&e); + SledAgentInstanceError::VmmGone { .. } => { slog::info!( opctx.log, - "sled-agent error indicates that this instance's \ - VMM has failed!"; - error, + "sled-agent error indicates that this instance's VMM has \ + failed!"; + "error" => InlineErrorChain::new(&error) ); + // TODO(eliza): if `CheckOutcome::Failure` used the same + // `VmmFailureReason` as the rest of nexus, this could all be a + // bit less gross eventually. self.outcome = CheckOutcome::Failure(Failure::NoSuchInstance); mk_failed(VmmFailureReason::NoSuchInstance) } + SledAgentInstanceError::SledNotInService { .. } => { + slog::info!( + opctx.log, + "sled-agent no longer in service, failing VMM"; + "error" => InlineErrorChain::new(&error), + ); + self.outcome = CheckOutcome::Failure(Failure::SledExpunged); + mk_failed(VmmFailureReason::SledExpunged) + } // We were able to contact the sled-agent, but it responded with an // error which does *not* tell us that the VMM has failed. Either // the sled-agent is unhealthy, or we sent an invalid request for // some reason. In either case, the check is inconclusive and the // instance's state will not change. - Err(SledAgentInstanceError(ClientError::ErrorResponse(rsp))) => { + SledAgentInstanceError::Other(ClientError::ErrorResponse(rsp)) => { // This is a bit goofy: we destructure the error because // `ResponseValue` has a `status()` which is not optional (so we // don't have to unwrap it), but then we re-construct the @@ -319,7 +341,9 @@ impl Check { // powered on. If we determine conclusively that it is not present // and in A0, the instance is moved to `Failed`. Otherwise, the // check is inconclusive and the instance's state will not change. - Err(SledAgentInstanceError(ClientError::CommunicationError(e))) => { + SledAgentInstanceError::Other(ClientError::CommunicationError( + e, + )) => { slog::info!( opctx.log, "sled-agent is unreachable"; @@ -359,7 +383,7 @@ impl Check { None } // Any other errors mean that the check is inconclusive. - Err(SledAgentInstanceError(e)) => { + SledAgentInstanceError::Other(e) => { slog::warn!( opctx.log, "error checking up on instance"; diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 60fe51f2c21..c6086dac5ee 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -17,7 +17,6 @@ use nexus_db_queries::db::DataStore; use nexus_db_queries::db::datastore::SQL_BATCH_SIZE; use nexus_db_queries::db::pagination::Paginator; use nexus_types::instance::SledVmmState; -use nexus_types::instance::VmmFailureReason; use nexus_types::instance::VmmRuntimeState; use nexus_types::instance::VmmState; use nexus_types::internal_api::background::VmmStopForUpdateStatus; @@ -282,29 +281,31 @@ impl VmmStopForUpdate { { Ok(res) => res.into_inner().updated_runtime.map(Into::into), Err(e) => { - let err = SledAgentInstanceError(e); + let err = SledAgentInstanceError::new( + &opctx, &datastore, sled_id, e, + ) + .await; - // First we check if the VMM is there, if it isn't, we - // want to report it as such so we don't have to wait on the - // instance watcher to find and restart this instance. - if err.vmm_gone() { + // First we check if the VMM should transition to `Failed` + // based on this error. If it does, we mark it as such so we + // don't have to wait on the instance watcher to find and + // restart this instance. + if let Some(reason) = err.vmm_failure_reason() { slog::info!( opctx.log, - "Failed to mark VMM as Stopped; VMM was already \ - gone. Marking as Failed"; + "Attempted to stop a VMM, but discovered that it \ + should transition to Failed."; "sled_id" => %sled_id, "vmm_id" => %vmm_id, "instance_id" => %instance_id, - InlineErrorChain::new(&err), + "failure_reason" => %reason, ); result.errors.push(err.into()); Some(SledVmmState { - vmm_state: vmm.runtime().transition( - VmmState::Failed( - VmmFailureReason::NoSuchInstance, - ), - ), + vmm_state: vmm + .runtime() + .transition(VmmState::Failed(reason)), migration_in: None, migration_out: None, }) diff --git a/nexus/src/app/instance.rs b/nexus/src/app/instance.rs index 606514917e2..92b8c5c41f2 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -43,6 +43,7 @@ use nexus_db_queries::db::model::InstanceStateComputer; use nexus_types::external_api::disk; use nexus_types::external_api::external_ip; use nexus_types::external_api::instance; +use nexus_types::instance::VmmFailureReason; use nexus_types::internal_api::params::InstanceMigrateDestination; use omicron_common::http_client::internal_http_client; // We'll use `InstanceState` frequently, and the code that uses it is already @@ -105,25 +106,39 @@ use uuid::Uuid; type SledAgentClientError = sled_agent_client::Error; -// Newtype wrapper to avoid the orphan type rule. +// Errors returned #[derive(Debug, thiserror::Error)] -pub struct SledAgentInstanceError(pub SledAgentClientError); - -impl std::fmt::Display for SledAgentInstanceError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", self.0) - } -} - -impl From for SledAgentInstanceError { - fn from(value: SledAgentClientError) -> Self { - Self(value) - } +pub enum SledAgentInstanceError { + #[error(transparent)] + Other(SledAgentClientError), + #[error("sled {sled_id} is no longer in service")] + SledNotInService { + sled_id: SledUuid, + #[source] + source: SledAgentClientError, + }, + #[error("VMM no longer exists on sled {sled_id}")] + VmmGone { + sled_id: SledUuid, + #[source] + source: SledAgentClientError, + }, } impl From for omicron_common::api::external::Error { fn from(value: SledAgentInstanceError) -> Self { - value.0.into() + match value { + SledAgentInstanceError::Other(e) => Error::from(e) + .internal_context("error sending request to sled-agent"), + SledAgentInstanceError::VmmGone { source, .. } => { + Error::from(source) + .internal_context("VMM no longer exists on sled") + } + SledAgentInstanceError::SledNotInService { source, .. } => { + Error::from(source) + .internal_context("sled is no longer in service") + } + } } } @@ -137,7 +152,9 @@ impl From for dropshot::HttpError { match value { // Errors communicating with the sled-agent should return 502 Bad // Gateway to the caller. - SledAgentInstanceError(ClientError::CommunicationError(e)) => { + SledAgentInstanceError::Other(ClientError::CommunicationError( + e, + )) => { // N.B.: it sure *seems* like we should be using the // `HttpError::for_status` constructor here, but that // constructor secretly calls the `for_client_error` @@ -157,30 +174,68 @@ impl From for dropshot::HttpError { } // Invalid request errors from the sled-agent should return 500 // Internal Server Errors. - SledAgentInstanceError(ClientError::InvalidRequest(s)) => { + SledAgentInstanceError::Other(ClientError::InvalidRequest(s)) => { HttpError::for_internal_error(s) } // Error responses from sled-agent that indicate the instance is // unhealthy should be mapped to a 503 error. - e if e.vmm_gone() => { + e @ SledAgentInstanceError::VmmGone { .. } => { let mut error = HttpError::for_unavail(None, e.to_string()); error.external_message = "The instance was running but is no \ longer reachable. It is being moved to the Failed state." .to_string(); error } + // Similarly, if the sled is no longer in service, return a 503 + // error. + e @ SledAgentInstanceError::SledNotInService { sled_id, .. } => { + let mut error = HttpError::for_unavail(None, e.to_string()); + error.external_message = format!( + "The instance was running on sled {sled_id}, but that sled \ + is no longer in service. The instance is being moved to \ + the Failed state." + ); + error + } // Other client errors can be handled by the normal // `external::Error` to `HttpError` conversions. - SledAgentInstanceError(e) => HttpError::from(Error::from(e)), + SledAgentInstanceError::Other(e) => HttpError::from(Error::from(e)), } } } impl SledAgentInstanceError { - /// Returns `true` if this error is of a class that indicates that the - /// VMM is no longer known to the sled-agent. - pub fn vmm_gone(&self) -> bool { - match &self.0 { + pub async fn new( + opctx: &OpContext, + datastore: &DataStore, + sled_id: SledUuid, + source: SledAgentClientError, + ) -> Self { + use progenitor_client::Error as ClientError; + match &source { + ClientError::CommunicationError(_) + if !datastore + .check_sled_in_service(opctx, sled_id) + .await + .unwrap_or_else(|e| { + // We cannot determine conclusively whether or not the + // sled is still in service, so assume it has not been + // expunged until we receive evidence to the contrary, + // and complain about it for now. + warn!( + opctx.log, + "failed to check whether or not sled {sled_id} is \ + still in service (in order to determine the \ + disposition for a communication error); we shall \ + treat it as though it is for now."; + "sled_id" => %sled_id, + "error" => InlineErrorChain::new(&e), + ); + true + }) => + { + Self::SledNotInService { sled_id, source } + } // See RFD 486 §6.3: // https://rfd.shared.oxide.computer/rfd/486#_stopping_or_rebooting_a_running_instance // @@ -191,21 +246,25 @@ impl SledAgentInstanceError { // > Failed state and trigger an instance update saga that will // > remove the dangling VMM ID from the instance and clean up the // > VMM’s resources. - progenitor_client::Error::ErrorResponse(rv) => { - rv.status() == http::StatusCode::NOT_FOUND - && rv.error_code.as_deref() == Some("NO_SUCH_INSTANCE") + ClientError::ErrorResponse(rv) + if rv.status() == http::StatusCode::NOT_FOUND + && rv.error_code.as_deref() == Some("NO_SUCH_INSTANCE") => + { + SledAgentInstanceError::VmmGone { sled_id, source } } - _ => false, + _ => SledAgentInstanceError::Other(source), } } /// If this error indicates that the VMM should be marked as `Failed`, /// returns the reason for the failure. - pub fn vmm_failure_reason(&self) -> Option { - if self.vmm_gone() { - Some(db::model::VmmFailureReason::NoSuchInstance) - } else { - None + pub fn vmm_failure_reason(&self) -> Option { + match self { + Self::VmmGone { .. } => Some(VmmFailureReason::NoSuchInstance), + Self::SledNotInService { .. } => { + Some(VmmFailureReason::SledExpunged) + } + Self::Other(_) => None, } } } @@ -1202,16 +1261,24 @@ impl super::Nexus { /// this sled, this operation rudely terminates it. pub(crate) async fn instance_ensure_unregistered( &self, + opctx: &OpContext, propolis_id: &PropolisUuid, sled_id: &SledUuid, ) -> Result, InstanceStateChangeError> { let sa = self.sled_client(&sled_id).await?; - sa.vmm_unregister(propolis_id) - .await - .map(|res| res.into_inner().updated_runtime.map(Into::into)) - .map_err(|e| { - InstanceStateChangeError::SledAgent(SledAgentInstanceError(e)) - }) + match sa.vmm_unregister(propolis_id).await { + Ok(res) => Ok(res.into_inner().updated_runtime.map(Into::into)), + Err(e) => { + let error = SledAgentInstanceError::new( + opctx, + self.datastore(), + *sled_id, + e, + ) + .await; + Err(InstanceStateChangeError::SledAgent(error)) + } + } } /// Determines the action to take on an instance's active VMM given a @@ -1445,8 +1512,9 @@ impl super::Nexus { &VmmPutStateBody { state: requested.into() }, ) .await - .map(|res| res.into_inner().updated_runtime.map(Into::into)) - .map_err(|e| SledAgentInstanceError(e)); + .map(|res| { + res.into_inner().updated_runtime.map(Into::into) + }); // If the operation succeeded, write the instance state back, // returning any subsequent errors that occurred during that @@ -1463,7 +1531,16 @@ impl super::Nexus { .await .map_err(Into::into), Ok(None) => Ok(()), - Err(e) => Err(InstanceStateChangeError::SledAgent(e)), + Err(e) => { + let err = SledAgentInstanceError::new( + opctx, + self.datastore(), + sled_id, + e, + ) + .await; + Err(InstanceStateChangeError::SledAgent(err)) + } } } } @@ -1728,7 +1805,8 @@ impl super::Nexus { }; let instance_id = InstanceUuid::from_untyped_uuid(db_instance.id()); - let sa = self.sled_client(&initial_vmm.sled_id()).await?; + let sled_id = initial_vmm.sled_id(); + let sa = self.sled_client(&sled_id).await?; // The state of a freshly-created VMM record in the database is always // `VmmState::Creating`. Based on whether this VMM is created in order // to start or migrate an instance, determine the VMM state we will want @@ -1765,8 +1843,7 @@ impl super::Nexus { }, ) .await - .map(|res| res.into_inner().into()) - .map_err(|e| SledAgentInstanceError(e)); + .map(|res| res.into_inner().into()); match instance_register_result { Ok(state) => { @@ -1783,20 +1860,27 @@ impl super::Nexus { }) } Err(e) => { + let err = SledAgentInstanceError::new( + opctx, + self.datastore(), + sled_id, + e, + ) + .await; // If this error indicates that the VMM should be marked as // `Failed`, do that now. - if let Some(reason) = e.vmm_failure_reason() { + if let Some(reason) = err.vmm_failure_reason() { let _ = self .mark_vmm_failed( &opctx, authz_instance.clone(), &initial_vmm, - &e, + &err, reason, ) .await; } - Err(InstanceStateChangeError::SledAgent(e)) + Err(InstanceStateChangeError::SledAgent(err)) } } } @@ -1823,7 +1907,7 @@ impl super::Nexus { authz_instance: authz::Instance, vmm: &db::model::Vmm, error: &SledAgentInstanceError, - reason: db::model::VmmFailureReason, + reason: VmmFailureReason, ) -> Result<(), Error> { let instance_id = InstanceUuid::from_untyped_uuid(authz_instance.id()); let vmm_id = PropolisUuid::from_untyped_uuid(vmm.id); @@ -1838,7 +1922,7 @@ impl super::Nexus { let new_runtime = vmm .runtime() - .transition(nexus_types::instance::VmmState::Failed(reason.into())); + .transition(nexus_types::instance::VmmState::Failed(reason)); match self.db_datastore.vmm_update_runtime(&vmm_id, &new_runtime).await { diff --git a/nexus/src/app/sagas/instance_common.rs b/nexus/src/app/sagas/instance_common.rs index 99d3d2e4a28..4bc2e5630e5 100644 --- a/nexus/src/app/sagas/instance_common.rs +++ b/nexus/src/app/sagas/instance_common.rs @@ -20,9 +20,13 @@ use nexus_db_queries::db::datastore::ExternalSubnetBeginOpResult; use nexus_db_queries::db::datastore::sled::SledReservationReason; use nexus_db_queries::{authn, context::OpContext, db, db::DataStore}; use nexus_types::saga::saga_action_failed; -use omicron_common::api::external::{Error, IpVersion, NameOrId}; +use omicron_common::api::external::{Error, IpVersion, NameOrId, ResourceType}; +use omicron_common::backoff::{ + BackoffError, retry_notify_ext, retry_policy_internal_service, +}; use omicron_uuid_kinds::{GenericUuid, InstanceUuid, PropolisUuid, SledUuid}; use serde::{Deserialize, Serialize}; +use slog_error_chain::InlineErrorChain; use steno::ActionError; /// The port propolis-server listens on inside the propolis zone. @@ -690,6 +694,138 @@ pub(super) async fn instance_ip_remove_opte( Ok(()) } +pub(super) async fn unwind_instance_ensure_registered( + sagactx: &NexusActionContext, + serialized_authn: &authn::saga::Serialized, + instance_id: &InstanceUuid, + propolis_id: &PropolisUuid, + sled_id: &SledUuid, +) { + use crate::app::instance::InstanceStateChangeError as ChangeError; + use crate::app::instance::SledAgentInstanceError as SledAgentError; + + let osagactx = sagactx.user_data(); + let opctx = + crate::context::op_context_for_saga_action(&sagactx, serialized_authn); + + retry_notify_ext( + retry_policy_internal_service(), + || async { + let unregister_result = osagactx + .nexus() + .instance_ensure_unregistered(&opctx, &propolis_id, &sled_id) + .await; + match unregister_result { + // We have unregistered the instance successfully. That's great. + Ok(_) => { + debug!( + osagactx.log(), + "unwinding saga: instance unregistered from sled"; + "instance_id" => %instance_id, + "propolis_id" => %propolis_id, + "sled_id" => %sled_id, + ); + Ok(()) + } + // If the sled-agent reports that it does not know about this + // VMM, that's fine. This indicates either that this undo action + // has already executed and successfully unregistered the VMM, + // or that the action which registered the VMM with the sled ` + // failed *before* the `instance_ensure_registered` request was + // sent to the sled-agent in the first place. In either case, we + // now know affirmatively that there is no Propolis zone we must + // clean up, so we may safely continue unwinding. + Err(ChangeError::SledAgent(SledAgentError::VmmGone { + .. + })) => { + debug!( + osagactx.log(), + "unwinding saga: instance already unregistered from \ + sled, continuing to unwind"; + "instance_id" => %instance_id, + "propolis_id" => %propolis_id, + "sled_id" => %sled_id, + ); + Ok(()) + } + // If the sled is no longer in service, give up on trying to + // talk to it and similarly assume that the VMM is gone. + Err(ChangeError::SledAgent( + SledAgentError::SledNotInService { .. }, + )) => { + debug!( + osagactx.log(), + "unwinding saga: the sled this instance may have been \ + started on has been expunged, assuming the VMM is now \ + gone"; + "instance_id" => %instance_id, + "propolis_id" => %propolis_id, + "sled_id" => %sled_id, + ); + Ok(()) + } + // Well, that's real sketchy: the sled record for the sled with + // this ID was not found. Our only real options here are to + // either assume that this means anything that may have been + // running on that sled is similarly erased from reality, or to + // return `ActionFailed` and let the saga get stuck unwinding, + // requiring support intervention to un-stick. Soo. Let's say + // it's probably gone. + Err(ChangeError::Other(Error::ObjectNotFound { + type_name: ResourceType::Sled, + .. + })) => { + warn!( + osagactx.log(), + "unwinding saga::attempting to unregister instance \ + from sled, but the sled appears to no longer exist in \ + the database! hopefully, this means the VMM is also \ + gone..."; + "instance_id" => %instance_id, + "propolis_id" => %propolis_id, + "sled_id" => %sled_id, + ); + Ok(()) + } + // Any other error encountered here must be retried + // indefinitely, since we cannot be certain as to whether or not + // a Propolis zone started by this saga still exists. + Err(e) => Err(BackoffError::transient(e)), + } + }, + |err, tries, elapsed| { + let error = InlineErrorChain::new(&err); + if tries == 0 { + info!( + osagactx.log(), + "unwinding saga: retrying attempt to unregister instance \ + from sled"; + "instance_id" => %instance_id, + "propolis_id" => %propolis_id, + "sled_id" => %sled_id, + error + ); + } else { + warn!( + osagactx.log(), + "unwinding saga: {} attempts to unregister instance from \ + sled have failed, retrying again...", tries + 1; + "instance_id" => %instance_id, + "propolis_id" => %propolis_id, + "sled_id" => %sled_id, + "elapsed" => ?elapsed, + error + ); + } + }, + ) + .await + .expect( + "infinite retry loop never fails, as we never return a \ + `BackoffError::permanent`", + ); +} + #[derive(Clone, Debug, Deserialize, Serialize)] pub enum ExternalIpAttach { Ephemeral { pool: Option, ip_version: Option }, diff --git a/nexus/src/app/sagas/instance_migrate.rs b/nexus/src/app/sagas/instance_migrate.rs index e773f090627..e3d990826bf 100644 --- a/nexus/src/app/sagas/instance_migrate.rs +++ b/nexus/src/app/sagas/instance_migrate.rs @@ -517,6 +517,7 @@ async fn sim_ensure_destination_propolis_undo( sagactx: NexusActionContext, ) -> Result<(), anyhow::Error> { let osagactx = sagactx.user_data(); + let params = sagactx.saga_params::()?; let dst_propolis_id = sagactx.lookup::("dst_propolis_id")?; let dst_sled_id = sagactx.lookup::("dst_sled_id")?; let db_instance = @@ -533,17 +534,15 @@ async fn sim_ensure_destination_propolis_undo( // of unwind to take care of cleaning up the migration IDs in the instance // record. Otherwise the unwind has failed and manual intervention is // needed. - match osagactx - .nexus() - .instance_ensure_unregistered(&dst_propolis_id, &dst_sled_id) - .await - { - Ok(_) => Ok(()), - Err(InstanceStateChangeError::SledAgent(inner)) => { - if !inner.vmm_gone() { Ok(()) } else { Err(inner.0.into()) } - } - Err(e) => Err(e.into()), - } + super::instance_common::unwind_instance_ensure_registered( + &sagactx, + ¶ms.serialized_authn, + &InstanceUuid::from_untyped_uuid(db_instance.id()), + &dst_propolis_id, + &dst_sled_id, + ) + .await; + Ok(()) } async fn sim_instance_migrate( @@ -609,9 +608,11 @@ async fn sim_instance_migrate( "instance_id" => %db_instance.id(), "error" => ?inner); - Err(saga_action_failed(omicron_common::api::external::Error::from( - inner, - ))) + Err(saga_action_failed( + // TODO(eliza): want to get the failure reason written to the + // unwound VMM record here... + omicron_common::api::external::Error::from(inner), + )) } Err(InstanceStateChangeError::Other(inner)) => { info!(osagactx.log(), diff --git a/nexus/src/app/sagas/instance_start.rs b/nexus/src/app/sagas/instance_start.rs index f278b4ed059..176f2b270ab 100644 --- a/nexus/src/app/sagas/instance_start.rs +++ b/nexus/src/app/sagas/instance_start.rs @@ -46,7 +46,6 @@ use progenitor_extras::retry::{ use seq_macro::seq; use serde::{Deserialize, Serialize}; use sled_agent_client::types::LocalStorageDatasetEnsureRequest; -use slog::error; use slog::info; use steno::ActionError; @@ -928,15 +927,9 @@ async fn sis_ensure_registered_undo( ) -> Result<(), anyhow::Error> { let osagactx = sagactx.user_data(); let params = sagactx.saga_params::()?; - let datastore = osagactx.datastore(); let instance_id = InstanceUuid::from_untyped_uuid(params.db_instance.id()); let propolis_id = sagactx.lookup::("propolis_id")?; let sled_id = sagactx.lookup::("sled_id")?; - let db_vmm = sagactx.lookup::(REGISTERED_VMM_RECORD)?; - let opctx = crate::context::op_context_for_saga_action( - &sagactx, - ¶ms.serialized_authn, - ); info!(osagactx.log(), "start saga: unregistering instance from sled"; "instance_id" => %instance_id, @@ -944,104 +937,15 @@ async fn sis_ensure_registered_undo( "sled_id" => %sled_id, "start_reason" => ?params.reason); - // Fetch the latest record so that this callee can drive the instance into - // a Failed state if the unregister call fails. - let (.., authz_instance, _) = LookupPath::new(&opctx, datastore) - .instance_id(instance_id.into_untyped_uuid()) - .fetch() - .await - .map_err(saga_action_failed)?; - - // If the sled successfully unregistered the instance, allow the rest of - // saga unwind to restore the instance record to its prior state (without - // writing back the state returned from sled agent). Otherwise, try to - // reason about the next action from the specific kind of error that was - // returned. - let unregister_result = osagactx - .nexus() - .instance_ensure_unregistered(&propolis_id, &sled_id) - .await; - - // Handle the unregister result - if let Err(e) = unregister_result { - error!(osagactx.log(), - "start saga: failed to unregister instance from sled"; - "instance_id" => %instance_id, - "start_reason" => ?params.reason, - "error" => ?e); - - // If the failure came from talking to sled agent, and the error code - // indicates the instance or sled might be unhealthy, manual - // intervention is likely to be needed, so try to mark the instance as - // Failed and then bail on unwinding. - // - // If sled agent is in good shape but just doesn't know about the - // instance, this saga still owns the instance's state, so allow - // unwinding to continue. - // - // If some other Nexus error occurred, this saga is in bad shape, so - // return an error indicating that intervention is needed without trying - // to modify the instance further. - // - // TODO(#3238): `instance_unhealthy` does not take an especially nuanced - // view of the meanings of the error codes sled agent could return, so - // assuming that an error that isn't `instance_unhealthy` means - // that everything is hunky-dory and it's OK to continue unwinding may - // be a bit of a stretch. See the definition of `instance_unhealthy` for - // more details. - match e { - InstanceStateChangeError::SledAgent(inner) => { - if let Some(reason) = inner.vmm_failure_reason() { - error!(osagactx.log(), - "start saga: failing instance after unregister failure"; - "instance_id" => %instance_id, - "start_reason" => ?params.reason, - "error" => ?inner, - "reason" => %reason); - - if let Err(set_failed_error) = osagactx - .nexus() - .mark_vmm_failed( - &opctx, - authz_instance, - &db_vmm, - &inner, - reason, - ) - .await - { - error!(osagactx.log(), - "start saga: failed to mark instance as failed"; - "instance_id" => %instance_id, - "start_reason" => ?params.reason, - "error" => ?set_failed_error); - - Err(set_failed_error.into()) - } else { - Err(inner.0.into()) - } - } else { - info!(osagactx.log(), - "start saga: instance already unregistered from sled"; - "instance_id" => %instance_id, - "start_reason" => ?params.reason); - - Ok(()) - } - } - InstanceStateChangeError::Other(inner) => { - error!(osagactx.log(), - "start saga: internal error unregistering instance"; - "instance_id" => %instance_id, - "start_reason" => ?params.reason, - "error" => ?inner); - - Err(inner.into()) - } - } - } else { - Ok(()) - } + super::instance_common::unwind_instance_ensure_registered( + &sagactx, + ¶ms.serialized_authn, + &instance_id, + &propolis_id, + &sled_id, + ) + .await; + Ok(()) } async fn sis_update_multicast_sled_id(