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 1a44f1d3a5e..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; @@ -247,6 +246,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 +271,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, @@ -1231,8 +1232,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 +1318,11 @@ 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() - }, + instance_state.runtime().with_new_nexus_state( + db::model::InstanceState::NoVmm, + ), ), ); } @@ -1339,10 +1335,82 @@ 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::UpdateRuntime( + instance_state + .runtime() + .with_new_nexus_state( + 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 is rebooting \ + 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 +1474,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, @@ -3065,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::{ @@ -3320,6 +3390,208 @@ mod tests { logctx.cleanup_successful(); } + #[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, + &InstanceStateChangeRequest::Migrate( + InstanceMigrationTargetParams { + src_propolis_addr: "[::1]:12400".parse().unwrap(), + }, + ), + ) + .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, + InstanceStateChangeRequest::Migrate( + InstanceMigrationTargetParams { + src_propolis_addr: "[::1]:12400".parse().unwrap(), + }, + ), + ] { + 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(