Repository navigation
instance lifecycle: allow stopping instances stopped for sled evacuation #11639
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -247,6 +247,7 @@ impl From<InstanceStateChangeError> 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<InstanceStateChangeRequest> | |||||
|
|
||||||
| /// 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( | ||||||
| log: &slog::Logger, | ||||||
| instance_state: &db::model::Instance, | ||||||
| vmm_state: &Option<db::model::Vmm>, | ||||||
| requested: &InstanceStateChangeRequest, | ||||||
|
|
@@ -1314,19 +1338,10 @@ impl super::Nexus { | |||||
| )); | ||||||
| } | ||||||
|
|
||||||
| return Ok( | ||||||
| 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 { | ||||||
|
Contributor
Author
There was a problem hiding this comment. Maching on
Member
There was a problem hiding this comment. I agree that it's a bit unfortunate how much it complicates this code, but I think I agree with you that it's correct. A quasi-related change I've been working on will probably also require matching on internal states here, too (see #11608 (comment)). So, I think it's probably the right thing. |
||||||
| // 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 \ | ||||||
|
Member
There was a problem hiding this comment. it might be slightly nicer to say
Suggested change
because that's how it appears to the user?
Contributor
Author
There was a problem hiding this comment. Yeah I waffled on this one quite a lot (e.g., is it okay to say "due to sled evacuation" when that's not really a user-visible state) before realizing that a user can't actually see it - there's no external API to induce a VM migration. This is only reachable via the internal lockstep API. But I'm happy to reword it in case that changes. |
||||||
| 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!(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<db::model::Vmm>, | ||||||
| requested: InstanceStateChangeRequest, | ||||||
| ) -> Result<(), InstanceStateChangeError> { | ||||||
| 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( | ||||||
|
|
||||||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is nice, thanks for adding it! i do think there might be places outside of this module (i.e. in the instance-update saga) where we might want to use the internal part that takes a state and bumps the generation and sets the new state. so, I wonder a bit if it might be better to have an
InstanceRuntimeState::update_state(self, new_nexus_state)that's defined indb ::model, so that more code can use it, and have this helper use that?Not a blocker here though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this to
InstanceRuntimeState::with_new_nexus_state()in 1289b81