[reconfigurator] background task for stopping marked VMMs by karencfv · Pull Request #11380 · oxidecomputer/omicron

karencfv · 2026-09-30T05:28:19Z

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

    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

    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

    VMMs stopped for sled evacuation: 0 total

Note: graceful shutdown preferences will need to be integrated in #11565, or once it lands

Related: #11169

karencfv · 2026-10-08T08:19:21Z

+                            // Propolis is alive but does not find a VMM. This
+                            // implies that it restarted and lost the previously
+                            // created VMM. The VMM is reported as Failed. It's
+                            // not necessary to stop it anymore, so we don't add
+                            // it to the `vmms_failed_by_sled` list.
+                            VmmState::Failed => {


Just leaving a comment here to double check that I'm correct in what I'm saying here

Not exactly. If the sled-agent returns VmmState::Failed, which is what happened here, that affirmatively indicates that either Propolis or the guest OS have crashed. In this case, we do not need to stop it...but we must update the database record by calling notify_instance_updated, as I mentioned above.

The other case, where the sled-agent or Propolis have restarted and "forgotten" about the instance, is represented by the sled-agent returning an error with a particular error code. That case would be handled by code like this:

omicron/nexus/src/app/instance.rs

Lines 1161 to 1168 in e45eb5c

if let (InstanceStateChangeError::SledAgent(inner), Some(vmm)) =

(&e, state.vmm())

&& let Some(reason) = inner.vmm_failure_reason()

{

let _ = self

.mark_vmm_failed(opctx, authz_instance, vmm, inner, reason)

.await;

}

Here, we are adding that to the list of errors.

Hm, what about the case in the put_state method? It doesn't return an error, and the comment implies pretty much the same thing I wrote in my comment. For context, this case is the one I looked at when deciding to not treat this as an error.

omicron/sled-agent/src/instance.rs

Lines 2296 to 2312 in f6cd60d

// Propolis is alive but thinks there's no VM here,

// which implies that it restarted and lost the

// previously-created VM. This is grounds to mark the

// VMM as Failed.

error!(self.log,

"Propolis VM is missing, can't change its state";

"code" => ?code,

);

self.fail_vmm_and_terminate().await;

// Return the newly-installed Failed state here so it

// doesn't get clobbered by the published VMM state

// chosen above. (Note that returning `Ok` here means it

// is possible for a state change operation to "succeed"

// even though the outcome is a failed instance.)

return Ok(self.state.sled_instance_state());

Ah, yeah, this is the case where the Propolis process has restarted, not the case where the sled-agent (or the whole sled) has restarted. That case will return an error with a specific status code, and we need to handle that one specially as well. You're right that sled-agent will return Failed if the Propolis process has crashed, and this behavior is correct, we just need to handle the other case as well.

Awesome thanks! I took a closer look, and it seems to me that in any situation where a sled agent/sled has restarted, then vmm_put_state would return an Error instead of VmmState::Failed. In those cases, we are already adding the error to the list of errrors above, and adding the failed VMM to the list of failed VMMs. So there's not much to change here? I added a comment to explain this.

That said, I want to make sure I got this right, so if I'm fundamentally misunderstanding something here, then happy to change things.

karencfv · 2026-10-08T08:21:23Z

+                        && active_vmm
+                            .stop_for_update_disposition_generation
+                            .is_none()


In a follow up PR we will be using the intended state to reincarnate the instance

karencfv · 2026-10-08T08:30:59Z

+        // TODO-K: For a follow up PR. Do we want to restart instances we are
+        // evacuating regardless of the auto-restart policy?


Thoughts? My initial inclination is to respect whatever policy the user set. They did so for a reason, and we shouldn't override settings like these.

My inclination is the opposite, actually, since I think that when we have talked about the auto-restart policy to users (i.e. in the API docs), we explicitly state that the auto-restart behavior is intended to describe what we should do if the VM fails (i.e. the guest OS panics, or the VMM panics, or the sled disappears for some reason). My thinking on why a user might want to disable automatic restarts is if you don't really trust the VM to not crash itself or the sled it's on, and want us to avoid restarting it automatically if it crashes, because it might just crash again. Whether or not we should restart the VM if we stopped it on purpose in order to update the sled is quite different than whether we should restart it because it crashed, IMO.

If anything, I think perhaps we might want to consider an AutoRestartPolicy::UpdatesOnly or something?

Gotcha. Thinking also about the case of a VMM failing during a sled evacuation. In this case I think it still makes sense to follow whatever the user had previously set.

I can think of other scenarios where a user may want to set the auto-restart policy to Never. Perhaps they are running their software on some sort of orchestrator like Kubernetes and do not wish for random nodes to pop up again after they have been replaced.

If anything, I think perhaps we might want to consider an AutoRestartPolicy::UpdatesOnly or something?

Maybe? I'm not sure this is necessary yet. This may solve a problem no one has yet. It'd be good to wait and hear feedback from customers before implementing another policy.

Gotcha. Thinking also about the case of a VMM failing during a sled evacuation. In this case I think it still makes sense to follow whatever the user had previously set.

Yeah, that's correct, if the instance winds up in the Failed state, the decision of whether or not to restart it should be based on the configured auto-restart policy (and the instance's intended_state). I think I may have misinterpreted your comment as asking whether we should determine if instances in the Stopped state¹ due to sled evacuation would be restarted based on the auto-restart policy. In that case, when a VMM stops gracefully because the sled was being evacuated, we do want to restart it elsewhere even if the auto-restart policy is Never, since the auto-restart policy is intended to configure whether instances are restarted on unexpected failures.

If anything, I think perhaps we might want to consider an AutoRestartPolicy::UpdatesOnly or something?

Maybe? I'm not sure this is necessary yet. This may solve a problem no one has yet. It'd be good to wait and hear feedback from customers before implementing another policy.

Yeah, I agree; what I meant was that if we do end up getting customer feedback that they want instances stopped for sled evacuation to not restart, I think that ought to be a separate knob, rather than making the Never auto-restart policy also control restarting evacuated instances.

Footnotes

i.e. those which were told to stop for sled evacuation and which stopped cleanly ↩

hawkw · 2026-10-08T20:24:33Z

+        // TODO-K: For a follow up PR. Do we want to restart instances we are
+        // evacuating regardless of the auto-restart policy?


My inclination is the opposite, actually, since I think that when we have talked about the auto-restart policy to users (i.e. in the API docs), we explicitly state that the auto-restart behavior is intended to describe what we should do if the VM fails (i.e. the guest OS panics, or the VMM panics, or the sled disappears for some reason). My thinking on why a user might want to disable automatic restarts is if you don't really trust the VM to not crash itself or the sled it's on, and want us to avoid restarting it automatically if it crashes, because it might just crash again. Whether or not we should restart the VM if we stopped it on purpose in order to update the sled is quite different than whether we should restart it because it crashed, IMO.

If anything, I think perhaps we might want to consider an AutoRestartPolicy::UpdatesOnly or something?

hawkw · 2026-10-08T20:59:56Z

+                            // Propolis is alive but does not find a VMM. This
+                            // implies that it restarted and lost the previously
+                            // created VMM. The VMM is reported as Failed. It's
+                            // not necessary to stop it anymore, so we don't add
+                            // it to the `vmms_failed_by_sled` list.
+                            VmmState::Failed => {


Not exactly. If the sled-agent returns VmmState::Failed, which is what happened here, that affirmatively indicates that either Propolis or the guest OS have crashed. In this case, we do not need to stop it...but we must update the database record by calling notify_instance_updated, as I mentioned above.

The other case, where the sled-agent or Propolis have restarted and "forgotten" about the instance, is represented by the sled-agent returning an error with a particular error code. That case would be handled by code like this:

omicron/nexus/src/app/instance.rs

Lines 1161 to 1168 in e45eb5c

if let (InstanceStateChangeError::SledAgent(inner), Some(vmm)) =

(&e, state.vmm())

&& let Some(reason) = inner.vmm_failure_reason()

{

let _ = self

.mark_vmm_failed(opctx, authz_instance, vmm, inner, reason)

.await;

}

Here, we are adding that to the list of errors.

hawkw · 2026-10-19T21:36:00Z

            };

-        for VmmsBySled { sled_id, vmm_ids } in &vmms_by_sled {
+        for vmms_per_sled in vmms.chunk_by(|a, b| a.sled_id() == b.sled_id()) {


(i may have to go and refactor instance_watcher to do something more like this, btw!)

hawkw · 2026-10-19T22:23:35Z

+                                result.errors.push(err);
+                            }
+                            Ok(Some((_, saga))) => {
+                                match self.sagas.saga_run(saga).await {


I don't think we should do that. While John is correct that it is not required for correctness, that was never the reason I said we should be calling process_vmm_update here, it's reduce the latency with which the the instance transitions to the next state. We have already received a new VMM state from the sled-agent, and the rest of the system is designed around the expectation that when updated states are received, they are immediately persisted to the database and any saga required to advance the state is started. While other mechanisms to ensure this happens eventually exist, they are primarily intended to guard against the instance getting stuck forever in the event of a Nexus crash or network partition. The expectation is that when there isn't a failure like that, whoever receives the new state goes ahead and ensures it's processed.

If we really don't like spawning the saga here, we should at least be calling Datastore::vmm_and_migration_update_runtime to write the new state to the DB as process_vmm_update does, and then activating the instance_updater background task to run any saga required by that new state, rather than triggering instance_watcher to go and send an entirely separate HTTP request to every single VMM in the rack. Going through that route will avoid having to wait until a bunch of other VMMs are health checked first, at least.

karencfv

@jgallagher @hawkw thanks a million for taking the time to go over this thoroughly. I think I've addressed all concerns now, and this PR is ready for another look :)

jgallagher · 2026-10-20T18:21:34Z

+
+        // Build the reqwest client beforehand to avoid building a new one per
+        // page or sled below.
+        let reqwest_client =


Sorry to belabor this one, but I think Eliza's observation in https://github.com/oxidecomputer/omicron/pull/11380/changes#r4288351566 is that we only need to build this once ever; here, we still build it every time the task activates, right?

That said, I don't think it's trivial to do this in VmmStopForUpdate::new() since that doesn't return a result. Maybe we do it here but cache it it self the first time we succeed?

Now that we have fixed the bug where we were trying to load a bunch of TLS root certs in build(), I think any Err returned by default_reqwest_client_builder().build() would indicate invalid parameters supplied to the builder --- i.e. a programmer error in default_reqwest_client_builder(), since we are not supplying any non-default settings here. So, IMO, it's probably okay to unwrap() this in new()...

unwrap()-ing always makes me a little nervous tbh, I think I'll go with John's suggestion with this one.

jgallagher · 2026-10-20T18:32:46Z

+                    // First we check if the VMM is there, if it isn't, we
+                    // want to report it as such so we don't have to wait on the
+                    // instance watcher to find and restart this instance.
+                    if err.vmm_gone() {


Hmm, I'm now less sure it's okay to try to stop VMMs in the Creating state. If we get vmm_gone() here but the instance was in Creating, we don't actually know that the VMM is already gone and should be marked as failed - it might not exist yet, right?

As written, IIUC, in this situation we'll end up calling process_vmm_state() with a Failed state, but could that be racing with the instance start saga trying to create it?

To start off, I just want to clarify that vmm_gone() only means that there is no VMM (i.e. a 404) not that a VMM existed and is no longer there.

I investigated any possible races (with AI help), and this is what I found:

Firstly, it's the start saga that creates the VMM row with a Creating state, so at any given moment that the VMM is in a creating state, we can assume that a saga is running.

First scenario:

Let's suppose vmm_gone() returns true right before ENSURE_REGISTERED updates the runtime state to Starting but the VMM already exists. In this case, the task's attempt to write Failed will fail because the start saga has already written Starting at generation G+1 before this task attempts to write at that same generation. The update query only accepts a higher generation and will reject this task's write. This run fails to stop the VMM.

Now, the next task run runs, The VMM is still marked to stop, and is now in Starting or Started, this run should stop the VMM. No harm done.

Second scenario:

This time vmm_gone() returns true right before MARK_AS_STARTING (actually write the Starting state). In this case, this task will win the race to write Failed. The task's update saga is then a noop because there is no active VMM yet. Now MARK_AS_STARTING runs, and it links this VMM to the instance. The instance is now linked to a Failed VMM. The instances state is Stopping at this point.

Now ENSURE_REGISTERED runs. Registration will return a 503 due to SLED_EVACUATING. MARK_AS_STARTING will now attempt to "undo" and write NoVmm. This may will fail if it attempts to write at the same generation that this task used to write Failed. If it manages to write at a higher generation then the write will succeed. This means the instance ends up in either NoVmm or Failed and the VMM row will end up as Failed if the task won the race, SagaUnwound if the start saga won the race or deleted if an update saga started by the instance-updater task wins the race.

In any of these 3 cases the next run of this task will not list these VMMs so it will not attempt to do anything with them.

If the MARK_AS_STARTING undo wins the race, the instance ends up with NoVmm and no propolis id. The VMM ends up as SagaUnwound, but it is not linked to the instance. In this scenario, the instance will end up and remain Stopped because it does not meet the requirements for reincarnation (to reincarnate it needs either a Failed instance or an active SagaUnwound VMM)

If the instance-updater task's update saga wins the race, the instance is Failed with no propolis id and the VMM row is deleted. The instance is reincarnated (🎉 ).

Proposal:

There is only a very minor chance that an instance will not return to its intended state, but the chance does exist.

My proposal here is to change this task so that it doesn't include VMMs in Creating when attempting to stop, but the task that marks VMMs to be stopped still marks VMMs in Creating as needing to be stopped. If everything goes right, we stop the VMM cleanly in another task run, if the instance never manages to create itself due to unrelated reasons, there are no changes in how the system acts today in relation to those VMMs. This way, we go the safe route, and try to keep the latency as low as we can (this task runs every 60s and the marking task runs every 300s).

There is just one caveat, it may be annoying for a customer to have their newly created instance come up completely only to be almost immediately restarted by the system. But, I guess there is a chance of that happening already so 🤷‍♀️

Thoughts?

Now ENSURE_REGISTERED runs. Registration will return a 503 due to SLED_EVACUATING.

I'm not sure this is guaranteed - the "mark for stopping" and "tell sled-agent it's evacuating" processes both act on the blueprint, but AFAIK they're not synchronized, right? We could have a VMM marked for stopping before its sled-agent knows it's evacuating?

My proposal here is to change this task so that it doesn't include VMMs in Creating when attempting to stop, but the task that marks VMMs to be stopped still marks VMMs in Creating as needing to be stopped.

This sounds reasonable to me. I'm mildly worried about instances getting stuck in creating (e.g., #11351 from a few weeks ago) keeping a sled from being evacuated, but given this whole conversation thread I'm not sure we can safely stop them in that state. 😬

There is just one caveat, it may be annoying for a customer to have their newly created instance come up completely only to be almost immediately restarted by the system. But, I guess there is a chance of that happening already so 🤷‍♀️

Yeah I think this is unavoidable given the current setup. (We could have something like a "pre-evacuating" state where we stop placing new instances on a sled for some period of time before we start evacuating? But that'd be separate from this work and feels like a much later "optimize if we need it" kind of project; it'd also make updates take even longer so isn't an obvious win either.)

I'm not sure this is guaranteed - the "mark for stopping" and "tell sled-agent it's evacuating" processes both act on the blueprint, but AFAIK they're not synchronized, right? We could have a VMM marked for stopping before its sled-agent knows it's evacuating?

I guess this increases the chance of a race and an instance ending up in a weird place then 🙃

This sounds reasonable to me. I'm mildly worried about instances getting stuck in creating (e.g., #11351 from a few weeks ago) keeping a sled from being evacuated, but given this whole conversation thread I'm not sure we can safely stop them in that state. 😬

Yeah, would be nice to include them here. The chance of an instance staying Stopped is very low, but if it happens it could potentially be extremely problematic for our customers.

jgallagher · 2026-10-20T18:55:22Z

+                    if err.vmm_gone() {
+                        slog::error!(
+                            opctx.log,
+                            "Failed to mark VMM as Stopped; VMM was already \


If we log this error here, we'll also end up info!-logging "Unable to find VMM. No need to stop it" down below. Should this be a warn! at most? (Same question for all the other error!s, actually - none of these seem all that problematic from Nexus's point of view, and some of them are more or less expected in some racy situations.)

Fair, I'll keep the ones where we failed to build the clients (reqwest and sled agent) though

karencfv

Thanks for the discussions @jgallagher @hawkw ! I've made the changes and I think this should be ready to go now! 🤞

karencfv · 2026-10-22T01:57:25Z

+                    // First we check if the VMM is there, if it isn't, we
+                    // want to report it as such so we don't have to wait on the
+                    // instance watcher to find and restart this instance.
+                    if err.vmm_gone() {


I'm not sure this is guaranteed - the "mark for stopping" and "tell sled-agent it's evacuating" processes both act on the blueprint, but AFAIK they're not synchronized, right? We could have a VMM marked for stopping before its sled-agent knows it's evacuating?

I guess this increases the chance of a race and an instance ending up in a weird place then 🙃

This sounds reasonable to me. I'm mildly worried about instances getting stuck in creating (e.g., #11351 from a few weeks ago) keeping a sled from being evacuated, but given this whole conversation thread I'm not sure we can safely stop them in that state. 😬

Yeah, would be nice to include them here. The chance of an instance staying Stopped is very low, but if it happens it could potentially be extremely problematic for our customers.

hawkw · 2026-10-23T19:56:58Z

+        let reqwest_client = match self.reqwest_client.clone() {
+            Some(client) => client,
+            None => {
+                match nexus_networking::default_reqwest_client_builder().build()
+                {
+                    Ok(client) => {
+                        self.reqwest_client = Some(client.clone());
+                        client
+                    }
+                    Err(e) => {
+                        let err = InlineErrorChain::new(&e);



default_reqwest_client_builder().build() should really only ever return an Err if default_reqwest_client_builder() were to assemble a set of builder settings that were always invalid, since (after #11482), building a reqwest client configured by the default builder should never do any I/O or other fallible operations. Therefore, I think we would be safe constructing the client in VmmStopForUpdate::new and expect()ing a call to default_reqwest_client_builder().build(). If that ever fails, it will always fail, implying that we have a programmer error in nexus_networking that would leave the entire Nexus process unable to construct clients ever, and panicking seems quite reasonable in those circumstances. Doing that lets us avoid all this complexity around conditionally re-initializing the client...

That seems a little spicy - we'd be unwrapping something here that depends on the implementation details of default_reqwest_client_builder(), right? I think if we want to go that road we should put something infallible in nexus_networking and put the unwrap there where it's near the bits that make it okay. (I'd also be fine with keeping this request caching stuff but moving it to a helper method, personally.)

I think if we want to go that road we should put something infallible in nexus_networking and put the unwrap there where it's near the bits that make it okay.

That's reasonable, I could imagine adding an infallible default_reqwest_client() that builds the default builder and expects it, because that way, the caller cannot change the settings in such a way that make the build() call fail.

However, I do still think that, because we are now always disabling the TLS cert store, any error building a reqwest client implies that we have built a configuration that will never succeed, and that probably should crash the process violently so that it fails during testing rather than having a client that never succeeds at runtime but which we can miss in tests if that particular background task's behavior is not being exercised. That's just my take though.

That seems a little spicy - we'd be unwrapping something here that depends on the implementation details of default_reqwest_client_builder(), right?

This is my main concern as well

However, I do still think that, because we are now always disabling the TLS cert store, any error building a reqwest client implies that we have built a configuration that will never succeed, and that probably should crash the process violently so that it fails during testing rather than having a client that never succeeds at runtime

This is a fair point, I've added assertions during testing in 304c2f8 for this purpose. I tested manually by forcing an invalid config in the ClientBuilder by changing

match nexus_networking::default_reqwest_client_builder().build()

To have an invalid user agent with:

match nexus_networking::default_reqwest_client_builder().user_agent("\n").build()

And, the test failed nicely

... thread 'app::background::tasks::vmm_stop_for_update::tests::test_vmm_stop_for_update_activation' (2) panicked at nexus/src/app/background/tasks/vmm_stop_for_update.rs:1027:9: assertion `left == right` failed: unexpected errors: ["Internal Error: failed to build the HTTP client: builder error: failed to parse header value"] left: 1 right: 0 ...

jgallagher

Thanks for all the back and forth on this one. Changes LGTM, just a couple comment nits.

hawkw · 2026-10-23T20:36:14Z

+    ///
+    /// The query performed by this function is paginated by the sled and
+    /// VMM UUIDs, in that order.
+    pub async fn vmm_list_stoppable_for_update_by_sled_id(


take it or leave it, but since we've renamed the state constants to SHOULD_STOP_FOR_EVACUATION/SHOULD_MARK_TO_STOP_FOR_EVACUATION, perhaps this ought to be

Suggested change

pub async fn vmm_list_stoppable_for_update_by_sled_id(

pub async fn vmm_list_stoppable_for_evacuation_by_sled_id(

(i'm also personally a bit unconvinced we need to include _by_sled_id here, since there isn't a query that lists stoppable VMMs ordered by anything else, but that's up to you...)

I'd like to leave this one as is because we are filtering based on whether a VMM is marked to stop for update specifically. If we ever change the marker to mean stop for evacuation instead of update, then I'd change this one as well

hawkw · 2026-10-23T20:39:51Z

+        // Trigger anything waiting on this task to run.
+        self.tx.send_modify(|count| *count = *count + 1);


hmm, do we need to do this if we haven't actually marked any VMMs? perhaps this should go in the if vmms_marked > 0 branch above?

I'm following the pattern we laid out in the other task. Specifically in this comment #11170 (comment)

I think this task should activate any time the blueprint_rendezvous task marks a sled as unavailable. Maybe it would be fine (or even better in terms of futureproofing?) to activate this any time the blueprint_rendezvous task runs at all?

hawkw · 2026-10-23T20:41:16Z

            };

-        for VmmsBySled { sled_id, vmm_ids } in &vmms_by_sled {
+        for vmms_per_sled in vmms.chunk_by(|a, b| a.sled_id() == b.sled_id()) {


(i may have to go and refactor instance_watcher to do something more like this, btw!)

…by reconfigurator (#11491) Adds a new internal `SledEvacuating` instance state to indicate that an instance has been stopped due to a sled evacuation. It differs to `NoVmm` in the sense that we know the instance will be automatically started again soon. In addition, we don't allow any changes (e.g. add/detach a disk, deletion, etc) during this state, and we map it to the external instance state `Rebooting`. Also, the instance update saga is modified to account for VMMs in this new state. _NB:_ This work will be tested through #11380. Once that is merged, the `vmm_stop_for_update` background task's tests will fail since they currently check for a `NoVmm` state. This PR will include the changes in those tests. I just want to get some eyes in this PR since it's basically ready to go otherwise. Depends on: #11380 Related: #11169

[reconfigurator] background task for stopping marked VMMs

310551c

karencfv mentioned this pull request Sep 30, 2026

Tracking issue for RFD 739 (Instance restart during live update) #11169

Open

4 of 7 tasks

hawkw reviewed Sep 30, 2026

View reviewed changes

Comment thread nexus/src/app/background/tasks/vmm_stop_for_update.rs Resolved

karencfv added 18 commits October 2, 2026 14:53

let's do batches

e1f14c3

fix some comments

99e577d

use IdOrdMap everywhere

cb3ea34

have nicer return types

b1a9a19

pondering...

055b6b0

merge main

81b1f62

Do the stopping and add a bunch of random thoughts

a8f3e4e

merge main

787853b

integrate with vmm marking bg task

f4fea86

handle all possible scenarios attempting to stop a VMM

a91d7d1

Do not change the intended state if we're stopping for an update

56d116c

better logging

267ed82

expectorate

5f3b05e

add test for task

bf73666

organise test

3a62920

clean up

8c2fdae

add db-query test

6196cb9

clean up

9ef8b8e

karencfv commented Oct 8, 2026

View reviewed changes

karencfv requested review from davepacheco, hawkw, iximeow and jgallagher October 8, 2026 08:31

karencfv marked this pull request as ready for review October 8, 2026 08:31

hawkw reviewed Oct 8, 2026

View reviewed changes

formatting

97e2afc

log the instance id

179a441

hawkw reviewed Oct 19, 2026

View reviewed changes

karencfv added 5 commits October 20, 2026 13:10

handle vmm_gone

29db8ef

optimise the optimisation

f56309c

clean up

a603bfa

improve documentation

80eafe6

make field more explicit

a30797d

karencfv commented Oct 20, 2026

View reviewed changes

jgallagher reviewed Oct 20, 2026

View reviewed changes

This was referenced Oct 20, 2026

[sled-agent] requesting that a VMM stop before it has transitioned to Running will panic #11536

Closed

[sled-agent] don't panic when stopping a VMM that hasn't started #11537

Open

karencfv added 7 commits October 21, 2026 12:57

cache the reqwest client

2533d11

fix logs

73529c9

fix typos

1e77105

do not stop Creating VMMs

7355d30

only retrieve vmms in in-service sleds

a0b22ca

adapt tests

6e18f90

fix typos before John finds them

63d3678

karencfv commented Oct 22, 2026

View reviewed changes

karencfv mentioned this pull request Oct 23, 2026

Support for instance 'graceful' shutdown policy #11565

Draft

hawkw reviewed Oct 23, 2026

View reviewed changes

jgallagher approved these changes Oct 23, 2026

View reviewed changes

Comment thread nexus/src/app/background/tasks/vmm_stop_for_update.rs Resolved

Comment thread nexus/src/app/background/tasks/vmm_stop_for_update.rs Resolved

Comment thread nexus/db-queries/src/db/datastore/vmm.rs Resolved

hawkw approved these changes Oct 23, 2026

View reviewed changes

karencfv added 2 commits October 27, 2026 17:20

address comments

54357d1

improve testing on failures

304c2f8

karencfv enabled auto-merge (squash) October 27, 2026 05:00

karencfv merged commit af0a32e into oxidecomputer:main Oct 27, 2026
19 checks passed

karencfv deleted the vmm-stopper-bg-task branch October 28, 2026 01:57

hawkw mentioned this pull request Nov 4, 2026

[nexus] allow instance stop to cancel a Creating vmm's start saga #11696

Open

	if let (InstanceStateChangeError::SledAgent(inner), Some(vmm)) =
	(&e, state.vmm())
	&& let Some(reason) = inner.vmm_failure_reason()
	{
	let _ = self
	.mark_vmm_failed(opctx, authz_instance, vmm, inner, reason)
	.await;
	}

	// Propolis is alive but thinks there's no VM here,
	// which implies that it restarted and lost the
	// previously-created VM. This is grounds to mark the
	// VMM as Failed.
	error!(self.log,
	"Propolis VM is missing, can't change its state";
	"code" => ?code,
	);

	self.fail_vmm_and_terminate().await;

	// Return the newly-installed Failed state here so it
	// doesn't get clobbered by the published VMM state
	// chosen above. (Note that returning `Ok` here means it
	// is possible for a state change operation to "succeed"
	// even though the outcome is a failed instance.)
	return Ok(self.state.sled_instance_state());

		// TODO-K: For a follow up PR. Do we want to restart instances we are
		// evacuating regardless of the auto-restart policy?

	pub async fn vmm_list_stoppable_for_update_by_sled_id(
	pub async fn vmm_list_stoppable_for_evacuation_by_sled_id(

		// Trigger anything waiting on this task to run.
		self.tx.send_modify(\|count\| count = count + 1);

Conversation

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

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!

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!

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!

Choose a reason for hiding this comment

Footnotes

Uh oh!

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!

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!

karencfv left a comment

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!

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

First scenario:

Second scenario:

Proposal:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

karencfv left a comment

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!

Choose a reason for hiding this comment

Uh oh!

karencfv commented Sep 30, 2026 •

edited

Loading