Support for instance 'graceful' shutdown policy by lifning · Pull Request #11565 · oxidecomputer/omicron

lifning · 2026-10-22T09:58:29Z

This feature is intended to allow VM guest shutdowns to be triggered through the API and as part of sled evacuations for updates. An intent of this feature is to afford non-ephemeral instances a chance to cleanly unmount their filesystems, such that taking a new Oxide software release does not carry with it an unnecessary risk of filesystem corruption.

To quote RFD 622:

Oxide users must manually access each instance to perform a stop, which is very inconvenient when dealing with large numbers of instances. This is particularly relevant for mupdates, and until live-migration is available users are likely to frequently want to stop their instances. Even with live-migration, they may want to gracefully stop instances that are too large to migrate on a fully utilized rack.

A note on terminology: ACPI calls the feature currently implemented on the Propolis side "Fixed Power Button". ACPI also specifies an altogether different feature that it calls "Graceful Shutdown Request" which is not presently implemented by Propolis. We may sometimes talk about "gracefully" stopping an instance, but for the sake of clarity, we may wish to be take care not to confuse operators and customers with the term "Graceful Shutdown" in documentation.

POST /v1/instances now takes a shutdown_policy parameter, either HardOff or PowerButton with a (mandatory) timeout between 1 and 3600 seconds. New instances with no shutdown policy specified in their create request will default to a policy of the pre-existing behavior: Hard-poweroff of the guest OS with no forewarning. Existing instances will also default to hard_off in the CRDB migration.

PUT /v1/instances/{instance} will only change an instance's shutdown_policy if one is explicitly included in the request, such that clients of an older API revision don't potentially clobber an existing instance's shutdown policy.

POST /v1/instances/{instance}/stop and POST /v1/instances/{instance}/reboot now may be passed skip_os_shutdown=true to force a hard-poweroff even if an instance has a soft shutdown policy. If an instance does have a PowerButton shutdown policy, and skip_os_shutdown isn't passed, the instance will have its simulated Power Button pressed, and then following either the guest-initiated power event or the exhaustion of the configured timeout, the instance will be accordingly stopped or rebooted by Propolis.

Further requests to stop or reboot an instance while it is still in the process of shutting down (i.e. the guest hasn't emitted a power event and the configured timeout hasn't expired) are permitted to change the instance's fate, i.e. whether or not Propolis will reboot the instance once it has stopped. However, changes to the shutdown policy's timeout will not be propagated to instances that are already shutting down; only the first timeout Propolis receives in a stop or reboot request is considered. Non-ACPI stop/reset requests, either through skip_os_shutdown or changing to a HardOff policy, will be honored.

The sled-agent's existing 10-minute propolis-zone-reaping timeout is extended by the configured timeout of a PowerButton-type shutdown; that is to say, the zone will be forcibly terminated if propolis does not stop in the ACPI timeout plus 10 minutes. If a hard-stop request is sent during this timeout, and there are more than 10 minutes remaining, it will be replaced by a 10-minute timeout. If a reboot request is sent to propolis while it is still waiting for an ACPI shutdown, the zone-reaping timeout is dropped.

tbd: merge propolis branch, change Cargo.toml and package-manifest.toml accordingly

karencfv · 2026-10-23T01:38:06Z

Heya! Just a heads up that this -> #11380 will be landing very soon. It's part of a larger issue to orchestrate instance restarts during live update (RFD 739).

Notably, in the specific PR that I linked we have to use vmm_put_state() directly because we need to make sure the intended state continues to be what it was. #11380 (comment) . Looking at the code here, I'm not sure how the graceful shutdown process would fit in as it is in this draft.

Anyway, happy to discuss further if you have any questions about what we're doing in update land or whatever :)

lifning · 2026-10-23T03:04:11Z

Notably, in the specific PR that I linked we have to use vmm_put_state() directly because we need to make sure the intended state continues to be what it was. #11380 (comment) . Looking at the code here, I'm not sure how the graceful shutdown process would fit in as it is in this draft.

re: your note from from that PR (copied here),

Note: I took a look at how graceful shutdown is being implemented on the control plane side in main...lif/acpi-shutdown. It appears there will be a shutdown_policy_action knob similar to how the auto_restart_policy works today. Once this functionality is implemented, we can decide whether we override this preference or just let whatever settings the user set to carry on to this task.

my understanding is that one of the main desires for shutting down instances gracefully is customers wanting not to have uncleanly unmounted disks in the "shutting everything down to take an update" use-case, so it's my assumption that we would want to send the vmm_put_state with the appropriate timeout from policy in the instance table.

karencfv · 2026-10-23T03:16:34Z

re: your note from from that PR (copied here),

Argh, I wrote that note ages ago, sorry! I should have removed it before writing this 😅

my understanding is that one of the main desires for shutting down instances gracefully is customers wanting not to have uncleanly unmounted disks in the "shutting everything down to take an update" use-case, so it's my assumption that we would want to send the vmm_put_state with the appropriate timeout from policy in the instance table.

Yes, definitely. Unless the customers don't want to wait for graceful shutdown (disabling graceful shutdown would really speed up updates). In this case, I guess that means if they chose the HardOff shutdown policy.

lifning · 2026-10-23T03:34:10Z

in that case, looking at stop_batch in there, should i perhaps consider propagating the shutdown policy info into the vmm table so it's more readily accessible to the background task for that purpose? (forgive my relative diesel naivete if that's a silly question)

karencfv · 2026-10-23T03:54:05Z

Do you mean propagating the policy to each VMM entry and then passing the policy via vmm_put_state() in stop_vmm() ?

I've only taken a look at this PR superficially, but I wonder if there is a way to have it so the background task doesn't need to know about the policy at all. When this task stops the VMM the system automatically shuts down the instances gracefully or not depending on the policy in each instance. Is that possible?

lifning · 2026-10-23T06:16:49Z

I've only taken a look at this PR superficially, but I wonder if there is a way to have it so the background task doesn't need to know about the policy at all. When this task stops the VMM the system automatically shuts down the instances gracefully or not depending on the policy in each instance. Is that possible?

as i've implemented the sled-agent side of this (per the rfd), vmm_put_state just forwards whatever timeout is passed to it by nexus to propolis. so nexus is the thing responsible for retrieving the instance's shutdown policy from the instance table in the database whenever an instance stop/reboot is requested, and the sled-agent has no knowledge of what the instance's shutdown policy when it's servicing a vmm_put_state request besides what's included in the request itself.

karencfv · 2026-10-26T20:59:07Z

Ah aha! I see now. Then yeah, I think your proposal of propagating the information to the VMM table makes sense.

Adds a task that gathers all stoppable VMMs that are marked to stop for a sled evacuation during an update, and stops them. Sample of how the information will be displayed: Various successes, failures and errors ```console VMMs stopped for sled evacuation: 13 total sled_id=1b8c2e9a-4d3f-4b1e-9c7a-2f5e8d6b0a11 n_stopped=6 sled_id=a4f0d7c3-8e2b-4c6d-b1f9-7e3a5c9d2b44 n_stopped=7 VMMs that failed to stop: 7 total sled_id=1b8c2e9a-4d3f-4b1e-9c7a-2f5e8d6b0a11 n_failed_to_stop=3 vmm_id=5c3e9f12-7a4b-4d8e-a2c6-1f9b3e7d5a08 vmm_id=7f1a2b3c-4d5e-4f60-8a7b-9c0d1e2f3a41 vmm_id=9d4e6f80-2b1c-4a3d-b5e6-0f7a8b9c1d52 sled_id=a4f0d7c3-8e2b-4c6d-b1f9-7e3a5c9d2b44 n_failed_to_stop=4 vmm_id=e2d8b4f6-1c9a-4e7b-8d3f-6a2c5b9e1f37 vmm_id=0a1b2c3d-5e6f-4a70-9b81-c2d3e4f5a663 vmm_id=3c4d5e6f-7a8b-4c9d-8e0f-1a2b3c4d5e74 vmm_id=b6c7d8e9-0f1a-4b2c-9d3e-4f5a6b7c8d85 Error messages: 2 total fake error 1 fake error 2 ``` No failures ```console VMMs stopped for sled evacuation: 13 total sled_id=1b8c2e9a-4d3f-4b1e-9c7a-2f5e8d6b0a11 n_stopped=6 sled_id=a4f0d7c3-8e2b-4c6d-b1f9-7e3a5c9d2b44 n_stopped=7 ``` Nothing happened ```console VMMs stopped for sled evacuation: 0 total ``` Note: graceful shutdown preferences will need to be integrated in #11565, or once it lands Related: #11169

hawkw

As we discussed in the hypervisor huddle today, I think the best way to resolve the problem of needing the configured policy in the vmm_stop_for_update task is to make the query it's running do a JOIN with the VMM's instance record, rather than copying the policy into the vmm record. The latter means we would have to keep the two in sync when the policy changes, which seems fraught.

hawkw · 2026-10-29T20:59:41Z

+                                        let acpi_timeout = Duration::from_secs(
+                                            acpi_timeout_secs.unwrap_or(0)
+                                        );
                                        stop_timeout = Some(Box::pin(tokio::time::sleep(
-                                            Self::STOP_GRACE_PERIOD
+                                            Self::STOP_GRACE_PERIOD + acpi_timeout
                                        )));


Now that the timeout duration is variable, it might be nice if there was some way to get the timeout duration included in the log message that's logged when the VM fails to stop within the timeout. currently, that's logged here:

omicron/sled-agent/src/instance.rs

Lines 894 to 898 in d58f564

warn!(

self.log,

"Instance failed to stop within the grace period, \

terminating it violently!",

);

That might require changing up the stop timeout code a bit, perhaps by making the future return the timeout duration when it completes...

Can Nexus send a different, non-Stopped desired state to this VMM while we're in the shutdown grace period (e.g., "stop shutting down and reboot instead")? If so, do we need to disarm this timeout if we get that?

@jgallagher as currently written, I believe whether the sled-agent will pay any attention to a subsequent request will depend on whether the call to the Propolis API returns only after the shutdown has completed. We will await the call to self.put_state(...) (which is what sends the propolis request) here:

omicron/sled-agent/src/instance.rs

Line 986 in 3fb3cb6

tx.send(self.put_state(state, acpi_timeout_secs).await

before we pop any more requests from the queue. I think the Propolis request just puts the stop in a queue and returns immediately, and we rely on the instance-state-monitor endpoint to inform us that the guest has finished stopping, but would have to double-check.

However, I don't think there's a way for us to cancel the request to propolis once it's in the queue, so I'm not sure what a request to reboot the instance will actually do off the top of my head. Will have to dig a bit deeper into the propolis behavior.

on the propolis side of this change, i do allow an ongoing soft shutdown request's fated state to change to a reboot: https://github.com/oxidecomputer/propolis/pull/1217/changes#diff-83dcbf33dccd9b502248446b840ea28f33ebc239fbde1dbc1859bba0a11d20b7R894-R900
in which case, indeed we should disarm the timeout - but possibly not by completely trashing the future, as we also allow it to be changed back to a stop request while propolis' side of the timeout is still running, at which point it would make sense for its original contemporary stop timeout to be rearmed.

(good catch btw - my original propolis impl didn't allow for this, it was added in response to PR feedback and i neglected to account for that here)

(changed to disarm the stop_timeout for reboots, and shorten it if appropriate for force-offs)

hawkw · 2026-10-29T21:02:37Z

+        /// The number of seconds to wait between sending the VM guest a power
+        /// button signal and forcefully terminating it, if it does not power
+        /// itself down by then after receiving the signal.
+        pub acpi_timeout_secs: Option<u64>,


I think it's a bit of a bummer that we send this with every state change request, even ones where it doesn't really make sense, like Running...might it make sense to instead make it part of the Stopped and Reboot variants of VmmStateRequested, so that it's only present when we are actually stopping the guest?

jgallagher

Skimmed over this in the same context as Karen (i.e., how this fits in with the reconfigurator work for sled evacuation) and had a few questions. Sorry if they're premature - I know this is a draft, but I'd be happy to cobble together a test branch that includes this and the outstanding reconfigurator bits to start testing it as soon as that's feasible.

jgallagher · 2026-10-30T19:19:27Z

+            auto_restart_status: value.auto_restart_status,
+            cpu_platform: value.cpu_platform,
+            enable_jumbo_frames: value.enable_jumbo_frames,
+            shutdown_policy: None,


Trying to follow all of the defaults / version compatibility bits around this field. IIUC:

If we get request from a new client with shutdown_policy: None, we'll assume they wanted PowerButton { 10 minutes } (in project_create_instance() in Nexus). If the client then views the instance, they'll see Some(PowerButton { 10 minutes }), not None.

If we get a request from an old client, we fill in None during this conversion, which Nexus will then turn into PowerButton { 10 minutes }. If the client then views the instance, they'll get a 500 error due to the TryFrom below because Some(PowerButton { .. }) can't be represented.

For any existing instances, we assign them hard_off during the db migration.

Some questions/comments from those:

1 seems a little weird in isolation, in particular that we accept None but never return it. Does the field need to be optional, or could we require it? If we can't require it, could we still make it non-optional but defaulted to PowerOff { 10 minutes }?

I'm not sure what to do about 2, if the default is supposed to be PowerOff { 10 minutes }, but it doesn't seem very sporting to not let older clients see instances they can create. Could we turn the TryFrom into a From and always drop the field?

3 seems a little inconsistent with 1 - if the default should be PowerOff { 10 minutes }, should we also make that the setting we apply to preexisting instances?

I know we talked about this a bit at the update sync yesterday, I've tried to digest that conversation into RFD discussion at https://github.com/oxidecomputer/rfd/pull/973#discussion_r4473974092 . the proposal there is that we do not change the default to PowerButton, since it invites all the complications above and it's not clear what timeout would be appropriate generally.

if we agree, then I think we'd want to take some of the other words about comparable shutdown timeouts and at least have some guidance for users about what timeouts make sense for them and the way it may relate to operations (e.g. stalling updates [and sled evacuation for expunge?])

jgallagher · 2026-10-30T19:30:19Z

+                                        let acpi_timeout = Duration::from_secs(
+                                            acpi_timeout_secs.unwrap_or(0)
+                                        );
                                        stop_timeout = Some(Box::pin(tokio::time::sleep(
-                                            Self::STOP_GRACE_PERIOD
+                                            Self::STOP_GRACE_PERIOD + acpi_timeout
                                        )));


Can Nexus send a different, non-Stopped desired state to this VMM while we're in the shutdown grace period (e.g., "stop shutting down and reboot instead")? If so, do we need to disarm this timeout if we get that?

hawkw · 2026-10-30T20:06:44Z

+                                        let acpi_timeout = Duration::from_secs(
+                                            acpi_timeout_secs.unwrap_or(0)
+                                        );
                                        stop_timeout = Some(Box::pin(tokio::time::sleep(
-                                            Self::STOP_GRACE_PERIOD
+                                            Self::STOP_GRACE_PERIOD + acpi_timeout
                                        )));


@jgallagher as currently written, I believe whether the sled-agent will pay any attention to a subsequent request will depend on whether the call to the Propolis API returns only after the shutdown has completed. We will await the call to self.put_state(...) (which is what sends the propolis request) here:

omicron/sled-agent/src/instance.rs

Line 986 in 3fb3cb6

tx.send(self.put_state(state, acpi_timeout_secs).await

before we pop any more requests from the queue. I think the Propolis request just puts the stop in a queue and returns immediately, and we rely on the instance-state-monitor endpoint to inform us that the guest has finished stopping, but would have to double-check.

However, I don't think there's a way for us to cancel the request to propolis once it's in the queue, so I'm not sure what a request to reboot the instance will actually do off the top of my head. Will have to dig a bit deeper into the propolis behavior.

(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 PR addresses items 1 and 2 from #11565 (comment). (Item 3 was addressed yesterday in 28daa74, which changed the default for the API to `hard_off`.) The changes for those two are: 1. The `shutdown_policy` field is no longer an `Option` for either `Instance` or `InstanceCreate`. `InstanceCreate` has a `#[serde(default)]`, so it's not required at the API layer, but this way (a) we don't have to deal with `None` and (b) we don't have the oddity of always returning `Some(_)` to clients even if they had specified `None`. 2. The type conversion for old clients just throws away the `shutdown_policy` field instead of failing if it isn't `HardOff`. This is less important now that the default is `HardOff`, but I still think it's reasonable - this way old clients can still see / list instances that were created by new clients. They just won't be able to tell what those instance's shutdown policies are. (`shutdown_policy` is still `None` in `InstanceUpdate`, which means "don't change", so an instance update from an old client is fine: it doesn't know about `shutdown_policy`, which turns into `None`, which turns into "don't change it".) Most of the bulk off the diff is trivial fallout from 1 - changing a bunch of `None`s to `InstanceShutdownPolicy::default()`s in a lot of tests. The end-to-end test had to switch from specifying all the fields to using the progenitor-client's builder API (which allows you to _not_ specify defaultable fields). A few other cleanup things came along for the ride: 1. I moved the enforcement of the ACPI timeout second range into a newtype in the API. This means it's enforced at deserialization time, so we don't have to remember to validate it every place it comes in in Nexus. 2. The instance update handler had duplicated the logic of "convert the `InstanceShutdownPolicy` into the 2-column database representation". I moved this into an `InstanceUpdate::new()` function in the db-model; I don't love that it takes a bunch of args but I think it's still a net win? 3. I changed the propolis pointer in `package-manifest.toml` to match what's in `Cargo.toml` (all of which now point to the latest commit on oxidecomputer/propolis#1217). I'm hoping this lets CI make more progress; I'd like to confirm the end-to-end test change mentioned above, at least, which I think requires CI.

(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 PR addresses items 1 and 2 from #11565 (comment). (Item 3 was addressed yesterday in 28daa74, which changed the default for the API to `hard_off`.) The changes for those two are: 1. The `shutdown_policy` field is no longer an `Option` for either `Instance` or `InstanceCreate`. They do both have a `#[serde(default)]` on them, so they're not required at the API layer, but this way (a) we don't have to deal with `None` and (b) we don't have the oddity of always returning `Some(_)` to clients even if they had specified `None`. 2. The type conversion for old clients just throws away the `shutdown_policy` field instead of failing if it isn't `HardOff`. This is less important now that the default is `HardOff`, but I still think it's reasonable - this way old clients can still see / list instances that were created by new clients. They just won't be able to tell what those instance's shutdown policies are. (`shutdown_policy` is still `None` in `InstanceUpdate`, which means "don't change", so an instance update from an old client is fine: it doesn't know about `shutdown_policy`, which turns into `None`, which turns into "don't change it".) Most of the bulk off the diff is trivial fallout from 1 - changing a bunch of `None`s to `InstanceShutdownPolicy::default()`s in a lot of tests. The end-to-end test had to switch from specifying all the fields to using the progenitor-client's builder API (which allows you to _not_ specify defaultable fields). A few other cleanup things came along for the ride: 1. I moved the enforcement of the ACPI timeout second range into a newtype in the API. This means it's enforced at deserialization time, so we don't have to remember to validate it every place it comes in in Nexus. 2. The instance update handler had duplicated the logic of "convert the `InstanceShutdownPolicy` into the 2-column database representation". I moved this into an `InstanceUpdate::new()` function in the db-model; I don't love that it takes a bunch of args but I think it's still a net win? 3. I changed the propolis pointer in `package-manifest.toml` to match what's in `Cargo.toml` (all of which now point to the latest commit on oxidecomputer/propolis#1217). I'm hoping this lets CI make more progress; I'd like to confirm the end-to-end test change mentioned above, at least, which I think requires CI.

(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 PR addresses items 1 and 2 from #11565 (comment). (Item 3 was addressed yesterday in 28daa74, which changed the default for the API to `hard_off`.) The changes for those two are: 1. The `shutdown_policy` field is no longer an `Option` for either `Instance` or `InstanceCreate`. They do both have a `#[serde(default)]` on them, so they're not required at the API layer, but this way (a) we don't have to deal with `None` and (b) we don't have the oddity of always returning `Some(_)` to clients even if they had specified `None`. 2. The type conversion for old clients just throws away the `shutdown_policy` field instead of failing if it isn't `HardOff`. This is less important now that the default is `HardOff`, but I still think it's reasonable - this way old clients can still see / list instances that were created by new clients. They just won't be able to tell what those instance's shutdown policies are. (`shutdown_policy` is still `None` in `InstanceUpdate`, which means "don't change", so an instance update from an old client is fine: it doesn't know about `shutdown_policy`, which turns into `None`, which turns into "don't change it".) Most of the bulk off the diff is trivial fallout from 1 - changing a bunch of `None`s to `InstanceShutdownPolicy::default()`s in a lot of tests. The end-to-end test had to switch from specifying all the fields to using the progenitor-client's builder API (which allows you to _not_ specify defaultable fields). A few other cleanup things came along for the ride: 1. I moved the enforcement of the ACPI timeout second range into a newtype in the API. This means it's enforced at deserialization time, so we don't have to remember to validate it every place it comes in in Nexus. 2. The instance update handler had duplicated the logic of "convert the `InstanceShutdownPolicy` into the 2-column database representation". I moved this into an `InstanceUpdate::new()` function in the db-model; I don't love that it takes a bunch of args but I think it's still a net win? 3. I changed the propolis pointer in `package-manifest.toml` to match what's in `Cargo.toml` (all of which now point to the latest commit on oxidecomputer/propolis#1217). I'm hoping this lets CI make more progress; I'd like to confirm the end-to-end test change mentioned above, at least, which I think requires CI.

lifning · 2026-11-07T09:06:22Z

-                VmmStateRequested::Running => {
-                    let vmms = self.vmms.clone();
-                    let log = self.log.new(


the previous behavior of sled-agent-sim here: when there's a mock propolis, and Run is requested: not send anything to propolis, spawn a task that sleeps 10 seconds before calling sim_ensure(), and early-return VmmPutStateResponse { updated_runtime:None }. this was peculiar and seems like just a bug, as none of the small handful of integ tests calling start_local_mock_propolis_server rely on this behavior at all, and it does not apply to the vast majority of tests not starting a mock propolis.

prototyping rfd622: acpi api plumbing further wip: figuring out db schema for acpi power button external api versioning wrappers for instance endpoints still working on the db schema wip - wired enough up of shutdown policy to pass simple cargo check further wip: now with cargo check --tests sled agent api versioning, regen openapi consistent column order between dbinit and up* missed a spot with column order consistency & default to 10 minutes when shutdown policy underspecified in create request & also just point to the propolis PR branch for this fill in some TODOs, mostly doc comments add override param to instance stop and reboot nexus endpoints, and wire up instance update params in diesel start making tests for shutdown policy. also, consider the case of an instance update request that *doesn't* change the shutdown policy more acpi shutdown integ test. i think the most appropriate way to test this will be to add a little escape hatch to mock propolis integ test for shutdown feature wip - acpi shutdown cleanups, trying to get integ test working still working on shutdown integ test, but now figured out the workarounds needed for mock propolis server to participate in the way we need don't clamp acpi timeout in DbInstance elaborate a bit on InstanceCreate and InstanceUpdate request shutdown_policy fields wip - notes on my current fixquest wip - out of one bug, into another.. fix further issues with mock-propolis / sled-agent-sim (former does not like being started with different vmm id), now shutdown integ tests partially passing, but need to convince nexus to be comfortable sending hard-stop requests to an instance that only received a soft-stop so far wip - another test case. still puzzling on the most appropriate way to address the fact the instance update saga yoinks the vmm record for the first soft-off request such that the subsequent hard-off doesn't make it through oh, it was sled-agent-sim again. good. oops sorry clippy move consts to a common denominator of nexus-db-model and nexus - per pr feedback (thanks eliza) helix got a bit confused this evening addressing a few pr comments also fixup: unwritten buffer amid a rebase :( missed the upper bound on the CHECK conditions

…is gracefully shutting down

(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.

Co-authored-by: Eliza Weisman <eliza@elizas.website>

This PR addresses items 1 and 2 from #11565 (comment). (Item 3 was addressed yesterday in 28daa74, which changed the default for the API to `hard_off`.) The changes for those two are: 1. The `shutdown_policy` field is no longer an `Option` for either `Instance` or `InstanceCreate`. They do both have a `#[serde(default)]` on them, so they're not required at the API layer, but this way (a) we don't have to deal with `None` and (b) we don't have the oddity of always returning `Some(_)` to clients even if they had specified `None`. 2. The type conversion for old clients just throws away the `shutdown_policy` field instead of failing if it isn't `HardOff`. This is less important now that the default is `HardOff`, but I still think it's reasonable - this way old clients can still see / list instances that were created by new clients. They just won't be able to tell what those instance's shutdown policies are. (`shutdown_policy` is still `None` in `InstanceUpdate`, which means "don't change", so an instance update from an old client is fine: it doesn't know about `shutdown_policy`, which turns into `None`, which turns into "don't change it".) Most of the bulk off the diff is trivial fallout from 1 - changing a bunch of `None`s to `InstanceShutdownPolicy::default()`s in a lot of tests. The end-to-end test had to switch from specifying all the fields to using the progenitor-client's builder API (which allows you to _not_ specify defaultable fields). A few other cleanup things came along for the ride: 1. I moved the enforcement of the ACPI timeout second range into a newtype in the API. This means it's enforced at deserialization time, so we don't have to remember to validate it every place it comes in in Nexus. 2. The instance update handler had duplicated the logic of "convert the `InstanceShutdownPolicy` into the 2-column database representation". I moved this into an `InstanceUpdate::new()` function in the db-model; I don't love that it takes a bunch of args but I think it's still a net win? 3. I changed the propolis pointer in `package-manifest.toml` to match what's in `Cargo.toml` (all of which now point to the latest commit on oxidecomputer/propolis#1217). I'm hoping this lets CI make more progress; I'd like to confirm the end-to-end test change mentioned above, at least, which I think requires CI.

…having a mock propolis

hawkw self-requested a review October 22, 2026 20:55

karencfv mentioned this pull request Oct 23, 2026

[reconfigurator] background task for stopping marked VMMs #11380

Merged

charliepark mentioned this pull request Oct 26, 2026

Console to handle instances' graceful shutdown policy oxidecomputer/console#3424

Open

davepacheco assigned lifning Oct 27, 2026

hawkw reviewed Oct 29, 2026

View reviewed changes

Comment thread nexus/db-model/src/instance.rs Resolved

Comment thread nexus/src/app/mod.rs Resolved

lifning force-pushed the lif/acpi-shutdown branch from d1d85f0 to 1e06789 Compare October 29, 2026 06:46

hawkw reviewed Oct 29, 2026

View reviewed changes

jgallagher reviewed Oct 30, 2026

View reviewed changes

hawkw reviewed Oct 30, 2026

View reviewed changes

jgallagher mentioned this pull request Nov 4, 2026

wire shutdown policy into vmm_stop_for_update bg task #11703

Merged

jgallagher mentioned this pull request Nov 5, 2026

graceful shutdown: rework API types / compatibility #11709

Merged

lifning force-pushed the lif/acpi-shutdown branch from 06a367e to 8a5fdc9 Compare November 5, 2026 20:56

lifning force-pushed the lif/acpi-shutdown branch from 8a5fdc9 to e385a54 Compare November 6, 2026 03:38

lifning commented Nov 6, 2026

View reviewed changes

Comment thread nexus/tests/integration_tests/instances.rs Resolved

lifning commented Nov 7, 2026

View reviewed changes

lif and others added 14 commits November 8, 2026 01:55

disarm zone-termination timeout if a reboot occurs while an instance …

7006717

…is gracefully shutting down

Apply suggestion from @hawkw

86f8a94

Co-authored-by: Eliza Weisman <eliza@elizas.website>

change default shutdown policy to hard-off

428b237

test that acpi reboot correctly passes through nexus as well

1716152

rebase fixes

f431844

move acpi timeout out of toplevel vmm_put_state and into enum variant

8fcddb9

propolis branch bump

2e9b511

sled-agent-sim may be causing us trouble with the reboot integ tests …

946c650

…having a mock propolis

fmt.. et tu, helix lsp integ? also finally drop that broken doc link

56ea1d3

fix propolis hash in package-manifest.toml

fe74e48

turning toward my sled-agent-sim problems more directly

df5a5b6

regenerate api w/o broken doc link

0ae62aa

lifning force-pushed the lif/acpi-shutdown branch from 432f45c to 0ae62aa Compare November 8, 2026 10:00

	warn!(
	self.log,
	"Instance failed to stop within the grace period, \
	terminating it violently!",
	);

Conversation

lifning commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karencfv commented Oct 23, 2026

Uh oh!

lifning commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karencfv commented Oct 23, 2026

Uh oh!

lifning commented Oct 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

karencfv commented Oct 23, 2026

Uh oh!

lifning commented Oct 23, 2026

Uh oh!

karencfv commented Oct 26, 2026

Uh oh!

Uh oh!

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lifning Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgallagher left a comment

Choose a reason for hiding this comment

Uh oh!

jgallagher Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lifning commented Oct 22, 2026 •

edited

Loading

lifning commented Oct 23, 2026 •

edited

Loading

lifning commented Oct 23, 2026 •

edited

Loading

lifning Oct 30, 2026 •

edited

Loading

jgallagher Oct 30, 2026 •

edited

Loading