From 341968c4622a0e44b2da9bb6b0be8bec25da1537 Mon Sep 17 00:00:00 2001 From: karencfv Date: Thu, 15 Oct 2026 19:48:29 +1300 Subject: [PATCH 01/19] [live-update] Modify instance reincarnation to account for sled evacuations --- nexus/db-queries/src/db/datastore/instance.rs | 10 ++++++++++ .../src/app/background/tasks/instance_reincarnation.rs | 3 +++ nexus/types/src/internal_api/background.rs | 3 +++ 3 files changed, 16 insertions(+) diff --git a/nexus/db-queries/src/db/datastore/instance.rs b/nexus/db-queries/src/db/datastore/instance.rs index 8065f606918..b4f5a8b3b69 100644 --- a/nexus/db-queries/src/db/datastore/instance.rs +++ b/nexus/db-queries/src/db/datastore/instance.rs @@ -464,6 +464,16 @@ impl DataStore { ) .await } + ReincarnationReason::SledEvacuation => { + // The instance must be in the SledEvacuating state. + q.filter(dsl::state.eq(InstanceState::SledEvacuating)) + .filter(dsl::active_propolis_id.is_null()) + .select(Instance::as_select()) + .load_async::( + &*self.pool_connection_authorized(opctx).await?, + ) + .await + } } .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } diff --git a/nexus/src/app/background/tasks/instance_reincarnation.rs b/nexus/src/app/background/tasks/instance_reincarnation.rs index 50f9878d2e0..8a9851576dc 100644 --- a/nexus/src/app/background/tasks/instance_reincarnation.rs +++ b/nexus/src/app/background/tasks/instance_reincarnation.rs @@ -611,6 +611,9 @@ mod test { } } + // TODO-K: Add a test here that calls the vmm_stop_for_update background + // task first + #[nexus_test(server = crate::Server)] async fn test_reincarnates_failed_instances( cptestctx: &ControlPlaneTestContext, diff --git a/nexus/types/src/internal_api/background.rs b/nexus/types/src/internal_api/background.rs index 19cbf05bb01..39e68618f15 100644 --- a/nexus/types/src/internal_api/background.rs +++ b/nexus/types/src/internal_api/background.rs @@ -229,6 +229,8 @@ pub enum ReincarnationReason { Failed, /// A previous instance-start saga for this instance has failed. SagaUnwound, + /// A sled has been evacuated and its instances need to be started again. + SledEvacuation, } impl std::fmt::Display for ReincarnationReason { @@ -236,6 +238,7 @@ impl std::fmt::Display for ReincarnationReason { f.write_str(match self { Self::Failed => "instance failed", Self::SagaUnwound => "start saga failed", + Self::SledEvacuation => "sled evacuated", }) } } From 701f1c6d70ff71fabc3bcebf65ef32f924f7b96e Mon Sep 17 00:00:00 2001 From: karencfv Date: Fri, 16 Oct 2026 15:23:51 +1300 Subject: [PATCH 02/19] polish --- nexus/db-model/src/instance.rs | 48 +++++++++++++++++----- nexus/src/app/sagas/instance_update/mod.rs | 15 +++---- 2 files changed, 45 insertions(+), 18 deletions(-) diff --git a/nexus/db-model/src/instance.rs b/nexus/db-model/src/instance.rs index b7298886736..e07a6f0516a 100644 --- a/nexus/db-model/src/instance.rs +++ b/nexus/db-model/src/instance.rs @@ -240,17 +240,25 @@ impl Instance { active_vmm: Option<&Vmm>, ) -> InstanceKarmicStatus { let runtime = self.runtime(); - // Instances only need to be automatically restarted if they are in the - // `Failed` state, or if their active VMM is in the `SagaUnwound` state. + // Instances only need to be automatically restarted if they are in + // either one of the `Failed` or `SledEvacuating` states, or if their + // active VMM is in the `SagaUnwound` state. // - // If the instance's intended state is not Running, it should - // not be reincarnated, even if it has failed. + // If the instance's intended state is not Running, it should not be + // reincarnated, even if it has failed or its VMM's sled is being + // evacuated. let needs_reincarnation = match (self.intended_state, self.nexus_state, active_vmm) { - (IntendedState::Running, InstanceState::Failed, _vmm) => { + ( + IntendedState::Running, + InstanceState::Failed | InstanceState::SledEvacuating, + _vmm, + ) => { debug_assert!( _vmm.is_none(), - "a Failed instance will never have an active VMM!" + "an instance in a {} state will never have an active \ + VMM!", + self.nexus_state ); true } @@ -473,11 +481,29 @@ impl InstanceAutoRestart { &self, state: &InstanceRuntimeState, ) -> Reincarnatability { - // Check if the instance's configured auto-restart policy permits the - // control plane to automatically restart it. - let policy = self.policy.unwrap_or(Self::DEFAULT_POLICY); - if policy == InstanceAutoRestartPolicy::Never { - return Reincarnatability::Nirvana; + // There is not much need for a `match` here, but we use one in case a + // new state is introduced. This forces the person introducing the new + // state to decide whether this is a state where we are going to honour + // the auto-restart policy or we are forced to override it. + match state.nexus_state { + InstanceState::Failed + | InstanceState::Creating + | InstanceState::Destroyed + | InstanceState::NoVmm + | InstanceState::Vmm => { + // Check if the instance's configured auto-restart policy permits the + // control plane to automatically restart it. + let policy = self.policy.unwrap_or(Self::DEFAULT_POLICY); + if policy == InstanceAutoRestartPolicy::Never { + return Reincarnatability::Nirvana; + } + } + // An instance that is in the `SledEvacuating` state will always be + // reincarnatable. This is because the instance has not actually failed, + // but rather its VMM has been stopped due to its sled being evacuated. + // The instance should be restarted automatically regardless of the + // auto-restart policy. + InstanceState::SledEvacuating => {} } // If the instance is permitted to reincarnate, ensure that its last diff --git a/nexus/src/app/sagas/instance_update/mod.rs b/nexus/src/app/sagas/instance_update/mod.rs index 4d99e6592a4..4378bfed94c 100644 --- a/nexus/src/app/sagas/instance_update/mod.rs +++ b/nexus/src/app/sagas/instance_update/mod.rs @@ -1601,19 +1601,20 @@ async fn siu_chain_successor_saga( } fn reincarnate_if_needed(osagactx: &SagaContext, state: &InstanceGestalt) { - // If the instance has transitioned to the `Failed` state and no - // additional update saga is required, check if the instance's - // auto-restart policy allows it to be automatically restarted. If - // it does, activate the instance-reincarnation background task to - // automatically restart it. + // Check if the instance has transitioned to one of `Failed` or + // `SledEvacuating` states, and no additional update saga is required. + // Unless the instance is in the `SledEvacuating` state, check if the + // instance's auto-restart policy allows it to be automatically restarted. + // If these conditions are true, activate the instance-reincarnation + // background task to automatically restart it. let karmic_state = state.instance.auto_restart_status(state.active_vmm.as_ref()); if karmic_state.should_reincarnate() { info!( &osagactx.log(), - "instance update: instance transitioned to Failed, \ + "instance update: instance transitioned to {}, \ but can be automatically restarted; activating \ - reincarnation."; + reincarnation.", state.instance.nexus_state; "instance_id" => %state.instance.id(), "auto_restart_config" => ?state.instance.auto_restart, "runtime_state" => ?state.instance.runtime(), From 6ee6cd90a863548c36ba30bf53e8854100027612 Mon Sep 17 00:00:00 2001 From: karencfv Date: Wed, 28 Oct 2026 16:12:24 +1300 Subject: [PATCH 03/19] ensure SledEvacuating instances are reincarnated and test --- nexus/db-model/src/instance.rs | 5 +- .../tasks/instance_reincarnation.rs | 72 ++++++++++++++++++- 2 files changed, 75 insertions(+), 2 deletions(-) diff --git a/nexus/db-model/src/instance.rs b/nexus/db-model/src/instance.rs index 2a5b7252ed9..ce5d3e5a4c3 100644 --- a/nexus/db-model/src/instance.rs +++ b/nexus/db-model/src/instance.rs @@ -564,7 +564,10 @@ impl InstanceAutoRestart { .eq(InstanceAutoRestartPolicy::BestEffort) // If the auto-restart policy is null, then it should // default to "best effort". - .or(dsl::auto_restart_policy.is_null())) + .or(dsl::auto_restart_policy.is_null()) + // When an instance is rebooting due to its sled evacuating, it + // should be reincarnatable + .or(dsl::state.eq(InstanceState::SledEvacuating))) // An instance should only be automatically restarted if it is intended // to be in the `Running` state. Instances which were requested to stop // should not be automatically restarted, even if they have failed. diff --git a/nexus/src/app/background/tasks/instance_reincarnation.rs b/nexus/src/app/background/tasks/instance_reincarnation.rs index 034730d7ea7..3fadeb93725 100644 --- a/nexus/src/app/background/tasks/instance_reincarnation.rs +++ b/nexus/src/app/background/tasks/instance_reincarnation.rs @@ -104,6 +104,26 @@ impl BackgroundTask for InstanceReincarnation { )); } + if let Err(error) = self + .reincarnate_all( + &opctx, + ReincarnationReason::SledEvacuation, + &mut status, + &mut running_sagas, + ) + .await + { + error!( + opctx.log, + "failed to find all instances from a sled evacuation \ + in need of reincarnation"; + "error" => %error, + ); + status.errors.push(format!( + "finding instances from a sled evacuation: {error}" + )); + } + if status.total_errors() > 0 { warn!( &opctx.log, @@ -399,7 +419,7 @@ mod test { // `instance-update` saga do so). Therefore, make our // resource requests as small as possible. ncpus: 1i64.try_into().unwrap(), - memory: ByteCount::from_gibibytes_u32(2), + memory: ByteCount::from_gibibytes_u32(1), hostname: "myhostname".try_into().unwrap(), user_data: Vec::new(), network_interfaces: @@ -773,6 +793,42 @@ mod test { }); } + // Create instances in the `SledEvacuating` state that are eligible to + // be reincarnated. When an instance state is reincarnated due to its + // sled evacuating, the task ignores the auto-restart policy and always + // restarts the the instance, so we include instances with both policies + let num_sled_evacuating = 4; + for i in 0..num_sled_evacuating / 2 { + let instance = create_instance( + &cptestctx, + &opctx, + &format!("kolankola-{i}"), + InstanceAutoRestartPolicy::BestEffort, + InstanceState::SledEvacuating, + InstanceIntendedState::Running, + ) + .await; + will_reincarnate.insert(ReincarnatableInstance { + instance_id: instance.id(), + reason: ReincarnationReason::SledEvacuation, + }); + } + for i in 0..num_sled_evacuating / 2 { + let instance = create_instance( + &cptestctx, + &opctx, + &format!("ekabijin-{i}"), + InstanceAutoRestartPolicy::Never, + InstanceState::SledEvacuating, + InstanceIntendedState::Running, + ) + .await; + will_reincarnate.insert(ReincarnatableInstance { + instance_id: instance.id(), + reason: ReincarnationReason::SledEvacuation, + }); + } + // Create some instances that will not reincarnate. let mut will_not_reincarnate = std::collections::BTreeSet::new(); // Some instances which are `Failed` but don't have policies permitting @@ -850,6 +906,16 @@ mod test { .await; attach_saga_unwound_vmm(&cptestctx, &opctx, &unwound_stopped).await; will_not_reincarnate.insert(unwound_stopped.id()); + let evacuating_stopped = create_instance( + &cptestctx, + &opctx, + "anagami-6", + InstanceAutoRestartPolicy::BestEffort, + InstanceState::SledEvacuating, + InstanceIntendedState::Stopped, + ) + .await; + will_not_reincarnate.insert(evacuating_stopped.id()); // Activate the task again, and check that our instance had an // instance-start saga started. @@ -863,6 +929,10 @@ mod test { status.instances_found.get(&ReincarnationReason::SagaUnwound), Some(&num_saga_unwound) ); + assert_eq!( + status.instances_found.get(&ReincarnationReason::SledEvacuation), + Some(&num_sled_evacuating) + ); assert_eq!(status.instances_reincarnated.len(), will_reincarnate.len()); assert_eq!(status.changed_state, Vec::new()); assert_eq!(status.errors, Vec::::new()); From 518aa01eb20b772f25e4ec3c2cafb51cde1578a7 Mon Sep 17 00:00:00 2001 From: karencfv Date: Wed, 28 Oct 2026 18:59:27 +1300 Subject: [PATCH 04/19] add test --- .../tasks/instance_reincarnation.rs | 205 +++++++++++++++++- 1 file changed, 202 insertions(+), 3 deletions(-) diff --git a/nexus/src/app/background/tasks/instance_reincarnation.rs b/nexus/src/app/background/tasks/instance_reincarnation.rs index 3fadeb93725..03d4d38113a 100644 --- a/nexus/src/app/background/tasks/instance_reincarnation.rs +++ b/nexus/src/app/background/tasks/instance_reincarnation.rs @@ -341,8 +341,12 @@ impl InstanceReincarnation { #[cfg(test)] mod test { use super::*; + use crate::app::background::tasks::vmm_stop_for_update::VmmStopForUpdate; use crate::app::sagas::test_helpers; + use async_bb8_diesel::AsyncRunQueryDsl; use chrono::Utc; + use diesel::ExpressionMethods; + use nexus_db_lookup::AsyncConnection; use nexus_db_lookup::LookupPath; use nexus_db_model::Generation; use nexus_db_model::InstanceIntendedState; @@ -351,7 +355,9 @@ mod test { use nexus_db_model::Vmm; use nexus_db_model::VmmCpuPlatform; use nexus_db_model::VmmState; + use nexus_db_model::to_db_typed_generation; use nexus_db_queries::authz; + use nexus_test_utils::ControlPlaneBuilder; use nexus_test_utils::resource_helpers::{ create_default_ip_pools, create_project, object_create, }; @@ -362,6 +368,7 @@ mod test { use omicron_common::api::external::ByteCount; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_generation_kinds::InstanceStateGeneration; + use omicron_generation_kinds::UpdateDispositionGeneration; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::InstanceUuid; use omicron_uuid_kinds::ProjectUuid; @@ -607,6 +614,25 @@ mod test { authz_instance } + async fn mark_vmm_stopped_for_update( + conn: &AsyncConnection, + vmm_id: Uuid, + generation: UpdateDispositionGeneration, + ) { + use nexus_db_schema::schema::vmm::dsl; + + let updated = diesel::update(dsl::vmm) + .filter(dsl::id.eq(vmm_id)) + .set( + dsl::stop_for_update_disposition_generation + .eq(Some(to_db_typed_generation(generation))), + ) + .execute_async(conn) + .await + .expect("VMM should be marked"); + assert_eq!(updated, 1, "exactly one VMM should be marked"); + } + // Boilerplate reducer. // // This is a macro so that the `dbg!` has the line number of the place where @@ -632,9 +658,6 @@ mod test { } } - // TODO-K: Add a test here that calls the vmm_stop_for_update background - // task first - #[nexus_test(server = crate::Server)] async fn test_reincarnates_failed_instances( cptestctx: &ControlPlaneTestContext, @@ -1082,4 +1105,180 @@ mod test { ) .await; } + + #[tokio::test] + async fn test_reincarnates_instances_in_sled_evacuating() { + // Test setup + let cptestctx = ControlPlaneBuilder::new( + "test_reincarnates_instances_in_sled_evacuating", + ) + .customize_nexus_config(&|config| { + let one_day = Duration::from_secs(86_400); + let tasks = &mut config.pkg.background_tasks; + // The `instance-update` saga activates the real + // `instance_reincarnation` background task when an instance's state + // changes to `SledEvacuating`. Disable that task, so that only the + // task created by this test reincarnates instances. + tasks.instance_reincarnation.disable = true; + // Increase the intervals for background tasks that could modify + // VMM records while this test is running. + tasks.instance_watcher.period_secs = one_day; + tasks.abandoned_vmm_reaper.period_secs = one_day; + tasks.vmm_mark_stop_for_update.period_secs = one_day; + tasks.vmm_stop_for_update.period_secs = one_day; + }) + .start::() + .await; + + let nexus = &cptestctx.server.server_context().nexus; + let datastore = nexus.datastore(); + let opctx = OpContext::for_tests( + cptestctx.logctx.log.clone(), + datastore.clone(), + ); + + setup_test_project(&cptestctx, &opctx).await; + + let mut task = InstanceReincarnation::new( + datastore.clone(), + nexus.sagas.clone(), + false, + Activator::new(), + ); + + let conn = datastore.pool_connection_for_tests().await.unwrap(); + let generation = UpdateDispositionGeneration::from(1); + let mut will_reincarnate = std::collections::BTreeSet::new(); + + // Create running instances with different auto-restart policies, and + // mark their VMMs to be stopped for an update. + for (i, policy) in [ + InstanceAutoRestartPolicy::BestEffort, + InstanceAutoRestartPolicy::Never, + ] + .into_iter() + .enumerate() + { + let instance = create_instance( + &cptestctx, + &opctx, + &format!("sattakkhattaparama-{i}"), + policy, + InstanceState::Vmm, + InstanceIntendedState::Running, + ) + .await; + let instance_id = InstanceUuid::from_untyped_uuid(instance.id()); + test_helpers::instance_simulate(&cptestctx, &instance_id).await; + + let vmm = test_helpers::instance_fetch(&cptestctx, instance_id) + .await + .vmm() + .clone() + .expect("instance should have an active VMM"); + // To avoid complexity, the VMMs are marked directly in the db + // instead of running the `vmm_mark_stop_for_update` task + mark_vmm_stopped_for_update(&conn, vmm.id, generation).await; + will_reincarnate.insert(ReincarnatableInstance { + instance_id: instance.id(), + reason: ReincarnationReason::SledEvacuation, + }); + } + + // The VMMs are marked, but they have not been stopped yet; there is + // nothing to reincarnate. + let status = assert_activation_ok!(task.activate(&opctx).await); + assert_eq!( + status.total_instances_found(), + 0, + "unexpected instances found: {:?}", + status.instances_found + ); + assert_eq!( + status.instances_reincarnated.len(), + 0, + "unexpected instances reincarnated: {:?}", + status.instances_reincarnated + ); + assert_eq!( + status.changed_state.len(), + 0, + "unexpected instances changed state: {:?}", + status.changed_state + ); + + // Run the `vmm_stop_for_update` task to actually stop the marked VMMs. + let mut stop_task = + VmmStopForUpdate::new(datastore.clone(), nexus.sagas.clone()); + let stop_status = stop_task.actually_activate(&opctx).await; + assert_eq!( + stop_status.error_messages.len(), + 0, + "unexpected errors: {:?}", + stop_status.error_messages + ); + assert!( + stop_status.vmms_failed_to_stop_by_sled.is_empty(), + "unexpected vmms failed to stop: {:?}", + stop_status.vmms_failed_to_stop_by_sled + ); + // Make sure the amount of VMMs that stopped is the same as the + // instances we expect will be reincarnated + let stopped_count: usize = stop_status + .vmms_stopped_by_sled + .iter() + .map(|sled| sled.vmm_ids.len()) + .sum(); + assert_eq!(stopped_count, will_reincarnate.len()); + + // Poke each instance, so sled agent reports the stopped VMM to Nexus. + // The `instance-update` saga should change the instance state to + // `SledEvacuating`. + for instance in &will_reincarnate { + let instance_id = + InstanceUuid::from_untyped_uuid(instance.instance_id); + test_helpers::instance_simulate(&cptestctx, &instance_id).await; + test_helpers::instance_wait_for_state( + &cptestctx, + instance_id, + InstanceState::SledEvacuating, + ) + .await; + } + test_helpers::wait_for_running_sagas(&cptestctx).await; + + // Activate the reincarnation task again. Check that every instance had + // an instance-start saga started, regardless of its auto-restart policy + let status = assert_activation_ok!(task.activate(&opctx).await); + assert_eq!( + status.instances_found.get(&ReincarnationReason::SledEvacuation), + Some(&will_reincarnate.len()) + ); + assert_eq!( + status + .instances_reincarnated + .iter() + .copied() + .collect::>(), + will_reincarnate + ); + assert_eq!( + status.changed_state.len(), + 0, + "unexpected instances changed state: {:?}", + status.changed_state + ); + + // Instances reincarnate! + for instance in will_reincarnate { + test_helpers::instance_wait_for_state( + &cptestctx, + InstanceUuid::from_untyped_uuid(instance.instance_id), + InstanceState::Vmm, + ) + .await; + } + + cptestctx.teardown().await; + } } From 1ead204bb3f9b940e01895de5cd79f64c033f622 Mon Sep 17 00:00:00 2001 From: karencfv Date: Wed, 28 Oct 2026 19:11:10 +1300 Subject: [PATCH 05/19] expectorate --- dev-tools/omdb/tests/successes.out | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dev-tools/omdb/tests/successes.out b/dev-tools/omdb/tests/successes.out index bd7800adaaa..af6e3b5cc9e 100644 --- a/dev-tools/omdb/tests/successes.out +++ b/dev-tools/omdb/tests/successes.out @@ -950,6 +950,7 @@ task: "instance_reincarnation" instances eligible for reincarnation: 0 instance failed instances: 0 start saga failed instances: 0 + sled evacuated instances: 0 instances reincarnated successfully: 0 instances which changed state before they could reincarnate: 0 instances which failed to reincarnate: 0 @@ -1705,6 +1706,7 @@ task: "instance_reincarnation" instances eligible for reincarnation: 0 instance failed instances: 0 start saga failed instances: 0 + sled evacuated instances: 0 instances reincarnated successfully: 0 instances which changed state before they could reincarnate: 0 instances which failed to reincarnate: 0 From b28318bf96321df205f5ebc4e24d255fffe6019c Mon Sep 17 00:00:00 2001 From: karencfv Date: Wed, 28 Oct 2026 19:29:30 +1300 Subject: [PATCH 06/19] typos --- nexus/src/app/background/tasks/instance_reincarnation.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/nexus/src/app/background/tasks/instance_reincarnation.rs b/nexus/src/app/background/tasks/instance_reincarnation.rs index 03d4d38113a..16caae0e5b7 100644 --- a/nexus/src/app/background/tasks/instance_reincarnation.rs +++ b/nexus/src/app/background/tasks/instance_reincarnation.rs @@ -817,9 +817,9 @@ mod test { } // Create instances in the `SledEvacuating` state that are eligible to - // be reincarnated. When an instance state is reincarnated due to its - // sled evacuating, the task ignores the auto-restart policy and always - // restarts the the instance, so we include instances with both policies + // be reincarnated. When an instance is reincarnated due to its sled + // evacuating, the task ignores the auto-restart policy and always + // restarts the instance, so we include instances with both policies let num_sled_evacuating = 4; for i in 0..num_sled_evacuating / 2 { let instance = create_instance( @@ -1162,7 +1162,7 @@ mod test { let instance = create_instance( &cptestctx, &opctx, - &format!("sattakkhattaparama-{i}"), + &format!("sattakkhattuparama-{i}"), policy, InstanceState::Vmm, InstanceIntendedState::Running, From b03ce368d6a0cbcb26506b349020a03a9db3fa05 Mon Sep 17 00:00:00 2001 From: karencfv Date: Wed, 28 Oct 2026 20:22:01 +1300 Subject: [PATCH 07/19] fix test --- nexus/src/app/background/tasks/vmm_stop_for_update.rs | 7 +++++++ 1 file changed, 7 insertions(+) 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..c3c665b0bf2 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -883,6 +883,13 @@ mod tests { .customize_nexus_config(&|config| { let one_day = Duration::from_secs(86_400); let tasks = &mut config.pkg.background_tasks; + // The `instance-update` saga activates the real + // `instance_reincarnation` background task when an + // instance's state changes to `SledEvacuating`. Disable + // that task so we can accurately verify the instance + // ends up in that state after the `vmm_stop_for_update` + // task runs. + tasks.instance_reincarnation.disable = true; tasks.instance_watcher.period_secs = one_day; tasks.abandoned_vmm_reaper.period_secs = one_day; tasks.vmm_mark_stop_for_update.period_secs = one_day; From 740b8dd4a189ad07f7cf13c60c87a085f76f7600 Mon Sep 17 00:00:00 2001 From: karencfv Date: Mon, 2 Nov 2026 20:09:36 +1300 Subject: [PATCH 08/19] small cleanup --- nexus/db-model/src/instance.rs | 2 +- nexus/src/app/background/tasks/instance_reincarnation.rs | 2 ++ nexus/src/app/sagas/instance_update/mod.rs | 6 ------ 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/nexus/db-model/src/instance.rs b/nexus/db-model/src/instance.rs index ce5d3e5a4c3..6aff23c9a16 100644 --- a/nexus/db-model/src/instance.rs +++ b/nexus/db-model/src/instance.rs @@ -263,7 +263,7 @@ impl Instance { ) => { debug_assert!( _vmm.is_none(), - "an instance in a {} state will never have an active \ + "an instance in the {} state will never have an active \ VMM!", self.nexus_state ); diff --git a/nexus/src/app/background/tasks/instance_reincarnation.rs b/nexus/src/app/background/tasks/instance_reincarnation.rs index 16caae0e5b7..63892ff55ef 100644 --- a/nexus/src/app/background/tasks/instance_reincarnation.rs +++ b/nexus/src/app/background/tasks/instance_reincarnation.rs @@ -929,6 +929,8 @@ mod test { .await; attach_saga_unwound_vmm(&cptestctx, &opctx, &unwound_stopped).await; will_not_reincarnate.insert(unwound_stopped.id()); + // TODO-K: check this case. The instance's external state would + // permanently be "rebooting", which is wrong let evacuating_stopped = create_instance( &cptestctx, &opctx, diff --git a/nexus/src/app/sagas/instance_update/mod.rs b/nexus/src/app/sagas/instance_update/mod.rs index a61bfde6ea2..7173eca5798 100644 --- a/nexus/src/app/sagas/instance_update/mod.rs +++ b/nexus/src/app/sagas/instance_update/mod.rs @@ -1622,12 +1622,6 @@ async fn siu_chain_successor_saga( } fn reincarnate_if_needed(osagactx: &SagaContext, state: &InstanceGestalt) { - // Check if the instance has transitioned to one of `Failed` or - // `SledEvacuating` states, and no additional update saga is required. - // Unless the instance is in the `SledEvacuating` state, check if the - // instance's auto-restart policy allows it to be automatically restarted. - // If these conditions are true, activate the instance-reincarnation - // background task to automatically restart it. let karmic_state = state.instance.auto_restart_status(state.active_vmm.as_ref()); if karmic_state.should_reincarnate() { From e143283f4817ff5bdd7c03bf8e6077a2d5144fcc Mon Sep 17 00:00:00 2001 From: karencfv Date: Mon, 2 Nov 2026 21:09:02 +1300 Subject: [PATCH 09/19] set to SledEvacuating only if intended state is Running --- nexus/src/app/sagas/instance_update/mod.rs | 84 +++++++++++++++++++++- 1 file changed, 83 insertions(+), 1 deletion(-) diff --git a/nexus/src/app/sagas/instance_update/mod.rs b/nexus/src/app/sagas/instance_update/mod.rs index 7173eca5798..1e2e514693f 100644 --- a/nexus/src/app/sagas/instance_update/mod.rs +++ b/nexus/src/app/sagas/instance_update/mod.rs @@ -751,7 +751,10 @@ impl UpdatesRequired { // We no longer have a VMM. new_runtime.nexus_state = if active_vmm_failed { InstanceState::Failed - } else if let Some(g) = active_vmm_update_disposition_gen { + } else if let Some(g) = active_vmm_update_disposition_gen + && snapshot.instance.intended_state + == InstanceIntendedState::Running + { info!( log, "instance update: VMM was stopped due to its sled being \ @@ -2055,6 +2058,85 @@ mod test { .await; } + #[test] + fn test_ensure_sled_evacuating_only_if_intended_state_running() { + let logctx = omicron_test_utils::dev::test_setup_log( + "test_ensure_sled_evacuating_only_if_intended_state_running", + ); + + let params = instance_types::InstanceCreate { + identity: IdentityMetadataCreateParams { + name: INSTANCE_NAME.parse().unwrap(), + description: format!("instance {:?}", INSTANCE_NAME), + }, + ncpus: InstanceCpuCount(1), + memory: ByteCount::from_gibibytes_u32(1), + hostname: INSTANCE_NAME.parse().unwrap(), + user_data: Vec::new(), + ssh_public_keys: None, + network_interfaces: + instance_types::InstanceNetworkInterfaceAttachment::None, + external_ips: Vec::new(), + disks: Vec::new(), + boot_disk: None, + cpu_platform: None, + start: true, + auto_restart_policy: None, + anti_affinity_groups: Vec::new(), + multicast_groups: Vec::new(), + enable_jumbo_frames: false, + }; + + // Check the instance state changes to `SledEvacuating` only if it's + // intended to be running. Otherwise, it should change to `NoVmm` + for (intended_state, expected_instance_state) in [ + (InstanceIntendedState::Running, InstanceState::SledEvacuating), + (InstanceIntendedState::Stopped, InstanceState::NoVmm), + (InstanceIntendedState::GuestShutdown, InstanceState::NoVmm), + (InstanceIntendedState::Destroyed, InstanceState::NoVmm), + ] { + let instance_id = InstanceUuid::new_v4(); + let vmm_id = PropolisUuid::new_v4(); + + let mut instance = + Instance::new(instance_id, ProjectUuid::new_v4(), ¶ms); + instance.nexus_state = InstanceState::Vmm; + instance.propolis_id = Some(vmm_id.into_untyped_uuid()); + instance.intended_state = intended_state; + + let mut vmm = Vmm::new( + vmm_id, + instance_id, + SledUuid::new_v4(), + "10.1.9.42".parse().unwrap(), + 420, + db::model::VmmCpuPlatform::SledDefault, + ); + vmm.state = VmmState::Destroyed; + vmm.stop_for_update_disposition_generation = Some( + omicron_generation_kinds::UpdateDispositionGeneration::from(1) + .into(), + ); + + let snapshot = InstanceGestalt { + instance, + active_vmm: Some(vmm), + target_vmm: None, + migration: None, + }; + let update = UpdatesRequired::for_instance(&logctx.log, &snapshot) + .expect("the destroyed active VMM requires an update"); + + let found_instance_state = update.new_runtime.nexus_state; + assert_eq!( + found_instance_state, expected_instance_state, + "unexpected instance state when the intended state is {intended_state:?}", + ); + } + + logctx.cleanup_successful(); + } + // === Active VMM destroyed tests === #[nexus_test(server = crate::Server)] From e366718f2d06b9dc89b33e5ed7674fdaf6b3a3c4 Mon Sep 17 00:00:00 2001 From: karencfv Date: Mon, 2 Nov 2026 21:28:49 +1300 Subject: [PATCH 10/19] fix query --- nexus/db-model/src/instance.rs | 52 +++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 20 deletions(-) diff --git a/nexus/db-model/src/instance.rs b/nexus/db-model/src/instance.rs index 6aff23c9a16..a658cd38080 100644 --- a/nexus/db-model/src/instance.rs +++ b/nexus/db-model/src/instance.rs @@ -535,7 +535,8 @@ impl InstanceAutoRestart { } /// Filters a database query to include only instances whose auto-restart - /// configs permit them to reincarnate. + /// configs permit them to reincarnate, or are being evacuated from their + /// sled. /// /// Yes, this should probably be in `nexus-db-queries`, but it seemed nice /// for it to be defined on the same struct as the in-memory logic @@ -554,27 +555,23 @@ impl InstanceAutoRestart { let now = diesel::dsl::now.into_sql::(); + // Query fragment for selecting *failed* instances that are eligible to + // restart. + // // The instance's auto-restart policy must allow the control plane // to restart it automatically. // // N.B. that this may become more complex in the future if we grow // additional auto-restart policies that require additional logic // (such as restart limits...) - (dsl::auto_restart_policy + let can_restart_from_failure = (dsl::auto_restart_policy .eq(InstanceAutoRestartPolicy::BestEffort) // If the auto-restart policy is null, then it should // default to "best effort". - .or(dsl::auto_restart_policy.is_null()) - // When an instance is rebooting due to its sled evacuating, it - // should be reincarnatable - .or(dsl::state.eq(InstanceState::SledEvacuating))) - // An instance should only be automatically restarted if it is intended - // to be in the `Running` state. Instances which were requested to stop - // should not be automatically restarted, even if they have failed. - .and(dsl::intended_state.eq(IntendedState::Running)) - // instance whose last reincarnation was within the cooldown interval - // from now must remain in _bardo_ --- the liminal state between death - // and rebirth --- before its next reincarnation. + .or(dsl::auto_restart_policy.is_null())) + // An instance whose last reincarnation was within the cooldown from now + // must remain in _bardo_ --- the liminal state between death and + // rebirth --- before its next reincarnation. .and( // If the instance has never previously been reincarnated, then // it's allowed to reincarnate. @@ -591,13 +588,28 @@ impl InstanceAutoRestart { dsl::time_last_auto_restarted .le((now - Self::DEFAULT_COOLDOWN).nullable()), )), - ) - // Deleted instances may not be reincarnated. - .and(dsl::time_deleted.is_null()) - // If the instance is currently in the process of being updated, - // let's not mess with it for now and try to restart it on another - // pass. - .and(dsl::updater_id.is_null()) + ); + + // An instance should only be automatically restarted if it is intended + // to be in the `Running` state. Instances which were requested to stop + // should not be automatically restarted, even if they have failed. + dsl::intended_state + .eq(IntendedState::Running) + // Either the failed-instance preduicate from above matches, *or* + // the instance is rebooting due to its sled evacuating. When an + // instance had to stop for sled evacuation, can always be + // reincarnated, regardless of its auto-restart policy and when it + // last reincarnated. + .and( + can_restart_from_failure + .or(dsl::state.eq(InstanceState::SledEvacuating)), + ) + // Deleted instances may not be reincarnated. + .and(dsl::time_deleted.is_null()) + // If the instance is currently in the process of being updated, + // let's not mess with it for now and try to restart it on another + // pass. + .and(dsl::updater_id.is_null()) } } /// The parts of an Instance that can be directly updated after creation. From 96a3df05d0ded5fec2e58fd2edbdb19345e28dfd Mon Sep 17 00:00:00 2001 From: karencfv Date: Tue, 3 Nov 2026 15:11:18 +1300 Subject: [PATCH 11/19] no cooldown period for SledEvacuating --- nexus/db-model/src/instance.rs | 203 ++++++++++++++++++++++++++++----- 1 file changed, 173 insertions(+), 30 deletions(-) diff --git a/nexus/db-model/src/instance.rs b/nexus/db-model/src/instance.rs index b171b82d443..5a627dcedfa 100644 --- a/nexus/db-model/src/instance.rs +++ b/nexus/db-model/src/instance.rs @@ -526,46 +526,44 @@ impl InstanceAutoRestart { // new state is introduced. This forces the person introducing the new // state to decide whether this is a state where we are going to honour // the auto-restart policy or we are forced to override it. - match state.nexus_state { + let reincarnability = match state.nexus_state { InstanceState::Failed | InstanceState::Creating | InstanceState::Destroyed | InstanceState::NoVmm | InstanceState::Vmm => { - // Check if the instance's configured auto-restart policy permits the - // control plane to automatically restart it. + // Check if the instance's configured auto-restart policy + // permits the control plane to automatically restart it. let policy = self.policy.unwrap_or(Self::DEFAULT_POLICY); - if policy == InstanceAutoRestartPolicy::Never { - return Reincarnatability::Nirvana; + match policy { + InstanceAutoRestartPolicy::Never => { + Reincarnatability::Nirvana + } + InstanceAutoRestartPolicy::BestEffort => { + // If the instance is permitted to reincarnate, ensure + // that its last reincarnation was at least one cooldown + // period ago. + if let Some(until) = self.cooldown_expiry_time(state) { + Reincarnatability::CoolingDown { until } + } else { + Reincarnatability::WillReincarnate + } + } } } // An instance that is in the `SledEvacuating` state will always be - // reincarnatable. This is because the instance has not actually failed, - // but rather its VMM has been stopped due to its sled being evacuated. - // The instance should be restarted automatically regardless of the - // auto-restart policy. - InstanceState::SledEvacuating => {} - } - - // If the instance is permitted to reincarnate, ensure that its last - // reincarnation was at least one cooldown period ago. - if let Some(last) = state.time_last_auto_restarted { - // If no explicit cooldown is present, use the default. - // Eventually, we may also allow a project-level default, so we will - // need to consider that as well. - let cooldown = self.cooldown.unwrap_or(Self::DEFAULT_COOLDOWN); - // The instance is eligible to reincarnate if the current time is - // greater than or equal to the last restart time plus the cooldown - // duration. - let until = last + cooldown; - if Utc::now() >= until { - return Reincarnatability::WillReincarnate; - } else { - return Reincarnatability::CoolingDown { until }; - } - } + // reincarnatable. This is because the instance has not actually + // failed, but rather its VMM has been stopped due to its sled being + // evacuated. The instance should be restarted automatically + // regardless of the auto-restart policy. + // + // In addition, we ignore the cooling down period. Any given + // instance will be reincarnated due to evacuation multiple times + // during an update. + InstanceState::SledEvacuating => Reincarnatability::WillReincarnate, + }; - Reincarnatability::WillReincarnate + reincarnability } /// Filters a database query to include only instances whose auto-restart @@ -645,6 +643,33 @@ impl InstanceAutoRestart { // pass. .and(dsl::updater_id.is_null()) } + + /// The wall-clock time at which the cooldown period will expire and the + /// instance will once again be eligible to reincarnate, if the instance + /// is still cooling down. + fn cooldown_expiry_time( + &self, + state: &InstanceRuntimeState, + ) -> Option> { + let Some(last) = state.time_last_auto_restarted else { + // The instance has never restarted due to a failure. There is no + // cooldown expiry time. + return None; + }; + + // If no explicit cooldown is present, use the default. Eventually, we + // may also allow a project-level default, so we will need to consider + // that as well. + let until = last + self.cooldown.unwrap_or(Self::DEFAULT_COOLDOWN); + if Utc::now() >= until { + // If the current time is greater than or equal to the last restart + // time plus the cooldown duration, the instance is no longer + // cooling down. There is no cooldown expiry time. + None + } else { + Some(until) + } + } } /// The parts of an Instance that can be directly updated after creation. #[derive(Clone, Debug, AsChangeset, Serialize, Deserialize)] @@ -666,3 +691,121 @@ pub struct InstanceUpdate { pub enable_jumbo_frames: bool, } + +#[cfg(test)] +mod test { + use super::*; + + #[derive(Clone, Copy, Debug)] + enum LastAutoRestart { + NeverRestarted, + WithinCooldown, + CooldownElapsed, + } + + impl LastAutoRestart { + const ALL: [Self; 3] = + [Self::NeverRestarted, Self::WithinCooldown, Self::CooldownElapsed]; + + fn time(self) -> Option> { + let now = Utc::now(); + match self { + Self::NeverRestarted => None, + Self::WithinCooldown => Some(now), + Self::CooldownElapsed => Some( + now - InstanceAutoRestart::DEFAULT_COOLDOWN + - TimeDelta::hours(1), + ), + } + } + } + + fn is_expected_reincarnatability( + found_reincarnability: Reincarnatability, + found_state: InstanceState, + found_policy: InstanceAutoRestartPolicy, + found_last_auto_restart: LastAutoRestart, + ) -> bool { + match found_state { + InstanceState::Creating + | InstanceState::NoVmm + | InstanceState::Vmm + | InstanceState::Failed + | InstanceState::Destroyed => { + match (found_policy, found_last_auto_restart) { + (InstanceAutoRestartPolicy::Never, _) => { + matches!( + found_reincarnability, + Reincarnatability::Nirvana + ) + } + ( + InstanceAutoRestartPolicy::BestEffort, + LastAutoRestart::WithinCooldown, + ) => matches!( + found_reincarnability, + Reincarnatability::CoolingDown { .. } + ), + ( + InstanceAutoRestartPolicy::BestEffort, + LastAutoRestart::NeverRestarted + | LastAutoRestart::CooldownElapsed, + ) => matches!( + found_reincarnability, + Reincarnatability::WillReincarnate + ), + } + } + // An instance whose sled is being evacuated always reincarnates, + // regardless of its auto-restart policy and cooldown period. + InstanceState::SledEvacuating => { + matches!( + found_reincarnability, + Reincarnatability::WillReincarnate + ) + } + } + } + + #[test] + fn test_can_reincarnate() { + // Loop through every combination of `InstanceState`, + // `InstanceAutoRestartPolicy` and `LastAutoRestart` and verify the + // `Reincarnatability` is the one we expect. + for &state in InstanceState::ALL_STATES { + for policy in [ + InstanceAutoRestartPolicy::Never, + InstanceAutoRestartPolicy::BestEffort, + ] { + for last_auto_restart in LastAutoRestart::ALL { + let auto_restart = InstanceAutoRestart { + policy: Some(policy), + cooldown: None, + }; + let runtime = InstanceRuntimeState { + time_updated: Utc::now(), + generation: InstanceStateGeneration::new().into(), + propolis_id: None, + dst_propolis_id: None, + migration_id: None, + nexus_state: state, + time_last_auto_restarted: last_auto_restart.time(), + }; + + let found_reincarnability = + auto_restart.can_reincarnate(&runtime); + assert!( + is_expected_reincarnatability( + found_reincarnability, + state, + policy, + last_auto_restart, + ), + "unexpected reincarnability: {found_reincarnability:?} for instance state: {state:?}, restart policy: \ + {policy:?}, last auto-restart time: {last_auto_restart:?}", + ); + } + } + } + } +} From 08b9e31bb12238f8db6bb3c903dd0682711ef54d Mon Sep 17 00:00:00 2001 From: karencfv Date: Tue, 3 Nov 2026 17:04:22 +1300 Subject: [PATCH 12/19] don't change time_last_auto_restarted --- nexus/db-model/src/instance.rs | 6 +- .../tasks/instance_reincarnation.rs | 107 ++++++++++++++---- nexus/src/app/sagas/instance_start.rs | 19 +++- 3 files changed, 103 insertions(+), 29 deletions(-) diff --git a/nexus/db-model/src/instance.rs b/nexus/db-model/src/instance.rs index 5a627dcedfa..926cc4b5f11 100644 --- a/nexus/db-model/src/instance.rs +++ b/nexus/db-model/src/instance.rs @@ -557,9 +557,9 @@ impl InstanceAutoRestart { // evacuated. The instance should be restarted automatically // regardless of the auto-restart policy. // - // In addition, we ignore the cooling down period. Any given - // instance will be reincarnated due to evacuation multiple times - // during an update. + // In addition, we ignore the cooldown period. Any given instance + // will be reincarnated due to evacuation multiple times during an + // update. InstanceState::SledEvacuating => Reincarnatability::WillReincarnate, }; diff --git a/nexus/src/app/background/tasks/instance_reincarnation.rs b/nexus/src/app/background/tasks/instance_reincarnation.rs index 63892ff55ef..65eefebff55 100644 --- a/nexus/src/app/background/tasks/instance_reincarnation.rs +++ b/nexus/src/app/background/tasks/instance_reincarnation.rs @@ -224,11 +224,23 @@ impl InstanceReincarnation { ); let running_saga = async { + let start_reason = match reason { + // We are starting due to a failure + ReincarnationReason::Failed + | ReincarnationReason::SagaUnwound => { + instance_start::Reason::AutoRestart + } + // Nothing bad happened here, the sled is just being + // evacuated + ReincarnationReason::SledEvacuation => { + instance_start::Reason::SledEvacuation + } + }; let dag = instance_start::SagaInstanceStart::prepare( &instance_start::Params { db_instance, serialized_authn: serialized_authn.clone(), - reason: instance_start::Reason::AutoRestart, + reason: start_reason, }, )?; self.sagas.saga_run(dag).await @@ -633,6 +645,22 @@ mod test { assert_eq!(updated, 1, "exactly one VMM should be marked"); } + async fn set_time_last_auto_restarted( + conn: &AsyncConnection, + instance_id: Uuid, + time: chrono::DateTime, + ) { + use nexus_db_schema::schema::instance::dsl; + + let updated = diesel::update(dsl::instance) + .filter(dsl::id.eq(instance_id)) + .set(dsl::time_last_auto_restarted.eq(Some(time))) + .execute_async(conn) + .await + .expect("instance should be updated"); + assert_eq!(updated, 1, "exactly one instance should be updated"); + } + // Boilerplate reducer. // // This is a macro so that the `dbg!` has the line number of the place where @@ -820,32 +848,46 @@ mod test { // be reincarnated. When an instance is reincarnated due to its sled // evacuating, the task ignores the auto-restart policy and always // restarts the instance, so we include instances with both policies - let num_sled_evacuating = 4; - for i in 0..num_sled_evacuating / 2 { - let instance = create_instance( - &cptestctx, - &opctx, - &format!("kolankola-{i}"), + // + // The task should also ignore the cooldown period for these instances, + // and their auto-restart timestamp should not change when they are + // reincarnated. + let conn = datastore.pool_connection_for_tests().await.unwrap(); + let recent_auto_restart = omicron_common::now_db_precision(); + // For each policy, include an instance that has never been automatically + // restarted, and one that is still within the cooldown period. + let sled_evacuating_instances = [ + ("kolankola-0", InstanceAutoRestartPolicy::BestEffort, None), + ( + "kolankola-1", InstanceAutoRestartPolicy::BestEffort, - InstanceState::SledEvacuating, - InstanceIntendedState::Running, - ) - .await; - will_reincarnate.insert(ReincarnatableInstance { - instance_id: instance.id(), - reason: ReincarnationReason::SledEvacuation, - }); - } - for i in 0..num_sled_evacuating / 2 { + Some(recent_auto_restart), + ), + ("ekabijin-0", InstanceAutoRestartPolicy::Never, None), + ( + "ekabijin-1", + InstanceAutoRestartPolicy::Never, + Some(recent_auto_restart), + ), + ]; + let num_sled_evacuating = sled_evacuating_instances.len(); + let mut evacuated_last_auto_restarted = + std::collections::BTreeMap::new(); + for (name, policy, last_auto_restarted) in sled_evacuating_instances { let instance = create_instance( &cptestctx, &opctx, - &format!("ekabijin-{i}"), - InstanceAutoRestartPolicy::Never, + name, + policy, InstanceState::SledEvacuating, InstanceIntendedState::Running, ) .await; + if let Some(time) = last_auto_restarted { + set_time_last_auto_restarted(&conn, instance.id(), time).await; + } + evacuated_last_auto_restarted + .insert(instance.id(), last_auto_restarted); will_reincarnate.insert(ReincarnatableInstance { instance_id: instance.id(), reason: ReincarnationReason::SledEvacuation, @@ -929,8 +971,6 @@ mod test { .await; attach_saga_unwound_vmm(&cptestctx, &opctx, &unwound_stopped).await; will_not_reincarnate.insert(unwound_stopped.id()); - // TODO-K: check this case. The instance's external state would - // permanently be "rebooting", which is wrong let evacuating_stopped = create_instance( &cptestctx, &opctx, @@ -982,12 +1022,35 @@ mod test { status.instances_reincarnated ); - test_helpers::instance_wait_for_state( + let state = test_helpers::instance_wait_for_state( &cptestctx, InstanceUuid::from_untyped_uuid(instance.instance_id), InstanceState::Vmm, ) .await; + + let time_last_auto_restarted = + state.instance().time_last_auto_restarted; + match instance.reason { + ReincarnationReason::Failed + | ReincarnationReason::SagaUnwound => { + assert!( + time_last_auto_restarted.is_some(), + "expected {instance} to have an auto-restart timestamp", + ); + } + ReincarnationReason::SledEvacuation => { + let found_time_auto_restarted = + evacuated_last_auto_restarted + .get(&instance.instance_id) + .unwrap(); + assert_eq!( + time_last_auto_restarted, *found_time_auto_restarted, + "expected the auto-restart timestamp of {instance} to \ + be unchanged", + ); + } + } } } diff --git a/nexus/src/app/sagas/instance_start.rs b/nexus/src/app/sagas/instance_start.rs index 447cf28f7b3..ec1c58dae0e 100644 --- a/nexus/src/app/sagas/instance_start.rs +++ b/nexus/src/app/sagas/instance_start.rs @@ -79,6 +79,9 @@ pub(crate) enum Reason { /// The instance has failed and is being automatically restarted by the /// control plane. AutoRestart, + /// The instance's VMM was stopped because its sled is being evacuated. The + /// instance is being restarted by the control plane. + SledEvacuation, } declare_saga_actions! { @@ -471,10 +474,18 @@ async fn sis_move_to_starting( let new_runtime = { // If we are performing an automated restart of a Failed instance, // remember to update the timestamp. - let time_last_auto_restarted = if params.reason == Reason::AutoRestart { - Some(Utc::now()) - } else { - db_instance.runtime().time_last_auto_restarted + + //let time_last_auto_restarted = if params.reason == Reason::AutoRestart { + // Some(Utc::now()) + //} else { + // db_instance.runtime().time_last_auto_restarted + //}; + + let time_last_auto_restarted = match params.reason { + Reason::AutoRestart => Some(Utc::now()), + Reason::SledEvacuation | Reason::User | Reason::AutoStart => { + db_instance.runtime().time_last_auto_restarted + } }; db::model::InstanceRuntimeState { nexus_state: db::model::InstanceState::Vmm, From 378eb9b9109d35b4264a07b6a191265975faa180 Mon Sep 17 00:00:00 2001 From: karencfv Date: Tue, 3 Nov 2026 20:46:26 +1300 Subject: [PATCH 13/19] clean up --- nexus/db-model/src/instance.rs | 2 +- nexus/src/app/sagas/instance_start.rs | 7 ------- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/nexus/db-model/src/instance.rs b/nexus/db-model/src/instance.rs index 926cc4b5f11..1cd88008112 100644 --- a/nexus/db-model/src/instance.rs +++ b/nexus/db-model/src/instance.rs @@ -558,7 +558,7 @@ impl InstanceAutoRestart { // regardless of the auto-restart policy. // // In addition, we ignore the cooldown period. Any given instance - // will be reincarnated due to evacuation multiple times during an + // may be reincarnated due to evacuation multiple times during an // update. InstanceState::SledEvacuating => Reincarnatability::WillReincarnate, }; diff --git a/nexus/src/app/sagas/instance_start.rs b/nexus/src/app/sagas/instance_start.rs index ec1c58dae0e..6c2723e6001 100644 --- a/nexus/src/app/sagas/instance_start.rs +++ b/nexus/src/app/sagas/instance_start.rs @@ -474,13 +474,6 @@ async fn sis_move_to_starting( let new_runtime = { // If we are performing an automated restart of a Failed instance, // remember to update the timestamp. - - //let time_last_auto_restarted = if params.reason == Reason::AutoRestart { - // Some(Utc::now()) - //} else { - // db_instance.runtime().time_last_auto_restarted - //}; - let time_last_auto_restarted = match params.reason { Reason::AutoRestart => Some(Utc::now()), Reason::SledEvacuation | Reason::User | Reason::AutoStart => { From 2521adf02fae1091c20069dba4b6f785fc9e77d4 Mon Sep 17 00:00:00 2001 From: karencfv Date: Wed, 4 Nov 2026 16:32:39 +1300 Subject: [PATCH 14/19] handle saga unwind when SledEvacuating, but also try to avoid a race --- nexus/src/app/sagas/instance_start.rs | 258 +++++++++++++++++++++----- 1 file changed, 208 insertions(+), 50 deletions(-) diff --git a/nexus/src/app/sagas/instance_start.rs b/nexus/src/app/sagas/instance_start.rs index 6c2723e6001..f1e7cbc7e1b 100644 --- a/nexus/src/app/sagas/instance_start.rs +++ b/nexus/src/app/sagas/instance_start.rs @@ -520,14 +520,74 @@ async fn sis_move_to_starting_undo( sagactx: NexusActionContext, ) -> Result<(), anyhow::Error> { let osagactx = sagactx.user_data(); + let params = sagactx.saga_params::()?; let db_instance = sagactx.lookup::("started_record")?; let instance_id = InstanceUuid::from_untyped_uuid(db_instance.id()); - info!(osagactx.log(), "start saga failed; returning instance to Stopped"; - "instance_id" => %instance_id); + + // If an instance was being restarted due to its VMM's sled being evacuated, + // and its intended state is currently `Running` we return it to + // `SledEvacuating`. We want the instance reincarnation task to attempt to + // restart it again. Any other reason returns the instance to `NoVmm`. In + // this state, the reincarnation task will no longer attempt to reincarnate + // it, and will remain stopped for the end user. + let nexus_state = match params.reason { + Reason::SledEvacuation => { + // To avoid a race where a user may have stopped the instance while + // this saga was running, we check the instance first to make sure + // the intended state is still `Running`. + let opctx = crate::context::op_context_for_saga_action( + &sagactx, + ¶ms.serialized_authn, + ); + match LookupPath::new(&opctx, osagactx.datastore()) + .instance_id(instance_id.into_untyped_uuid()) + .fetch() + .await + { + Ok(( + _authz_silo, + _authz_project, + _authz_instance, + current_instance, + )) => match current_instance.intended_state { + db::model::InstanceIntendedState::Running => { + db::model::InstanceState::SledEvacuating + } + db::model::InstanceIntendedState::Destroyed + | db::model::InstanceIntendedState::GuestShutdown + | db::model::InstanceIntendedState::Stopped => { + db::model::InstanceState::NoVmm + } + }, + // If the instance cannot be read, we set its state to `NoVmm` + // to reduce the possibility of leaving it as `Rebooting` + // indefinitely. + Err(error) => { + slog::warn!( + osagactx.log(), + "could not read instance's intended state; \ + setting instance state to NoVmm"; + "instance_id" => %instance_id, + "error" => %error, + ); + db::model::InstanceState::NoVmm + } + } + } + Reason::AutoStart | Reason::User | Reason::AutoRestart => { + db::model::InstanceState::NoVmm + } + }; + info!( + osagactx.log(), + "start saga failed; resetting instance state to {nexus_state};"; + "instance_id" => %instance_id, + "start_reason" => ?params.reason + ); let new_runtime = db::model::InstanceRuntimeState { - nexus_state: db::model::InstanceState::NoVmm, + nexus_state, propolis_id: None, generation: InstanceStateGeneration::from(db_instance.state_generation) .next() @@ -540,9 +600,13 @@ async fn sis_move_to_starting_undo( .instance_update_runtime(&instance_id, &new_runtime) .await? { - info!(osagactx.log(), - "did not return instance to Stopped: old generation number"; - "instance_id" => %instance_id); + info!( + osagactx.log(), + "did not reset instance state to {nexus_state}: \ + old generation number"; + "instance_id" => %instance_id, + "start_reason" => ?params.reason + ); } Ok(()) @@ -1638,31 +1702,16 @@ mod test { let client = &cptestctx.external_client; let nexus = &cptestctx.server.server_context().nexus; + let datastore = nexus.datastore(); let _project_id = setup_test_project(&client).await; let opctx = test_helpers::test_opctx(&cptestctx); let instance = create_instance(client).await; let instance_id = InstanceUuid::from_untyped_uuid(instance.identity.id); - let db_instance = test_helpers::instance_fetch(&cptestctx, instance_id) + let (.., authz_instance) = LookupPath::new(&opctx, datastore) + .instance_id(instance_id.into_untyped_uuid()) + .lookup_for(authz::Action::Modify) .await - .instance() - .clone(); - - let params = Params { - serialized_authn: authn::saga::Serialized::for_opctx(&opctx), - db_instance, - reason: Reason::User, - }; - - let dag = create_saga_dag::(params).unwrap(); - - // The ensure_running node is last in the saga. This should be the node - // where the failure ultimately occurs. - let last_node_name = dag - .get_nodes() - .last() - .expect("saga should have at least one node") - .name() - .clone(); + .expect("instance must exist"); // Inject failure at the simulated sled agent level. This allows the // ensure-running node to attempt to change the instance's state, but @@ -1675,34 +1724,143 @@ mod test { ))) .await; - let runnable_saga = nexus.sagas.saga_prepare(dag).await.unwrap(); - let saga_result = runnable_saga - .run_to_completion() - .await - .expect("saga execution should have started") - .into_raw_result(); - let saga_error = saga_result - .kind - .expect_err("saga should fail due to injected error"); + // The state that the instance returns to when the saga unwinds depends + // on the instance's reason for starting, and on whether a user stopped + // the instance while the saga was running. + // + // For any reason other than `SledEvacuation`, the end state will be + // `NoVmm` (stopped), so it doesn't really matter if the user stops the + // instance while the saga is running. We don't test a user stopping the + // instance in those cases. + struct Case { + reason: Reason, + user_stops_during_saga: bool, + expected_state: nexus_db_model::InstanceState, + } + let cases = [ + Case { + reason: Reason::User, + user_stops_during_saga: false, + expected_state: nexus_db_model::InstanceState::NoVmm, + }, + Case { + reason: Reason::AutoRestart, + user_stops_during_saga: false, + expected_state: nexus_db_model::InstanceState::NoVmm, + }, + Case { + reason: Reason::AutoStart, + user_stops_during_saga: false, + expected_state: nexus_db_model::InstanceState::NoVmm, + }, + Case { + reason: Reason::SledEvacuation, + user_stops_during_saga: false, + expected_state: nexus_db_model::InstanceState::SledEvacuating, + }, + Case { + reason: Reason::SledEvacuation, + user_stops_during_saga: true, + expected_state: nexus_db_model::InstanceState::NoVmm, + }, + ]; + + for Case { reason, user_stops_during_saga, expected_state } in cases { + // The instance's intended state should be `Running` when the saga + // starts. + datastore + .instance_set_intended_state( + &opctx, + &authz_instance, + nexus_db_model::InstanceIntendedState::Running, + ) + .await + .expect("intended state should be set"); + + let db_instance = + test_helpers::instance_fetch(&cptestctx, instance_id) + .await + .instance() + .clone(); + + let params = Params { + serialized_authn: authn::saga::Serialized::for_opctx(&opctx), + db_instance, + reason, + }; - assert_eq!(saga_error.error_node_name, last_node_name); + let dag = create_saga_dag::(params).unwrap(); + + // The ensure_running node is last in the saga. This should be the + // node where the failure ultimately occurs. + let last_node_name = dag + .get_nodes() + .last() + .expect("saga should have at least one node") + .name() + .clone(); + + let next_registration = if user_stops_during_saga { + // To simulate a user stopping the instance while the saga is + // running, pause the saga when it registers the VMM with the + // sled agent. + Some(sled_agent.pause_next_vmm_registration()) + } else { + None + }; - let db_instance = - test_helpers::instance_fetch(&cptestctx, instance_id).await; + let runnable_saga = nexus.sagas.saga_prepare(dag).await.unwrap(); + let (saga_result, ()) = + tokio::join!(runnable_saga.run_to_completion(), async { + let Some(next_registration) = next_registration else { + // Return here if we're not simulating a user stopping + // the instance mid-saga + return; + }; + let paused = next_registration + .wait_for_request(Duration::from_secs(60)) + .await; + datastore + .instance_set_intended_state( + &opctx, + &authz_instance, + nexus_db_model::InstanceIntendedState::Stopped, + ) + .await + .expect("intended state should be set"); + paused.resume(); + },); + let saga_error = saga_result + .expect("saga execution should have started") + .into_raw_result() + .kind + .expect_err("saga should fail due to injected error"); + + assert_eq!( + saga_error.error_node_name, last_node_name, + "start reason: {reason:?}, user stops during saga: \ + {user_stops_during_saga}", + ); - assert_eq!( - db_instance.instance().nexus_state, - nexus_db_model::InstanceState::NoVmm - ); - assert!(db_instance.vmm().is_none()); + let db_instance = + test_helpers::instance_fetch(&cptestctx, instance_id).await; - assert!( - test_helpers::no_virtual_provisioning_resource_records_exist( - &cptestctx - ) - .await - ); - assert!(test_helpers::no_virtual_provisioning_collection_records_using_instances(&cptestctx).await); + assert_eq!( + db_instance.instance().nexus_state, + expected_state, + "start reason: {reason:?}, user stops during saga: \ + {user_stops_during_saga}", + ); + assert!(db_instance.vmm().is_none()); + + assert!( + test_helpers::no_virtual_provisioning_resource_records_exist( + &cptestctx + ) + .await + ); + assert!(test_helpers::no_virtual_provisioning_collection_records_using_instances(&cptestctx).await); + } cptestctx.teardown().await; } From 6db0cc9a29822f97be8f47764f7cbe793e7cf26d Mon Sep 17 00:00:00 2001 From: karencfv Date: Wed, 4 Nov 2026 17:07:44 +1300 Subject: [PATCH 15/19] add some comments and fix docs --- nexus/db-model/src/instance.rs | 2 +- nexus/examples/config-second.toml | 2 +- nexus/examples/config.toml | 2 +- .../tasks/instance_reincarnation.rs | 20 +++++++++++++------ nexus/tests/config.test.toml | 2 +- smf/nexus/multi-sled/config-partial.toml | 2 +- smf/nexus/single-sled/config-partial.toml | 2 +- 7 files changed, 20 insertions(+), 12 deletions(-) diff --git a/nexus/db-model/src/instance.rs b/nexus/db-model/src/instance.rs index 1cd88008112..3e485b286d4 100644 --- a/nexus/db-model/src/instance.rs +++ b/nexus/db-model/src/instance.rs @@ -627,7 +627,7 @@ impl InstanceAutoRestart { // should not be automatically restarted, even if they have failed. dsl::intended_state .eq(IntendedState::Running) - // Either the failed-instance preduicate from above matches, *or* + // Either the failed-instance predicate from above matches, *or* // the instance is rebooting due to its sled evacuating. When an // instance had to stop for sled evacuation, can always be // reincarnated, regardless of its auto-restart policy and when it diff --git a/nexus/examples/config-second.toml b/nexus/examples/config-second.toml index c95e7f32d19..abb0ccecd1e 100644 --- a/nexus/examples/config-second.toml +++ b/nexus/examples/config-second.toml @@ -158,7 +158,7 @@ instance_watcher.period_secs = 30 capacity_metrics.period_secs = 60 # How frequently to schedule new instance update sagas. instance_updater.period_secs = 30 -# How frequently to attempt to restart Failed instances? +# How frequently to attempt to restart instances that require it. instance_reincarnation.period_secs = 60 service_firewall_propagation.period_secs = 300 v2p_mapping_propagation.period_secs = 30 diff --git a/nexus/examples/config.toml b/nexus/examples/config.toml index 605b6bcc71a..dc9c1bc7692 100644 --- a/nexus/examples/config.toml +++ b/nexus/examples/config.toml @@ -149,7 +149,7 @@ instance_watcher.period_secs = 30 capacity_metrics.period_secs = 60 # How frequently to schedule new instance update sagas. instance_updater.period_secs = 30 -# How frequently to attempt to restart Failed instances? +# How frequently to attempt to restart instances that require it. instance_reincarnation.period_secs = 60 service_firewall_propagation.period_secs = 300 v2p_mapping_propagation.period_secs = 30 diff --git a/nexus/src/app/background/tasks/instance_reincarnation.rs b/nexus/src/app/background/tasks/instance_reincarnation.rs index 65eefebff55..472112c72a8 100644 --- a/nexus/src/app/background/tasks/instance_reincarnation.rs +++ b/nexus/src/app/background/tasks/instance_reincarnation.rs @@ -2,7 +2,7 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -//! Background task for automatically restarting failed instances. +//! Background task for automatically restarting instances that require it. use crate::app::background::Activator; use crate::app::background::BackgroundTask; @@ -1171,6 +1171,15 @@ mod test { .await; } + // This test differs from `test_only_reincarnates_eligible_instances` in + // that it checks the `SledEvacuating` case more thoroughly. Specifically, + // it does this by following the real sequence of events, instead of writing + // the `SledEvacuating` state directly to the database. Like a little + // integration test. We emulate this sequence, first by marking the VMMs to + // be stopped for an update, then running the `vmm_stop_for_update` task to + // stop them, verifying the `instance-update` saga changes each instance's + // state to `SledEvacuating`, and finally making sure they are successfully + // reincarnated. #[tokio::test] async fn test_reincarnates_instances_in_sled_evacuating() { // Test setup @@ -1178,7 +1187,6 @@ mod test { "test_reincarnates_instances_in_sled_evacuating", ) .customize_nexus_config(&|config| { - let one_day = Duration::from_secs(86_400); let tasks = &mut config.pkg.background_tasks; // The `instance-update` saga activates the real // `instance_reincarnation` background task when an instance's state @@ -1187,10 +1195,10 @@ mod test { tasks.instance_reincarnation.disable = true; // Increase the intervals for background tasks that could modify // VMM records while this test is running. - tasks.instance_watcher.period_secs = one_day; - tasks.abandoned_vmm_reaper.period_secs = one_day; - tasks.vmm_mark_stop_for_update.period_secs = one_day; - tasks.vmm_stop_for_update.period_secs = one_day; + tasks.instance_watcher.period_secs = Duration::MAX; + tasks.abandoned_vmm_reaper.period_secs = Duration::MAX; + tasks.vmm_mark_stop_for_update.period_secs = Duration::MAX; + tasks.vmm_stop_for_update.period_secs = Duration::MAX; }) .start::() .await; diff --git a/nexus/tests/config.test.toml b/nexus/tests/config.test.toml index 45fbfce8c93..55081653b1c 100644 --- a/nexus/tests/config.test.toml +++ b/nexus/tests/config.test.toml @@ -168,7 +168,7 @@ lookup_region_port.period_secs = 60 # Therefore, disable the background task during tests. instance_updater.disable = true instance_updater.period_secs = 60 -# How frequently to attempt to restart Failed instances? +# How frequently to attempt to restart instances that require it. # # The default activation period for this task is once every sixty seconds. # However, the task's unit tests rely on explicit activations of the task diff --git a/smf/nexus/multi-sled/config-partial.toml b/smf/nexus/multi-sled/config-partial.toml index c6c08b4a994..0fa9e230d5d 100644 --- a/smf/nexus/multi-sled/config-partial.toml +++ b/smf/nexus/multi-sled/config-partial.toml @@ -72,7 +72,7 @@ vmm_stop_for_update.period_secs = 60 saga_recovery.period_secs = 600 lookup_region_port.period_secs = 60 instance_updater.period_secs = 30 -# How frequently to attempt to restart Failed instances? +# How frequently to attempt to restart instances that require it. instance_reincarnation.period_secs = 60 region_snapshot_replacement_start.period_secs = 30 region_snapshot_replacement_garbage_collection.period_secs = 30 diff --git a/smf/nexus/single-sled/config-partial.toml b/smf/nexus/single-sled/config-partial.toml index 41c8dc38142..ed36407c95f 100644 --- a/smf/nexus/single-sled/config-partial.toml +++ b/smf/nexus/single-sled/config-partial.toml @@ -72,7 +72,7 @@ vmm_stop_for_update.period_secs = 60 saga_recovery.period_secs = 600 lookup_region_port.period_secs = 60 instance_updater.period_secs = 30 -# How frequently to attempt to restart Failed instances? +# How frequently to attempt to restart instances that require it. instance_reincarnation.period_secs = 60 region_snapshot_replacement_start.period_secs = 30 region_snapshot_replacement_garbage_collection.period_secs = 30 From c88ed9e5f76596125ca0b867e43bcff51b097371 Mon Sep 17 00:00:00 2001 From: karencfv Date: Thu, 5 Nov 2026 14:56:22 +1300 Subject: [PATCH 16/19] address comments on small fixes --- nexus/src/app/sagas/instance_start.rs | 352 ++++++++++++++------------ 1 file changed, 193 insertions(+), 159 deletions(-) diff --git a/nexus/src/app/sagas/instance_start.rs b/nexus/src/app/sagas/instance_start.rs index 341098d6de1..0115864888f 100644 --- a/nexus/src/app/sagas/instance_start.rs +++ b/nexus/src/app/sagas/instance_start.rs @@ -545,21 +545,18 @@ async fn sis_move_to_starting_undo( .fetch() .await { - Ok(( - _authz_silo, - _authz_project, - _authz_instance, - current_instance, - )) => match current_instance.intended_state { - db::model::InstanceIntendedState::Running => { - db::model::InstanceState::SledEvacuating - } - db::model::InstanceIntendedState::Destroyed - | db::model::InstanceIntendedState::GuestShutdown - | db::model::InstanceIntendedState::Stopped => { - db::model::InstanceState::NoVmm + Ok((.., current_instance)) => { + match current_instance.intended_state { + db::model::InstanceIntendedState::Running => { + db::model::InstanceState::SledEvacuating + } + db::model::InstanceIntendedState::Destroyed + | db::model::InstanceIntendedState::GuestShutdown + | db::model::InstanceIntendedState::Stopped => { + db::model::InstanceState::NoVmm + } } - }, + } // If the instance cannot be read, we set its state to `NoVmm` // to reduce the possibility of leaving it as `Rebooting` // indefinitely. @@ -1683,7 +1680,7 @@ mod test { cptestctx.teardown().await; } - /// Tests that if a start saga unwinds because sled agent returned failure + /// Ensures that if a start saga unwinds because sled agent returned failure /// from a call to ensure the instance was running, then the system returns /// to the correct state. /// @@ -1691,13 +1688,19 @@ mod test { /// test causes saga nodes to "fail" without actually executing anything, /// whereas this test injects a failure into the normal operation of the /// ensure-running node. - #[tokio::test] - async fn test_ensure_running_unwind() { - let cptestctx = test_helpers::instance_saga_test_builder( - "test_ensure_running_unwind", - ) - .start::() - .await; + async fn ensure_running_unwind( + test_name: &str, + case: EnsureRunningUnwindCase, + ) { + let EnsureRunningUnwindCase { + reason, + user_stops_during_saga, + expected_state, + } = case; + + let cptestctx = test_helpers::instance_saga_test_builder(test_name) + .start::() + .await; let client = &cptestctx.external_client; let nexus = &cptestctx.server.server_context().nexus; @@ -1714,164 +1717,195 @@ mod test { let sled_agent = cptestctx.first_sled_agent(); - // The state that the instance returns to when the saga unwinds depends - // on the instance's reason for starting, and on whether a user stopped - // the instance while the saga was running. - // - // For any reason other than `SledEvacuation`, the end state will be - // `NoVmm` (stopped), so it doesn't really matter if the user stops the - // instance while the saga is running. We don't test a user stopping the - // instance in those cases. - struct Case { - reason: Reason, - user_stops_during_saga: bool, - expected_state: nexus_db_model::InstanceState, + // The instance's intended state should be `Running` when the saga + // starts. + datastore + .instance_set_intended_state( + &opctx, + &authz_instance, + nexus_db_model::InstanceIntendedState::Running, + ) + .await + .expect("intended state should be set"); + + let db_instance = test_helpers::instance_fetch(&cptestctx, instance_id) + .await + .instance() + .clone(); + + let params = Params { + serialized_authn: authn::saga::Serialized::for_opctx(&opctx), + db_instance, + reason, + }; + + let dag = create_saga_dag::(params).unwrap(); + + // The ensure_running node is last in the saga. This should be the node + // where the failure ultimately occurs. + let last_node_name = dag + .get_nodes() + .last() + .expect("saga should have at least one node") + .name() + .clone(); + + let next_registration = if user_stops_during_saga { + // To simulate a user stopping the instance while the saga is + // running, pause the saga when it registers the VMM with the sled + // agent. + Some(sled_agent.pause_next_vmm_registration()) + } else { + None + }; + + // Inject failure at the simulated sled agent level. This allows the + // ensure-running node to attempt to change the instance's state, but + // forces this operation to fail and produce whatever side effects + // result from that failure. The ensure-running node's request to run + // the VMM is the first `vmm_put_state` request the saga sends. + let next_put_state = sled_agent.pause_next_vmm_put_state(); + + let running_saga = nexus + .sagas + .saga_prepare(dag) + .await + .expect("saga should be prepared") + .start() + .await + .expect("saga should start"); + + if let Some(next_registration) = next_registration { + let paused = next_registration + .wait_for_request(Duration::from_secs(60)) + .await; + datastore + .instance_set_intended_state( + &opctx, + &authz_instance, + nexus_db_model::InstanceIntendedState::Stopped, + ) + .await + .expect("intended state should be set"); + paused.resume(); } - let cases = [ - Case { + + next_put_state.wait_for_request(Duration::from_secs(60)).await.fail( + Error::internal_error("injected by test_ensure_running_unwind"), + ); + + let saga_error = running_saga + .wait_until_stopped() + .await + .into_raw_result() + .kind + .expect_err("saga should fail due to injected error"); + + assert_eq!( + saga_error.error_node_name, last_node_name, + "start reason: {reason:?}, user stops during saga: \ + {user_stops_during_saga}", + ); + + let db_instance = + test_helpers::instance_fetch(&cptestctx, instance_id).await; + + assert_eq!( + db_instance.instance().nexus_state, + expected_state, + "start reason: {reason:?}, user stops during saga: \ + {user_stops_during_saga}", + ); + assert!(db_instance.vmm().is_none()); + + assert!( + test_helpers::no_virtual_provisioning_resource_records_exist( + &cptestctx + ) + .await + ); + assert!(test_helpers::no_virtual_provisioning_collection_records_using_instances(&cptestctx).await); + + cptestctx.teardown().await; + } + + // The state that the instance returns to when the saga unwinds depends + // on the instance's reason for starting, and on whether a user stopped + // the instance while the saga was running. + // + // For any reason other than `SledEvacuation`, the end state will be + // `NoVmm` (stopped), so it doesn't really matter if the user stops the + // instance while the saga is running. We don't test a user stopping the + // instance in those cases. + struct EnsureRunningUnwindCase { + reason: Reason, + user_stops_during_saga: bool, + expected_state: nexus_db_model::InstanceState, + } + + #[tokio::test] + async fn test_ensure_running_unwind_user() { + ensure_running_unwind( + "test_ensure_running_unwind_user", + EnsureRunningUnwindCase { reason: Reason::User, user_stops_during_saga: false, expected_state: nexus_db_model::InstanceState::NoVmm, }, - Case { + ) + .await; + } + + #[tokio::test] + async fn test_ensure_running_unwind_auto_restart() { + ensure_running_unwind( + "test_ensure_running_unwind_auto_restart", + EnsureRunningUnwindCase { reason: Reason::AutoRestart, user_stops_during_saga: false, expected_state: nexus_db_model::InstanceState::NoVmm, }, - Case { + ) + .await; + } + + #[tokio::test] + async fn test_ensure_running_unwind_auto_start() { + ensure_running_unwind( + "test_ensure_running_unwind_auto_start", + EnsureRunningUnwindCase { reason: Reason::AutoStart, user_stops_during_saga: false, expected_state: nexus_db_model::InstanceState::NoVmm, }, - Case { + ) + .await; + } + + #[tokio::test] + async fn test_ensure_running_unwind_sled_evacuation() { + ensure_running_unwind( + "test_ensure_running_unwind_sled_evacuation", + EnsureRunningUnwindCase { reason: Reason::SledEvacuation, user_stops_during_saga: false, expected_state: nexus_db_model::InstanceState::SledEvacuating, }, - Case { + ) + .await; + } + + #[tokio::test] + async fn test_ensure_running_unwind_sled_evacuation_user_stops() { + ensure_running_unwind( + "test_ensure_running_unwind_sled_evacuation_user_stops", + EnsureRunningUnwindCase { reason: Reason::SledEvacuation, user_stops_during_saga: true, expected_state: nexus_db_model::InstanceState::NoVmm, }, - ]; - - for Case { reason, user_stops_during_saga, expected_state } in cases { - // The instance's intended state should be `Running` when the saga - // starts. - datastore - .instance_set_intended_state( - &opctx, - &authz_instance, - nexus_db_model::InstanceIntendedState::Running, - ) - .await - .expect("intended state should be set"); - - let db_instance = - test_helpers::instance_fetch(&cptestctx, instance_id) - .await - .instance() - .clone(); - - let params = Params { - serialized_authn: authn::saga::Serialized::for_opctx(&opctx), - db_instance, - reason, - }; - - let dag = create_saga_dag::(params).unwrap(); - - // The ensure_running node is last in the saga. This should be the - // node where the failure ultimately occurs. - let last_node_name = dag - .get_nodes() - .last() - .expect("saga should have at least one node") - .name() - .clone(); - - let next_registration = if user_stops_during_saga { - // To simulate a user stopping the instance while the saga is - // running, pause the saga when it registers the VMM with the - // sled agent. - Some(sled_agent.pause_next_vmm_registration()) - } else { - None - }; - - // Inject failure at the simulated sled agent level. This allows the - // ensure-running node to attempt to change the instance's state, - // but forces this operation to fail and produce whatever side - // effects result from that failure. The ensure-running node's - // request to run the VMM is the first `vmm_put_state` request the - // saga sends. - let next_put_state = sled_agent.pause_next_vmm_put_state(); - - let running_saga = nexus - .sagas - .saga_prepare(dag) - .await - .expect("saga should be prepared") - .start() - .await - .expect("saga should start"); - - if let Some(next_registration) = next_registration { - let paused = next_registration - .wait_for_request(Duration::from_secs(60)) - .await; - datastore - .instance_set_intended_state( - &opctx, - &authz_instance, - nexus_db_model::InstanceIntendedState::Stopped, - ) - .await - .expect("intended state should be set"); - paused.resume(); - } - - next_put_state - .wait_for_request(Duration::from_secs(60)) - .await - .fail(Error::internal_error( - "injected by test_ensure_running_unwind", - )); - - let saga_error = running_saga - .wait_until_stopped() - .await - .into_raw_result() - .kind - .expect_err("saga should fail due to injected error"); - - assert_eq!( - saga_error.error_node_name, last_node_name, - "start reason: {reason:?}, user stops during saga: \ - {user_stops_during_saga}", - ); - - let db_instance = - test_helpers::instance_fetch(&cptestctx, instance_id).await; - - assert_eq!( - db_instance.instance().nexus_state, - expected_state, - "start reason: {reason:?}, user stops during saga: \ - {user_stops_during_saga}", - ); - assert!(db_instance.vmm().is_none()); - - assert!( - test_helpers::no_virtual_provisioning_resource_records_exist( - &cptestctx - ) - .await - ); - assert!(test_helpers::no_virtual_provisioning_collection_records_using_instances(&cptestctx).await); - } - - cptestctx.teardown().await; + ) + .await; } /// Regression test for omicron#11560: if a VMM record is created by a start From b588441161ca875d1e310c0637c6ed46440a2c6e Mon Sep 17 00:00:00 2001 From: karencfv Date: Thu, 5 Nov 2026 16:00:03 +1300 Subject: [PATCH 17/19] if at first you don't succeed, try try again --- nexus/src/app/sagas/instance_start.rs | 168 ++++++++++++++++---------- 1 file changed, 102 insertions(+), 66 deletions(-) diff --git a/nexus/src/app/sagas/instance_start.rs b/nexus/src/app/sagas/instance_start.rs index 0115864888f..b63328d371d 100644 --- a/nexus/src/app/sagas/instance_start.rs +++ b/nexus/src/app/sagas/instance_start.rs @@ -524,6 +524,10 @@ async fn sis_move_to_starting_undo( let db_instance = sagactx.lookup::("started_record")?; let instance_id = InstanceUuid::from_untyped_uuid(db_instance.id()); + let opctx = crate::context::op_context_for_saga_action( + &sagactx, + ¶ms.serialized_authn, + ); // If an instance was being restarted due to its VMM's sled being evacuated, // and its intended state is currently `Running` we return it to @@ -531,84 +535,116 @@ async fn sis_move_to_starting_undo( // restart it again. Any other reason returns the instance to `NoVmm`. In // this state, the reincarnation task will no longer attempt to reincarnate // it, and will remain stopped for the end user. - let nexus_state = match params.reason { - Reason::SledEvacuation => { - // To avoid a race where a user may have stopped the instance while - // this saga was running, we check the instance first to make sure - // the intended state is still `Running`. - let opctx = crate::context::op_context_for_saga_action( - &sagactx, - ¶ms.serialized_authn, - ); - match LookupPath::new(&opctx, osagactx.datastore()) - .instance_id(instance_id.into_untyped_uuid()) - .fetch() - .await - { - Ok((.., current_instance)) => { - match current_instance.intended_state { - db::model::InstanceIntendedState::Running => { - db::model::InstanceState::SledEvacuating + retry_notify_ext( + retry_policy_internal_service(), + || async { + let nexus_state = match params.reason { + Reason::SledEvacuation => { + // To avoid a race where a user may have stopped the + // instance while this saga was running, we check the + // instance first to make sure the intended state is still + // `Running`. + match LookupPath::new(&opctx, osagactx.datastore()) + .instance_id(instance_id.into_untyped_uuid()) + .fetch() + .await + { + Ok((.., current_instance)) => { + match current_instance.intended_state { + db::model::InstanceIntendedState::Running => { + db::model::InstanceState::SledEvacuating + } + db::model::InstanceIntendedState::Destroyed + | db::model::InstanceIntendedState::GuestShutdown + | db::model::InstanceIntendedState::Stopped => { + db::model::InstanceState::NoVmm + } + } } - db::model::InstanceIntendedState::Destroyed - | db::model::InstanceIntendedState::GuestShutdown - | db::model::InstanceIntendedState::Stopped => { - db::model::InstanceState::NoVmm + // The instance is not there anymore; there is no state + // to reset. + Err( + error @ (Error::ObjectNotFound { .. } + | Error::NotFound { .. } + | Error::Gone), + ) => { + info!( + osagactx.log(), + "start saga failed; instance has been \ + deleted, not resetting its state"; + "instance_id" => %instance_id, + "start_reason" => ?params.reason, + "error" => %error, + ); + return Ok(()); } + Err(error) => return Err(into_backoff_error(error)), } } - // If the instance cannot be read, we set its state to `NoVmm` - // to reduce the possibility of leaving it as `Rebooting` - // indefinitely. - Err(error) => { - slog::warn!( - osagactx.log(), - "could not read instance's intended state; \ - setting instance state to NoVmm"; - "instance_id" => %instance_id, - "error" => %error, - ); + Reason::AutoStart | Reason::User | Reason::AutoRestart => { db::model::InstanceState::NoVmm } - } - } - Reason::AutoStart | Reason::User | Reason::AutoRestart => { - db::model::InstanceState::NoVmm - } - }; - info!( - osagactx.log(), - "start saga failed; resetting instance state to {nexus_state};"; - "instance_id" => %instance_id, - "start_reason" => ?params.reason - ); + }; + info!( + osagactx.log(), + "start saga failed; resetting instance state to {nexus_state};"; + "instance_id" => %instance_id, + "start_reason" => ?params.reason + ); - let new_runtime = db::model::InstanceRuntimeState { - nexus_state, - propolis_id: None, - generation: InstanceStateGeneration::from(db_instance.state_generation) - .next() - .into(), - ..db_instance.runtime() - }; + let new_runtime = db::model::InstanceRuntimeState { + nexus_state, + propolis_id: None, + generation: InstanceStateGeneration::from( + db_instance.state_generation, + ) + .next() + .into(), + ..db_instance.runtime() + }; - if !osagactx - .datastore() - .instance_update_runtime(&instance_id, &new_runtime) - .await? - { - info!( - osagactx.log(), - "did not reset instance state to {nexus_state}: \ - old generation number"; - "instance_id" => %instance_id, - "start_reason" => ?params.reason - ); - } + if !osagactx + .datastore() + .instance_update_runtime(&instance_id, &new_runtime) + .await + .map_err(into_backoff_error)? + { + info!( + osagactx.log(), + "did not reset instance state to {nexus_state}: \ + old generation number"; + "instance_id" => %instance_id, + "start_reason" => ?params.reason + ); + } + + Ok(()) + }, + |error, call_count, total_duration| { + warn!( + osagactx.log(), + "start saga: failed to reset instance state; retrying"; + "instance_id" => %instance_id, + "start_reason" => ?params.reason, + "call_count" => call_count, + "total_duration" => ?total_duration, + InlineErrorChain::new(&error), + ); + }, + ) + .await?; Ok(()) } +fn into_backoff_error(error: Error) -> BackoffError { + if error.retryable() { + BackoffError::transient(error) + } else { + BackoffError::permanent(error) + } +} + async fn sis_account_virtual_resources( sagactx: NexusActionContext, ) -> Result<(), ActionError> { From d35c0b3f55fc83829c59eced0cd4debec181502e Mon Sep 17 00:00:00 2001 From: karencfv Date: Thu, 5 Nov 2026 16:27:37 +1300 Subject: [PATCH 18/19] tiny fixes --- nexus/src/app/sagas/instance_start.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/nexus/src/app/sagas/instance_start.rs b/nexus/src/app/sagas/instance_start.rs index b63328d371d..1f8243ad8e9 100644 --- a/nexus/src/app/sagas/instance_start.rs +++ b/nexus/src/app/sagas/instance_start.rs @@ -530,9 +530,9 @@ async fn sis_move_to_starting_undo( ); // If an instance was being restarted due to its VMM's sled being evacuated, - // and its intended state is currently `Running` we return it to + // and its intended state is currently `Running`, we return it to // `SledEvacuating`. We want the instance reincarnation task to attempt to - // restart it again. Any other reason returns the instance to `NoVmm`. In + // restart it again. Any other case returns the instance to `NoVmm`. In // this state, the reincarnation task will no longer attempt to reincarnate // it, and will remain stopped for the end user. retry_notify_ext( @@ -1826,9 +1826,10 @@ mod test { paused.resume(); } - next_put_state.wait_for_request(Duration::from_secs(60)).await.fail( - Error::internal_error("injected by test_ensure_running_unwind"), - ); + next_put_state + .wait_for_request(Duration::from_secs(60)) + .await + .fail(Error::internal_error(format!("injected by {test_name}"))); let saga_error = running_saga .wait_until_stopped() From fb3685eaecf93c253f36f3314b93a236767c9809 Mon Sep 17 00:00:00 2001 From: karencfv Date: Fri, 6 Nov 2026 12:46:05 +1300 Subject: [PATCH 19/19] address comments --- .../app/background/tasks/instance_reincarnation.rs | 9 +++++---- nexus/src/app/sagas/instance_start.rs | 12 ++++++++---- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/nexus/src/app/background/tasks/instance_reincarnation.rs b/nexus/src/app/background/tasks/instance_reincarnation.rs index 472112c72a8..ce8e6972e3c 100644 --- a/nexus/src/app/background/tasks/instance_reincarnation.rs +++ b/nexus/src/app/background/tasks/instance_reincarnation.rs @@ -1195,10 +1195,11 @@ mod test { tasks.instance_reincarnation.disable = true; // Increase the intervals for background tasks that could modify // VMM records while this test is running. - tasks.instance_watcher.period_secs = Duration::MAX; - tasks.abandoned_vmm_reaper.period_secs = Duration::MAX; - tasks.vmm_mark_stop_for_update.period_secs = Duration::MAX; - tasks.vmm_stop_for_update.period_secs = Duration::MAX; + tasks.instance_watcher.period_secs = Duration::from_hours(24); + tasks.abandoned_vmm_reaper.period_secs = Duration::from_hours(24); + tasks.vmm_mark_stop_for_update.period_secs = + Duration::from_hours(24); + tasks.vmm_stop_for_update.period_secs = Duration::from_hours(24); }) .start::() .await; diff --git a/nexus/src/app/sagas/instance_start.rs b/nexus/src/app/sagas/instance_start.rs index 1f8243ad8e9..c43fa38f355 100644 --- a/nexus/src/app/sagas/instance_start.rs +++ b/nexus/src/app/sagas/instance_start.rs @@ -540,10 +540,14 @@ async fn sis_move_to_starting_undo( || async { let nexus_state = match params.reason { Reason::SledEvacuation => { - // To avoid a race where a user may have stopped the - // instance while this saga was running, we check the - // instance first to make sure the intended state is still - // `Running`. + // At the time of writing this comment, there is a + // possibility of a race where a user may have stopped the + // instance while this saga was running. More details in + // https://github.com/oxidecomputer/omicron/issues/11706 + // + // Meanwhile, to reduce the chance of this happening, we + // check the instance first to make sure the intended state + // is still `Running`. match LookupPath::new(&opctx, osagactx.datastore()) .instance_id(instance_id.into_untyped_uuid()) .fetch()