Repository navigation
wire shutdown policy into vmm_stop_for_update bg task - #11703
Conversation
| // FIXME(lif) before undraft PR: add shutdown policy data! | ||
| acpi_timeout_secs: None, | ||
| // TODO can sled-agent take the shutdown policy directly | ||
| // instead of an option? |
There was a problem hiding this comment.
I think it'd probably make sense to combine this with @hawkw's suggestion that we only send this as a part of the state requests where it's relevant, if possible?
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
This looks good to me overall, and I agree with the notes you left about changes to the base branch. I had a few small questions and notes.
| /// Just the columns needed to decode an instance's shutdown policy. | ||
| /// | ||
| /// Used by queries that join against `instance` but only care about the | ||
| /// shutdown policy. |
| // The `shutdown_policy_*` fields are private to avoid leaking the db | ||
| // representation of a stronger type; callers should use `shutdown_policy()` | ||
| // to get the reconstructed value from these db columns. | ||
| /// When `shutdown_policy_action` is `PowerButton`, this represents the time | ||
| /// propolis will wait for the guest to power itself down after sending it | ||
| /// the power button event, before forcefully terminating it. | ||
| /// Must be None if the action is `HardOff`. | ||
| pub shutdown_policy_timeout: Option<TimeDelta>, | ||
| shutdown_policy_timeout: Option<TimeDelta>, | ||
|
|
||
| /// The means by which to stop the instance. | ||
| pub shutdown_policy_action: InstanceShutdownAction, | ||
| shutdown_policy_action: InstanceShutdownAction, |
There was a problem hiding this comment.
could this be achieved by #[diesel(embed)]ding InstanceShutdownPolicyColumns directly in the Instance struct, so that we don't have to do this part? I guess we can't currently, because as written, InstanceShutdownPolicyColumns also has the UUID, but maybe the queries could .select((Uuid::as_select(), InstanceShutdownPolicyColumns::as_select()) or whatever?
Not a big deal to me if we don't do it like that, though, just a thought. I know we've had some problems with #[diesel(embed)] and column ordering in the past, but I don't think that would happen here, because they're being added at the end of the struct in the same order as they appear on the schema?
There was a problem hiding this comment.
could this be achieved by
#[diesel(embed)]dingInstanceShutdownPolicyColumnsdirectly in theInstancestruct, so that we don't have to do this part? I guess we can't currently, because as written,InstanceShutdownPolicyColumnsalso has the UUID, but maybe the queries could.select((Uuid::as_select(), InstanceShutdownPolicyColumns::as_select())or whatever?
Actually, after reading the code, I don't think we even need to select the instance ID from the instance record at all, since we're already selecting it from the vmm record? I think diesel shouldn't require us to select the key we join on from both tables, right?
There was a problem hiding this comment.
I don't think we need to select it for the use there, but I am using it in the shutdown_policy() method (only to include in the error message in the event of a bogus row - maybe it's fine to omit it there too?).
I'm pretty skittish about diesel(embed), I think partly from the problems you referenced and partly because I've been burned so badly by serde(flatten). I think I'd prefer to keep it separate, unless you feel strongly about it? And if we're keeping it separate, it seems okay to select the ID too for that error message usage?
There was a problem hiding this comment.
Yeah, I'm fine with that! Just felt it was worth mentioning.
| /// The combination of a VMM that should be stopped for its hosting sled to be | ||
| /// evacuated for an update and its instance's shutdown policy, returned by | ||
| /// [`DataStore::vmm_list_stoppable_for_update_by_sled_id`]. |
There was a problem hiding this comment.
super nitpicky, sorry: this sentence is kind of a mouthful, perhaps we could split it like:
| /// The combination of a VMM that should be stopped for its hosting sled to be | |
| /// evacuated for an update and its instance's shutdown policy, returned by | |
| /// [`DataStore::vmm_list_stoppable_for_update_by_sled_id`]. | |
| /// The combination of a VMM that should be stopped for its hosting sled to be | |
| /// evacuated for an update, and its instance's shutdown policy. This is | |
| /// returned by [`DataStore::vmm_list_stoppable_for_update_by_sled_id`]. |
| vmms_and_instances | ||
| .into_iter() | ||
| .map(|(vmm, instance)| { | ||
| let shutdown_policy = instance.shutdown_policy()?; |
There was a problem hiding this comment.
the ?ing here means we'll throw out the whole batch of VMMs if we get one that has an invalid shutdown policy. I'm not sure how much I care about this, because it is basically what we do every other place where we read stuff back from the database and have a try_from or similar that would fail if a CHECK constraint has been violated, but on the other hand, it seems like it's a little bit sketchy that we would give up on stopping all VMMs because we encountered one with a malformed shutdown policy...on the third hand, I am pretty sure that if we don't stop that VMM too, it would still hang the update indefinitely, but if we allow an operator to force-terminate the one VMM that's holding up sled-evacuation, we might still prefer to have already stopped all the other ones gracefully?
Sorry, hopefully that basically makes sense, I realize it's a bit of a rant...
There was a problem hiding this comment.
It does make sense, but I think I'm basically okay with everything you described. I think we have rather a lot of db-model methods that are fallible only in the event of a broken db constraint, and at least historically some subset of those were even just assert!()s. If we had a hung update because of a bad row, we'd really want to know (because that means something went horribly wrong somewhere), and also the workaround of "explicitly kill that bad instance" would allow us to progress, because we'd no longer find that VMM as one that needs to be stopped here, right?
lifning
left a comment
lifning
left a comment
There was a problem hiding this comment.
looks good to me, thank you!
| use omicron_common::api::external; | ||
|
|
||
| #[test] | ||
| fn test_instance_shutdown_policy() { |
There was a problem hiding this comment.
quite right, makes more sense here 😅
(This is pointed at #11565 and is one of that patches needed for it to work with sled evacuation during updates.) The main change here is that `vmm_list_stoppable_for_update_by_sled_id()` now returns each VMM's instance's shutdown policy alongside the VMM, and the `vmm_stop_for_update` background task now sends that policy when it's shutting down instances for sled evacuation. A smaller change that came along for the ride was some cleanup around the db representation of the shutdown policy: * the model fields are now private, forcing callers to go through the `shutdown_policy()` helper that reconstructs the Rust enum for the policy * `shutdown_policy()` now returns a `Result` instead of an `Option` - returning an `Option` means the caller has to decide what to do with `None`, which doesn't seem correct if the only way it can happen means something has gone horribly wrong (i.e., our db constraints have been violated). This bubbled out to changing some conversions from `.into()` to `.try_into()`, but I think that's basically fine and consistent with other types where we have to check at runtime that some unrepresentable state hasn't made its way back to us from the db. I don't expect CI tests to work on this branch since they don't on #11565 either; I don't think that should block merging this since it's really a part of that branch anyway.
(This is pointed at #11565 and is one of that patches needed for it to work with sled evacuation during updates.) The main change here is that `vmm_list_stoppable_for_update_by_sled_id()` now returns each VMM's instance's shutdown policy alongside the VMM, and the `vmm_stop_for_update` background task now sends that policy when it's shutting down instances for sled evacuation. A smaller change that came along for the ride was some cleanup around the db representation of the shutdown policy: * the model fields are now private, forcing callers to go through the `shutdown_policy()` helper that reconstructs the Rust enum for the policy * `shutdown_policy()` now returns a `Result` instead of an `Option` - returning an `Option` means the caller has to decide what to do with `None`, which doesn't seem correct if the only way it can happen means something has gone horribly wrong (i.e., our db constraints have been violated). This bubbled out to changing some conversions from `.into()` to `.try_into()`, but I think that's basically fine and consistent with other types where we have to check at runtime that some unrepresentable state hasn't made its way back to us from the db. I don't expect CI tests to work on this branch since they don't on #11565 either; I don't think that should block merging this since it's really a part of that branch anyway.
(This is pointed at #11565 and is one of that patches needed for it to work with sled evacuation during updates.) The main change here is that `vmm_list_stoppable_for_update_by_sled_id()` now returns each VMM's instance's shutdown policy alongside the VMM, and the `vmm_stop_for_update` background task now sends that policy when it's shutting down instances for sled evacuation. A smaller change that came along for the ride was some cleanup around the db representation of the shutdown policy: * the model fields are now private, forcing callers to go through the `shutdown_policy()` helper that reconstructs the Rust enum for the policy * `shutdown_policy()` now returns a `Result` instead of an `Option` - returning an `Option` means the caller has to decide what to do with `None`, which doesn't seem correct if the only way it can happen means something has gone horribly wrong (i.e., our db constraints have been violated). This bubbled out to changing some conversions from `.into()` to `.try_into()`, but I think that's basically fine and consistent with other types where we have to check at runtime that some unrepresentable state hasn't made its way back to us from the db. I don't expect CI tests to work on this branch since they don't on #11565 either; I don't think that should block merging this since it's really a part of that branch anyway.
(This is pointed at #11565 and is one of that patches needed for it to work with sled evacuation during updates.)
The main change here is that
vmm_list_stoppable_for_update_by_sled_id()now returns each VMM's instance's shutdown policy alongside the VMM, and thevmm_stop_for_updatebackground task now sends that policy when it's shutting down instances for sled evacuation.A smaller change that came along for the ride was some cleanup around the db representation of the shutdown policy:
shutdown_policy()helper that reconstructs the Rust enum for the policyshutdown_policy()now returns aResultinstead of anOption- returning anOptionmeans the caller has to decide what to do withNone, which doesn't seem correct if the only way it can happen means something has gone horribly wrong (i.e., our db constraints have been violated). This bubbled out to changing some conversions from.into()to.try_into(), but I think that's basically fine and consistent with other types where we have to check at runtime that some unrepresentable state hasn't made its way back to us from the db.I don't expect CI tests to work on this branch since they don't on #11565 either; I don't think that should block merging this since it's really a part of that branch anyway.