Repository navigation
instance lifecycle: allow stopping instances stopped for sled evacuation - #11639
Conversation
| // still allow some changes in this state (in particular: the | ||
| // user should be able to stop it). | ||
| InstanceState::Rebooting => { | ||
| match instance_state.nexus_state { |
There was a problem hiding this comment.
Maching on nexus_state inside the match on InstanceState feels a little grimy, since the nexus_state was part of the input to pick which InstanceState we're in. But I think it's still worth doing purely as an "ensure we don't have some other combination of internal states we don't expect" measure?
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.
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
This looks good to me, with a couple notes.
| // instances. But there's no source VMM to migrate from. | ||
| InstanceStateChangeRequest::Migrate(_) => { | ||
| return Err(Error::invalid_request( | ||
| "cannot migrate instance that was stopped \ |
There was a problem hiding this comment.
it might be slightly nicer to say
| "cannot migrate instance that was stopped \ | |
| "cannot migrate instance that is rebooting \ |
because that's how it appears to the user?
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.
| 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 | ||
| }) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
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 in db ::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.
Moved this to InstanceRuntimeState::with_new_nexus_state() in 1289b81
This allows users to stop an instance that appears stuck in rebooting but is actually stopped for sled evacuation and waiting to be reincarnated, as described in #11504 (review).