Skip to content

[reconfigurator] background task for marking VMMs to be stopped for update #11170

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
karencfv wants to merge 17 commits into
base: main
Choose a base branch
from
Open

[reconfigurator] background task for marking VMMs to be stopped for update #11170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
bd86820
[reconfigurator] rendezvous subtask for marking VMMs to be stopped fo…
karencfv Aug 26, 2026
61c1251
Merge branch 'main' into rendezvous-vmm-stop-for-update
karencfv Aug 27, 2026
6e317a8
fixes after merge
karencfv Aug 27, 2026
10ce875
fmt
karencfv Aug 27, 2026
87864be
modify to query rendezvous table
karencfv Aug 28, 2026
f9d662c
improve test
karencfv Aug 28, 2026
80be57a
add the background task
karencfv Aug 28, 2026
5796d5f
expectorate
karencfv Aug 28, 2026
0e9d65a
add a disable flag
karencfv Aug 28, 2026
4598157
test disabled
karencfv Aug 28, 2026
53139af
clean up
karencfv Aug 28, 2026
aa0c46b
clean up more
karencfv Aug 28, 2026
84e868e
stoppable stated
karencfv Aug 31, 2026
effee09
test for deleted VMMs
karencfv Aug 31, 2026
ac7cbd3
add test case
karencfv Aug 31, 2026
fc8c765
clean up
karencfv Aug 31, 2026
4e3f9b6
clean up
karencfv Aug 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions dev-tools/omdb/src/bin/omdb/nexus.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ use nexus_types::internal_api::background::TufArtifactReplicationCounters;
use nexus_types::internal_api::background::TufArtifactReplicationRequest;
use nexus_types::internal_api::background::TufArtifactReplicationStatus;
use nexus_types::internal_api::background::TufRepoPrunerStatus;
use nexus_types::internal_api::background::VmmMarkStopForUpdateStatus;
use nexus_types::internal_api::background::fm_rendezvous;
use omicron_uuid_kinds::BlueprintUuid;
use omicron_uuid_kinds::CollectionUuid;
Expand Down Expand Up @@ -1426,6 +1427,9 @@ fn print_task_details(bgtask: &BackgroundTask, details: &serde_json::Value) {
"switch_port_config_manager" => {
print_task_switch_port_settings_manager(details);
}
"vmm_mark_stop_for_update" => {
print_task_vmm_mark_stop_for_update(details);
}
_ => {
println!(
"warning: unknown background task: {:?} \
Expand Down Expand Up @@ -2842,6 +2846,33 @@ fn print_task_audit_log_cleanup(details: &serde_json::Value) {
};
}

fn print_task_vmm_mark_stop_for_update(details: &serde_json::Value) {
match serde_json::from_value::<VmmMarkStopForUpdateStatus>(details.clone())
{
Err(error) => eprintln!(
"warning: failed to interpret task details: {:?}: {:?}",
error, details
),
Ok(status) => {
let VmmMarkStopForUpdateStatus { disabled, vmms_marked, error } =
status;

if disabled {
println!(" task explicitly disabled by config!");
}

const MARKED: &str = "VMMs marked:";

@hawkw hawkw Aug 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@hawkw hawkw Aug 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


println!(" {MARKED:<WIDTH$}{}", vmms_marked);
if let Some(error) = &error {
println!(" {ERROR:<WIDTH$}{error}");
}
}
};
}

fn print_task_audit_log_timeout_incomplete(details: &serde_json::Value) {
match serde_json::from_value::<AuditLogTimeoutIncompleteStatus>(
details.clone(),
Expand Down
12 changes: 12 additions & 0 deletions dev-tools/omdb/tests/env.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,10 @@ task: "v2p_manager"
manages opte v2p mappings for vpc networking


task: "vmm_mark_stop_for_update"
marks VMMs on evacuating sleds as needing to be stopped for an update


task: "vpc_route_manager"
propagates updated VPC routes to all OPTE ports

Expand Down Expand Up @@ -531,6 +535,10 @@ task: "v2p_manager"
manages opte v2p mappings for vpc networking


task: "vmm_mark_stop_for_update"
marks VMMs on evacuating sleds as needing to be stopped for an update


task: "vpc_route_manager"
propagates updated VPC routes to all OPTE ports

Expand Down Expand Up @@ -786,6 +794,10 @@ task: "v2p_manager"
manages opte v2p mappings for vpc networking


task: "vmm_mark_stop_for_update"
marks VMMs on evacuating sleds as needing to be stopped for an update


task: "vpc_route_manager"
propagates updated VPC routes to all OPTE ports

Expand Down
16 changes: 16 additions & 0 deletions dev-tools/omdb/tests/successes.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,10 @@ task: "v2p_manager"
manages opte v2p mappings for vpc networking


task: "vmm_mark_stop_for_update"
marks VMMs on evacuating sleds as needing to be stopped for an update


task: "vpc_route_manager"
propagates updated VPC routes to all OPTE ports

Expand Down Expand Up @@ -1091,6 +1095,12 @@ task: "v2p_manager"
started at <REDACTED_TIMESTAMP> (<REDACTED DURATION>s ago) and ran for <REDACTED DURATION>ms
warning: unknown background task: "v2p_manager" (don't know how to interpret details: Object {})

task: "vmm_mark_stop_for_update"
configured period: every <REDACTED_DURATION>m
last completed activation: <REDACTED ITERATIONS>, triggered by <TRIGGERED_BY_REDACTED>
started at <REDACTED_TIMESTAMP> (<REDACTED DURATION>s ago) and ran for <REDACTED DURATION>ms
VMMs marked: 0

task: "vpc_route_manager"
configured period: every <REDACTED_DURATION>s
last completed activation: <REDACTED ITERATIONS>, triggered by <TRIGGERED_BY_REDACTED>
Expand Down Expand Up @@ -1813,6 +1823,12 @@ task: "v2p_manager"
started at <REDACTED_TIMESTAMP> (<REDACTED DURATION>s ago) and ran for <REDACTED DURATION>ms
warning: unknown background task: "v2p_manager" (don't know how to interpret details: Object {})

task: "vmm_mark_stop_for_update"
configured period: every <REDACTED_DURATION>m
last completed activation: <REDACTED ITERATIONS>, triggered by <TRIGGERED_BY_REDACTED>
started at <REDACTED_TIMESTAMP> (<REDACTED DURATION>s ago) and ran for <REDACTED DURATION>ms
VMMs marked: 0

task: "vpc_route_manager"
configured period: every <REDACTED_DURATION>s
last completed activation: <REDACTED ITERATIONS>, triggered by <TRIGGERED_BY_REDACTED>
Expand Down
25 changes: 25 additions & 0 deletions nexus-config/src/nexus_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,8 @@ pub struct BackgroundTaskConfig {
pub audit_log_cleanup: AuditLogCleanupConfig,
/// configuration for populate switch ports task
pub populate_switch_ports: PopulateSwitchPortsConfig,
/// configuration for the task that marks VMMs to stop for an update
pub vmm_mark_stop_for_update: VmmMarkStopForUpdateConfig,
}

#[serde_as]
Expand Down Expand Up @@ -523,6 +525,23 @@ pub struct AuditLogCleanupConfig {
pub max_deleted_per_activation: u32,
}

#[serde_as]
#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)]
pub struct VmmMarkStopForUpdateConfig {
/// period (in seconds) for periodic activations of this task
#[serde_as(as = "DurationSeconds<u64>")]
pub period_secs: Duration,

/// disable marking VMMs to stop for a sled update.
///
/// This is an emergency lever for support / operations. It should only be
/// necessary if something has gone extremely wrong.
Comment on lines +537 to +538

@jgallagher jgallagher Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hawkw hawkw Aug 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@karencfv karencfv Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jgallagher jgallagher Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

///
/// Default: Off
#[serde(default)]
pub disable: bool,
}

#[serde_as]
#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)]
pub struct PopulateSwitchPortsConfig {
Expand Down Expand Up @@ -1376,6 +1395,7 @@ mod test {
audit_log_cleanup.retention_days = 90
audit_log_cleanup.max_deleted_per_activation = 10000
populate_switch_ports.period_secs = 31
vmm_mark_stop_for_update.period_secs = 300
[default_region_allocation_strategy]
type = "random"
seed = 0
Expand Down Expand Up @@ -1656,6 +1676,10 @@ mod test {
populate_switch_ports: PopulateSwitchPortsConfig {
period_secs: Duration::from_secs(31),
},
vmm_mark_stop_for_update: VmmMarkStopForUpdateConfig {
period_secs: Duration::from_secs(300),
disable: false,
},
},
multicast: MulticastConfig { enabled: false },
default_region_allocation_strategy:
Expand Down Expand Up @@ -1772,6 +1796,7 @@ mod test {
audit_log_cleanup.retention_days = 90
audit_log_cleanup.max_deleted_per_activation = 10000
populate_switch_ports.period_secs = 31
vmm_mark_stop_for_update.period_secs = 300

[default_region_allocation_strategy]
type = "random"
Expand Down
1 change: 1 addition & 0 deletions nexus/background-task-interface/src/init.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ pub struct BackgroundTasks {
pub task_attached_subnet_manager: Activator,
pub task_session_cleanup: Activator,
pub task_populate_switch_ports: Activator,
pub task_vmm_mark_stop_for_update: Activator,

// Handles to activate background tasks that do not get used by Nexus
// at-large. These background tasks are implementation details as far as
Expand Down
25 changes: 25 additions & 0 deletions nexus/db-model/src/vmm_state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,31 @@ impl VmmState {
pub fn exists_on_sled(&self) -> bool {
self.to_nexus_state().exists_on_sled()
}

/// 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()
Comment on lines +110 to +132

@hawkw hawkw Aug 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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"
  2. 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.
];

@hawkw hawkw Aug 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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?
  2. 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...".

@jgallagher jgallagher Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));
        }
    }
}

@hawkw hawkw Aug 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hawkw hawkw Aug 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

impl fmt::Display for VmmState {
Expand Down
Loading
Loading