From b0f7dc853e25636af29a1b29c74b39a9e8abef3c Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Thu, 29 Oct 2026 17:14:28 -0400 Subject: [PATCH 1/4] instance lifecycle: allow stopping instances stopped for sled evacuation --- nexus/src/app/instance.rs | 315 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 300 insertions(+), 15 deletions(-) diff --git a/nexus/src/app/instance.rs b/nexus/src/app/instance.rs index 1a44f1d3a5e..f447ef1f442 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -247,6 +247,7 @@ impl From for dropshot::HttpError { /// The kinds of state changes that can be requested of an instance's current /// VMM (i.e. the VMM pointed to be the instance's `propolis_id` field). +#[derive(Debug)] pub(crate) enum InstanceStateChangeRequest { Run, Reboot, @@ -271,6 +272,7 @@ impl From /// The actions that can be taken in response to an /// [`InstanceStateChangeRequest`]. +#[derive(Debug)] enum InstanceStateChangeRequestAction { /// The instance is already in the correct state, so no action is needed. AlreadyDone, @@ -284,6 +286,25 @@ enum InstanceStateChangeRequestAction { UpdateRuntime(db::model::InstanceRuntimeState), } +impl InstanceStateChangeRequestAction { + /// Construct a [`Self::UpdateRuntime`] with a runtime state constructed by + /// updating `prev_runtime` with `new_nexus_state` (and the expected + /// accounting for a state change, such as bumping the generation). + fn update_runtime( + prev_runtime: db::model::InstanceRuntimeState, + new_nexus_state: db::model::InstanceState, + ) -> Self { + Self::UpdateRuntime(db::model::InstanceRuntimeState { + time_updated: chrono::Utc::now(), + generation: InstanceStateGeneration::from(prev_runtime.generation) + .next() + .into(), + nexus_state: new_nexus_state, + ..prev_runtime + }) + } +} + /// What is the higher level operation that is calling /// `instance_ensure_registered`? pub(crate) enum InstanceRegisterReason { @@ -1231,8 +1252,11 @@ impl super::Nexus { /// - `Ok(action)` if the request is allowed to proceed. The result payload /// specifies how to handle the request. /// - `Err` if the request should be denied. + // + // This is an associated function rather than a method so it can be tested + // independently. fn select_runtime_change_action( - &self, + log: &slog::Logger, instance_state: &db::model::Instance, vmm_state: &Option, requested: &InstanceStateChangeRequest, @@ -1314,19 +1338,10 @@ impl super::Nexus { )); } - let prev_runtime = instance_state.runtime(); return Ok( - InstanceStateChangeRequestAction::UpdateRuntime( - db::model::InstanceRuntimeState { - time_updated: chrono::Utc::now(), - generation: InstanceStateGeneration::from( - prev_runtime.generation, - ) - .next() - .into(), - nexus_state: db::model::InstanceState::NoVmm, - ..prev_runtime.clone() - }, + InstanceStateChangeRequestAction::update_runtime( + instance_state.runtime(), + db::model::InstanceState::NoVmm, ), ); } @@ -1339,10 +1354,79 @@ impl super::Nexus { ))); } + // An internal `SledEvacuating` state surfaces as a `Rebooting` + // effective state, but it's nontrivially different in practice: + // in particular, there is no associated VMM with an instance + // that's currently stopped due to sled evacuation. We represent + // it as `Rebooting` because it will be reincarnated, but we can + // still allow some changes in this state (in particular: the + // user should be able to stop it). + InstanceState::Rebooting => { + match instance_state.nexus_state { + // This is the state we expect to see, and it should not + // have an associated VMM. + nexus_db_model::InstanceState::SledEvacuating => { + if vmm_state.is_some() { + return Err(Error::internal_error( + "an instance should not be in the \ + SledEvacuating state if it has an \ + active VMM", + )); + } + } + // These states should not be reachable at this point: + // either they don't result in an effective state of + // `Rebooting`, or if they do, there should be an + // associated sled/VMM. + nexus_db_model::InstanceState::Creating + | nexus_db_model::InstanceState::NoVmm + | nexus_db_model::InstanceState::Vmm + | nexus_db_model::InstanceState::Failed + | nexus_db_model::InstanceState::Destroyed => { + return Err(Error::internal_error(format!( + "instance is rebooting but not resident on a \ + sled; internal state is {:?}", + instance_state.nexus_state, + ))); + } + } + + match requested { + // The instance reincarnation task is already trying to + // run instances in this state; these requests are fine + // and require no work on our part. + InstanceStateChangeRequest::Run + | InstanceStateChangeRequest::Reboot => { + return Ok( + InstanceStateChangeRequestAction::AlreadyDone, + ); + } + // Stopping an instance in this state is fine; we'll + // stop trying to reincarnate it. + InstanceStateChangeRequest::Stop => { + return Ok( + InstanceStateChangeRequestAction::update_runtime( + instance_state.runtime(), + db::model::InstanceState::NoVmm, + ), + ); + } + // We can't migrate instances in this state, which is an + // unfortunate asymmetry with "normal" rebooting + // instances. But there's no source VMM to migrate from. + InstanceStateChangeRequest::Migrate(_) => { + return Err(Error::invalid_request( + "cannot migrate instance that was stopped \ + due to sled evacuation", + )); + } + } + } + // In other states, the instance should have a sled, and an // internal invariant has been violated if it doesn't have one. _ => { - error!(self.log, "instance has no sled but isn't halted"; + error!(log, "instance has no sled but isn't halted"; "instance_id" => %instance_state.id(), "state" => ?effective_state); @@ -1406,7 +1490,8 @@ impl super::Nexus { prev_vmm_state: &Option, requested: InstanceStateChangeRequest, ) -> Result<(), InstanceStateChangeError> { - match self.select_runtime_change_action( + match Self::select_runtime_change_action( + &self.log, prev_instance_state, prev_vmm_state, &requested, @@ -3320,6 +3405,206 @@ mod tests { logctx.cleanup_successful(); } + fn migrate_request() -> InstanceStateChangeRequest { + InstanceStateChangeRequest::Migrate(InstanceMigrationTargetParams { + src_propolis_addr: "[::1]:12400".parse().unwrap(), + }) + } + + #[test] + fn test_select_action_sled_evacuating_stop() { + let logctx = test_setup_log("test_select_action_sled_evacuating_stop"); + let (mut instance, _vmm) = make_instance_and_vmm(); + instance.nexus_state = DbInstanceState::SledEvacuating; + let action = Nexus::select_runtime_change_action( + &logctx.log, + &instance, + &None, + &InstanceStateChangeRequest::Stop, + ) + .expect("stopping a SledEvacuating instance should be allowed"); + let InstanceStateChangeRequestAction::UpdateRuntime(rt) = action else { + panic!("expected UpdateRuntime, got {action:?}"); + }; + assert_eq!(rt.nexus_state, DbInstanceState::NoVmm); + assert_eq!(rt.propolis_id, None); + assert_eq!( + InstanceStateGeneration::from(rt.generation), + InstanceStateGeneration::from(instance.state_generation).next(), + ); + logctx.cleanup_successful(); + } + + #[test] + fn test_select_action_sled_evacuating_reboot_and_run() { + let logctx = + test_setup_log("test_select_action_sled_evacuating_reboot_and_run"); + let (mut instance, _vmm) = make_instance_and_vmm(); + instance.nexus_state = DbInstanceState::SledEvacuating; + for requested in [ + InstanceStateChangeRequest::Reboot, + InstanceStateChangeRequest::Run, + ] { + let action = Nexus::select_runtime_change_action( + &logctx.log, + &instance, + &None, + &requested, + ) + .unwrap_or_else(|e| panic!("{requested:?} should succeed: {e}")); + assert!( + matches!(action, InstanceStateChangeRequestAction::AlreadyDone), + "{requested:?} should be AlreadyDone, got {action:?}" + ); + } + logctx.cleanup_successful(); + } + + #[test] + fn test_select_action_sled_evacuating_migrate() { + let logctx = + test_setup_log("test_select_action_sled_evacuating_migrate"); + let (mut instance, _vmm) = make_instance_and_vmm(); + instance.nexus_state = DbInstanceState::SledEvacuating; + let err = Nexus::select_runtime_change_action( + &logctx.log, + &instance, + &None, + &migrate_request(), + ) + .expect_err("migrating a SledEvacuating instance should fail"); + assert!( + matches!(err, Error::InvalidRequest { .. }), + "expected InvalidRequest, got {err:?}" + ); + logctx.cleanup_successful(); + } + + #[test] + fn test_select_action_vmm_rebooting_stop_goes_to_sled() { + let logctx = test_setup_log( + "test_select_action_vmm_rebooting_stop_goes_to_sled", + ); + let (mut instance, mut vmm) = make_instance_and_vmm(); + instance.nexus_state = DbInstanceState::Vmm; + instance.propolis_id = Some(vmm.id); + vmm.state = DbVmmState::Rebooting; + let expected_sled_id = vmm.sled_id(); + let expected_propolis_id = PropolisUuid::from_untyped_uuid(vmm.id); + let action = Nexus::select_runtime_change_action( + &logctx.log, + &instance, + &Some(vmm), + &InstanceStateChangeRequest::Stop, + ) + .expect("stopping a rebooting instance with a VMM should be allowed"); + match action { + InstanceStateChangeRequestAction::SendToSled { + sled_id, + propolis_id, + } => { + assert_eq!(sled_id, expected_sled_id); + assert_eq!(propolis_id, expected_propolis_id); + } + other => panic!("expected SendToSled, got {other:?}"), + } + logctx.cleanup_successful(); + } + + #[test] + fn test_select_action_failed_stop() { + let logctx = test_setup_log("test_select_action_failed_stop"); + let (mut instance, _vmm) = make_instance_and_vmm(); + instance.nexus_state = DbInstanceState::Failed; + let action = Nexus::select_runtime_change_action( + &logctx.log, + &instance, + &None, + &InstanceStateChangeRequest::Stop, + ) + .expect("stopping a Failed instance should be allowed"); + let InstanceStateChangeRequestAction::UpdateRuntime(rt) = action else { + panic!("expected UpdateRuntime, got {action:?}"); + }; + assert_eq!(rt.nexus_state, DbInstanceState::NoVmm); + assert_eq!(rt.propolis_id, None); + assert_eq!( + InstanceStateGeneration::from(rt.generation), + InstanceStateGeneration::from(instance.state_generation).next(), + ); + logctx.cleanup_successful(); + } + + #[test] + fn test_select_action_no_vmm_stop_is_idempotent() { + let logctx = + test_setup_log("test_select_action_no_vmm_stop_is_idempotent"); + let (mut instance, _vmm) = make_instance_and_vmm(); + instance.nexus_state = DbInstanceState::NoVmm; + let action = Nexus::select_runtime_change_action( + &logctx.log, + &instance, + &None, + &InstanceStateChangeRequest::Stop, + ) + .expect("stopping a stopped instance should succeed"); + assert!( + matches!(action, InstanceStateChangeRequestAction::AlreadyDone), + "expected AlreadyDone, got {action:?}" + ); + logctx.cleanup_successful(); + } + + #[test] + fn test_select_action_no_vmm_run_reboot_migrate_rejected() { + let logctx = test_setup_log( + "test_select_action_no_vmm_run_reboot_migrate_rejected", + ); + let (mut instance, _vmm) = make_instance_and_vmm(); + instance.nexus_state = DbInstanceState::NoVmm; + for requested in [ + InstanceStateChangeRequest::Run, + InstanceStateChangeRequest::Reboot, + migrate_request(), + ] { + let err = match Nexus::select_runtime_change_action( + &logctx.log, + &instance, + &None, + &requested, + ) { + Ok(action) => { + panic!("{requested:?} should fail, got {action:?}") + } + Err(err) => err, + }; + assert!( + matches!(err, Error::InvalidRequest { .. }), + "{requested:?}: expected InvalidRequest, got {err:?}" + ); + } + logctx.cleanup_successful(); + } + + #[test] + fn test_select_action_creating_rejected() { + let logctx = test_setup_log("test_select_action_creating_rejected"); + let (mut instance, _vmm) = make_instance_and_vmm(); + instance.nexus_state = DbInstanceState::Creating; + let err = Nexus::select_runtime_change_action( + &logctx.log, + &instance, + &None, + &InstanceStateChangeRequest::Stop, + ) + .expect_err("changing state of a Creating instance should fail"); + assert!( + matches!(err, Error::InvalidRequest { .. }), + "expected InvalidRequest, got {err:?}" + ); + logctx.cleanup_successful(); + } + /// Build a DbInstance from `make_instance_and_vmm`'s base params with the /// `enable_jumbo_frames` field overridden. fn make_instance_with_jumbo_frames( From b411a705072270a3934af2f5b0af3c8059cfc20f Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 30 Oct 2026 09:18:25 -0400 Subject: [PATCH 2/4] drop test helper fn that isn't carrying its weight --- nexus/src/app/instance.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/nexus/src/app/instance.rs b/nexus/src/app/instance.rs index f447ef1f442..9fd31c10ef0 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -3405,12 +3405,6 @@ mod tests { logctx.cleanup_successful(); } - fn migrate_request() -> InstanceStateChangeRequest { - InstanceStateChangeRequest::Migrate(InstanceMigrationTargetParams { - src_propolis_addr: "[::1]:12400".parse().unwrap(), - }) - } - #[test] fn test_select_action_sled_evacuating_stop() { let logctx = test_setup_log("test_select_action_sled_evacuating_stop"); @@ -3470,7 +3464,11 @@ mod tests { &logctx.log, &instance, &None, - &migrate_request(), + &InstanceStateChangeRequest::Migrate( + InstanceMigrationTargetParams { + src_propolis_addr: "[::1]:12400".parse().unwrap(), + }, + ), ) .expect_err("migrating a SledEvacuating instance should fail"); assert!( @@ -3565,7 +3563,11 @@ mod tests { for requested in [ InstanceStateChangeRequest::Run, InstanceStateChangeRequest::Reboot, - migrate_request(), + InstanceStateChangeRequest::Migrate( + InstanceMigrationTargetParams { + src_propolis_addr: "[::1]:12400".parse().unwrap(), + }, + ), ] { let err = match Nexus::select_runtime_change_action( &logctx.log, From 6c2ea5d5ad17b1a1f71497faa828479631e97e71 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 30 Oct 2026 13:14:56 -0400 Subject: [PATCH 3/4] reword error message trying to migrate sled_evacuating instances --- nexus/src/app/instance.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/src/app/instance.rs b/nexus/src/app/instance.rs index 9fd31c10ef0..5e4c17fba5d 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -1416,7 +1416,7 @@ impl super::Nexus { // instances. But there's no source VMM to migrate from. InstanceStateChangeRequest::Migrate(_) => { return Err(Error::invalid_request( - "cannot migrate instance that was stopped \ + "cannot migrate instance that is rebooting \ due to sled evacuation", )); } From 1289b81883d78a90fc7916f873057b089357cd21 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 30 Oct 2026 13:27:18 -0400 Subject: [PATCH 4/4] move InstanceRuntimeState helper to db-model type --- nexus/db-model/src/instance.rs | 34 +++++++++++++++++++++++++++++++ nexus/src/app/instance.rs | 37 ++++++++++------------------------ 2 files changed, 45 insertions(+), 26 deletions(-) diff --git a/nexus/db-model/src/instance.rs b/nexus/db-model/src/instance.rs index 125182722ac..13a0a47a82f 100644 --- a/nexus/db-model/src/instance.rs +++ b/nexus/db-model/src/instance.rs @@ -387,6 +387,40 @@ pub struct InstanceRuntimeState { pub time_last_auto_restarted: Option>, } +impl InstanceRuntimeState { + /// Construct an `InstanceRuntimeState` with a new Nexus state, performing + /// the necessary accounting for a state change (such as bumping the + /// generation). + /// + /// Fields not related to modifying the Nexus state remain unchanged from + /// `self`. + pub fn with_new_nexus_state(self, new_nexus_state: InstanceState) -> Self { + let Self { + // fields we overwrite + time_updated: _, + nexus_state: _, + + // fields we modify + generation, + + // fields we carry forward unchanged + propolis_id, + dst_propolis_id, + migration_id, + time_last_auto_restarted, + } = self; + Self { + time_updated: chrono::Utc::now(), + nexus_state: new_nexus_state, + generation: DbTypedGeneration(generation.0.next()), + propolis_id, + dst_propolis_id, + migration_id, + time_last_auto_restarted, + } + } +} + /// Configuration for automatic instance restarts. #[derive( Clone, diff --git a/nexus/src/app/instance.rs b/nexus/src/app/instance.rs index 5e4c17fba5d..e279a1b29e7 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -67,7 +67,6 @@ use omicron_common::api::external::LookupResult; use omicron_common::api::external::NameOrId; use omicron_common::api::external::UpdateResult; use omicron_common::api::external::http_pagination::PaginatedBy; -use omicron_generation_kinds::InstanceStateGeneration; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::InstanceUuid; use omicron_uuid_kinds::MulticastGroupUuid; @@ -286,25 +285,6 @@ enum InstanceStateChangeRequestAction { UpdateRuntime(db::model::InstanceRuntimeState), } -impl InstanceStateChangeRequestAction { - /// Construct a [`Self::UpdateRuntime`] with a runtime state constructed by - /// updating `prev_runtime` with `new_nexus_state` (and the expected - /// accounting for a state change, such as bumping the generation). - fn update_runtime( - prev_runtime: db::model::InstanceRuntimeState, - new_nexus_state: db::model::InstanceState, - ) -> Self { - Self::UpdateRuntime(db::model::InstanceRuntimeState { - time_updated: chrono::Utc::now(), - generation: InstanceStateGeneration::from(prev_runtime.generation) - .next() - .into(), - nexus_state: new_nexus_state, - ..prev_runtime - }) - } -} - /// What is the higher level operation that is calling /// `instance_ensure_registered`? pub(crate) enum InstanceRegisterReason { @@ -1339,9 +1319,10 @@ impl super::Nexus { } return Ok( - InstanceStateChangeRequestAction::update_runtime( - instance_state.runtime(), - db::model::InstanceState::NoVmm, + InstanceStateChangeRequestAction::UpdateRuntime( + instance_state.runtime().with_new_nexus_state( + db::model::InstanceState::NoVmm, + ), ), ); } @@ -1405,9 +1386,12 @@ impl super::Nexus { // stop trying to reincarnate it. InstanceStateChangeRequest::Stop => { return Ok( - InstanceStateChangeRequestAction::update_runtime( - instance_state.runtime(), - db::model::InstanceState::NoVmm, + InstanceStateChangeRequestAction::UpdateRuntime( + instance_state + .runtime() + .with_new_nexus_state( + db::model::InstanceState::NoVmm, + ), ), ); } @@ -3150,6 +3134,7 @@ mod tests { use omicron_common::api::external::{ Hostname, IdentityMetadataCreateParams, Name, }; + use omicron_generation_kinds::InstanceStateGeneration; use omicron_test_utils::dev::test_setup_log; use propolis_client::support::tungstenite::protocol::Role; use propolis_client::support::{