[reconfigurator] background task for marking VMMs to be stopped for update by karencfv · Pull Request #11170 · oxidecomputer/omicron

karencfv · 2026-08-26T08:36:55Z

Related: #11169

…r update

karencfv · 2026-08-27T07:53:25Z

@sunshowers I have a question for you. This is the first time I write a rendezvous subtask so I might just be missing some context.

In #11127 I added a stopped_for_update_disposition_generation column to the vmm table so that we can mark each VMM directly.

After reading https://rfd.shared.oxide.computer/rfd/0541#_proposal_reconciliation_rpw and taking a look at #11115 , I understand that I shouldn't have added the column to that table at all and instead I should have created a separate rendezvous table.

You suggested using a rendezvous subtask, but the thing is, I don't think we can follow that approach here. We do want the vmm table to have the additional row. Otherwise we would have to change a lot of the code. As part of #11169 we'll be updating the instance update saga, and adapting the reincarnation background task, both of which use the vmm table as the source of truth.

In addition, https://rfd.shared.oxide.computer/rfd/0541#_creating_rows_in_rendezvous_tables the resource exisiting in inventory. I don't think VMMs should be collected as part of an inventory collection, there are just too many.

I think I'd rather go with a background task, thoughts?

sunshowers · 2026-08-27T16:54:43Z

I think it's fine for rendezvous subtasks to not only write to a rendezvous table, though I'll let @smklein and @davepacheco chime in.

karencfv · 2026-08-28T06:04:41Z

Update: We had a chat about this in the update watercooler and decided to go with a normal background task. The task should get which sleds to mark from the rendezvous table though.

karencfv · 2026-08-28T06:06:59Z

+            .filter(dsl::state.eq_any([
+                DbVmmState::Creating,
+                DbVmmState::Starting,
+                DbVmmState::Running,
+                DbVmmState::Rebooting,
+            ]))


@hawkw I'd like to get your input on whether these are the correct states the VMM should be in, in order to mark it to be stopped

Definitely want to defer to Eliza about the specific states, but regardless of the answer today, we probably want to put this in a match somewhere so we have to consider new states added in the future? Maybe something like a DbVmmState::stoppable_states() or something that has an explicit match over all variants?

This set of states looks right to me; I left another comment on the way the stoppable_states is currently implemented

jgallagher · 2026-08-28T18:27:47Z

+    /// This is an emergency lever for support / operations. It should only be
+    /// necessary if something has gone extremely wrong.


I don't think the nexus config is really something support / operations can control - it's not persistent if the sled (or Nexus zone) restarts, and requires manually bouncing the service within the zone for it to take effect.

If we need an emergency stop for support, I think we need a config in crdb that can be toggled via omdb, like the controls we have on the blueprint planner? If having an easy way to enable/disable this task between releases is all we need, then putting it here is great.

Yeah, I would put this in the DB config in this case, given the comment that it's an emergency disable switch.

Yup! I was planning to implement the omdb commands in a follow up PR. It's the penultimate item in #11169.

The comment on there is basically the same one from other configs, was just following the pattern there.

Yup! I was planning to implement the omdb commands in a follow up PR. It's the penultimate item in #11169.

Hmm if we're going to do omdb commands to control this, then we don't need a flag here right?

The comment on there is basically the same one from other configs, was just following the pattern there.

Which configs are those? AFAIK a flag at this level should really only be for "this is still in development so disabled by default" kinds of things.

jgallagher · 2026-08-28T18:29:09Z

+            .filter(dsl::state.eq_any([
+                DbVmmState::Creating,
+                DbVmmState::Starting,
+                DbVmmState::Running,
+                DbVmmState::Rebooting,
+            ]))


Definitely want to defer to Eliza about the specific states, but regardless of the answer today, we probably want to put this in a match somewhere so we have to consider new states added in the future? Maybe something like a DbVmmState::stoppable_states() or something that has an explicit match over all variants?

jgallagher · 2026-08-28T18:30:46Z

+
+        let updated = diesel::update(dsl::vmm)
+            .filter(dsl::time_deleted.is_null())
+            .filter(dsl::stop_for_update_disposition_generation.is_null())


Just confirming I understand: for a given VMM row, this column can only ever go from NULL to a single non-NULL value, which puts it in a terminal state of "needs to be stopped", right? It can never go back to NULL nor do we ever need to change the specific generation value once it has one?

Yes. I think prematurely adding the ability to "revert" could potentially add unnecessary complexity, and would make the code more brittle. This background task would have to have some way of knowing with absolute certainty when a this action is no longer reversible and then block it from happening somehow. I don't see any simple way of implementing that safely.

In addition, I wouldn't want to couple this background task with the task that stops instances unless absolutely necessary.

nor do we ever need to change the specific generation value once it has one?

No, the generation number is only for debugging purposes really. Just to know the generation of the update disposition that called for this VMM to be restarted

jgallagher · 2026-08-28T18:34:21Z

+                }
+            };
+
+        if vmms_marked > 0 {


If we marked any VMMs, are there any other bg tasks we should activate as a result? (Or will there be in the future?)

In #11169 the following task is to create another background task that gathers VMMs that have been marked as needed to be stopped. I had envisioned this other task to be entirely decoupled from this task. All the other task does is look for VMMs that are marked but still in a stoppable state, and stops them. It is my understanding that stopping an instance is idempotent, so there is no harm if a VMM were to be "stopped" twice.

What do you think about this approach?

That said, if we do end up kicking off another task here, it'll be in a follow up PR.

That all sounds fine, and I don't think it's inconsistent with this task activating the stopper task once it exists. Activation can't communicate anything, so it's just an optimization for latency. I think that's probably worth doing here, since the planner will end up waiting for the VMMs marked stopped to actually be stopped? But yeah it's not required for correctness.

jgallagher · 2026-08-28T19:34:10Z

+                        .select(rz_dsl::update_disposition_generation)
+                        .single_value(),
+                ),
+            )


I diesel::debug_query()'d this to see what SQL it's running, and that gave me this:

UPDATE "vmm" SET "stop_for_update_disposition_generation" = ( SELECT "rendezvous_sled_bp_availability"."update_disposition_generation" FROM "rendezvous_sled_bp_availability" WHERE "rendezvous_sled_bp_availability"."sled_id" = "vmm"."sled_id" AND "rendezvous_sled_bp_availability"."bp_availability" = 'unavailable' LIMIT 1 ) WHERE "vmm"."time_deleted" IS NULL AND "vmm"."stop_for_update_disposition_generation" IS NULL AND "vmm"."state" = ANY('creating', 'starting', 'running', 'rebooting') AND "vmm"."sled_id" = ANY( SELECT "rendezvous_sled_bp_availability"."sled_id" FROM "rendezvous_sled_bp_availability" WHERE "rendezvous_sled_bp_availability"."bp_availability" = 'unavailable' )

That looks correct, I think, but is pretty complicated and contains two subqueries. I think this is equivalent with no subqueries:

UPDATE vmm SET stop_for_update_disposition_generation = r.update_disposition_generation FROM rendezvous_sled_bp_availability AS r WHERE r.sled_id = vmm.sled_id AND r.bp_availability = 'unavailable' AND vmm.time_deleted IS NULL AND vmm.stop_for_update_disposition_generation IS NULL AND vmm.state IN ('creating', 'starting', 'running', 'rebooting')

I tried hitting both of these with EXPLAIN, and the output was pretty similar, so maybe cockroach is doing a good job of turning the former into the latter? I don't think diesel supports UPDATE ... FROM ..., so to get the latter we'd have to use one of the raw query builder gadgets. I'm not sure that's worth it.

One other note from the EXPLAIN: both of these queries induce a FULL SCAN over the vmm@lookup_vmms_by_sled_id partial index. I think it's only because this is a partial index that the query is allowed, but that partial index is still going to be "all non-deleted VMMs on all sleds". Do we need to figure out how to paginate this? EDIT: This is "all non-deleted VMMs on all evacuating sleds", which is a much smaller number. This is probably fine!

thanks for taking a look at this in depth. I initially almost opened this PR using a raw query, but then backtracked on that decision.

I was also going to leave a comment about limiting the size of the query, but then I noticed that @jgallagher beat me to it. I agree that the potential size of this query should generally not be huge because (IIUC) we are gonna be evacuating one sled at a time and it will only have so many VMMs on it, but...I still feel a bit sketched out by queries that do potentially unbounded amounts of work. Also, this might be putting the cart before the horse, but I wondered if "well, we won't be evacuating more than one sled at a time" would still be true in a multirack world?

I feel like there isn't any huge reason not to slap a .limit(SQL_BATCH_SIZE) on this just to be safe? I don't think it actually needs to be paginated per se, because the WHERE vmm.stop_for_update_generation IS NULL clause will already exclude VMMs marked by a previous execution of the query, right? So we can just add a maximum number to mark per execution and either have the BG task run it until it has marked zero new VMMs, or rely on subsequent activations of the bg task to do it again?

jgallagher · 2026-08-28T19:38:32Z

+                        Vmm {
+                            id: Uuid::new_v4(),
+                            time_created: Utc::now(),
+                            time_deleted: None,


Is it worth confirming we don't touch VMMs with a non-NULL time_deleted?

Yeah, good idea. I'll do that

jgallagher · 2026-08-28T19:40:07Z

+        // Sleds A and B are both evacuating (`unavailable`), at different
+        // generations, and sled C is available. In a single pass the stoppable
+        // VMMs on both sled A and sled B should be marked, each at their own
+        // sled's generation, regardless of which generation that is.


Assuming I understood correctly in https://github.com/oxidecomputer/omicron/pull/11170/changes#r3883069906, should we confirm that if one of these sleds becomes available again, its VMMs remain marked, and if it then becomes unavailable with a higher generation, the VMMs we marked the first time keep their original generation?

hawkw · 2026-08-28T21:12:59Z

+
+            const MARKED: &str = "VMMs marked:";
+            const ERROR: &str = "error:";
+            const WIDTH: usize = const_max_len(&[MARKED, ERROR]) + 1;


every time I see someone use my silly little const_max_len helper, I feel no small amount of glee...

hawkw · 2026-08-28T21:13:54Z

+                println!("    task explicitly disabled by config!");
+            }
+
+            const MARKED: &str = "VMMs marked:";


unless the whole status object is being printed under a heading that says what it's doing, i might expand this so that it's clear to the reader what we are marking them for...

hawkw · 2026-08-28T21:14:42Z

+    /// This is an emergency lever for support / operations. It should only be
+    /// necessary if something has gone extremely wrong.


Yeah, I would put this in the DB config in this case, given the comment that it's an emergency disable switch.

hawkw · 2026-08-31T18:05:52Z

+    /// Returns the states from which a VMM can still be stopped during sled
+    /// evacuation.
+    pub fn stoppable_states() -> Vec<Self> {
+        Self::ALL_STATES
+            .iter()
+            .copied()
+            .filter(|state| match state {
+                // A VMM in one of these states is on its way, or is already
+                // running, and can be stopped.
+                VmmState::Creating
+                | VmmState::Starting
+                | VmmState::Running
+                | VmmState::Rebooting => true,
+                // A VMM in one of these states is already stopping/stopped,
+                // migrating, or terminal, so there is nothing to stop.
+                VmmState::Stopping
+                | VmmState::Stopped
+                | VmmState::Migrating
+                | VmmState::Failed
+                | VmmState::Destroyed
+                | VmmState::SagaUnwound => false,
+            })
+            .collect()


I like that we're explicitly declaring the list of states in which it's okay to stop a VMM for update. However, I do have some notes:

I don't know how I feel about the name "stoppable states". To me, that sounds like it would also be the list of states in which it's okay to stop a VMM if we're handling a stop request to the public API...but that's expressed here, in terms of instance states rather than VMM states, and includes instance states that correspond to VMM states that are in the list where we don't stop the VMM for update, such as Migrating. I think the name should make it clear that this is specifically for stopping a VMM in order to evacuate a sled, rather than just the generic "stoppable"

I'm not sure if I get why this is implemented by iterating over the complete list of states and filtering that list to include only a specific set of states and then collecting it into a Vec, every time we query for instances in such states. That seems like a lot of extra work to express a list of states which never changes at runtime. If you look slightly earlier in this file, you'll see that there are other lists of states which we just express as const arrays, like this: https://github.com/karencfv/omicron/blob/4e3f9b64afcdc77bfd0f002815194b1fec8a7bb4/nexus/db-model/src/vmm_state.rs#L79-L92
These can be used directly in Diesel queries, and we don't need to do the complicated iterate/filter/collect thing.

So, in sum, I would probably change this to something more like:

Suggested change

/// Returns the states from which a VMM can still be stopped during sled

/// evacuation.

pub fn stoppable_states() -> Vec<Self> {

Self::ALL_STATES

.iter()

.copied()

.filter(|state| match state {

// A VMM in one of these states is on its way, or is already

// running, and can be stopped.

VmmState::Creating

| VmmState::Starting

| VmmState::Running

| VmmState::Rebooting => true,

// A VMM in one of these states is already stopping/stopped,

// migrating, or terminal, so there is nothing to stop.

VmmState::Stopping

| VmmState::Stopped

| VmmState::Migrating

| VmmState::Failed

| VmmState::Destroyed

| VmmState::SagaUnwound => false,

})

.collect()

/// The states in which a VMM should be stopped during sled

/// evacuation.

pub const SHOULD_STOP_FOR_EVACUATION: &[Self] = [

// A VMM in one of these states is on its way, or is already

// running, and can be stopped.

VmmState::Creating,

VmmState::Starting,

VmmState::Running,

VmmState::Rebooting,

// If it is not in one of these states, it is already

// stopping/stopped, migrating, or has terminated, so it

// does not need to be stopped.

];

Ah, hmm. After re-reading @jgallagher's comment in #11170 (comment), I see that he explicitly mentions using a match for this because it will fail to compile if new states are added, forcing the person who adds them to consider whether they should be added to this list. I do see the value in that, but I feel like there's a tradeoff between explicitly catching newly added states and how convoluted this feels to me. I do like John's suggestion and I think we should keep it like this, but maybe it's worth making the following changes:

Sticking the whole thing in a LazyLock or something so that we don't re-evaluate it every time the query is run. I realize that the code path that this runs in is not hot enough for the performance to actually matter, but...it would make me feel less bad, and maybe it makes the intent a little clearer that this is spiritually a constant even if it isn't one in real life?

Adding a comment noting that the reason it's implemented like that instead of a const array is so that adding a new state breaks the match and forces you to reconsider it, so that nobody comes along later and thinks "hey, this looks unnecessarily convoluted, I should refactor it...".

Maybe another option - should we move the match I want to a test? I.e., keep the "here's a static list of states" implementation, then write a test of the form

// ... long comment explaining what to do if you get here due to adding a new state ... for state in VmmState::iter() { match state { VmmState::Creating | /* .. explicit list ... */ => { assert!(VmmState::THE_STATES.contains(state)); } VmmState::Failed | /* ... explicit list ... */ => { assert!(!VmmState::THE_STATES.contains(state)); } } }

Hm, I'm fine with that approach too, as long as we structure the test in such a way that the failure occurs at compile-time --- which, if you're writing out the explicit list of states in a match, it will.

(this conversation is making me want to go back and revisit some of the other places where I've written code with a list of enum variants in a const, like the other lists of VMM states, for instance...)

hawkw · 2026-08-31T18:07:51Z

+    /// VMMs that are already stopping/stopped, migrating or in a terminal state
+    /// do not need to be stopped, so they are left untouched. VMMs that are
+    /// already marked to be stopped by update are also excluded.


I might have this comment explicitly reference the list of states which should stop as expressed in in VmmState

hawkw · 2026-08-31T18:21:49Z

+                        .select(rz_dsl::update_disposition_generation)
+                        .single_value(),
+                ),
+            )


I was also going to leave a comment about limiting the size of the query, but then I noticed that @jgallagher beat me to it. I agree that the potential size of this query should generally not be huge because (IIUC) we are gonna be evacuating one sled at a time and it will only have so many VMMs on it, but...I still feel a bit sketched out by queries that do potentially unbounded amounts of work. Also, this might be putting the cart before the horse, but I wondered if "well, we won't be evacuating more than one sled at a time" would still be true in a multirack world?

I feel like there isn't any huge reason not to slap a .limit(SQL_BATCH_SIZE) on this just to be safe? I don't think it actually needs to be paginated per se, because the WHERE vmm.stop_for_update_generation IS NULL clause will already exclude VMMs marked by a previous execution of the query, right? So we can just add a maximum number to mark per execution and either have the BG task run it until it has marked zero new VMMs, or rely on subsequent activations of the bg task to do it again?

hawkw · 2026-08-31T18:22:52Z

+                    slog::error!(
+                        &opctx.log,
+                        "failed to mark VMMs to stop for a sled update";
+                        &err,
+                    );
+                    return VmmMarkStopForUpdateStatus {
+                        disabled: false,
+                        vmms_marked: 0,
+                        error: Some(InlineErrorChain::new(&err).to_string()),


I think this should be:

Suggested change

slog::error!(

&opctx.log,

"failed to mark VMMs to stop for a sled update";

&err,

);

return VmmMarkStopForUpdateStatus {

disabled: false,

vmms_marked: 0,

error: Some(InlineErrorChain::new(&err).to_string()),

let err = InlineErrorChain::new(&err);

slog::error!(

&opctx.log,

"failed to mark VMMs to stop for a sled update";

&err,

);

return VmmMarkStopForUpdateStatus {

disabled: false,

vmms_marked: 0,

error: Some(err.to_string()),

so that we log the whole error chain, too?

hawkw · 2026-08-31T18:24:29Z

+# This task reads from the blueprint rendezvous table directly, so it doesn't
+# need to run more often
+vmm_mark_stop_for_update.period_secs = 300


If we were to change the query to be batched and rely on successive applications to ensure that all VMMs get marked, would this still be the case?

hawkw · 2026-08-31T18:25:52Z

+            .filter(dsl::state.eq_any([
+                DbVmmState::Creating,
+                DbVmmState::Starting,
+                DbVmmState::Running,
+                DbVmmState::Rebooting,
+            ]))


This set of states looks right to me; I left another comment on the way the stoppable_states is currently implemented

[reconfigurator] rendezvous subtask for marking VMMs to be stopped fo…

bd86820

…r update

karencfv mentioned this pull request Aug 27, 2026

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

Open

7 tasks

Merge branch 'main' into rendezvous-vmm-stop-for-update

61c1251

karencfv added 2 commits August 27, 2026 19:55

fixes after merge

6e317a8

fmt

10ce875

karencfv changed the title ~~[reconfigurator] rendezvous subtask for marking VMMs to be stopped for update~~ [reconfigurator] background task for marking VMMs to be stopped for update Aug 27, 2026

karencfv added 7 commits August 28, 2026 15:01

modify to query rendezvous table

87864be

improve test

f9d662c

add the background task

80be57a

expectorate

5796d5f

add a disable flag

0e9d65a

test disabled

4598157

clean up

53139af

karencfv marked this pull request as ready for review August 28, 2026 06:05

karencfv requested review from hawkw, jgallagher and sunshowers August 28, 2026 06:05

karencfv commented Aug 28, 2026

View reviewed changes

clean up more

aa0c46b

jgallagher reviewed Aug 28, 2026

View reviewed changes

karencfv added 5 commits August 31, 2026 16:27

stoppable stated

84e868e

test for deleted VMMs

effee09

add test case

ac7cbd3

clean up

fc8c765

clean up

4e3f9b6

hawkw reviewed Aug 31, 2026

View reviewed changes

		/// This is an emergency lever for support / operations. It should only be
		/// necessary if something has gone extremely wrong.

Uh oh!

Conversation

karencfv commented Aug 26, 2026

Uh oh!

karencfv commented Aug 27, 2026

Uh oh!

sunshowers commented Aug 27, 2026

Uh oh!

karencfv commented Aug 28, 2026

Uh oh!

karencfv Aug 28, 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!

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

Uh oh!

jgallagher Aug 28, 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!

karencfv Aug 31, 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!

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

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!

karencfv Aug 28, 2026 •

edited

Loading

jgallagher Aug 28, 2026 •

edited

Loading

karencfv Aug 31, 2026 •

edited

Loading