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

There are several like this:

omicron/nexus-config/src/nexus_config.rs

Lines 592 to 597 in 903d357

/// A toggle to disable support bundle collection

///

/// Default: Off

#[serde(default)]

pub disable: bool,

}

Do we not want something similar here?

Hmm, as far as I know (and happy to be corrected), we've used config-level toggles for runtime configuration approximately never. We have used them for "this task is implemented but we don't want it to be on until other work is done", so shipped configs with disable = true that we then later removed once we were ready to enable it by default. That's a pretty different use case than what's in the comment here, though; since config changes aren't persistent, this really isn't a lever for support or operations to use.

If we're adding operational knobs that can be tweaked via omdb (and are presumably persisted in crdb), I don't think this config option is useful, unless we're specifically in the case of "we want to merge this in a disabled-by-default state" (although even then, I'd be strongly tempted to say "make the default of the operational knob disabled" and still argue for not having a config-level disable).

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?

FWIW in one of the chats we had about this feature I remember @davepacheco mentioning this should be done in a single transaction. Maybe it'd be good to get his input here?

It seems important that the UPDATE be conditional on the other things (rather than doing a paginated SELECT and then updating those rows by id). I can't think of why it would be a problem to throw a LIMIT on it (and re-run if needed).

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?

Probably not, thanks for pointing this out! I'll keep this in mind if we decide to batch the query

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

karencfv

Thanks for the reviews everyone! I think this is ready for another look

hawkw

Overall, this looks good to me! I had a few more small suggestions, but none of them are blockers.

hawkw · 2026-09-30T02:23:24Z

+    /// VMMs that are in the `Failed`, `Stopping`, `Stopped`, `Migrating`,
+    /// `Destroyed` or `SagaUnwound` states do not need to be stopped, so they
+    /// are left untouched. VMMs that are already marked to be stopped by update
+    /// are also excluded.


take it or leave it: i might just reference the constant here, instead of listing all the states not in the constant; that way, this doesn't need to be rewritten in the future...

hawkw · 2026-09-30T16:54:54Z

+        // This test exists to make sure that when a new state is added, we
+        // consider whether it is a state where reconfigurator should mark the
+        // VMM to be stopped for sled evacuation. More detailed information about
+        // the process of restarting instances during a live update can be found
+        // in RFD 739.


thanks for documenting what this is for. i think we might do well to add a slightly more explicit "what to do if this test no longer compiles" bit to this comment, like say:

Suggested change

// This test exists to make sure that when a new state is added, we

// consider whether it is a state where reconfigurator should mark the

// VMM to be stopped for sled evacuation. More detailed information about

// the process of restarting instances during a live update can be found

// in RFD 739.

// This test exists to make sure that when a new state is added, we

// consider whether it is a state where reconfigurator should mark the

// VMM to be stopped for sled evacuation. More detailed information about

// the process of restarting instances during a live update can be found

// in RFD 739.

//

// The use of an exhaustive `match state` here ensures that the addition

// of a new `VmmState` variant will result in a compiler error until it

// is added to this test. If you have added a new variant to VmmState,

// consider whether or not that state should be added to the list of

// states in which VMMs are marked to stop. Do *not* change the match

// here to be non-exhaustive!

hawkw · 2026-09-30T16:58:13Z

+        // We loop until a batch marks nothing, so a backlog larger than one
+        // batch is drained in a single call. Each newly marked VMM's generation
+        // is no longer NULL, so it drops out of the next batch's filter.


the wording here triggers my Claude-detection senses a bit, how about something like:

Suggested change

// We loop until a batch marks nothing, so a backlog larger than one

// batch is drained in a single call. Each newly marked VMM's generation

// is no longer NULL, so it drops out of the next batch's filter.

// This loop executes a query that marks up to `SQL_BATCH_SIZE` rows

// at a time until it indicates that no records were marked. Because

// there is a filter that excludes VMMs that have already been marked,

// subsequent batches will not include these VMMs.

lol yeah, that's a claude comment I missed 😄 will fix

hawkw · 2026-09-30T17:03:36Z

+            // Diesel's ValidSubselect doesn't allow a subquery from the same
+            // table being updated, so we alias the `vmm` table for the subquery
+            let vmm_sub =
+                diesel::alias!(nexus_db_schema::schema::vmm as vmm_sub);
+
+            // Diesel (and CockroachDB) don't support LIMIT on an UPDATE, so we
+            // select a bounded batch of VMM ids and update those
+            let batch =
+                vmm_sub
+                    .filter(vmm_sub.field(vmm::time_deleted).is_null())
+                    .filter(
+                        vmm_sub
+                            .field(vmm::stop_for_update_disposition_generation)
+                            .is_null(),
+                    )
+                    .filter(
+                        vmm_sub
+                            .field(vmm::state)
+                            .eq_any(DbVmmState::SHOULD_STOP_FOR_EVACUATION),
+                    )
+                    .filter(
+                        vmm_sub.field(vmm::sled_id).eq_any(
+                            rz_dsl::rendezvous_sled_bp_availability
+                                .filter(rz_dsl::bp_availability.eq(
+                                    model::DbSledBpAvailability::Unavailable,
+                                ))
+                                .select(rz_dsl::sled_id),
+                        ),
+                    )
+                    .limit(i64::from(SQL_BATCH_SIZE.get()))
+                    .select(vmm_sub.field(vmm::id));
+
+            let marked =
+                diesel::update(dsl::vmm)
+                    .filter(dsl::id.eq_any(batch))
+                    .set(
+                        dsl::stop_for_update_disposition_generation.eq(
+                            rz_dsl::rendezvous_sled_bp_availability
+                                .filter(rz_dsl::sled_id.eq(dsl::sled_id))
+                                .filter(rz_dsl::bp_availability.eq(
+                                    model::DbSledBpAvailability::Unavailable,
+                                ))
+                                .select(rz_dsl::update_disposition_generation)
+                                .single_value(),
+                        ),
+                    )


since this is a rather complex query, what do you think about following the pattern where there is a separate internal helper function that just constructs the query and returns impl RunnableQuery, like this: https://github.com/karencfv/omicron/blob/87331dad265dadae5c5e6df6995e4e1fb77ed7ba/nexus/db-queries/src/db/datastore/fm.rs#L744-L764

so that we can then have expectorate_query_contents and explain tests for the query, like this:

https://github.com/karencfv/omicron/blob/87331dad265dadae5c5e6df6995e4e1fb77ed7ba/nexus/db-queries/src/db/datastore/fm.rs#L2202-L2233

this is certainly not mandatory, but I find it generally useful to be able to have a test that writes out the generated SQL for complicated queries, and I like having the EXPLAIN test to assert that they won't do full scans a bit more easily than having to actually test the code that uses the query...

hawkw · 2026-09-30T17:05:35Z

 audit_log_cleanup.max_deleted_per_activation = 10000
 populate_switch_ports.period_secs = 30
+# This task reads from the blueprint rendezvous table directly, so it doesn't
+# need to run more often


not totally sure if I understand this comment --- is 300 seconds the pace at which the rendezvous table is updated? or am I missing something?

yup, it is. I can rephrase so this is more obvious

hawkw · 2026-09-30T17:07:32Z

+    /// Number of VMMs that were marked as needing to be stopped for update in
+    /// this activation.
+    pub vmms_marked: usize,
+    /// Error encountered during this activation, if any.
+    pub error: Option<String>,


teensy nit: it might be nice if the status object also included the batch size (which is currently always SQL_BATCH_SIZE), so that omdb db background-tasks show can also indicate how many batches were run?

I'm not sure how I feel about this one. We already have the amount of VMMs we marked, and the amount of batches is kind of an internal implementation detail. I worry that for some reason we decide to change the batch size in the future and we forget to update here and end up misleading people.

I'm not sure why someone would need to know how many batches were run, but if they did, they could look at the Datastore method no?

I worry that for some reason we decide to change the batch size in the future and we forget to update here and end up misleading people.

that's why i was imagining this would send whatever the current value of SQL_BATCH_SIZE as part of the status object, and then omdb would divide vmms_marked by that to tell you how many batches happened, so it's always correct based on the build of Nexus that produced the status. there are some other background tasks that do this, though after further consideration it is possible i wrote most of them (?). it's certainly not that important.

karencfv

Thanks for taking another look @hawkw! I think I've made sufficient changes to the queries that they warrant another look even though this PR is approved.

karencfv · 2026-10-01T01:07:04Z

+        // We loop until a batch marks nothing, so a backlog larger than one
+        // batch is drained in a single call. Each newly marked VMM's generation
+        // is no longer NULL, so it drops out of the next batch's filter.


lol yeah, that's a claude comment I missed 😄 will fix

jgallagher · 2026-10-01T18:56:58Z

+            } = status;
+
+            const MARKED: &str = "VMMs marked to be stopped for an update:";
+            const BATCHES: &str = "  batches:";


tiny nit - are there supposed to be leading spaces in this one?

Yup, just following the pattern here

omicron/dev-tools/omdb/src/bin/omdb/nexus.rs

Line 3961 in 6c451ad

const BATCHES: &str = " batches:";

and here

omicron/dev-tools/omdb/src/bin/omdb/nexus.rs

Line 4057 in 6c451ad

println!(" {:<width$}{:>NUM_WIDTH$}", " batches:", stats.batches);

and other places that do a similar thing

jgallagher · 2026-10-01T20:26:11Z

+            })?;
+
+            let marked =
+                diesel::update(dsl::vmm)


I don't think this is correct - if a sled becomes available in between the query that loads the VMM IDs and this update, we'll set stop_for_update_disposition_generation to NULL, which might erase a stop generation written by another Nexus, right?

Putting these queries into an interactive transaction would fix that but I don't think that's great either; we're essentially doing subqueries on the client side, and I think we should be able to get cockroach to do that for us in a single query. Will poke at this some.

ugh yeah, I moved things around to get everything playing nicely together (batching/diesel/tests). SQL and diesel are definitely not my strong suit 😭

Ok, this appears to work; at least, it works in the cockroach shell and if I drop it in verbatim, the existing tests still pass:

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

This does contradict the comment on line 212 that says CRDB doesn't support LIMIT on UPDATE, but I think that comment is incorrect. 😅 I do think there's no way to represent this query in diesel, but it seems reasonable to drop down to a raw query for this, I think?

This does contradict the comment on line 212 that says CRDB doesn't support LIMIT on UPDATE

I think it was just diesel then, got the comment wrong sorry!

I do think there's no way to represent this query in diesel, but it seems reasonable to drop down to a raw query for this, I think?

Yes, at this point definitely yes, diesel has been doing my head in for a while now, thank you!!!!!

jgallagher · 2026-10-01T20:29:14Z

+        // subsequent batches will not include these VMMs.
+        let mut marked_total = 0;
+        let mut batches = 0;
+        loop {


Looping here seems a little iffy to me. Maybe it's fine? But if we're calling this from a bg task, it already does its own looping (via periodic activation); could/should we do just a single batch in any given bg task activation? I guess that doesn't play nicely with latency though, hm.

I guess that doesn't play nicely with latency though, hm.

Yeah :/ in the end that's why I chose to loop

jgallagher · 2026-10-01T20:29:42Z

 audit_log_cleanup.max_deleted_per_activation = 10000
 populate_switch_ports.period_secs = 30
+# This task reads directly from the blueprint rendezvous table, which is updated
+# every 300s. Therefore, this task doesn't need to run more often.


Does this comment imply we should be activating the new task any time the rendezvous table task makes a change?

not really, more like there's no point in running more often than that because there won't be any changes in the table

But these aren't necessarily synchronized in any way, right? You might run this task, then immediately run the rendezvous-table-writing task, then have to wait 5 minutes for this task to run again to act on the changes?

Fair, are you proposing we run this more often? Or just removing the comment?

Mostly I was proposing that we should be activating the new task any time the rendezvous table task makes a change, but yeah I think the comment is misleading.

In terms of running this more often - I think that kinda depends on where we land with the loop / batching question in the sql query. If we're okay with the task having a "loop until we make no changes", then only running this when the rendezvous table task makes a change seems okay. I'm pretty strongly tempted to suggest we get rid of the loop, only do a single batch per bg task activation, and make it run much more frequently. But I'd like to get other folks' input on that.

My biggest concern is latency between batches. We can't guarantee that a customer won't spin up thousands and thousands of tiny instances on their rack. Having the job run until completion sort of guarantees that we don't end up taking way too long to marks all VMMs that need to stop.

Yeah that's fair. If we keep the loop having a long period is fine, but I do think we need to proactively activate this task when we know it needs to run. IIUC the blueprint_rendezvous task is fine with a long activation period because it's explicitly activated whenever the blueprint or inventory changes:

omicron/nexus/src/app/background/init.rs

Lines 682 to 688 in d41a8c3

// A new target blueprint must reach the sled availability table

// promptly, even if inventory is stalled for whatever reason, so

// watch both channels.

watchers: vec![

Box::new(rx_blueprint.clone()),

Box::new(inventory_load_watcher.clone()),

],

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-01T20:59:38Z

+            // First we retrieve the ids of unavailable sleds. This prevents a
+            // full scan of the `lookup_vmms_by_sled_id` index when we select
+            // the VMMs to mark.
+            let unavailable_sled_ids =
+                DataStore::rendezvous_read_unavailable_sleds_query()
+                    .load_async::<Uuid>(&*conn)
+                    .await
+                    .map_err(|e| {
+                        public_error_from_diesel(e, ErrorHandler::Server)
+                            .internal_context(
+                                "failed to load unavailable sleds",
+                            )
+                    })?;

            // Diesel (and CockroachDB) don't support LIMIT on an UPDATE, so we
-            // select a bounded batch of VMM ids and update those
-            let batch =
-                vmm_sub
-                    .filter(vmm_sub.field(vmm::time_deleted).is_null())
-                    .filter(
-                        vmm_sub
-                            .field(vmm::stop_for_update_disposition_generation)
-                            .is_null(),
-                    )
-                    .filter(
-                        vmm_sub
-                            .field(vmm::state)
-                            .eq_any(DbVmmState::SHOULD_STOP_FOR_EVACUATION),
-                    )
-                    .filter(
-                        vmm_sub.field(vmm::sled_id).eq_any(
-                            rz_dsl::rendezvous_sled_bp_availability
-                                .filter(rz_dsl::bp_availability.eq(
-                                    model::DbSledBpAvailability::Unavailable,
-                                ))
-                                .select(rz_dsl::sled_id),
-                        ),
+            // select a bounded batch of VMM ids and update those.
+            let batch = DataStore::vmm_read_ready_to_stop_with_limit_query(
+                unavailable_sled_ids,
+                batch_size,
+            )


hmm, wait, wasn't the idea that this would be one query that does both the select subquery and the update subquery, rather than two? if changing that was necessary to implement my suggestion about having helpers that return RunnableQuery and putting it back, I think we should back out that change --- as @davepacheco noted in https://github.com/oxidecomputer/omicron/pull/11170/changes/6062fba56204dd850aedbd1d67680634ec105eb2..6323eb99fa6784168710addd37b7c7ee90cccb1e#r4133811458, I think it's important that this be done in one query. I think we can compose the two subqueries together in one impl RunnableQuery helper, but if we can't, I think the atomicity is more important than the expectorate tests...

if changing that was necessary to implement my suggestion about having helpers that return RunnableQuery and putting it back, I think we should back out that change

Yeah, I needed to do this in order to get the helpers/tests. I'll see if I can make it work, but otherwise I'll revert

jgallagher · 2026-10-01T21:34:03Z


+/* Add an index which lets us find sleds by availability */
+CREATE INDEX IF NOT EXISTS lookup_sled_by_bp_availability
+    ON omicron.public.rendezvous_sled_bp_availability (bp_availability);


Can this be further restricted to WHERE bp_availability = 'unavailable'? (Probably needs a slightly different name, but I think that's all we care about for the query, and this would keep the index small.)

jgallagher · 2026-10-01T21:36:58Z

+            })?;
+
+            let marked =
+                diesel::update(dsl::vmm)


Ok, this appears to work; at least, it works in the cockroach shell and if I drop it in verbatim, the existing tests still pass:

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

This does contradict the comment on line 212 that says CRDB doesn't support LIMIT on UPDATE, but I think that comment is incorrect. 😅 I do think there's no way to represent this query in diesel, but it seems reasonable to drop down to a raw query for this, I think?

[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

1 of 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

karencfv added 7 commits September 29, 2026 14:21

Merge main

3da238f

update tests after merging with main

62d17c5

expectorate

b00dc65

states should be a const

436d953

remove disable from config

c6477fe

batch the update query

08541f1

Test batching

213a265

karencfv commented Sep 30, 2026

View reviewed changes

karencfv requested review from davepacheco, hawkw and jgallagher September 30, 2026 02:19

karencfv added 2 commits September 30, 2026 15:22

merge main

fc81f09

fix remaining bits

6062fba

hawkw approved these changes Sep 30, 2026

View reviewed changes

karencfv added 4 commits October 1, 2026 14:21

documentation fixes

2e176d7

new index and explain tests

328158d

expectorate

1739d75

bubble up batch info

6323eb9

karencfv commented Oct 1, 2026

View reviewed changes

jgallagher reviewed Oct 1, 2026

View reviewed changes

hawkw reviewed Oct 1, 2026

View reviewed changes

hawkw self-requested a review October 1, 2026 21:00

jgallagher reviewed Oct 1, 2026

View reviewed changes

give up on diesel

1e08c78

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

	/// A toggle to disable support bundle collection
	///
	/// Default: Off
	#[serde(default)]
	pub disable: bool,
	}

	// A new target blueprint must reach the sled availability table
	// promptly, even if inventory is stalled for whatever reason, so
	// watch both channels.
	watchers: vec![
	Box::new(rx_blueprint.clone()),
	Box::new(inventory_load_watcher.clone()),
	],

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!

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

karencfv Aug 28, 2026 •

edited

Loading

jgallagher Aug 28, 2026 •

edited

Loading

karencfv Aug 31, 2026 •

edited

Loading