From 310551c02988aa2175f95bb1390717f72f7605b6 Mon Sep 17 00:00:00 2001 From: karencfv Date: Wed, 30 Sep 2026 18:27:29 +1300 Subject: [PATCH 01/44] [reconfigurator] background task for stopping marked VMMs --- dev-tools/omdb/src/bin/omdb/nexus.rs | 28 ++++ nexus-config/src/nexus_config.rs | 16 ++ nexus/background-task-interface/src/init.rs | 1 + nexus/db-queries/src/db/datastore/vmm.rs | 28 ++++ nexus/examples/config-second.toml | 1 + nexus/examples/config.toml | 1 + nexus/src/app/background/init.rs | 24 +++ nexus/src/app/background/tasks/mod.rs | 1 + .../background/tasks/vmm_stop_for_update.rs | 149 ++++++++++++++++++ nexus/tests/config.test.toml | 1 + nexus/types/src/internal_api/background.rs | 10 ++ smf/nexus/multi-sled/config-partial.toml | 2 + smf/nexus/single-sled/config-partial.toml | 2 + 13 files changed, 264 insertions(+) create mode 100644 nexus/src/app/background/tasks/vmm_stop_for_update.rs diff --git a/dev-tools/omdb/src/bin/omdb/nexus.rs b/dev-tools/omdb/src/bin/omdb/nexus.rs index fdc18091d22..b82bb92f09e 100644 --- a/dev-tools/omdb/src/bin/omdb/nexus.rs +++ b/dev-tools/omdb/src/bin/omdb/nexus.rs @@ -1295,6 +1295,9 @@ fn print_task_details(bgtask: &BackgroundTask, details: &serde_json::Value) { "abandoned_vmm_reaper" => { print_task_abandoned_vmm_reaper(details); } + "vmm_stop_for_update" => { + print_task_vmm_stop_for_update(details); + } "attached_subnet_manager" => { print_task_attached_subnet_manager_status(details); } @@ -1506,6 +1509,31 @@ fn print_task_abandoned_vmm_reaper(details: &serde_json::Value) { }; } +fn print_task_vmm_stop_for_update(details: &serde_json::Value) { + use nexus_types::internal_api::background::VmmStopForUpdateStatus; + + match serde_json::from_value::(details.clone()) { + Err(error) => eprintln!( + "warning: failed to interpret task details: {:?}: {:?}", + error, details + ), + Ok(VmmStopForUpdateStatus { vmms_stopped_by_sled, error }) => { + const STOPPED: &str = "VMMs stopped:"; + const ERROR: &str = "error:"; + const WIDTH: usize = const_max_len(&[STOPPED, ERROR]) + 1; + + let total: usize = vmms_stopped_by_sled.values().sum(); + println!(" {STOPPED:(details.clone()) diff --git a/nexus-config/src/nexus_config.rs b/nexus-config/src/nexus_config.rs index dac2e772f33..8e72adacd9c 100644 --- a/nexus-config/src/nexus_config.rs +++ b/nexus-config/src/nexus_config.rs @@ -457,6 +457,9 @@ pub struct BackgroundTaskConfig { pub v2p_mapping_propagation: V2PMappingPropagationConfig, /// configuration for abandoned VMM reaper task pub abandoned_vmm_reaper: AbandonedVmmReaperConfig, + /// configuration for the task that stops VMMs marked to be stopped for an + /// update + pub vmm_stop_for_update: VmmStopForUpdateConfig, /// configuration for saga recovery task pub saga_recovery: SagaRecoveryConfig, /// configuration for lookup region port task @@ -815,6 +818,14 @@ pub struct AbandonedVmmReaperConfig { pub period_secs: Duration, } +#[serde_as] +#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] +pub struct VmmStopForUpdateConfig { + /// period (in seconds) for periodic activations of this background task + #[serde_as(as = "DurationSeconds")] + pub period_secs: Duration, +} + #[serde_as] #[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] pub struct SagaRecoveryConfig { @@ -1377,6 +1388,7 @@ mod test { service_firewall_propagation.period_secs = 300 v2p_mapping_propagation.period_secs = 30 abandoned_vmm_reaper.period_secs = 60 + vmm_stop_for_update.period_secs = 300 saga_recovery.period_secs = 60 lookup_region_port.period_secs = 60 region_snapshot_replacement_start.period_secs = 30 @@ -1616,6 +1628,9 @@ mod test { abandoned_vmm_reaper: AbandonedVmmReaperConfig { period_secs: Duration::from_secs(60), }, + vmm_stop_for_update: VmmStopForUpdateConfig { + period_secs: Duration::from_secs(300), + }, saga_recovery: SagaRecoveryConfig { period_secs: Duration::from_secs(60), }, @@ -1789,6 +1804,7 @@ mod test { service_firewall_propagation.period_secs = 300 v2p_mapping_propagation.period_secs = 30 abandoned_vmm_reaper.period_secs = 60 + vmm_stop_for_update.period_secs = 300 saga_recovery.period_secs = 60 lookup_region_port.period_secs = 60 region_snapshot_replacement_start.period_secs = 30 diff --git a/nexus/background-task-interface/src/init.rs b/nexus/background-task-interface/src/init.rs index fdf9447d466..25c5b3584e0 100644 --- a/nexus/background-task-interface/src/init.rs +++ b/nexus/background-task-interface/src/init.rs @@ -36,6 +36,7 @@ pub struct BackgroundTasks { pub task_instance_reincarnation: Activator, pub task_service_firewall_propagation: Activator, pub task_abandoned_vmm_reaper: Activator, + pub task_vmm_stop_for_update: Activator, pub task_audit_log_cleanup: Activator, pub task_audit_log_timeout_incomplete: Activator, pub task_vpc_route_manager: Activator, diff --git a/nexus/db-queries/src/db/datastore/vmm.rs b/nexus/db-queries/src/db/datastore/vmm.rs index 51009fec829..8e2f0ad1884 100644 --- a/nexus/db-queries/src/db/datastore/vmm.rs +++ b/nexus/db-queries/src/db/datastore/vmm.rs @@ -488,6 +488,34 @@ impl DataStore { .await .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } + + /// TODO-K: Fix comment, Lists VMMs that have been marked as needing to be + /// stopped in order to update their sled, that have not been deleted, and + /// that are still in a state from which they need to be stopped. VMMs that + /// are already stopping, stopped, or destroyed are excluded. + pub async fn vmm_list_marked_stop_for_update( + &self, + opctx: &OpContext, + pagparams: &DataPageParams<'_, Uuid>, + ) -> ListResultVec { + paginated(dsl::vmm, dsl::id, pagparams) + .filter(dsl::time_deleted.is_null()) + .filter(dsl::stop_for_update_disposition_generation.is_not_null()) + // TODO-K: change to DbVmmState::SHOULD_STOP_FOR_EVACUATION once + // that branch is merged + .filter(dsl::state.eq_any(&[ + // A VMM in one of these states is on its way, or is already running, + // and can be stopped. + DbVmmState::Creating, + DbVmmState::Starting, + DbVmmState::Running, + DbVmmState::Rebooting, + ])) + .select(Vmm::as_select()) + .load_async(&*self.pool_connection_authorized(opctx).await?) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) + } } #[cfg(test)] diff --git a/nexus/examples/config-second.toml b/nexus/examples/config-second.toml index e4f79eec766..5f53eb95ea9 100644 --- a/nexus/examples/config-second.toml +++ b/nexus/examples/config-second.toml @@ -162,6 +162,7 @@ instance_reincarnation.period_secs = 60 service_firewall_propagation.period_secs = 300 v2p_mapping_propagation.period_secs = 30 abandoned_vmm_reaper.period_secs = 60 +vmm_stop_for_update.period_secs = 300 saga_recovery.period_secs = 600 lookup_region_port.period_secs = 60 region_snapshot_replacement_start.period_secs = 30 diff --git a/nexus/examples/config.toml b/nexus/examples/config.toml index 6102eafc66a..7b30d88972c 100644 --- a/nexus/examples/config.toml +++ b/nexus/examples/config.toml @@ -153,6 +153,7 @@ instance_reincarnation.period_secs = 60 service_firewall_propagation.period_secs = 300 v2p_mapping_propagation.period_secs = 30 abandoned_vmm_reaper.period_secs = 60 +vmm_stop_for_update.period_secs = 300 saga_recovery.period_secs = 600 lookup_region_port.period_secs = 60 region_snapshot_replacement_start.period_secs = 30 diff --git a/nexus/src/app/background/init.rs b/nexus/src/app/background/init.rs index 77bddee6c01..161410bc8d2 100644 --- a/nexus/src/app/background/init.rs +++ b/nexus/src/app/background/init.rs @@ -147,6 +147,7 @@ use super::tasks::vpc_routes; use super::tasks::webhook_deliverator; use crate::Nexus; use crate::app::background::tasks::populate_switch_ports; +use crate::app::background::tasks::vmm_stop_for_update; use crate::app::external_client::ExternalHttpClient; use crate::app::oximeter::PRODUCER_LEASE_DURATION; use crate::app::quiesce::NexusQuiesceHandle; @@ -253,6 +254,7 @@ impl BackgroundTasksInitializer { task_instance_reincarnation: Activator::new(), task_service_firewall_propagation: Activator::new(), task_abandoned_vmm_reaper: Activator::new(), + task_vmm_stop_for_update: Activator::new(), task_audit_log_cleanup: Activator::new(), task_audit_log_timeout_incomplete: Activator::new(), task_vpc_route_manager: Activator::new(), @@ -351,6 +353,7 @@ impl BackgroundTasksInitializer { task_instance_reincarnation, task_service_firewall_propagation, task_abandoned_vmm_reaper, + task_vmm_stop_for_update, task_vpc_route_manager, task_saga_recovery, task_lookup_region_port, @@ -895,6 +898,27 @@ impl BackgroundTasksInitializer { activator: task_abandoned_vmm_reaper, }); + // Background task: stop VMMs that are marked to be stopped for a sled + // update. + // + // TODO-K: This task is meant to be activated by the + // `vmm_mark_stop_for_update` task whenever that task marks one or more + // VMMs. Once that task lands, pass `task_vmm_stop_for_update.clone()` + // to `VmmMarkStopForUpdate::new()` and have it call `.activate()` when + // it marks at least one VMM. + driver.register(TaskDefinition { + name: "vmm_stop_for_update", + description: "stops VMMs that are marked to be stopped for a sled \ + update", + period: config.vmm_stop_for_update.period_secs, + task_impl: Box::new(vmm_stop_for_update::VmmStopForUpdate::new( + datastore.clone(), + )), + opctx: opctx.child(BTreeMap::new()), + watchers: vec![], + activator: task_vmm_stop_for_update, + }); + // Background task: saga recovery { let task_impl = Box::new(saga_recovery::SagaRecovery::new( diff --git a/nexus/src/app/background/tasks/mod.rs b/nexus/src/app/background/tasks/mod.rs index b78a217346e..b365a6c0a2a 100644 --- a/nexus/src/app/background/tasks/mod.rs +++ b/nexus/src/app/background/tasks/mod.rs @@ -57,5 +57,6 @@ pub mod trust_quorum; pub mod tuf_artifact_replication; pub mod tuf_repo_pruner; pub mod v2p_mappings; +pub mod vmm_stop_for_update; pub mod vpc_routes; pub mod webhook_deliverator; diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs new file mode 100644 index 00000000000..1e78acb68d1 --- /dev/null +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -0,0 +1,149 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! TODO-K: Fix comment, Background task that stops the VMMs that have been +//! marked as needing to be stopped in order to update their sled. +//! +//! TODO-K: Fix comment, VMMs are marked (via +//! `stop_for_update_disposition_generation` on the `vmm` table) by the +//! `vmm_mark_stop_for_update` background task, which activates this task +//! whenever it marks one or more VMMs. See RFD 739. + +use crate::app::background::BackgroundTask; +use futures::future::BoxFuture; +use iddqd::IdOrdItem; +use iddqd::IdOrdMap; +use iddqd::id_upcast; +use nexus_db_queries::context::OpContext; +use nexus_db_queries::db::DataStore; +use nexus_db_queries::db::datastore::SQL_BATCH_SIZE; +use nexus_db_queries::db::pagination::Paginator; +use nexus_types::internal_api::background::VmmStopForUpdateStatus; +use omicron_common::api::external::Error; +use omicron_uuid_kinds::GenericUuid; +use omicron_uuid_kinds::PropolisUuid; +use omicron_uuid_kinds::SledUuid; +use serde_json::json; +use slog_error_chain::InlineErrorChain; +use std::collections::BTreeMap; +use std::sync::Arc; + +/// The VMMs on a single sled that are marked to be stopped in order to update +/// that sled. +#[derive(Clone, Debug)] +struct SledVmmsToStop { + sled_id: SledUuid, + vmm_ids: Vec, +} + +impl IdOrdItem for SledVmmsToStop { + type Key<'a> = SledUuid; + + fn key(&self) -> Self::Key<'_> { + self.sled_id + } + + id_upcast!(); +} + +pub struct VmmStopForUpdate { + datastore: Arc, +} + +impl VmmStopForUpdate { + pub fn new(datastore: Arc) -> Self { + Self { datastore } + } + + /// TODO-K: Fix comment, Lists every non-deleted VMM that is marked to be + /// stopped for a sled update, and groups their IDs by the sled they run on. + async fn vmms_to_stop_by_sled( + &self, + opctx: &OpContext, + ) -> Result, Error> { + let mut vmms_by_sled = IdOrdMap::new(); + let mut paginator = Paginator::new( + SQL_BATCH_SIZE, + dropshot::PaginationOrder::Ascending, + ); + while let Some(p) = paginator.next() { + let vmms = self + .datastore + .vmm_list_marked_stop_for_update(opctx, &p.current_pagparams()) + .await?; + paginator = p.found_batch(&vmms, &|vmm| vmm.id); + for vmm in vmms { + let sled_id = vmm.sled_id(); + vmms_by_sled + .entry(sled_id) + .or_insert_with(|| SledVmmsToStop { + sled_id, + vmm_ids: Vec::new(), + }) + .vmm_ids + .push(PropolisUuid::from_untyped_uuid(vmm.id)); + } + } + Ok(vmms_by_sled) + } + + pub(crate) async fn actually_activate( + &mut self, + opctx: &OpContext, + ) -> VmmStopForUpdateStatus { + let vmms_by_sled = match self.vmms_to_stop_by_sled(opctx).await { + Ok(vmms_by_sled) => vmms_by_sled, + Err(err) => { + slog::error!( + &opctx.log, + "failed to list VMMs marked to stop for a sled update"; + &err, + ); + return VmmStopForUpdateStatus { + vmms_stopped_by_sled: BTreeMap::new(), + error: Some(InlineErrorChain::new(&err).to_string()), + }; + } + }; + + if vmms_by_sled.is_empty() { + slog::debug!( + &opctx.log, + "no VMMs are marked to stop for a sled update"; + ); + } else { + let vmm_count: usize = + vmms_by_sled.iter().map(|sled| sled.vmm_ids.len()).sum(); + slog::info!( + &opctx.log, + "found VMMs marked to stop for a sled update"; + "sleds" => vmms_by_sled.len(), + "vmms" => vmm_count, + ); + } + + // TODO-K: actually stop the VMMs in `vmms_by_sled` and record + // how many were stopped on each sled in `vmms_stopped_by_sled`. + let vmms_stopped_by_sled = BTreeMap::new(); + + VmmStopForUpdateStatus { vmms_stopped_by_sled, error: None } + } +} + +impl BackgroundTask for VmmStopForUpdate { + fn activate<'a>( + &'a mut self, + opctx: &'a OpContext, + ) -> BoxFuture<'a, serde_json::Value> { + Box::pin(async { + let status = self.actually_activate(opctx).await; + match serde_json::to_value(status) { + Ok(val) => val, + Err(err) => { + json!({ "error": format!("failed to serialize status: {err}") }) + } + } + }) + } +} diff --git a/nexus/tests/config.test.toml b/nexus/tests/config.test.toml index 40237b99f63..ff3d8600a24 100644 --- a/nexus/tests/config.test.toml +++ b/nexus/tests/config.test.toml @@ -151,6 +151,7 @@ instance_watcher.period_secs = 30 service_firewall_propagation.period_secs = 300 v2p_mapping_propagation.period_secs = 30 abandoned_vmm_reaper.period_secs = 60 +vmm_stop_for_update.period_secs = 300 saga_recovery.period_secs = 600 lookup_region_port.period_secs = 60 # The purpose of the `instance-updater` background task is to ensure that update diff --git a/nexus/types/src/internal_api/background.rs b/nexus/types/src/internal_api/background.rs index 20e1f38c366..1891b8cf935 100644 --- a/nexus/types/src/internal_api/background.rs +++ b/nexus/types/src/internal_api/background.rs @@ -106,6 +106,16 @@ pub struct AbandonedVmmReaperStatus { pub errors: Vec, } +/// The status of a `vmm_stop_for_update` background task activation. +#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Eq)] +pub struct VmmStopForUpdateStatus { + /// Number of VMMs stopped in this activation, keyed by the sled they were + /// running on. + pub vmms_stopped_by_sled: BTreeMap, + /// Error encountered during this activation, if any. + pub error: Option, +} + /// The status of an `instance_updater` background task activation. #[derive(Serialize, Deserialize, Default, Debug, PartialEq, Eq)] pub struct InstanceUpdaterStatus { diff --git a/smf/nexus/multi-sled/config-partial.toml b/smf/nexus/multi-sled/config-partial.toml index 9b59caea431..41385d336ef 100644 --- a/smf/nexus/multi-sled/config-partial.toml +++ b/smf/nexus/multi-sled/config-partial.toml @@ -67,6 +67,8 @@ service_firewall_propagation.period_secs = 300 v2p_mapping_propagation.period_secs = 30 instance_watcher.period_secs = 30 abandoned_vmm_reaper.period_secs = 60 +# TODO-K: double check the `period_secs` values configured for this task. +vmm_stop_for_update.period_secs = 300 saga_recovery.period_secs = 600 lookup_region_port.period_secs = 60 instance_updater.period_secs = 30 diff --git a/smf/nexus/single-sled/config-partial.toml b/smf/nexus/single-sled/config-partial.toml index e9826da7435..27ec93fed09 100644 --- a/smf/nexus/single-sled/config-partial.toml +++ b/smf/nexus/single-sled/config-partial.toml @@ -67,6 +67,8 @@ service_firewall_propagation.period_secs = 300 v2p_mapping_propagation.period_secs = 30 instance_watcher.period_secs = 30 abandoned_vmm_reaper.period_secs = 60 +# TODO-K: double check the `period_secs` values configured for this task. +vmm_stop_for_update.period_secs = 300 saga_recovery.period_secs = 600 lookup_region_port.period_secs = 60 instance_updater.period_secs = 30 From e1f14c323595e00df53be46eb35860acf962e621 Mon Sep 17 00:00:00 2001 From: karencfv Date: Fri, 2 Oct 2026 14:53:58 +1300 Subject: [PATCH 02/44] let's do batches --- .../background/tasks/vmm_stop_for_update.rs | 81 ++++++++++++------- 1 file changed, 53 insertions(+), 28 deletions(-) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 1e78acb68d1..338ac4a933d 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -56,13 +56,9 @@ impl VmmStopForUpdate { Self { datastore } } - /// TODO-K: Fix comment, Lists every non-deleted VMM that is marked to be - /// stopped for a sled update, and groups their IDs by the sled they run on. - async fn vmms_to_stop_by_sled( - &self, - opctx: &OpContext, - ) -> Result, Error> { - let mut vmms_by_sled = IdOrdMap::new(); + /// Retrieves a list of VMMs marked to be stopped for update by sled, and + /// stops them in batches. + async fn stop_all(&self, opctx: &OpContext) -> Result<(), Error> { let mut paginator = Paginator::new( SQL_BATCH_SIZE, dropshot::PaginationOrder::Ascending, @@ -73,6 +69,8 @@ impl VmmStopForUpdate { .vmm_list_marked_stop_for_update(opctx, &p.current_pagparams()) .await?; paginator = p.found_batch(&vmms, &|vmm| vmm.id); + + let mut vmms_by_sled = IdOrdMap::new(); for vmm in vmms { let sled_id = vmm.sled_id(); vmms_by_sled @@ -84,20 +82,47 @@ impl VmmStopForUpdate { .vmm_ids .push(PropolisUuid::from_untyped_uuid(vmm.id)); } + self.stop_batch(vmms_by_sled, opctx).await; } - Ok(vmms_by_sled) + + // TODO-K: Actually return some useful information like how many VMMs + // were stoppped, which sleds these we in, etc + Ok(()) + } + + /// Stops a batch of VMMs + async fn stop_batch( + &self, + vmms_by_sled: IdOrdMap, + opctx: &OpContext, + ) { + for sled in vmms_by_sled { + let SledVmmsToStop { sled_id, vmm_ids } = sled; + slog::info!( + opctx.log, + "Stopping VMMs for update"; + "sled_id" => %sled_id, + "vmms" => ?vmm_ids, + ); + + // TODO-K: actually stop the VMMs in `vmms_by_sled` and record + // how many were stopped on each sled, how many failed, whatever + } + + // TODO-K: Return some useful information like how many VMMs + // were stoppped, which sleds these we in, etc } pub(crate) async fn actually_activate( &mut self, opctx: &OpContext, ) -> VmmStopForUpdateStatus { - let vmms_by_sled = match self.vmms_to_stop_by_sled(opctx).await { - Ok(vmms_by_sled) => vmms_by_sled, + let _results = match self.stop_all(opctx).await { + Ok(results) => results, Err(err) => { slog::error!( &opctx.log, - "failed to list VMMs marked to stop for a sled update"; + "failed to stop VMMs marked to stop for a sled update"; &err, ); return VmmStopForUpdateStatus { @@ -107,24 +132,24 @@ impl VmmStopForUpdate { } }; - if vmms_by_sled.is_empty() { - slog::debug!( - &opctx.log, - "no VMMs are marked to stop for a sled update"; - ); - } else { - let vmm_count: usize = - vmms_by_sled.iter().map(|sled| sled.vmm_ids.len()).sum(); - slog::info!( - &opctx.log, - "found VMMs marked to stop for a sled update"; - "sleds" => vmms_by_sled.len(), - "vmms" => vmm_count, - ); - } + // TODO-K: Once we have useful information log it + // if results.vmms_by_sled.is_empty() { + // slog::debug!( + // &opctx.log, + // "no VMMs were stopped for a sled update"; + // ); + // } else { + // let vmm_count: usize = + // vmms_by_sled.iter().map(|sled| sled.vmm_ids.len()).sum(); + // slog::info!( + // &opctx.log, + // "stopped VMMs marked to stop for a sled update"; + // "sleds" => vmms_by_sled.len(), + // "vmms" => vmm_count, + // ); + // } - // TODO-K: actually stop the VMMs in `vmms_by_sled` and record - // how many were stopped on each sled in `vmms_stopped_by_sled`. + // TODO-K: Should return a nicely structured struct here instead let vmms_stopped_by_sled = BTreeMap::new(); VmmStopForUpdateStatus { vmms_stopped_by_sled, error: None } From 99e577dc60e71353857e4f448c74afe108abe885 Mon Sep 17 00:00:00 2001 From: karencfv Date: Mon, 5 Oct 2026 19:04:32 +1300 Subject: [PATCH 03/44] fix some comments --- nexus/db-queries/src/db/datastore/vmm.rs | 8 ++------ nexus/src/app/background/init.rs | 4 +--- .../src/app/background/tasks/vmm_stop_for_update.rs | 12 +++--------- smf/nexus/multi-sled/config-partial.toml | 4 +++- smf/nexus/single-sled/config-partial.toml | 4 +++- 5 files changed, 12 insertions(+), 20 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/vmm.rs b/nexus/db-queries/src/db/datastore/vmm.rs index 8e2f0ad1884..7d023d8a39b 100644 --- a/nexus/db-queries/src/db/datastore/vmm.rs +++ b/nexus/db-queries/src/db/datastore/vmm.rs @@ -489,10 +489,8 @@ impl DataStore { .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } - /// TODO-K: Fix comment, Lists VMMs that have been marked as needing to be - /// stopped in order to update their sled, that have not been deleted, and - /// that are still in a state from which they need to be stopped. VMMs that - /// are already stopping, stopped, or destroyed are excluded. + /// Lists VMMs that have been marked as needing to be stopped for a sled + /// update. pub async fn vmm_list_marked_stop_for_update( &self, opctx: &OpContext, @@ -504,8 +502,6 @@ impl DataStore { // TODO-K: change to DbVmmState::SHOULD_STOP_FOR_EVACUATION once // that branch is merged .filter(dsl::state.eq_any(&[ - // A VMM in one of these states is on its way, or is already running, - // and can be stopped. DbVmmState::Creating, DbVmmState::Starting, DbVmmState::Running, diff --git a/nexus/src/app/background/init.rs b/nexus/src/app/background/init.rs index 161410bc8d2..a155a55ede7 100644 --- a/nexus/src/app/background/init.rs +++ b/nexus/src/app/background/init.rs @@ -903,9 +903,7 @@ impl BackgroundTasksInitializer { // // TODO-K: This task is meant to be activated by the // `vmm_mark_stop_for_update` task whenever that task marks one or more - // VMMs. Once that task lands, pass `task_vmm_stop_for_update.clone()` - // to `VmmMarkStopForUpdate::new()` and have it call `.activate()` when - // it marks at least one VMM. + // VMMs. driver.register(TaskDefinition { name: "vmm_stop_for_update", description: "stops VMMs that are marked to be stopped for a sled \ diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 338ac4a933d..3391f3850a0 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -2,13 +2,8 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -//! TODO-K: Fix comment, Background task that stops the VMMs that have been -//! marked as needing to be stopped in order to update their sled. -//! -//! TODO-K: Fix comment, VMMs are marked (via -//! `stop_for_update_disposition_generation` on the `vmm` table) by the -//! `vmm_mark_stop_for_update` background task, which activates this task -//! whenever it marks one or more VMMs. See RFD 739. +//! Background task that stops VMMs that have been marked as needing to be +//! stopped for a sled update. use crate::app::background::BackgroundTask; use futures::future::BoxFuture; @@ -29,8 +24,7 @@ use slog_error_chain::InlineErrorChain; use std::collections::BTreeMap; use std::sync::Arc; -/// The VMMs on a single sled that are marked to be stopped in order to update -/// that sled. +/// The VMMs on a single sled that are marked to be stopped for a sled update #[derive(Clone, Debug)] struct SledVmmsToStop { sled_id: SledUuid, diff --git a/smf/nexus/multi-sled/config-partial.toml b/smf/nexus/multi-sled/config-partial.toml index 41385d336ef..4c7a577a3f9 100644 --- a/smf/nexus/multi-sled/config-partial.toml +++ b/smf/nexus/multi-sled/config-partial.toml @@ -67,7 +67,9 @@ service_firewall_propagation.period_secs = 300 v2p_mapping_propagation.period_secs = 30 instance_watcher.period_secs = 30 abandoned_vmm_reaper.period_secs = 60 -# TODO-K: double check the `period_secs` values configured for this task. +# The task that marks vmms as needing to be stopped runs every 300s. Therefore, +# this task doesn't need to run more often. In addition, every time that task +# runs, this task is also activated. vmm_stop_for_update.period_secs = 300 saga_recovery.period_secs = 600 lookup_region_port.period_secs = 60 diff --git a/smf/nexus/single-sled/config-partial.toml b/smf/nexus/single-sled/config-partial.toml index 27ec93fed09..5122fe83309 100644 --- a/smf/nexus/single-sled/config-partial.toml +++ b/smf/nexus/single-sled/config-partial.toml @@ -67,7 +67,9 @@ service_firewall_propagation.period_secs = 300 v2p_mapping_propagation.period_secs = 30 instance_watcher.period_secs = 30 abandoned_vmm_reaper.period_secs = 60 -# TODO-K: double check the `period_secs` values configured for this task. +# The task that marks vmms as needing to be stopped runs every 300s. Therefore, +# this task doesn't need to run more often. In addition, every time that task +# runs, this task is also activated. vmm_stop_for_update.period_secs = 300 saga_recovery.period_secs = 600 lookup_region_port.period_secs = 60 From cb3ea3480aace0c6ff45e9116529271fb3145ad6 Mon Sep 17 00:00:00 2001 From: karencfv Date: Mon, 5 Oct 2026 20:32:55 +1300 Subject: [PATCH 04/44] use IdOrdMap everywhere --- dev-tools/omdb/src/bin/omdb/nexus.rs | 32 +++++++++++--- .../background/tasks/vmm_stop_for_update.rs | 44 +++++++------------ nexus/types/src/internal_api/background.rs | 23 +++++++++- smf/nexus/multi-sled/config-partial.toml | 4 +- smf/nexus/single-sled/config-partial.toml | 4 +- 5 files changed, 69 insertions(+), 38 deletions(-) diff --git a/dev-tools/omdb/src/bin/omdb/nexus.rs b/dev-tools/omdb/src/bin/omdb/nexus.rs index b82bb92f09e..2c3bec6710d 100644 --- a/dev-tools/omdb/src/bin/omdb/nexus.rs +++ b/dev-tools/omdb/src/bin/omdb/nexus.rs @@ -1517,15 +1517,37 @@ fn print_task_vmm_stop_for_update(details: &serde_json::Value) { "warning: failed to interpret task details: {:?}: {:?}", error, details ), - Ok(VmmStopForUpdateStatus { vmms_stopped_by_sled, error }) => { - const STOPPED: &str = "VMMs stopped:"; + Ok(VmmStopForUpdateStatus { + vmms_stopped_by_sled, + vmms_failed_by_sled, + error, + }) => { + const STOPPED: &str = "VMMs stopped for sled evacuation:"; const ERROR: &str = "error:"; const WIDTH: usize = const_max_len(&[STOPPED, ERROR]) + 1; - let total: usize = vmms_stopped_by_sled.values().sum(); + let total: usize = + vmms_stopped_by_sled.iter().map(|s| s.vmm_ids.len()).sum(); println!(" {STOPPED:, -} - -impl IdOrdItem for SledVmmsToStop { - type Key<'a> = SledUuid; - - fn key(&self) -> Self::Key<'_> { - self.sled_id - } - - id_upcast!(); -} - pub struct VmmStopForUpdate { datastore: Arc, } @@ -69,7 +49,7 @@ impl VmmStopForUpdate { let sled_id = vmm.sled_id(); vmms_by_sled .entry(sled_id) - .or_insert_with(|| SledVmmsToStop { + .or_insert_with(|| VmmsBySled { sled_id, vmm_ids: Vec::new(), }) @@ -87,11 +67,11 @@ impl VmmStopForUpdate { /// Stops a batch of VMMs async fn stop_batch( &self, - vmms_by_sled: IdOrdMap, + vmms_by_sled: IdOrdMap, opctx: &OpContext, ) { for sled in vmms_by_sled { - let SledVmmsToStop { sled_id, vmm_ids } = sled; + let VmmsBySled { sled_id, vmm_ids } = sled; slog::info!( opctx.log, "Stopping VMMs for update"; @@ -114,14 +94,16 @@ impl VmmStopForUpdate { let _results = match self.stop_all(opctx).await { Ok(results) => results, Err(err) => { + let err = InlineErrorChain::new(&err); slog::error!( &opctx.log, "failed to stop VMMs marked to stop for a sled update"; &err, ); return VmmStopForUpdateStatus { - vmms_stopped_by_sled: BTreeMap::new(), - error: Some(InlineErrorChain::new(&err).to_string()), + vmms_stopped_by_sled: IdOrdMap::new(), + vmms_failed_by_sled: IdOrdMap::new(), + error: Some(err.to_string()), }; } }; @@ -141,12 +123,18 @@ impl VmmStopForUpdate { // "sleds" => vmms_by_sled.len(), // "vmms" => vmm_count, // ); + // TODO-K: for debug show IDs? also show which failed // } // TODO-K: Should return a nicely structured struct here instead - let vmms_stopped_by_sled = BTreeMap::new(); + let vmms_stopped_by_sled = IdOrdMap::new(); + let vmms_failed_by_sled = IdOrdMap::new(); - VmmStopForUpdateStatus { vmms_stopped_by_sled, error: None } + VmmStopForUpdateStatus { + vmms_stopped_by_sled, + vmms_failed_by_sled, + error: None, + } } } diff --git a/nexus/types/src/internal_api/background.rs b/nexus/types/src/internal_api/background.rs index 1891b8cf935..8bdf568055d 100644 --- a/nexus/types/src/internal_api/background.rs +++ b/nexus/types/src/internal_api/background.rs @@ -15,6 +15,7 @@ use omicron_uuid_kinds::AlertReceiverUuid; use omicron_uuid_kinds::AlertUuid; use omicron_uuid_kinds::BlueprintUuid; use omicron_uuid_kinds::CollectionUuid; +use omicron_uuid_kinds::PropolisUuid; use omicron_uuid_kinds::RackUuid; use omicron_uuid_kinds::SitrepUuid; use omicron_uuid_kinds::SledUuid; @@ -106,12 +107,32 @@ pub struct AbandonedVmmReaperStatus { pub errors: Vec, } +/// VMMs on a single sled +#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Eq)] +pub struct VmmsBySled { + pub sled_id: SledUuid, + pub vmm_ids: Vec, +} + +impl IdOrdItem for VmmsBySled { + type Key<'a> = SledUuid; + + fn key(&self) -> Self::Key<'_> { + self.sled_id + } + + id_upcast!(); +} + /// The status of a `vmm_stop_for_update` background task activation. #[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Eq)] pub struct VmmStopForUpdateStatus { /// Number of VMMs stopped in this activation, keyed by the sled they were /// running on. - pub vmms_stopped_by_sled: BTreeMap, + pub vmms_stopped_by_sled: IdOrdMap, + /// Number of VMMs that failed to stop in this activation, keyed by the sled + /// they are running on. + pub vmms_failed_by_sled: IdOrdMap, /// Error encountered during this activation, if any. pub error: Option, } diff --git a/smf/nexus/multi-sled/config-partial.toml b/smf/nexus/multi-sled/config-partial.toml index 4c7a577a3f9..bb22adecdb9 100644 --- a/smf/nexus/multi-sled/config-partial.toml +++ b/smf/nexus/multi-sled/config-partial.toml @@ -67,9 +67,9 @@ service_firewall_propagation.period_secs = 300 v2p_mapping_propagation.period_secs = 30 instance_watcher.period_secs = 30 abandoned_vmm_reaper.period_secs = 60 -# The task that marks vmms as needing to be stopped runs every 300s. Therefore, +# The task that marks VMMs as needing to be stopped runs every 300s. Therefore, # this task doesn't need to run more often. In addition, every time that task -# runs, this task is also activated. +# runs and marks any VMMs, this task is also activated. vmm_stop_for_update.period_secs = 300 saga_recovery.period_secs = 600 lookup_region_port.period_secs = 60 diff --git a/smf/nexus/single-sled/config-partial.toml b/smf/nexus/single-sled/config-partial.toml index 5122fe83309..91d6f22e7ae 100644 --- a/smf/nexus/single-sled/config-partial.toml +++ b/smf/nexus/single-sled/config-partial.toml @@ -67,9 +67,9 @@ service_firewall_propagation.period_secs = 300 v2p_mapping_propagation.period_secs = 30 instance_watcher.period_secs = 30 abandoned_vmm_reaper.period_secs = 60 -# The task that marks vmms as needing to be stopped runs every 300s. Therefore, +# The task that marks VMMs as needing to be stopped runs every 300s. Therefore, # this task doesn't need to run more often. In addition, every time that task -# runs, this task is also activated. +# runs and marks any VMMs, this task is also activated. vmm_stop_for_update.period_secs = 300 saga_recovery.period_secs = 600 lookup_region_port.period_secs = 60 From b1a9a191f812277f05c38fe0d98d81965db844da Mon Sep 17 00:00:00 2001 From: karencfv Date: Mon, 5 Oct 2026 21:26:52 +1300 Subject: [PATCH 05/44] have nicer return types --- dev-tools/omdb/src/bin/omdb/nexus.rs | 11 +- .../background/tasks/vmm_stop_for_update.rs | 112 ++++++++++++------ nexus/types/src/internal_api/background.rs | 4 +- 3 files changed, 89 insertions(+), 38 deletions(-) diff --git a/dev-tools/omdb/src/bin/omdb/nexus.rs b/dev-tools/omdb/src/bin/omdb/nexus.rs index 2c3bec6710d..7691d00d345 100644 --- a/dev-tools/omdb/src/bin/omdb/nexus.rs +++ b/dev-tools/omdb/src/bin/omdb/nexus.rs @@ -1520,7 +1520,7 @@ fn print_task_vmm_stop_for_update(details: &serde_json::Value) { Ok(VmmStopForUpdateStatus { vmms_stopped_by_sled, vmms_failed_by_sled, - error, + error_messages, }) => { const STOPPED: &str = "VMMs stopped for sled evacuation:"; const ERROR: &str = "error:"; @@ -1549,8 +1549,13 @@ fn print_task_vmm_stop_for_update(details: &serde_json::Value) { } } } - if let Some(error) = error { - println!(" {ERROR:, + vmms_failed_by_sled: IdOrdMap, + errors: Vec, +} + +impl VmmStopForUpdateResult { + fn new() -> Self { + Self { + vmms_stopped_by_sled: IdOrdMap::new(), + vmms_failed_by_sled: IdOrdMap::new(), + errors: Vec::new(), + } + } + + /// Merges the values of `self` with a given [`VmmStopForUpdateResult`] + fn merge(&mut self, other: VmmStopForUpdateResult) { + let VmmStopForUpdateResult { + vmms_stopped_by_sled, + vmms_failed_by_sled, + errors, + } = other; + + for VmmsBySled { sled_id, vmm_ids } in vmms_stopped_by_sled { + self.vmms_stopped_by_sled + .entry(sled_id) + .or_insert_with(|| VmmsBySled { sled_id, vmm_ids: Vec::new() }) + .vmm_ids + .extend(vmm_ids); + } + for VmmsBySled { sled_id, vmm_ids } in vmms_failed_by_sled { + self.vmms_failed_by_sled + .entry(sled_id) + .or_insert_with(|| VmmsBySled { sled_id, vmm_ids: Vec::new() }) + .vmm_ids + .extend(vmm_ids); + } + self.errors.extend(errors); + } +} + pub struct VmmStopForUpdate { datastore: Arc, } @@ -32,16 +73,26 @@ impl VmmStopForUpdate { /// Retrieves a list of VMMs marked to be stopped for update by sled, and /// stops them in batches. - async fn stop_all(&self, opctx: &OpContext) -> Result<(), Error> { + async fn stop_all(&self, opctx: &OpContext) -> VmmStopForUpdateResult { + let mut result = VmmStopForUpdateResult::new(); let mut paginator = Paginator::new( SQL_BATCH_SIZE, dropshot::PaginationOrder::Ascending, ); while let Some(p) = paginator.next() { - let vmms = self + let vmms = match self .datastore .vmm_list_marked_stop_for_update(opctx, &p.current_pagparams()) - .await?; + .await + { + Ok(vmms) => vmms, + Err(err) => { + // TODO-K: Verify how the paginator actually works. Should + // I break here or not? + result.errors.push(err); + break; + } + }; paginator = p.found_batch(&vmms, &|vmm| vmm.id); let mut vmms_by_sled = IdOrdMap::new(); @@ -56,12 +107,11 @@ impl VmmStopForUpdate { .vmm_ids .push(PropolisUuid::from_untyped_uuid(vmm.id)); } - self.stop_batch(vmms_by_sled, opctx).await; + let batch_result = self.stop_batch(vmms_by_sled, opctx).await; + result.merge(batch_result); } - // TODO-K: Actually return some useful information like how many VMMs - // were stoppped, which sleds these we in, etc - Ok(()) + result } /// Stops a batch of VMMs @@ -69,44 +119,36 @@ impl VmmStopForUpdate { &self, vmms_by_sled: IdOrdMap, opctx: &OpContext, - ) { - for sled in vmms_by_sled { - let VmmsBySled { sled_id, vmm_ids } = sled; + ) -> VmmStopForUpdateResult { + for VmmsBySled { sled_id, vmm_ids } in &vmms_by_sled { slog::info!( opctx.log, "Stopping VMMs for update"; "sled_id" => %sled_id, "vmms" => ?vmm_ids, ); - // TODO-K: actually stop the VMMs in `vmms_by_sled` and record // how many were stopped on each sled, how many failed, whatever } - // TODO-K: Return some useful information like how many VMMs - // were stoppped, which sleds these we in, etc + // TODO-K: actually return the results of the stopped vmms + VmmStopForUpdateResult::new() } pub(crate) async fn actually_activate( &mut self, opctx: &OpContext, ) -> VmmStopForUpdateStatus { - let _results = match self.stop_all(opctx).await { - Ok(results) => results, - Err(err) => { - let err = InlineErrorChain::new(&err); - slog::error!( - &opctx.log, - "failed to stop VMMs marked to stop for a sled update"; - &err, - ); - return VmmStopForUpdateStatus { - vmms_stopped_by_sled: IdOrdMap::new(), - vmms_failed_by_sled: IdOrdMap::new(), - error: Some(err.to_string()), - }; - } - }; + let VmmStopForUpdateResult { + vmms_stopped_by_sled, + vmms_failed_by_sled, + errors, + } = self.stop_all(opctx).await; + + let error_messages: Vec = errors + .iter() + .map(|err| InlineErrorChain::new(err).to_string()) + .collect(); // TODO-K: Once we have useful information log it // if results.vmms_by_sled.is_empty() { @@ -126,14 +168,18 @@ impl VmmStopForUpdate { // TODO-K: for debug show IDs? also show which failed // } - // TODO-K: Should return a nicely structured struct here instead - let vmms_stopped_by_sled = IdOrdMap::new(); - let vmms_failed_by_sled = IdOrdMap::new(); + if !error_messages.is_empty() { + slog::error!( + &opctx.log, + "failed to stop VMMs marked to stop for a sled update"; + "errors" => ?error_messages, + ); + } VmmStopForUpdateStatus { vmms_stopped_by_sled, vmms_failed_by_sled, - error: None, + error_messages, } } } diff --git a/nexus/types/src/internal_api/background.rs b/nexus/types/src/internal_api/background.rs index 8bdf568055d..be20eae2b9c 100644 --- a/nexus/types/src/internal_api/background.rs +++ b/nexus/types/src/internal_api/background.rs @@ -133,8 +133,8 @@ pub struct VmmStopForUpdateStatus { /// Number of VMMs that failed to stop in this activation, keyed by the sled /// they are running on. pub vmms_failed_by_sled: IdOrdMap, - /// Error encountered during this activation, if any. - pub error: Option, + /// Errors encountered during this activation, if any. + pub error_messages: Vec, } /// The status of an `instance_updater` background task activation. From 055b6b09efbd637e677a46cb74dac95c8ff2689f Mon Sep 17 00:00:00 2001 From: karencfv Date: Mon, 5 Oct 2026 21:50:33 +1300 Subject: [PATCH 06/44] pondering... --- nexus/src/app/instance.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/nexus/src/app/instance.rs b/nexus/src/app/instance.rs index b1fef90dc12..a66ba501dc8 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -1421,6 +1421,7 @@ impl super::Nexus { } } + // TODO-K: Do something like this to stop? Most likely yes I think InstanceStateChangeRequestAction::SendToSled { sled_id, propolis_id, @@ -1815,6 +1816,8 @@ impl super::Nexus { "reason" => %reason, ); + // TODO-K: Do something like this to stop? Probably not, I'd have to + // retrieve the entire VMM instead of just the ids let new_runtime = vmm .runtime() .transition(nexus_types::instance::VmmState::Failed(reason.into())); From a8f3e4e917e26d269051300680dba32c015e983b Mon Sep 17 00:00:00 2001 From: karencfv Date: Tue, 6 Oct 2026 20:20:59 +1300 Subject: [PATCH 07/44] Do the stopping and add a bunch of random thoughts --- nexus/db-model/src/instance.rs | 3 + nexus/db-model/src/vmm.rs | 1 + nexus/db-queries/src/db/datastore/instance.rs | 3 +- nexus/db-queries/src/db/datastore/vmm.rs | 3 + .../background/tasks/vmm_stop_for_update.rs | 140 +++++++++++++++++- nexus/src/app/sagas/instance_update/mod.rs | 5 + sled-agent/src/common/instance.rs | 1 + sled-agent/src/instance.rs | 2 + 8 files changed, 153 insertions(+), 5 deletions(-) diff --git a/nexus/db-model/src/instance.rs b/nexus/db-model/src/instance.rs index b7298886736..d1b9a1ff34f 100644 --- a/nexus/db-model/src/instance.rs +++ b/nexus/db-model/src/instance.rs @@ -521,6 +521,9 @@ impl InstanceAutoRestart { let now = diesel::dsl::now.into_sql::(); + // TODO-K: Do we want to restart instances we are evacuating regardless + // of the auto-restart policy??? + // The instance's auto-restart policy must allow the control plane // to restart it automatically. // diff --git a/nexus/db-model/src/vmm.rs b/nexus/db-model/src/vmm.rs index a35ca30768f..2f5c72ae10c 100644 --- a/nexus/db-model/src/vmm.rs +++ b/nexus/db-model/src/vmm.rs @@ -104,6 +104,7 @@ impl Vmm { /// /// The new VMM record will be in [`VmmState::Creating`] until it is /// registered with a sled-agent. + // TODO-K: Is the above something we care about here? pub fn new( id: PropolisUuid, instance_id: InstanceUuid, diff --git a/nexus/db-queries/src/db/datastore/instance.rs b/nexus/db-queries/src/db/datastore/instance.rs index 8065f606918..a6637a5ecc2 100644 --- a/nexus/db-queries/src/db/datastore/instance.rs +++ b/nexus/db-queries/src/db/datastore/instance.rs @@ -463,7 +463,8 @@ impl DataStore { &*self.pool_connection_authorized(opctx).await?, ) .await - } + } // TODO-K: Add a stopped for update reason, so the instances we + // stopped actually get restarted } .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } diff --git a/nexus/db-queries/src/db/datastore/vmm.rs b/nexus/db-queries/src/db/datastore/vmm.rs index 7d023d8a39b..e5632a4bc7d 100644 --- a/nexus/db-queries/src/db/datastore/vmm.rs +++ b/nexus/db-queries/src/db/datastore/vmm.rs @@ -502,6 +502,9 @@ impl DataStore { // TODO-K: change to DbVmmState::SHOULD_STOP_FOR_EVACUATION once // that branch is merged .filter(dsl::state.eq_any(&[ + // TODO-K: Caveat that a VMM in Creating state will not be + // registered with a sled agent yet. Does this affect what we're + // doing here? DbVmmState::Creating, DbVmmState::Starting, DbVmmState::Running, diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 8da0d98e522..b4b23922659 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -6,6 +6,7 @@ //! stopped for a sled update. use crate::app::background::BackgroundTask; +use crate::app::instance::SledAgentInstanceError; use futures::future::BoxFuture; use iddqd::IdOrdMap; use nexus_db_queries::context::OpContext; @@ -18,6 +19,11 @@ use omicron_common::api::external::Error; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::PropolisUuid; use serde_json::json; +use sled_agent_client::types::VmmPutStateBody; +use sled_agent_client::types::VmmStateRequested; +use sled_agent_types::instance::SledVmmState; +use sled_agent_types::instance::VmmRuntimeState; +use sled_agent_types::instance::VmmState; use slog_error_chain::InlineErrorChain; use std::sync::Arc; @@ -120,6 +126,8 @@ impl VmmStopForUpdate { vmms_by_sled: IdOrdMap, opctx: &OpContext, ) -> VmmStopForUpdateResult { + let mut result = VmmStopForUpdateResult::new(); + for VmmsBySled { sled_id, vmm_ids } in &vmms_by_sled { slog::info!( opctx.log, @@ -127,12 +135,136 @@ impl VmmStopForUpdate { "sled_id" => %sled_id, "vmms" => ?vmm_ids, ); - // TODO-K: actually stop the VMMs in `vmms_by_sled` and record - // how many were stopped on each sled, how many failed, whatever + + // Since we're using IdOrdMap, each sled id will only appear once. + // Because of this it's not a big deal to start a new sled agent + // client per sled id. + let sled_client = match nexus_networking::sled_client( + &self.datastore, + &opctx, + *sled_id, + &opctx.log, + ) + .await + { + Ok(client) => client, + Err(e) => { + slog::error!( + opctx.log, + "Failed to create sled agent client"; + "sled_id" => %sled_id, + InlineErrorChain::new(&e), + ); + + result.errors.push(e.clone()); + + continue; + } + }; + + for id in vmm_ids { + let response = match sled_client + .vmm_put_state( + id, + &VmmPutStateBody { state: VmmStateRequested::Stopped }, + ) + .await + { + Ok(res) => res.into_inner().updated_runtime, + Err(e) => { + let err = SledAgentInstanceError(e); + + slog::error!( + opctx.log, + "Failed to mark VMM as Stopped"; + "sled_id" => %sled_id, + "vmm_id" => %id, + InlineErrorChain::new(&err), + ); + + result.errors.push(err.into()); + result + .vmms_failed_by_sled + .entry(*sled_id) + .or_insert_with(|| VmmsBySled { + sled_id: *sled_id, + vmm_ids: Vec::new(), + }) + .vmm_ids + .push(*id); + + continue; + } + }; + + match response { + None => { + slog::debug!(opctx.log, "VMM already stopped, no changes to state"; "sled_id" => %sled_id, + "vmm_id" => %id); + } + Some(vmm) => { + let SledVmmState { + vmm_state, + migration_in: _, + migration_out: _, + } = vmm; + let VmmRuntimeState { state, generation, time_updated } = + vmm_state; + + match state { + // TODO-K: A VMM could have been set to stopped and + // then propolis destroyed it because another nexus + // told it to? Is this even possible? Does it + // matter? Check my claims here + VmmState::Stopped + | VmmState::Stopping + | VmmState::Destroyed + | VmmState::Failed => { + // TODO-K: Success! + result + .vmms_stopped_by_sled + .entry(*sled_id) + .or_insert_with(|| VmmsBySled { + sled_id: *sled_id, + vmm_ids: Vec::new(), + }) + .vmm_ids + .push(*id); + + slog::debug!( + opctx.log, + "Stopped VMM for sled evacuation during \ + an update"; + "sled_id" => %sled_id, + "vmm_id" => %id, + "vmm_generation" => ?generation, + "time_updated" => ?time_updated, + ); + } + VmmState::Migrating + | VmmState::Rebooting + | VmmState::Starting + | VmmState::Running => { + // TODO-K: Bollocks + + slog::error!( + opctx.log, + "Failed to mark VMM as Stopped, \ + state did not change"; + "sled_id" => %sled_id, + "vmm_id" => %id, + "state" => ?state, + "vmm_generation" => ?generation, + "time_updated" => ?time_updated, + ); + } + } + } + } + } } - // TODO-K: actually return the results of the stopped vmms - VmmStopForUpdateResult::new() + result } pub(crate) async fn actually_activate( diff --git a/nexus/src/app/sagas/instance_update/mod.rs b/nexus/src/app/sagas/instance_update/mod.rs index 4d99e6592a4..d3c64a54dc1 100644 --- a/nexus/src/app/sagas/instance_update/mod.rs +++ b/nexus/src/app/sagas/instance_update/mod.rs @@ -568,6 +568,11 @@ impl UpdatesRequired { == InstanceIntendedState::Running && snapshot.migration.is_none() { + // TODO-K: We definitely don't want the + // InstanceIntendedState to change if we're stopping for + // an update. We should make sure it doesn't. The best + // way to do this is to use the marker + // Did the active VMM shut itself down, when it was // intended to be running *and* we were not migrating // out? If so, update the instance's intended state to diff --git a/sled-agent/src/common/instance.rs b/sled-agent/src/common/instance.rs index 74faecdbddc..82c21ff40a3 100644 --- a/sled-agent/src/common/instance.rs +++ b/sled-agent/src/common/instance.rs @@ -45,6 +45,7 @@ impl From for VmmState { // perspective. State::Creating | State::Starting => VmmState::Starting, State::Running => VmmState::Running, + // TODO-K: Here are some notes about how the VMM lifecycle works State::Stopping => VmmState::Stopping, // A Propolis that is stopped but not yet destroyed should still // appear to be Stopping from an external API perspective, since diff --git a/sled-agent/src/instance.rs b/sled-agent/src/instance.rs index 97531fb4ab0..30c55465bb8 100644 --- a/sled-agent/src/instance.rs +++ b/sled-agent/src/instance.rs @@ -613,6 +613,7 @@ impl InstanceRunner { let mut state_owner = VmmStateOwner::Runner; + // TODO-K: Part of the VMM stop lifecycle // Timeout for stopping the instance gracefully. // // When we send Propolis a put-state request to transition to @@ -2248,6 +2249,7 @@ impl InstanceRunner { (Some(PropolisRequest::Run), None) } VmmStateRequested::Stopped => { + // TODO-K: More context about VMM lifecycle // If there's no running Propolis yet, unregister this instance // straightaway. Since nothing will ever send a state update in // this case, force the VMM into a terminal Destroyed state, From f4fea86a53faba2d872ae44b4bdc720cc077d141 Mon Sep 17 00:00:00 2001 From: karencfv Date: Wed, 7 Oct 2026 15:19:36 +1300 Subject: [PATCH 08/44] integrate with vmm marking bg task --- nexus/db-model/src/instance.rs | 6 ++- nexus/db-queries/src/db/datastore/instance.rs | 3 +- nexus/db-queries/src/db/datastore/vmm.rs | 12 +----- nexus/src/app/background/init.rs | 41 ++++++++++--------- .../tasks/vmm_mark_stop_for_update.rs | 12 +++++- nexus/types/src/internal_api/background.rs | 4 ++ 6 files changed, 42 insertions(+), 36 deletions(-) diff --git a/nexus/db-model/src/instance.rs b/nexus/db-model/src/instance.rs index d1b9a1ff34f..bd652c0a74f 100644 --- a/nexus/db-model/src/instance.rs +++ b/nexus/db-model/src/instance.rs @@ -521,8 +521,10 @@ impl InstanceAutoRestart { let now = diesel::dsl::now.into_sql::(); - // TODO-K: Do we want to restart instances we are evacuating regardless - // of the auto-restart policy??? + // TODO-K: For a follow up PR. Do we want to restart instances we are + // evacuating regardless of the auto-restart policy? + // + // Tracking issue: https://github.com/oxidecomputer/omicron/issues/11169 // The instance's auto-restart policy must allow the control plane // to restart it automatically. diff --git a/nexus/db-queries/src/db/datastore/instance.rs b/nexus/db-queries/src/db/datastore/instance.rs index a6637a5ecc2..8065f606918 100644 --- a/nexus/db-queries/src/db/datastore/instance.rs +++ b/nexus/db-queries/src/db/datastore/instance.rs @@ -463,8 +463,7 @@ impl DataStore { &*self.pool_connection_authorized(opctx).await?, ) .await - } // TODO-K: Add a stopped for update reason, so the instances we - // stopped actually get restarted + } } .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } diff --git a/nexus/db-queries/src/db/datastore/vmm.rs b/nexus/db-queries/src/db/datastore/vmm.rs index 84514a15213..b9749caf91a 100644 --- a/nexus/db-queries/src/db/datastore/vmm.rs +++ b/nexus/db-queries/src/db/datastore/vmm.rs @@ -580,17 +580,7 @@ impl DataStore { paginated(dsl::vmm, dsl::id, pagparams) .filter(dsl::time_deleted.is_null()) .filter(dsl::stop_for_update_disposition_generation.is_not_null()) - // TODO-K: change to DbVmmState::SHOULD_STOP_FOR_EVACUATION once - // that branch is merged - .filter(dsl::state.eq_any(&[ - // TODO-K: Caveat that a VMM in Creating state will not be - // registered with a sled agent yet. Does this affect what we're - // doing here? - DbVmmState::Creating, - DbVmmState::Starting, - DbVmmState::Running, - DbVmmState::Rebooting, - ])) + .filter(dsl::state.eq_any(DbVmmState::SHOULD_STOP_FOR_EVACUATION)) .select(Vmm::as_select()) .load_async(&*self.pool_connection_authorized(opctx).await?) .await diff --git a/nexus/src/app/background/init.rs b/nexus/src/app/background/init.rs index 9a36657e770..14ce6599624 100644 --- a/nexus/src/app/background/init.rs +++ b/nexus/src/app/background/init.rs @@ -903,25 +903,6 @@ impl BackgroundTasksInitializer { activator: task_abandoned_vmm_reaper, }); - // Background task: stop VMMs that are marked to be stopped for a sled - // update. - // - // TODO-K: This task is meant to be activated by the - // `vmm_mark_stop_for_update` task whenever that task marks one or more - // VMMs. - driver.register(TaskDefinition { - name: "vmm_stop_for_update", - description: "stops VMMs that are marked to be stopped for a sled \ - update", - period: config.vmm_stop_for_update.period_secs, - task_impl: Box::new(vmm_stop_for_update::VmmStopForUpdate::new( - datastore.clone(), - )), - opctx: opctx.child(BTreeMap::new()), - watchers: vec![], - activator: task_vmm_stop_for_update, - }); - // Background task: saga recovery { let task_impl = Box::new(saga_recovery::SagaRecovery::new( @@ -1363,12 +1344,16 @@ impl BackgroundTasksInitializer { activator: task_populate_switch_ports, }); + let vmm_mark_stop_for_update = + VmmMarkStopForUpdate::new(datastore.clone()); + let vmm_mark_stop_for_update_watcher = + vmm_mark_stop_for_update.watcher(); driver.register(TaskDefinition { name: "vmm_mark_stop_for_update", description: "marks VMMs on evacuating sleds as needing to be \ stopped for an update", period: config.vmm_mark_stop_for_update.period_secs, - task_impl: Box::new(VmmMarkStopForUpdate::new(datastore)), + task_impl: Box::new(vmm_mark_stop_for_update), opctx: opctx.child(BTreeMap::new()), // We activate this task any time the blueprint_rendezvous task runs. // We want to err on the side of running this task more often than @@ -1378,6 +1363,22 @@ impl BackgroundTasksInitializer { activator: task_vmm_mark_stop_for_update, }); + driver.register(TaskDefinition { + name: "vmm_stop_for_update", + description: "stops VMMs that are marked to be stopped for a sled \ + update", + period: config.vmm_stop_for_update.period_secs, + task_impl: Box::new(vmm_stop_for_update::VmmStopForUpdate::new( + datastore, + )), + opctx: opctx.child(BTreeMap::new()), + // Like the vmm_mark_stop_for_update task, this task will be + // activated each time that task runs regardless of the outcome. + // We do this in the spirit of futureproofing. + watchers: vec![Box::new(vmm_mark_stop_for_update_watcher)], + activator: task_vmm_stop_for_update, + }); + driver } } diff --git a/nexus/src/app/background/tasks/vmm_mark_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_mark_stop_for_update.rs index 02469495bfc..9e186d55c6f 100644 --- a/nexus/src/app/background/tasks/vmm_mark_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_mark_stop_for_update.rs @@ -21,14 +21,21 @@ use nexus_types::internal_api::background::VmmMarkStopForUpdateStatus; use serde_json::json; use slog_error_chain::InlineErrorChain; use std::sync::Arc; +use tokio::sync::watch; pub struct VmmMarkStopForUpdate { datastore: Arc, + tx: watch::Sender, } impl VmmMarkStopForUpdate { pub fn new(datastore: Arc) -> Self { - Self { datastore } + let (tx, _rx) = watch::channel(0); + Self { datastore, tx } + } + + pub fn watcher(&self) -> watch::Receiver { + self.tx.subscribe() } pub(crate) async fn actually_activate( @@ -68,6 +75,9 @@ impl VmmMarkStopForUpdate { ); } + // Trigger anything waiting on this task to run. + self.tx.send_modify(|count| *count = *count + 1); + VmmMarkStopForUpdateStatus { vmms_marked, batches, diff --git a/nexus/types/src/internal_api/background.rs b/nexus/types/src/internal_api/background.rs index ef851ffbb41..75605d33c94 100644 --- a/nexus/types/src/internal_api/background.rs +++ b/nexus/types/src/internal_api/background.rs @@ -260,6 +260,10 @@ pub enum ReincarnationReason { Failed, /// A previous instance-start saga for this instance has failed. SagaUnwound, + // TODO-K: In a follow up PR add a StoppedForUpdate reason, so the instances + // we stopped during sled evacuation actually get restarted + // + // Tracking issue: https://github.com/oxidecomputer/omicron/issues/11169 } impl std::fmt::Display for ReincarnationReason { From a91d7d1da78ed881cbda15d9e8cc3dae9abb8632 Mon Sep 17 00:00:00 2001 From: karencfv Date: Wed, 7 Oct 2026 18:29:04 +1300 Subject: [PATCH 09/44] handle all possible scenarios attempting to stop a VMM --- .../background/tasks/vmm_stop_for_update.rs | 47 +++++++++++++++---- 1 file changed, 37 insertions(+), 10 deletions(-) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index b4b23922659..8984ed2583c 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -212,15 +212,27 @@ impl VmmStopForUpdate { vmm_state; match state { - // TODO-K: A VMM could have been set to stopped and - // then propolis destroyed it because another nexus - // told it to? Is this even possible? Does it - // matter? Check my claims here + // If we succeeded, one of three things happened: + // + // - The VMM has not stopped yet, but propolis has + // accepted the `put_state` request and reports + // the VMM's state as `Stopping`. + // - A propolis zone does not exist yet. In this + // scenario a VMM could have been in the + // `Creating` state but had not reached the point + // where the propolis zone was created. In this + // case the request succeeded, but the VMM is + // reported as `Destroyed`. + // - A VMM should technically never be reported as + // `Stopped` since + // propolis_client::types::InstanceState::Stopped + // is always reported as `VmmState::Stopping`. + // But, if for some reason we get this state, + // it's what we want, so we consider it a + // success. VmmState::Stopped | VmmState::Stopping - | VmmState::Destroyed - | VmmState::Failed => { - // TODO-K: Success! + | VmmState::Destroyed => { result .vmms_stopped_by_sled .entry(*sled_id) @@ -241,16 +253,31 @@ impl VmmStopForUpdate { "time_updated" => ?time_updated, ); } + // 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 => { + slog::info!( + opctx.log, + "Unable to find VMM, possibly due to a \ + restart. No need to stop it"; + "sled_id" => %sled_id, + "vmm_id" => %id, + "state" => ?state, + "vmm_generation" => ?generation, + "time_updated" => ?time_updated, + ); + } VmmState::Migrating | VmmState::Rebooting | VmmState::Starting | VmmState::Running => { - // TODO-K: Bollocks - slog::error!( opctx.log, "Failed to mark VMM as Stopped, \ - state did not change"; + unexpected state reported"; "sled_id" => %sled_id, "vmm_id" => %id, "state" => ?state, From 56d116c47c0f5b5dcde7c239006f1f38bb86ca89 Mon Sep 17 00:00:00 2001 From: karencfv Date: Wed, 7 Oct 2026 19:05:07 +1300 Subject: [PATCH 10/44] Do not change the intended state if we're stopping for an update --- .../app/background/tasks/vmm_stop_for_update.rs | 15 ++++++++++----- nexus/src/app/instance.rs | 3 --- nexus/src/app/sagas/instance_update/mod.rs | 15 +++++++-------- 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 8984ed2583c..de94d30255f 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -93,8 +93,6 @@ impl VmmStopForUpdate { { Ok(vmms) => vmms, Err(err) => { - // TODO-K: Verify how the paginator actually works. Should - // I break here or not? result.errors.push(err); break; } @@ -199,8 +197,12 @@ impl VmmStopForUpdate { match response { None => { - slog::debug!(opctx.log, "VMM already stopped, no changes to state"; "sled_id" => %sled_id, - "vmm_id" => %id); + slog::debug!( + opctx.log, + "VMM already stopped, no changes to state"; + "sled_id" => %sled_id, + "vmm_id" => %id + ); } Some(vmm) => { let SledVmmState { @@ -208,6 +210,7 @@ impl VmmStopForUpdate { migration_in: _, migration_out: _, } = vmm; + let VmmRuntimeState { state, generation, time_updated } = vmm_state; @@ -353,7 +356,9 @@ impl BackgroundTask for VmmStopForUpdate { match serde_json::to_value(status) { Ok(val) => val, Err(err) => { - json!({ "error": format!("failed to serialize status: {err}") }) + json!({ + "error": format!("failed to serialize status: {err}") + }) } } }) diff --git a/nexus/src/app/instance.rs b/nexus/src/app/instance.rs index a66ba501dc8..b1fef90dc12 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -1421,7 +1421,6 @@ impl super::Nexus { } } - // TODO-K: Do something like this to stop? Most likely yes I think InstanceStateChangeRequestAction::SendToSled { sled_id, propolis_id, @@ -1816,8 +1815,6 @@ impl super::Nexus { "reason" => %reason, ); - // TODO-K: Do something like this to stop? Probably not, I'd have to - // retrieve the entire VMM instead of just the ids let new_runtime = vmm .runtime() .transition(nexus_types::instance::VmmState::Failed(reason.into())); diff --git a/nexus/src/app/sagas/instance_update/mod.rs b/nexus/src/app/sagas/instance_update/mod.rs index d3c64a54dc1..444b5a818dc 100644 --- a/nexus/src/app/sagas/instance_update/mod.rs +++ b/nexus/src/app/sagas/instance_update/mod.rs @@ -555,8 +555,8 @@ impl UpdatesRequired { let id = PropolisUuid::from_untyped_uuid(active_vmm.id); // Unlink the active VMM ID. If the active VMM was destroyed // because a migration out completed, the next block, which - // handles migration updates, will set this to the new VMM's ID, - // instead. + // handles migration updates, will set this to the new VMM's + // ID, instead. new_runtime.propolis_id = None; update_required = true; @@ -567,15 +567,14 @@ impl UpdatesRequired { } else if snapshot.instance.intended_state == InstanceIntendedState::Running && snapshot.migration.is_none() + && active_vmm + .stop_for_update_disposition_generation + .is_none() { - // TODO-K: We definitely don't want the - // InstanceIntendedState to change if we're stopping for - // an update. We should make sure it doesn't. The best - // way to do this is to use the marker - // Did the active VMM shut itself down, when it was // intended to be running *and* we were not migrating - // out? If so, update the instance's intended state to + // out, or destroying it intentionally for an update? + // If so, update the instance's intended state to // indicate that it stopped itself. new_intent = Some(InstanceIntendedState::GuestShutdown); info!( From 267ed82a8fc3fb8e3d8be9056dd682271724912e Mon Sep 17 00:00:00 2001 From: karencfv Date: Wed, 7 Oct 2026 19:19:39 +1300 Subject: [PATCH 11/44] better logging --- dev-tools/omdb/src/bin/omdb/nexus.rs | 5 +- .../background/tasks/vmm_stop_for_update.rs | 47 ++++++++++++------- 2 files changed, 32 insertions(+), 20 deletions(-) diff --git a/dev-tools/omdb/src/bin/omdb/nexus.rs b/dev-tools/omdb/src/bin/omdb/nexus.rs index 6619c45368e..0c1ef4a9b95 100644 --- a/dev-tools/omdb/src/bin/omdb/nexus.rs +++ b/dev-tools/omdb/src/bin/omdb/nexus.rs @@ -1554,11 +1554,10 @@ fn print_task_vmm_stop_for_update(details: &serde_json::Value) { } } - // TODO-K: double check this is actually what I want the errors to - // look like if !error_messages.is_empty() { + println!(" errors:"); for error in error_messages { - println!(" {ERROR: vmms_by_sled.len(), - // "vmms" => vmm_count, - // ); - // TODO-K: for debug show IDs? also show which failed - // } + if vmms_stopped_by_sled.is_empty() { + slog::info!( + &opctx.log, + "no VMMs were stopped for a sled update"; + ); + } else { + for sled in &vmms_stopped_by_sled { + let VmmsBySled { sled_id, vmm_ids } = sled; + slog::info!( + &opctx.log, + "stopped {} VMMs marked to stop for a sled update", + vmm_ids.len(); + "sled_id" => %sled_id, + "vmms" => ?vmm_ids, + ); + } + } + + if !vmms_failed_by_sled.is_empty() { + for sled in &vmms_failed_by_sled { + let VmmsBySled { sled_id, vmm_ids } = sled; + slog::info!( + &opctx.log, + "failed to stop {} VMMs marked to stop for a sled update", + vmm_ids.len(); + "sled_id" => %sled_id, + "vmms" => ?vmm_ids, + ); + } + } if !error_messages.is_empty() { slog::error!( From 5f3b05ecf3ae3334dc56694264b9f69feaf2dbb5 Mon Sep 17 00:00:00 2001 From: karencfv Date: Wed, 7 Oct 2026 19:44:01 +1300 Subject: [PATCH 12/44] expectorate --- dev-tools/omdb/tests/env.out | 12 ++++++++++++ dev-tools/omdb/tests/successes.out | 16 ++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/dev-tools/omdb/tests/env.out b/dev-tools/omdb/tests/env.out index 9ade7712ae8..8054218dd88 100644 --- a/dev-tools/omdb/tests/env.out +++ b/dev-tools/omdb/tests/env.out @@ -271,6 +271,10 @@ task: "vmm_mark_stop_for_update" marks VMMs on evacuating sleds as needing to be stopped for an update +task: "vmm_stop_for_update" + stops VMMs that are marked to be stopped for a sled update + + task: "vpc_route_manager" propagates updated VPC routes to all OPTE ports @@ -547,6 +551,10 @@ task: "vmm_mark_stop_for_update" marks VMMs on evacuating sleds as needing to be stopped for an update +task: "vmm_stop_for_update" + stops VMMs that are marked to be stopped for a sled update + + task: "vpc_route_manager" propagates updated VPC routes to all OPTE ports @@ -810,6 +818,10 @@ task: "vmm_mark_stop_for_update" marks VMMs on evacuating sleds as needing to be stopped for an update +task: "vmm_stop_for_update" + stops VMMs that are marked to be stopped for a sled update + + task: "vpc_route_manager" propagates updated VPC routes to all OPTE ports diff --git a/dev-tools/omdb/tests/successes.out b/dev-tools/omdb/tests/successes.out index dcbf3b3e98e..d7dcd0e5d56 100644 --- a/dev-tools/omdb/tests/successes.out +++ b/dev-tools/omdb/tests/successes.out @@ -601,6 +601,10 @@ task: "vmm_mark_stop_for_update" marks VMMs on evacuating sleds as needing to be stopped for an update +task: "vmm_stop_for_update" + stops VMMs that are marked to be stopped for a sled update + + task: "vpc_route_manager" propagates updated VPC routes to all OPTE ports @@ -1209,6 +1213,12 @@ task: "vmm_mark_stop_for_update" batch size: 1000 VMMs marked to be stopped for an update: 0 +task: "vmm_stop_for_update" + configured period: every m + last completed activation: , triggered by + started at (s ago) and ran for ms + VMMs stopped for sled evacuation: 0 total + task: "vpc_route_manager" configured period: every s last completed activation: , triggered by @@ -1946,6 +1956,12 @@ task: "vmm_mark_stop_for_update" batch size: 1000 VMMs marked to be stopped for an update: 0 +task: "vmm_stop_for_update" + configured period: every m + last completed activation: , triggered by + started at (s ago) and ran for ms + VMMs stopped for sled evacuation: 0 total + task: "vpc_route_manager" configured period: every s last completed activation: , triggered by From bf7366635518712c326d97d3884810d1ed420c0f Mon Sep 17 00:00:00 2001 From: karencfv Date: Thu, 8 Oct 2026 14:15:59 +1300 Subject: [PATCH 13/44] add test for task --- .../background/tasks/vmm_stop_for_update.rs | 432 ++++++++++++++++++ 1 file changed, 432 insertions(+) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 53f6a6e1a82..9e99e108c53 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -377,3 +377,435 @@ impl BackgroundTask for VmmStopForUpdate { }) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::app::sagas::test_helpers; + use async_bb8_diesel::AsyncRunQueryDsl; + use chrono::Utc; + use diesel::ExpressionMethods; + use diesel::QueryDsl; + use diesel::SelectableHelper; + use nexus_db_model::Generation; + use nexus_db_model::InstanceState; + use nexus_db_model::Vmm; + use nexus_db_model::VmmCpuPlatform; + use nexus_db_model::VmmFailureReason; + use nexus_db_model::VmmState as DbVmmState; + use nexus_db_model::to_db_typed_generation; + use nexus_test_utils::ControlPlaneBuilder; + use nexus_test_utils::resource_helpers::create_anti_affinity_group; + use nexus_test_utils::resource_helpers::create_default_ip_pools; + use nexus_test_utils::resource_helpers::create_project; + use nexus_test_utils::resource_helpers::object_create; + use nexus_types::external_api::instance; + use nexus_types_versions::latest; + use omicron_common::api::external::ByteCount; + use omicron_common::api::external::IdentityMetadataCreateParams; + use omicron_common::api::external::NameOrId; + use omicron_generation_kinds::UpdateDispositionGeneration; + use omicron_uuid_kinds::InstanceUuid; + use omicron_uuid_kinds::SledUuid; + use std::collections::BTreeMap; + use std::collections::BTreeSet; + use std::time::Duration; + use uuid::Uuid; + + type ControlPlaneTestContext = + nexus_test_utils::ControlPlaneTestContext; + + const PROJECT_NAME: &str = "vmm-stop-for-update"; + + // TODO-K: Fix comment The states of the real (simulated) VMMs that the + // task should stop. The label is used to name instances and groups. + // + // TODO-K: Explain why we can't have a `Creating` VMM + const REAL_VMM_STATES: [(&str, DbVmmState); 3] = [ + ("starting", DbVmmState::Starting), + ("running", DbVmmState::Running), + ("rebooting", DbVmmState::Rebooting), + ]; + + // TODO-K: Fix comment Creates and starts an instance that belongs to + // `anti_affinity_group`, and returns its ID. + async fn create_started_instance( + cptestctx: &ControlPlaneTestContext, + name: &str, + anti_affinity_group: &str, + ) -> InstanceUuid { + let created = object_create::<_, latest::instance::Instance>( + &cptestctx.external_client, + &format!("/v1/instances?project={PROJECT_NAME}"), + &instance::InstanceCreate { + identity: IdentityMetadataCreateParams { + name: name.parse().unwrap(), + description: format!("instance {name}"), + }, + ncpus: 1i64.try_into().unwrap(), + memory: ByteCount::from_gibibytes_u32(1), + hostname: "myhostname".try_into().unwrap(), + user_data: Vec::new(), + network_interfaces: + instance::InstanceNetworkInterfaceAttachment::None, + external_ips: Vec::new(), + disks: Vec::new(), + boot_disk: None, + cpu_platform: None, + ssh_public_keys: None, + start: true, + auto_restart_policy: None, + anti_affinity_groups: vec![NameOrId::Name( + anti_affinity_group.parse().unwrap(), + )], + multicast_groups: Vec::new(), + enable_jumbo_frames: false, + }, + ) + .await; + InstanceUuid::from_untyped_uuid(created.identity.id) + } + + // TODO-K: Fix comment Asks Nexus to reboot a running instance. The + // simulated sled agent leaves the VMM in `Rebooting` until it is poked. + async fn reboot_instance( + cptestctx: &ControlPlaneTestContext, + instance_id: InstanceUuid, + ) { + let nexus = &cptestctx.server.server_context().nexus; + let opctx = test_helpers::test_opctx(cptestctx); + let lookup = nexus + .instance_lookup( + &opctx, + instance::InstanceSelector { + project: None, + instance: NameOrId::from(instance_id.into_untyped_uuid()), + }, + ) + .expect("instance lookup should succeed"); + nexus + .instance_reboot(&opctx, &lookup) + .await + .expect("instance should reboot"); + } + + async fn active_vmm( + cptestctx: &ControlPlaneTestContext, + instance_id: InstanceUuid, + ) -> Vmm { + test_helpers::instance_fetch(cptestctx, instance_id) + .await + .vmm() + .clone() + .expect("instance should have an active VMM") + } + + // TODO-K: Fix comment Marks a single VMM to be stopped for an update. + async fn mark_vmm( + conn: &nexus_db_lookup::AsyncConnection, + vmm_id: Uuid, + generation: UpdateDispositionGeneration, + ) { + use nexus_db_schema::schema::vmm::dsl; + + let updated = diesel::update(dsl::vmm) + .filter(dsl::id.eq(vmm_id)) + .set( + dsl::stop_for_update_disposition_generation + .eq(Some(to_db_typed_generation(generation))), + ) + .execute_async(conn) + .await + .expect("VMM should be marked"); + assert_eq!(updated, 1, "exactly one VMM should be marked"); + } + + // TODO-K: Fix comment Inserts a VMM row that no sled agent knows about. + async fn insert_vmm( + datastore: &DataStore, + opctx: &OpContext, + sled_id: SledUuid, + state: DbVmmState, + marker: Option, + ) -> Vmm { + // TODO-K: Fix comment The `failure_reason_iff_failed` constraint + // requires a failure reason for `Failed` VMMs, and only for them. + let failure_reason = (state == DbVmmState::Failed) + .then_some(VmmFailureReason::FromSledAgent); + datastore + .vmm_insert( + opctx, + Vmm { + id: Uuid::new_v4(), + time_created: Utc::now(), + time_deleted: None, + instance_id: Uuid::new_v4(), + sled_id: sled_id.into(), + propolis_ip: "10.1.9.32".parse().unwrap(), + propolis_port: 420.into(), + cpu_platform: VmmCpuPlatform::SledDefault, + time_state_updated: Utc::now(), + generation: Generation::new(), + state, + failure_reason, + stop_for_update_disposition_generation: marker + .map(Into::into), + }, + ) + .await + .expect("VMM should be inserted") + } + + // TODO-K: Fix comment Fetches the given VMM rows, including deleted rows + // (unlike `vmm_fetch`, which skips them). + async fn fetch_vmms( + conn: &nexus_db_lookup::AsyncConnection, + ids: &[Uuid], + ) -> BTreeMap { + use nexus_db_schema::schema::vmm::dsl; + + dsl::vmm + .filter(dsl::id.eq_any(ids.to_vec())) + .select(Vmm::as_select()) + .load_async::(conn) + .await + .expect("VMMs should be fetched") + .into_iter() + .map(|vmm| (vmm.id, vmm)) + .collect() + } + + fn vmm_ids_in(sleds: &IdOrdMap) -> BTreeSet { + sleds + .iter() + .flat_map(|sled| { + sled.vmm_ids.iter().map(|id| id.into_untyped_uuid()) + }) + .collect() + } + + #[tokio::test] + async fn test_vmm_stop_for_update_activation() { + // TODO-K: Fix comment Use two simulated sleds. Lengthen the periods + // of the background tasks that could change the VMM rows while this + // test runs. Each of them still runs once when Nexus starts, before + // any of the rows below exist. + let cptestctx = + ControlPlaneBuilder::new("test_vmm_stop_for_update_activation") + .with_extra_sled_agents(1) + .customize_nexus_config(&|config| { + let one_day = Duration::from_secs(86_400); + let tasks = &mut config.pkg.background_tasks; + tasks.instance_watcher.period_secs = one_day; + tasks.abandoned_vmm_reaper.period_secs = one_day; + tasks.vmm_mark_stop_for_update.period_secs = one_day; + tasks.vmm_stop_for_update.period_secs = one_day; + }) + .start::() + .await; + let client = &cptestctx.external_client; + let datastore = + cptestctx.server.server_context().nexus.datastore().clone(); + let opctx = test_helpers::test_opctx(&cptestctx); + let sled_a = cptestctx.first_sled_id(); + let sled_b = cptestctx.second_sled_id(); + let generation = UpdateDispositionGeneration::from(1); + + create_default_ip_pools(client).await; + create_project(client, PROJECT_NAME).await; + + // TODO-K: Fix comment Step 1: create the VMM rows. + // + // The simulated sled agents only know about VMMs that an instance + // start registered with them, so the marked VMMs that the task should + // stop are real VMMs. For each state in `REAL_VMM_STATES`, start two + // instances in one anti-affinity group, so that one VMM lands on each + // sled, put the VMMs in that state, and mark them. + let conn = datastore.pool_connection_for_tests().await.unwrap(); + let mut instance_ids = Vec::new(); + let mut vmm_ids = Vec::new(); + for (label, state) in REAL_VMM_STATES { + let group = format!("group-{label}"); + create_anti_affinity_group(client, PROJECT_NAME, &group).await; + for i in 0..2 { + let id = create_started_instance( + &cptestctx, + &format!("{label}-{i}"), + &group, + ) + .await; + match state { + // A new VMM in a simulatated environemnt stays in + // `Starting` until the `poke` API is called. + DbVmmState::Starting => {} + DbVmmState::Running => { + test_helpers::instance_simulate(&cptestctx, &id).await; + } + DbVmmState::Rebooting => { + test_helpers::instance_simulate(&cptestctx, &id).await; + reboot_instance(&cptestctx, id).await; + } + _ => unreachable!("{state:?} is not in REAL_VMM_STATES"), + } + let vmm = active_vmm(&cptestctx, id).await; + assert_eq!(vmm.state, state, "VMM {} is not {state:?}", vmm.id); + mark_vmm(&conn, vmm.id, generation).await; + instance_ids.push(id); + vmm_ids.push(vmm.id); + } + } + + // TODO-K: Fix comment The task never contacts a sled agent about the + // other rows, so they are inserted directly: one unmarked VMM in every + // state, and one marked VMM in every state that no real VMM covers. + // That includes a marked `Creating` VMM, which no sled agent knows. + for sled_id in [sled_a, sled_b] { + for &state in DbVmmState::ALL_STATES { + let unmarked = + insert_vmm(&datastore, &opctx, sled_id, state, None).await; + vmm_ids.push(unmarked.id); + if !REAL_VMM_STATES.iter().any(|(_, real)| *real == state) { + let marked = insert_vmm( + &datastore, + &opctx, + sled_id, + state, + Some(generation), + ) + .await; + vmm_ids.push(marked.id); + } + } + } + + // TODO-K: Fix comment Step 2: retrieve every VMM row created above. + let vmms = fetch_vmms(&conn, &vmm_ids).await; + // TODO-K: Fix comment There should be 40 rows: 6 real VMMs (one in + // each of the 3 `REAL_VMM_STATES` on each of the 2 sleds), 20 unmarked + // VMMs (one in each of the 10 states on each sled), and 14 marked VMMs + // (one in each of the 7 states without a real VMM on each sled). + assert_eq!(vmms.len(), 40); + + // TODO-K: Fix comment Step 3: split the rows into the ones the task + // should stop and the ones it should leave unchanged. A marked + // `Creating` VMM is stoppable, but no sled agent knows about it, so + // the stop request fails (404) and the row stays unchanged. + let should_be_stopped = |vmm: &Vmm| { + vmm.stop_for_update_disposition_generation.is_some() + && DbVmmState::SHOULD_STOP_FOR_EVACUATION.contains(&vmm.state) + && vmm.state != DbVmmState::Creating + }; + let (expected_rows_to_be_stopped, expected_rows_to_be_unchanged): ( + Vec, + Vec, + ) = vmms.values().cloned().partition(should_be_stopped); + let expected_failed_ids: BTreeSet = expected_rows_to_be_unchanged + .iter() + .filter(|vmm| { + vmm.stop_for_update_disposition_generation.is_some() + && vmm.state == DbVmmState::Creating + }) + .map(|vmm| vmm.id) + .collect(); + assert_eq!(expected_rows_to_be_stopped.len(), 6); + assert_eq!(expected_failed_ids.len(), 2); + for sled_id in [sled_a, sled_b] { + for (_, state) in REAL_VMM_STATES { + let count = expected_rows_to_be_stopped + .iter() + .filter(|vmm| { + vmm.sled_id() == sled_id && vmm.state == state + }) + .count(); + assert_eq!( + count, 1, + "sled {sled_id} should have one marked VMM in {state:?}" + ); + } + } + + // TODO-K: Fix comment Step 4: run the task. + let mut task = VmmStopForUpdate::new(datastore.clone()); + let status = task.actually_activate(&opctx).await; + + // TODO-K: Fix comment Step 5: check the task's status. + let stopped_count: usize = status + .vmms_stopped_by_sled + .iter() + .map(|sled| sled.vmm_ids.len()) + .sum(); + assert_eq!(stopped_count, expected_rows_to_be_stopped.len()); + let expected_stopped_ids: BTreeSet = + expected_rows_to_be_stopped.iter().map(|vmm| vmm.id).collect(); + assert_eq!( + vmm_ids_in(&status.vmms_stopped_by_sled), + expected_stopped_ids + ); + let failed_count: usize = status + .vmms_failed_by_sled + .iter() + .map(|sled| sled.vmm_ids.len()) + .sum(); + assert_eq!(failed_count, expected_failed_ids.len()); + assert_eq!( + vmm_ids_in(&status.vmms_failed_by_sled), + expected_failed_ids + ); + assert_eq!(status.error_messages.len(), expected_failed_ids.len()); + + // TODO-K: Fix comment Step 6: the task does not write VMM states to + // the database. The simulated sled agents report the rest of each + // stop to Nexus only when they are poked. Poke each stopped VMM, wait + // for its instance to reach `NoVmm`, wait for the instance-update + // sagas to finish, and then retrieve every row again. + for id in &instance_ids { + test_helpers::instance_simulate(&cptestctx, id).await; + test_helpers::instance_wait_for_state( + &cptestctx, + *id, + InstanceState::NoVmm, + ) + .await; + } + test_helpers::wait_for_running_sagas(&cptestctx).await; + let vmms_after = fetch_vmms(&conn, &vmm_ids).await; + assert_eq!(vmms_after.len(), 40); + + // TODO-K: Fix comment Step 7: every VMM the task stopped is now + // `Destroyed`, the instance-update saga marked it deleted, and it is + // still marked to be stopped for an update. + for expected in &expected_rows_to_be_stopped { + let actual = &vmms_after[&expected.id]; + assert_eq!( + actual.state, + DbVmmState::Destroyed, + "VMM {} on sled {} was not stopped", + expected.id, + expected.sled_id(), + ); + assert!( + actual.time_deleted.is_some(), + "VMM {} was not deleted", + expected.id, + ); + assert_eq!( + actual.stop_for_update_disposition_generation, + expected.stop_for_update_disposition_generation, + ); + } + + // TODO-K: Fix comment Step 8: every other row is exactly as it was. + for expected in &expected_rows_to_be_unchanged { + assert_eq!( + &vmms_after[&expected.id], + expected, + "VMM {} on sled {} in state {:?} changed unexpectedly", + expected.id, + expected.sled_id(), + expected.state, + ); + } + + cptestctx.teardown().await; + } +} From 3a629208f802141cb4d2af55624f021ec166b826 Mon Sep 17 00:00:00 2001 From: karencfv Date: Thu, 8 Oct 2026 15:46:55 +1300 Subject: [PATCH 14/44] organise test --- .../background/tasks/vmm_stop_for_update.rs | 195 +++++++++++------- 1 file changed, 118 insertions(+), 77 deletions(-) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 9e99e108c53..c7338910ba2 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -381,12 +381,17 @@ impl BackgroundTask for VmmStopForUpdate { #[cfg(test)] mod tests { use super::*; - use crate::app::sagas::test_helpers; + use crate::app::sagas::test_helpers::instance_fetch; + use crate::app::sagas::test_helpers::instance_simulate; + use crate::app::sagas::test_helpers::instance_wait_for_state; + use crate::app::sagas::test_helpers::test_opctx; + use crate::app::sagas::test_helpers::wait_for_running_sagas; use async_bb8_diesel::AsyncRunQueryDsl; use chrono::Utc; use diesel::ExpressionMethods; use diesel::QueryDsl; use diesel::SelectableHelper; + use nexus_db_lookup::AsyncConnection; use nexus_db_model::Generation; use nexus_db_model::InstanceState; use nexus_db_model::Vmm; @@ -399,8 +404,10 @@ mod tests { use nexus_test_utils::resource_helpers::create_default_ip_pools; use nexus_test_utils::resource_helpers::create_project; use nexus_test_utils::resource_helpers::object_create; - use nexus_types::external_api::instance; - use nexus_types_versions::latest; + use nexus_types::external_api::instance::InstanceCreate; + use nexus_types_versions::latest::instance::Instance; + use nexus_types_versions::latest::instance::InstanceNetworkInterfaceAttachment; + use nexus_types_versions::latest::instance::InstanceSelector; use omicron_common::api::external::ByteCount; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::NameOrId; @@ -417,27 +424,28 @@ mod tests { const PROJECT_NAME: &str = "vmm-stop-for-update"; - // TODO-K: Fix comment The states of the real (simulated) VMMs that the - // task should stop. The label is used to name instances and groups. + // In order to stop a VMM, it needs to be registered with the simulated + // sled agent. We keep a record of the states of the real VMMs we will be + // stopping. Even though `Created` is a "stoppable" state, we do not include + // it in this list as a VMM is only in this state for a short period. This + // means we cannot reliably test a VMM in the `Creating` state. // - // TODO-K: Explain why we can't have a `Creating` VMM + // The label is used to name instances and groups. const REAL_VMM_STATES: [(&str, DbVmmState); 3] = [ ("starting", DbVmmState::Starting), ("running", DbVmmState::Running), ("rebooting", DbVmmState::Rebooting), ]; - // TODO-K: Fix comment Creates and starts an instance that belongs to - // `anti_affinity_group`, and returns its ID. async fn create_started_instance( cptestctx: &ControlPlaneTestContext, name: &str, anti_affinity_group: &str, ) -> InstanceUuid { - let created = object_create::<_, latest::instance::Instance>( + let created = object_create::<_, Instance>( &cptestctx.external_client, &format!("/v1/instances?project={PROJECT_NAME}"), - &instance::InstanceCreate { + &InstanceCreate { identity: IdentityMetadataCreateParams { name: name.parse().unwrap(), description: format!("instance {name}"), @@ -446,8 +454,7 @@ mod tests { memory: ByteCount::from_gibibytes_u32(1), hostname: "myhostname".try_into().unwrap(), user_data: Vec::new(), - network_interfaces: - instance::InstanceNetworkInterfaceAttachment::None, + network_interfaces: InstanceNetworkInterfaceAttachment::None, external_ips: Vec::new(), disks: Vec::new(), boot_disk: None, @@ -466,18 +473,16 @@ mod tests { InstanceUuid::from_untyped_uuid(created.identity.id) } - // TODO-K: Fix comment Asks Nexus to reboot a running instance. The - // simulated sled agent leaves the VMM in `Rebooting` until it is poked. async fn reboot_instance( cptestctx: &ControlPlaneTestContext, instance_id: InstanceUuid, ) { let nexus = &cptestctx.server.server_context().nexus; - let opctx = test_helpers::test_opctx(cptestctx); + let opctx = test_opctx(cptestctx); let lookup = nexus .instance_lookup( &opctx, - instance::InstanceSelector { + InstanceSelector { project: None, instance: NameOrId::from(instance_id.into_untyped_uuid()), }, @@ -493,16 +498,15 @@ mod tests { cptestctx: &ControlPlaneTestContext, instance_id: InstanceUuid, ) -> Vmm { - test_helpers::instance_fetch(cptestctx, instance_id) + instance_fetch(cptestctx, instance_id) .await .vmm() .clone() .expect("instance should have an active VMM") } - // TODO-K: Fix comment Marks a single VMM to be stopped for an update. - async fn mark_vmm( - conn: &nexus_db_lookup::AsyncConnection, + async fn mark_vmm_stopped_for_update( + conn: &AsyncConnection, vmm_id: Uuid, generation: UpdateDispositionGeneration, ) { @@ -520,16 +524,15 @@ mod tests { assert_eq!(updated, 1, "exactly one VMM should be marked"); } - // TODO-K: Fix comment Inserts a VMM row that no sled agent knows about. - async fn insert_vmm( + // Inserts a VMM row without sled agent knowing about it. This is used for + // VMM in states that we don't expect to stop. + async fn insert_vmm_in_state( datastore: &DataStore, opctx: &OpContext, sled_id: SledUuid, state: DbVmmState, marker: Option, ) -> Vmm { - // TODO-K: Fix comment The `failure_reason_iff_failed` constraint - // requires a failure reason for `Failed` VMMs, and only for them. let failure_reason = (state == DbVmmState::Failed) .then_some(VmmFailureReason::FromSledAgent); datastore @@ -556,10 +559,10 @@ mod tests { .expect("VMM should be inserted") } - // TODO-K: Fix comment Fetches the given VMM rows, including deleted rows - // (unlike `vmm_fetch`, which skips them). + // `vmm_fetch` does not retrieve rows with a non-null `time_deleted`, so we + // need a little helper that does. async fn fetch_vmms( - conn: &nexus_db_lookup::AsyncConnection, + conn: &AsyncConnection, ids: &[Uuid], ) -> BTreeMap { use nexus_db_schema::schema::vmm::dsl; @@ -575,8 +578,10 @@ mod tests { .collect() } - fn vmm_ids_in(sleds: &IdOrdMap) -> BTreeSet { - sleds + fn vmm_ids_in_all_sleds( + vmms_by_sled: &IdOrdMap, + ) -> BTreeSet { + vmms_by_sled .iter() .flat_map(|sled| { sled.vmm_ids.iter().map(|id| id.into_untyped_uuid()) @@ -586,10 +591,12 @@ mod tests { #[tokio::test] async fn test_vmm_stop_for_update_activation() { - // TODO-K: Fix comment Use two simulated sleds. Lengthen the periods - // of the background tasks that could change the VMM rows while this - // test runs. Each of them still runs once when Nexus starts, before - // any of the rows below exist. + // Set up the test environment. + // + // We want two sleds to test that the task stops VMMs across multiple + // sleds. Additionally, to avoid flakiness, we increase the intervals + // for background tasks that could potentially modify VMM rows while + // this test is running. let cptestctx = ControlPlaneBuilder::new("test_vmm_stop_for_update_activation") .with_extra_sled_agents(1) @@ -603,10 +610,11 @@ mod tests { }) .start::() .await; + let client = &cptestctx.external_client; let datastore = cptestctx.server.server_context().nexus.datastore().clone(); - let opctx = test_helpers::test_opctx(&cptestctx); + let opctx = test_opctx(&cptestctx); let sled_a = cptestctx.first_sled_id(); let sled_b = cptestctx.second_sled_id(); let generation = UpdateDispositionGeneration::from(1); @@ -614,13 +622,16 @@ mod tests { create_default_ip_pools(client).await; create_project(client, PROJECT_NAME).await; - // TODO-K: Fix comment Step 1: create the VMM rows. + // The simulated agent can only stopp VMMs that are registered with it. + // So, for each VMM that we intend to stop, we create a real instance + // via the external API. + // + // For each state in `REAL_VMM_STATES`, two instances are created per + // anti-affinity group. This ensures that both sleds have the same + // amount of instances in each state. // - // The simulated sled agents only know about VMMs that an instance - // start registered with them, so the marked VMMs that the task should - // stop are real VMMs. For each state in `REAL_VMM_STATES`, start two - // instances in one anti-affinity group, so that one VMM lands on each - // sled, put the VMMs in that state, and mark them. + // TODO-K: Add some instances in stoppable states that are not marked + // for stopping let conn = datastore.pool_connection_for_tests().await.unwrap(); let mut instance_ids = Vec::new(); let mut vmm_ids = Vec::new(); @@ -636,36 +647,48 @@ mod tests { .await; match state { // A new VMM in a simulatated environemnt stays in - // `Starting` until the `poke` API is called. + // `Starting` until the "poke" API is called, so we can + // confidently test a VMM in this state. DbVmmState::Starting => {} DbVmmState::Running => { - test_helpers::instance_simulate(&cptestctx, &id).await; + instance_simulate(&cptestctx, &id).await; } DbVmmState::Rebooting => { - test_helpers::instance_simulate(&cptestctx, &id).await; + instance_simulate(&cptestctx, &id).await; + // Like above, we can keep a VMM in `Rebooting` if we + // don't call the "poke" API reboot_instance(&cptestctx, id).await; } _ => unreachable!("{state:?} is not in REAL_VMM_STATES"), } let vmm = active_vmm(&cptestctx, id).await; assert_eq!(vmm.state, state, "VMM {} is not {state:?}", vmm.id); - mark_vmm(&conn, vmm.id, generation).await; + // Mark them to be stopped by the task + mark_vmm_stopped_for_update(&conn, vmm.id, generation).await; instance_ids.push(id); vmm_ids.push(vmm.id); } } - // TODO-K: Fix comment The task never contacts a sled agent about the - // other rows, so they are inserted directly: one unmarked VMM in every - // state, and one marked VMM in every state that no real VMM covers. - // That includes a marked `Creating` VMM, which no sled agent knows. + // For all other states that shouldn't be stopped by the task, we insert + // rows directly in the DB. The task should skip them entirely, so they + // don't need to be registered with sled agent. + // + // One caveat is that we are including the VMMs in `Creating` state. As + // mentioned earlier, these cannot be realiably tested to stop, but we + // can conveniently use them to simulate VMMs that failed to stop. We + // insert them and mark them as needing to be stopped. for sled_id in [sled_a, sled_b] { for &state in DbVmmState::ALL_STATES { - let unmarked = - insert_vmm(&datastore, &opctx, sled_id, state, None).await; + let unmarked = insert_vmm_in_state( + &datastore, &opctx, sled_id, state, None, + ) + .await; vmm_ids.push(unmarked.id); + + // TODO-K: Why are adding all states instead of just creating? if !REAL_VMM_STATES.iter().any(|(_, real)| *real == state) { - let marked = insert_vmm( + let marked = insert_vmm_in_state( &datastore, &opctx, sled_id, @@ -678,27 +701,38 @@ mod tests { } } - // TODO-K: Fix comment Step 2: retrieve every VMM row created above. let vmms = fetch_vmms(&conn, &vmm_ids).await; - // TODO-K: Fix comment There should be 40 rows: 6 real VMMs (one in - // each of the 3 `REAL_VMM_STATES` on each of the 2 sleds), 20 unmarked - // VMMs (one in each of the 10 states on each sled), and 14 marked VMMs - // (one in each of the 7 states without a real VMM on each sled). + // TODO-K: Verify this number after we make changes + // + // There should be 40 VMM rows: + // - 6 real VMMs (one in each of the 3 `REAL_VMM_STATES` on each of + // the 2 sleds) + // - 20 unmarked VMMs (one in each of the remaining states on each + // sled) + // - 14 marked VMMs (one in each of the 7 states without a real VMM + // on each sled). assert_eq!(vmms.len(), 40); // TODO-K: Fix comment Step 3: split the rows into the ones the task // should stop and the ones it should leave unchanged. A marked // `Creating` VMM is stoppable, but no sled agent knows about it, so // the stop request fails (404) and the row stays unchanged. + + // Collect which VMMs we expect the task will stop and which it won't. let should_be_stopped = |vmm: &Vmm| { vmm.stop_for_update_disposition_generation.is_some() && DbVmmState::SHOULD_STOP_FOR_EVACUATION.contains(&vmm.state) + // As mentioned above, we don't expect VMMs in `Creating` state + // to stop for the purposes of this test && vmm.state != DbVmmState::Creating }; let (expected_rows_to_be_stopped, expected_rows_to_be_unchanged): ( Vec, Vec, ) = vmms.values().cloned().partition(should_be_stopped); + + // Collect the ids of the VMMs in `Creating` state, which are the ones + // we expect to turn up as failed. let expected_failed_ids: BTreeSet = expected_rows_to_be_unchanged .iter() .filter(|vmm| { @@ -707,8 +741,12 @@ mod tests { }) .map(|vmm| vmm.id) .collect(); + assert_eq!(expected_rows_to_be_stopped.len(), 6); assert_eq!(expected_failed_ids.len(), 2); + + // Confirm there is one of each stoppable state per sled in + // `expected_rows_to_be_stopped` for sled_id in [sled_a, sled_b] { for (_, state) in REAL_VMM_STATES { let count = expected_rows_to_be_stopped @@ -724,23 +762,26 @@ mod tests { } } - // TODO-K: Fix comment Step 4: run the task. + // Run the task. let mut task = VmmStopForUpdate::new(datastore.clone()); let status = task.actually_activate(&opctx).await; - // TODO-K: Fix comment Step 5: check the task's status. + // We stopped the VMMs we expected to stop let stopped_count: usize = status .vmms_stopped_by_sled .iter() .map(|sled| sled.vmm_ids.len()) .sum(); assert_eq!(stopped_count, expected_rows_to_be_stopped.len()); + let expected_stopped_ids: BTreeSet = expected_rows_to_be_stopped.iter().map(|vmm| vmm.id).collect(); assert_eq!( - vmm_ids_in(&status.vmms_stopped_by_sled), + vmm_ids_in_all_sleds(&status.vmms_stopped_by_sled), expected_stopped_ids ); + + // The VMMs we expected to fail to stop, failed. let failed_count: usize = status .vmms_failed_by_sled .iter() @@ -748,32 +789,32 @@ mod tests { .sum(); assert_eq!(failed_count, expected_failed_ids.len()); assert_eq!( - vmm_ids_in(&status.vmms_failed_by_sled), + vmm_ids_in_all_sleds(&status.vmms_failed_by_sled), expected_failed_ids ); assert_eq!(status.error_messages.len(), expected_failed_ids.len()); + assert_eq!( + status.error_messages, + vec![ + "Invalid Request: Not Found".to_string(), + "Invalid Request: Not Found".to_string() + ] + ); - // TODO-K: Fix comment Step 6: the task does not write VMM states to - // the database. The simulated sled agents report the rest of each - // stop to Nexus only when they are poked. Poke each stopped VMM, wait - // for its instance to reach `NoVmm`, wait for the instance-update - // sagas to finish, and then retrieve every row again. + // Poke each simulated instance so each sled agent reports back to Nexus + // and wait until each one reports `NoVmm`. for id in &instance_ids { - test_helpers::instance_simulate(&cptestctx, id).await; - test_helpers::instance_wait_for_state( - &cptestctx, - *id, - InstanceState::NoVmm, - ) - .await; + instance_simulate(&cptestctx, id).await; + instance_wait_for_state(&cptestctx, *id, InstanceState::NoVmm) + .await; } - test_helpers::wait_for_running_sagas(&cptestctx).await; + // Before fetching the VMMs again, make sure all instance update sagas + // are finished + wait_for_running_sagas(&cptestctx).await; let vmms_after = fetch_vmms(&conn, &vmm_ids).await; assert_eq!(vmms_after.len(), 40); - // TODO-K: Fix comment Step 7: every VMM the task stopped is now - // `Destroyed`, the instance-update saga marked it deleted, and it is - // still marked to be stopped for an update. + // Every VMM we expected to stop should be destroyed at this point for expected in &expected_rows_to_be_stopped { let actual = &vmms_after[&expected.id]; assert_eq!( @@ -794,7 +835,7 @@ mod tests { ); } - // TODO-K: Fix comment Step 8: every other row is exactly as it was. + // Make sure all other rows are unchanged for expected in &expected_rows_to_be_unchanged { assert_eq!( &vmms_after[&expected.id], From 8c2fdaea0547d886d96b29ad00e86c127abd770f Mon Sep 17 00:00:00 2001 From: karencfv Date: Thu, 8 Oct 2026 16:08:09 +1300 Subject: [PATCH 15/44] clean up --- .../background/tasks/vmm_stop_for_update.rs | 38 ++++++++----------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index c7338910ba2..3b2e6cb537c 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -629,9 +629,6 @@ mod tests { // For each state in `REAL_VMM_STATES`, two instances are created per // anti-affinity group. This ensures that both sleds have the same // amount of instances in each state. - // - // TODO-K: Add some instances in stoppable states that are not marked - // for stopping let conn = datastore.pool_connection_for_tests().await.unwrap(); let mut instance_ids = Vec::new(); let mut vmm_ids = Vec::new(); @@ -674,10 +671,15 @@ mod tests { // rows directly in the DB. The task should skip them entirely, so they // don't need to be registered with sled agent. // - // One caveat is that we are including the VMMs in `Creating` state. As - // mentioned earlier, these cannot be realiably tested to stop, but we - // can conveniently use them to simulate VMMs that failed to stop. We - // insert them and mark them as needing to be stopped. + // Even though this shouldn't happen in practice, we insert marked + // non-stoppable VMMs. This ensures that even if the task to mark VMMs + // marks a VMM that is in a state that it shouldn't be stopped in, this + // task skips it entirely. + // + // Also, we are including the VMMs in `Creating` state. As mentioned + // earlier, these cannot be realiably tested to stop, but we can + // conveniently use them to simulate VMMs that failed to stop. We insert + // them and mark them as needing to be stopped. for sled_id in [sled_a, sled_b] { for &state in DbVmmState::ALL_STATES { let unmarked = insert_vmm_in_state( @@ -686,7 +688,6 @@ mod tests { .await; vmm_ids.push(unmarked.id); - // TODO-K: Why are adding all states instead of just creating? if !REAL_VMM_STATES.iter().any(|(_, real)| *real == state) { let marked = insert_vmm_in_state( &datastore, @@ -702,22 +703,15 @@ mod tests { } let vmms = fetch_vmms(&conn, &vmm_ids).await; - // TODO-K: Verify this number after we make changes - // + // There should be 40 VMM rows: - // - 6 real VMMs (one in each of the 3 `REAL_VMM_STATES` on each of - // the 2 sleds) - // - 20 unmarked VMMs (one in each of the remaining states on each - // sled) - // - 14 marked VMMs (one in each of the 7 states without a real VMM - // on each sled). + // - 6 VMMs registered with sled agent (one in each of the 3 + // `REAL_VMM_STATES` on each of the 2 sleds) + // - 20 unmarked VMMs (one in every state on each sled) + // - 14 marked VMMs (one in each of the states *not* in + // `REAL_VMM_STATES` on each sled) assert_eq!(vmms.len(), 40); - // TODO-K: Fix comment Step 3: split the rows into the ones the task - // should stop and the ones it should leave unchanged. A marked - // `Creating` VMM is stoppable, but no sled agent knows about it, so - // the stop request fails (404) and the row stays unchanged. - // Collect which VMMs we expect the task will stop and which it won't. let should_be_stopped = |vmm: &Vmm| { vmm.stop_for_update_disposition_generation.is_some() @@ -802,7 +796,7 @@ mod tests { ); // Poke each simulated instance so each sled agent reports back to Nexus - // and wait until each one reports `NoVmm`. + // and wait until each one reports `NoVmm` for id in &instance_ids { instance_simulate(&cptestctx, id).await; instance_wait_for_state(&cptestctx, *id, InstanceState::NoVmm) From 6196cb95fae31e7ef33fa57d916b032de670f7ee Mon Sep 17 00:00:00 2001 From: karencfv Date: Thu, 8 Oct 2026 21:03:53 +1300 Subject: [PATCH 16/44] add db-query test --- nexus/db-queries/src/db/datastore/vmm.rs | 103 ++++++++++++++++++ .../background/tasks/vmm_stop_for_update.rs | 30 ++++- 2 files changed, 130 insertions(+), 3 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/vmm.rs b/nexus/db-queries/src/db/datastore/vmm.rs index b9749caf91a..a2be7ec161f 100644 --- a/nexus/db-queries/src/db/datastore/vmm.rs +++ b/nexus/db-queries/src/db/datastore/vmm.rs @@ -609,6 +609,7 @@ mod tests { use omicron_uuid_kinds::InstanceUuid; use omicron_uuid_kinds::SledUuid; use sled_agent_types::instance::MigrationState; + use std::collections::BTreeSet; use std::collections::HashMap; #[tokio::test] @@ -1342,6 +1343,108 @@ mod tests { logctx.cleanup_successful(); } + #[tokio::test] + async fn test_vmm_list_marked_stop_for_update() { + // Setup + let logctx = + dev::test_setup_log("test_vmm_list_marked_stop_for_update"); + let db = TestDatabase::new_with_datastore(&logctx.log).await; + let (opctx, datastore) = (db.opctx(), db.datastore()); + + // TODO-K: Fix comment The states from which a VMM can still be stopped + // for an update. Only marked VMMs in these states should be listed by + // `vmm_list_marked_stop_for_update`. + let is_stoppable = |state: DbVmmState| { + matches!( + state, + DbVmmState::Creating + | DbVmmState::Starting + | DbVmmState::Running + | DbVmmState::Rebooting + ) + }; + + // Insert 2 VMMs (one marked to stop, one not) in each state per sled + // (2 sleds only). + let generation = UpdateDispositionGeneration::from(1); + let sled_a = SledUuid::new_v4(); + let sled_b = SledUuid::new_v4(); + let mut vmms = Vec::new(); + for sled_id in [sled_a, sled_b] { + for &state in DbVmmState::ALL_STATES { + for marker in [Some(generation), None] { + let failure_reason = (state == DbVmmState::Failed) + .then_some(db::model::VmmFailureReason::FromSledAgent); + let vmm = datastore + .vmm_insert( + &opctx, + Vmm { + id: Uuid::new_v4(), + time_created: Utc::now(), + time_deleted: None, + instance_id: Uuid::new_v4(), + sled_id: sled_id.into(), + propolis_ip: "10.1.9.32".parse().unwrap(), + propolis_port: 420.into(), + cpu_platform: VmmCpuPlatform::SledDefault, + time_state_updated: Utc::now(), + generation: Generation::new(), + state, + failure_reason, + stop_for_update_disposition_generation: marker + .map(Into::into), + }, + ) + .await + .expect("VMM should be inserted successfully"); + vmms.push(vmm); + } + } + } + + // There should be 40 VMM rows. 2 VMMs (one marked, one not) per state + // per sled + let row_count: i64 = dsl::vmm + // Add a filter to ensure there is no full table scan + .filter(dsl::id.ne(Uuid::nil())) + .count() + .get_result_async( + &*datastore.pool_connection_for_tests().await.unwrap(), + ) + .await + .expect("VMMs should be counted"); + assert_eq!(row_count, 40); + + // Check there are 8 stoppable VMMs (4 per sled) + let expected_ids: BTreeSet = vmms + .iter() + .filter(|vmm| { + vmm.stop_for_update_disposition_generation.is_some() + && is_stoppable(vmm.state) + }) + .map(|vmm| vmm.id) + .collect(); + assert_eq!(expected_ids.len(), 8); + + // Check the retrieved ids for stoppable VMMs are the same as the + // expected ones + let listed = datastore + .vmm_list_marked_stop_for_update( + &opctx, + &DataPageParams::max_page(), + ) + .await + .expect("marked VMMs should be listed"); + assert_eq!(listed.len(), expected_ids.len()); + let listed_ids: BTreeSet = + listed.iter().map(|vmm| vmm.id).collect(); + assert_eq!(listed_ids, expected_ids); + + // Clean up. + db.terminate().await; + logctx.cleanup_successful(); + } + async fn mark_failed_no_such_instance( opctx: &OpContext, datastore: &DataStore, diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 3b2e6cb537c..dfbfe352de8 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -134,9 +134,9 @@ impl VmmStopForUpdate { "vmms" => ?vmm_ids, ); - // Since we're using IdOrdMap, each sled id will only appear once. - // Because of this it's not a big deal to start a new sled agent - // client per sled id. + // Since we're using IdOrdMap, each sled id will only appear once + // per batch of VMMs. Because of this, it's ok to start a new sled + // agent client per sled id. let sled_client = match nexus_networking::sled_client( &self.datastore, &opctx, @@ -841,6 +841,30 @@ mod tests { ); } + // Run the task again. + let status_second_run = task.actually_activate(&opctx).await; + + // No new VMMs were marked to be stopped so nothing is stopped + assert!(status_second_run.vmms_stopped_by_sled.is_empty()); + + // The VMMs in `Creating` state still fail because no sled agent is + // aware of them + let failed_count_second_run: usize = status_second_run + .vmms_failed_by_sled + .iter() + .map(|sled| sled.vmm_ids.len()) + .sum(); + assert_eq!(failed_count_second_run, expected_failed_ids.len()); + assert_eq!( + vmm_ids_in_all_sleds(&status_second_run.vmms_failed_by_sled), + expected_failed_ids + ); + assert_eq!(status_second_run.error_messages, status.error_messages); + + // The second run makes no changes to the VMMs + let vmms_after_second_run = fetch_vmms(&conn, &vmm_ids).await; + assert_eq!(vmms_after_second_run, vmms_after); + cptestctx.teardown().await; } } From 9ef8b8edc727a5227e0cf05087870df48d27af98 Mon Sep 17 00:00:00 2001 From: karencfv Date: Thu, 8 Oct 2026 21:11:03 +1300 Subject: [PATCH 17/44] clean up --- nexus/db-queries/src/db/datastore/vmm.rs | 9 ++++----- sled-agent/src/common/instance.rs | 1 - sled-agent/src/instance.rs | 2 -- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/vmm.rs b/nexus/db-queries/src/db/datastore/vmm.rs index a2be7ec161f..628aa87d43e 100644 --- a/nexus/db-queries/src/db/datastore/vmm.rs +++ b/nexus/db-queries/src/db/datastore/vmm.rs @@ -1350,9 +1350,10 @@ mod tests { dev::test_setup_log("test_vmm_list_marked_stop_for_update"); let db = TestDatabase::new_with_datastore(&logctx.log).await; let (opctx, datastore) = (db.opctx(), db.datastore()); + let conn = datastore.pool_connection_for_tests().await.unwrap(); - // TODO-K: Fix comment The states from which a VMM can still be stopped - // for an update. Only marked VMMs in these states should be listed by + // The states from which a VMM can still be stopped for an update. Only + // marked VMMs in these states should be listed by // `vmm_list_marked_stop_for_update`. let is_stoppable = |state: DbVmmState| { matches!( @@ -1408,9 +1409,7 @@ mod tests { // Add a filter to ensure there is no full table scan .filter(dsl::id.ne(Uuid::nil())) .count() - .get_result_async( - &*datastore.pool_connection_for_tests().await.unwrap(), - ) + .get_result_async(&*conn) .await .expect("VMMs should be counted"); assert_eq!(row_count, 40); diff --git a/sled-agent/src/common/instance.rs b/sled-agent/src/common/instance.rs index 82c21ff40a3..74faecdbddc 100644 --- a/sled-agent/src/common/instance.rs +++ b/sled-agent/src/common/instance.rs @@ -45,7 +45,6 @@ impl From for VmmState { // perspective. State::Creating | State::Starting => VmmState::Starting, State::Running => VmmState::Running, - // TODO-K: Here are some notes about how the VMM lifecycle works State::Stopping => VmmState::Stopping, // A Propolis that is stopped but not yet destroyed should still // appear to be Stopping from an external API perspective, since diff --git a/sled-agent/src/instance.rs b/sled-agent/src/instance.rs index 30c55465bb8..97531fb4ab0 100644 --- a/sled-agent/src/instance.rs +++ b/sled-agent/src/instance.rs @@ -613,7 +613,6 @@ impl InstanceRunner { let mut state_owner = VmmStateOwner::Runner; - // TODO-K: Part of the VMM stop lifecycle // Timeout for stopping the instance gracefully. // // When we send Propolis a put-state request to transition to @@ -2249,7 +2248,6 @@ impl InstanceRunner { (Some(PropolisRequest::Run), None) } VmmStateRequested::Stopped => { - // TODO-K: More context about VMM lifecycle // If there's no running Propolis yet, unregister this instance // straightaway. Since nothing will ever send a state update in // this case, force the VMM into a terminal Destroyed state, From 97e2afc250354a86deab26b3305ed4f36d618ab6 Mon Sep 17 00:00:00 2001 From: karencfv Date: Mon, 12 Oct 2026 19:23:34 +1300 Subject: [PATCH 18/44] formatting --- dev-tools/omdb/src/bin/omdb/nexus.rs | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/dev-tools/omdb/src/bin/omdb/nexus.rs b/dev-tools/omdb/src/bin/omdb/nexus.rs index 0c1ef4a9b95..5da1ff76334 100644 --- a/dev-tools/omdb/src/bin/omdb/nexus.rs +++ b/dev-tools/omdb/src/bin/omdb/nexus.rs @@ -1526,13 +1526,9 @@ fn print_task_vmm_stop_for_update(details: &serde_json::Value) { vmms_failed_by_sled, error_messages, }) => { - const STOPPED: &str = "VMMs stopped for sled evacuation:"; - const ERROR: &str = "error:"; - const WIDTH: usize = const_max_len(&[STOPPED, ERROR]) + 1; - let total: usize = vmms_stopped_by_sled.iter().map(|s| s.vmm_ids.len()).sum(); - println!(" {STOPPED: Date: Mon, 12 Oct 2026 20:57:55 +1300 Subject: [PATCH 19/44] call notify_instance_updated --- nexus/src/app/background/init.rs | 5 +- .../background/tasks/vmm_stop_for_update.rs | 54 +++++++++++++++---- nexus/src/app/mod.rs | 1 + 3 files changed, 48 insertions(+), 12 deletions(-) diff --git a/nexus/src/app/background/init.rs b/nexus/src/app/background/init.rs index 14ce6599624..c21728de61b 100644 --- a/nexus/src/app/background/init.rs +++ b/nexus/src/app/background/init.rs @@ -321,6 +321,7 @@ impl BackgroundTasksInitializer { let resolver = args.resolver; let sagas = args.saga_starter; let producer_registry = &args.producer_registry; + let nexus = args.nexus; // This "let" construction helps catch mistakes where someone forgets to // wire up an activator to its corresponding background task. @@ -1369,7 +1370,7 @@ impl BackgroundTasksInitializer { update", period: config.vmm_stop_for_update.period_secs, task_impl: Box::new(vmm_stop_for_update::VmmStopForUpdate::new( - datastore, + datastore, nexus, )), opctx: opctx.child(BTreeMap::new()), // Like the vmm_mark_stop_for_update task, this task will be @@ -1405,6 +1406,8 @@ pub struct BackgroundTasksData { pub producer_registry: ProducerRegistry, /// Helpers for saga recovery pub saga_recovery: saga_recovery::SagaRecoveryHelpers>, + /// Handle to `Nexus` + pub nexus: Arc, /// Channel for TUF repository artifacts to be replicated out to sleds pub tuf_artifact_replication_rx: mpsc::Receiver, /// Channel for exposing the latest loaded blueprint diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index dfbfe352de8..9180c0011db 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -5,6 +5,7 @@ //! Background task that stops VMMs that have been marked as needing to be //! stopped for a sled update. +use crate::Nexus; use crate::app::background::BackgroundTask; use crate::app::instance::SledAgentInstanceError; use futures::future::BoxFuture; @@ -13,6 +14,9 @@ use nexus_db_queries::context::OpContext; use nexus_db_queries::db::DataStore; use nexus_db_queries::db::datastore::SQL_BATCH_SIZE; use nexus_db_queries::db::pagination::Paginator; +use nexus_types::instance::SledVmmState; +use nexus_types::instance::VmmRuntimeState; +use nexus_types::instance::VmmState; use nexus_types::internal_api::background::VmmStopForUpdateStatus; use nexus_types::internal_api::background::VmmsBySled; use omicron_common::api::external::Error; @@ -21,9 +25,6 @@ use omicron_uuid_kinds::PropolisUuid; use serde_json::json; use sled_agent_client::types::VmmPutStateBody; use sled_agent_client::types::VmmStateRequested; -use sled_agent_types::instance::SledVmmState; -use sled_agent_types::instance::VmmRuntimeState; -use sled_agent_types::instance::VmmState; use slog_error_chain::InlineErrorChain; use std::sync::Arc; @@ -70,11 +71,12 @@ impl VmmStopForUpdateResult { pub struct VmmStopForUpdate { datastore: Arc, + nexus: Arc, } impl VmmStopForUpdate { - pub fn new(datastore: Arc) -> Self { - Self { datastore } + pub fn new(datastore: Arc, nexus: Arc) -> Self { + Self { datastore, nexus } } /// Retrieves a list of VMMs marked to be stopped for update by sled, and @@ -168,7 +170,7 @@ impl VmmStopForUpdate { ) .await { - Ok(res) => res.into_inner().updated_runtime, + Ok(res) => res.into_inner().updated_runtime.map(Into::into), Err(e) => { let err = SledAgentInstanceError(e); @@ -204,12 +206,37 @@ impl VmmStopForUpdate { "vmm_id" => %id ); } - Some(vmm) => { + Some(sled_vmm_state) => { + // We attempt to notify Nexus that the VMM has been + // updated. This will reduce the time it takes for an + // instance to be reported as `Stopped`. + if let Err(err) = self + .nexus + .notify_vmm_updated(opctx, *id, &sled_vmm_state) + .await + { + // Sled agent will attempt to publish the new state + // to Nexus when the VMM process exits, and we also + // have the instance watcher background task which + // is constantly trying to read instances' states. + // So, it's not a huge deal if we fail to notify + // Nexus here. The instance state will be updated + // eventually. + slog::warn!( + opctx.log, + "Failed to notify Nexus that an instance has been updated"; + "sled_id" => %sled_id, + "vmm_id" => %id, + InlineErrorChain::new(&err), + ); + result.errors.push(err); + }; + let SledVmmState { vmm_state, migration_in: _, migration_out: _, - } = vmm; + } = sled_vmm_state; let VmmRuntimeState { state, generation, time_updated } = vmm_state; @@ -261,13 +288,15 @@ impl VmmStopForUpdate { // 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 => { + VmmState::Failed(reason) => { + let description = reason.description(); slog::info!( opctx.log, "Unable to find VMM, possibly due to a \ restart. No need to stop it"; "sled_id" => %sled_id, "vmm_id" => %id, + "reason" => description, "state" => ?state, "vmm_generation" => ?generation, "time_updated" => ?time_updated, @@ -276,7 +305,9 @@ impl VmmStopForUpdate { VmmState::Migrating | VmmState::Rebooting | VmmState::Starting - | VmmState::Running => { + | VmmState::Running + | VmmState::Creating + | VmmState::SagaUnwound => { slog::error!( opctx.log, "Failed to mark VMM as Stopped, \ @@ -614,6 +645,7 @@ mod tests { let client = &cptestctx.external_client; let datastore = cptestctx.server.server_context().nexus.datastore().clone(); + let nexus = cptestctx.server.server_context().nexus.clone(); let opctx = test_opctx(&cptestctx); let sled_a = cptestctx.first_sled_id(); let sled_b = cptestctx.second_sled_id(); @@ -757,7 +789,7 @@ mod tests { } // Run the task. - let mut task = VmmStopForUpdate::new(datastore.clone()); + let mut task = VmmStopForUpdate::new(datastore.clone(), nexus); let status = task.actually_activate(&opctx).await; // We stopped the VMMs we expected to stop diff --git a/nexus/src/app/mod.rs b/nexus/src/app/mod.rs index 69de95421d5..e77fec18625 100644 --- a/nexus/src/app/mod.rs +++ b/nexus/src/app/mod.rs @@ -728,6 +728,7 @@ impl Nexus { sagas_started_rx: saga_recovery_rx, quiesce: task_nexus.quiesce.sagas(), }, + nexus: task_nexus.clone(), tuf_artifact_replication_rx, mgs_updates_tx, blueprint_load_tx, From f076ccb8baa2a4fc4b8b2344ca27e18502907243 Mon Sep 17 00:00:00 2001 From: karencfv Date: Tue, 13 Oct 2026 13:36:42 +1300 Subject: [PATCH 20/44] add comment --- nexus/src/app/background/tasks/vmm_stop_for_update.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 9180c0011db..3ddc6a93090 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -288,6 +288,11 @@ impl VmmStopForUpdate { // 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. + // + // While a `VmmState::Failed` could also be due to a + // sled agent or sled restart, `vmm_put_state` would + // report those cases as an `Error`, so we don't + // need to handle those specific cases here. VmmState::Failed(reason) => { let description = reason.description(); slog::info!( From 1a636791f5227be80d4b07d40bc92b989a90344e Mon Sep 17 00:00:00 2001 From: karencfv Date: Thu, 15 Oct 2026 15:35:39 +1300 Subject: [PATCH 21/44] change interval for running task --- smf/nexus/multi-sled/config-partial.toml | 11 +++++++---- smf/nexus/single-sled/config-partial.toml | 11 +++++++---- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/smf/nexus/multi-sled/config-partial.toml b/smf/nexus/multi-sled/config-partial.toml index 114ada77b71..0f91d71a1df 100644 --- a/smf/nexus/multi-sled/config-partial.toml +++ b/smf/nexus/multi-sled/config-partial.toml @@ -67,10 +67,13 @@ service_firewall_propagation.period_secs = 300 v2p_mapping_propagation.period_secs = 30 instance_watcher.period_secs = 30 abandoned_vmm_reaper.period_secs = 60 -# The task that marks VMMs as needing to be stopped runs every 300s. Therefore, -# this task doesn't need to run more often. In addition, every time that task -# runs and marks any VMMs, this task is also activated. -vmm_stop_for_update.period_secs = 300 +# How frequently to attempt to stop VMMs that have been marked to stop for an +# update. +# The task that marks VMMs as needing to be stopped runs every 300s. In +# addition, every time that task runs and marks any VMMs, this task is also +# activated. We set this task to run every minute to ensure all VMMs are stopped +# in a timely manner. +vmm_stop_for_update.period_secs = 60 saga_recovery.period_secs = 600 lookup_region_port.period_secs = 60 instance_updater.period_secs = 30 diff --git a/smf/nexus/single-sled/config-partial.toml b/smf/nexus/single-sled/config-partial.toml index ac331089d63..77d9060fbdc 100644 --- a/smf/nexus/single-sled/config-partial.toml +++ b/smf/nexus/single-sled/config-partial.toml @@ -67,10 +67,13 @@ service_firewall_propagation.period_secs = 300 v2p_mapping_propagation.period_secs = 30 instance_watcher.period_secs = 30 abandoned_vmm_reaper.period_secs = 60 -# The task that marks VMMs as needing to be stopped runs every 300s. Therefore, -# this task doesn't need to run more often. In addition, every time that task -# runs and marks any VMMs, this task is also activated. -vmm_stop_for_update.period_secs = 300 +# How frequently to attempt to stop VMMs that have been marked to stop for an +# update. +# The task that marks VMMs as needing to be stopped runs every 300s. In +# addition, every time that task runs and marks any VMMs, this task is also +# activated. We set this task to run every minute to ensure all VMMs are stopped +# in a timely manner. +vmm_stop_for_update.period_secs = 60 saga_recovery.period_secs = 600 lookup_region_port.period_secs = 60 instance_updater.period_secs = 30 From 26cb9ac6a10b2bc0fb0e518c970c56d324be9cba Mon Sep 17 00:00:00 2001 From: karencfv Date: Thu, 15 Oct 2026 16:24:51 +1300 Subject: [PATCH 22/44] remove nexus --- nexus/src/app/background/init.rs | 6 +- .../background/tasks/vmm_stop_for_update.rs | 95 +++++++++++++------ nexus/src/app/mod.rs | 1 - 3 files changed, 69 insertions(+), 33 deletions(-) diff --git a/nexus/src/app/background/init.rs b/nexus/src/app/background/init.rs index c21728de61b..26a44e4fbd4 100644 --- a/nexus/src/app/background/init.rs +++ b/nexus/src/app/background/init.rs @@ -321,7 +321,6 @@ impl BackgroundTasksInitializer { let resolver = args.resolver; let sagas = args.saga_starter; let producer_registry = &args.producer_registry; - let nexus = args.nexus; // This "let" construction helps catch mistakes where someone forgets to // wire up an activator to its corresponding background task. @@ -1370,7 +1369,8 @@ impl BackgroundTasksInitializer { update", period: config.vmm_stop_for_update.period_secs, task_impl: Box::new(vmm_stop_for_update::VmmStopForUpdate::new( - datastore, nexus, + datastore, + sagas.clone(), )), opctx: opctx.child(BTreeMap::new()), // Like the vmm_mark_stop_for_update task, this task will be @@ -1406,8 +1406,6 @@ pub struct BackgroundTasksData { pub producer_registry: ProducerRegistry, /// Helpers for saga recovery pub saga_recovery: saga_recovery::SagaRecoveryHelpers>, - /// Handle to `Nexus` - pub nexus: Arc, /// Channel for TUF repository artifacts to be replicated out to sleds pub tuf_artifact_replication_rx: mpsc::Receiver, /// Channel for exposing the latest loaded blueprint diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 3ddc6a93090..30a17ce67b1 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -5,9 +5,9 @@ //! Background task that stops VMMs that have been marked as needing to be //! stopped for a sled update. -use crate::Nexus; use crate::app::background::BackgroundTask; use crate::app::instance::SledAgentInstanceError; +use crate::app::saga::StartSaga; use futures::future::BoxFuture; use iddqd::IdOrdMap; use nexus_db_queries::context::OpContext; @@ -71,12 +71,15 @@ impl VmmStopForUpdateResult { pub struct VmmStopForUpdate { datastore: Arc, - nexus: Arc, + sagas: Arc, } impl VmmStopForUpdate { - pub fn new(datastore: Arc, nexus: Arc) -> Self { - Self { datastore, nexus } + pub(crate) fn new( + datastore: Arc, + sagas: Arc, + ) -> Self { + Self { datastore, sagas } } /// Retrieves a list of VMMs marked to be stopped for update by sled, and @@ -207,30 +210,65 @@ impl VmmStopForUpdate { ); } Some(sled_vmm_state) => { - // We attempt to notify Nexus that the VMM has been - // updated. This will reduce the time it takes for an - // instance to be reported as `Stopped`. - if let Err(err) = self - .nexus - .notify_vmm_updated(opctx, *id, &sled_vmm_state) - .await + // We attempt to write the new VMM state to the db. + // This will reduce the time it takes for an instance + // to be reported as `Stopped`. + match crate::app::instance::process_vmm_update( + &self.datastore, + opctx, + *id, + &sled_vmm_state, + ) + .await { - // Sled agent will attempt to publish the new state - // to Nexus when the VMM process exits, and we also - // have the instance watcher background task which - // is constantly trying to read instances' states. - // So, it's not a huge deal if we fail to notify - // Nexus here. The instance state will be updated - // eventually. - slog::warn!( - opctx.log, - "Failed to notify Nexus that an instance has been updated"; - "sled_id" => %sled_id, - "vmm_id" => %id, - InlineErrorChain::new(&err), - ); - result.errors.push(err); - }; + Err(err) => { + // Sled agent will attempt to publish the new + // state to Nexus when the VMM process exits, + // and we also have the instance watcher + // background task which is constantly trying to + // read instances' states. So, it's not a huge + // deal if we fail here. The instance state will + // be updated eventually. + slog::warn!( + opctx.log, + "Failed to record new VMM state"; + "sled_id" => %sled_id, + "vmm_id" => %id, + "state" => ?sled_vmm_state, + InlineErrorChain::new(&err), + ); + result.errors.push(err); + } + Ok(Some((_, saga))) => { + match self.sagas.saga_run(saga).await { + Ok((saga_id, completed)) => { + if let Err(err) = completed.await { + slog::warn!( + opctx.log, + "Instance update saga failed"; + "sled_id" => %sled_id, + "vmm_id" => %id, + "saga_id" => %saga_id, + InlineErrorChain::new(&err), + ); + result.errors.push(err); + } + } + Err(err) => { + slog::warn!( + opctx.log, + "Instance update saga could not \ + be started"; + "sled_id" => %sled_id, + "vmm_id" => %id, + InlineErrorChain::new(&err), + ); + result.errors.push(err); + } + } + } + Ok(None) => {} + } let SledVmmState { vmm_state, @@ -794,7 +832,8 @@ mod tests { } // Run the task. - let mut task = VmmStopForUpdate::new(datastore.clone(), nexus); + let mut task = + VmmStopForUpdate::new(datastore.clone(), nexus.sagas.clone()); let status = task.actually_activate(&opctx).await; // We stopped the VMMs we expected to stop diff --git a/nexus/src/app/mod.rs b/nexus/src/app/mod.rs index e77fec18625..69de95421d5 100644 --- a/nexus/src/app/mod.rs +++ b/nexus/src/app/mod.rs @@ -728,7 +728,6 @@ impl Nexus { sagas_started_rx: saga_recovery_rx, quiesce: task_nexus.quiesce.sagas(), }, - nexus: task_nexus.clone(), tuf_artifact_replication_rx, mgs_updates_tx, blueprint_load_tx, From 26779a3a5ee6cafbd222648039b94a754382ade7 Mon Sep 17 00:00:00 2001 From: karencfv Date: Thu, 15 Oct 2026 18:55:26 +1300 Subject: [PATCH 23/44] one reqwest client --- nexus/db-model/src/vmm.rs | 1 - .../background/tasks/vmm_stop_for_update.rs | 33 +++++++++++++++++-- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/nexus/db-model/src/vmm.rs b/nexus/db-model/src/vmm.rs index 2f5c72ae10c..a35ca30768f 100644 --- a/nexus/db-model/src/vmm.rs +++ b/nexus/db-model/src/vmm.rs @@ -104,7 +104,6 @@ impl Vmm { /// /// The new VMM record will be in [`VmmState::Creating`] until it is /// registered with a sled-agent. - // TODO-K: Is the above something we care about here? pub fn new( id: PropolisUuid, instance_id: InstanceUuid, diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 30a17ce67b1..c8383b33584 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -6,6 +6,7 @@ //! stopped for a sled update. use crate::app::background::BackgroundTask; +use crate::app::instance; use crate::app::instance::SledAgentInstanceError; use crate::app::saga::StartSaga; use futures::future::BoxFuture; @@ -131,6 +132,31 @@ impl VmmStopForUpdate { ) -> VmmStopForUpdateResult { let mut result = VmmStopForUpdateResult::new(); + // Build the reqwest client beforehand to avoid building a new one per + // sled below. + let reqwest_client = + match nexus_networking::default_reqwest_client_builder().build() { + Ok(client) => client, + Err(e) => { + let err = InlineErrorChain::new(&e); + + slog::error!( + opctx.log, + "Failed to build the HTTP client"; + &err, + ); + + result.errors.push(Error::InternalError { + internal_message: format!( + "failed to build the HTTP client: {}", + err + ), + }); + + return result; + } + }; + for VmmsBySled { sled_id, vmm_ids } in &vmms_by_sled { slog::info!( opctx.log, @@ -142,11 +168,12 @@ impl VmmStopForUpdate { // Since we're using IdOrdMap, each sled id will only appear once // per batch of VMMs. Because of this, it's ok to start a new sled // agent client per sled id. - let sled_client = match nexus_networking::sled_client( + let sled_client = match nexus_networking::sled_client_ext( &self.datastore, &opctx, *sled_id, &opctx.log, + reqwest_client.clone(), ) .await { @@ -154,7 +181,7 @@ impl VmmStopForUpdate { Err(e) => { slog::error!( opctx.log, - "Failed to create sled agent client"; + "Failed to create the sled agent client"; "sled_id" => %sled_id, InlineErrorChain::new(&e), ); @@ -213,7 +240,7 @@ impl VmmStopForUpdate { // We attempt to write the new VMM state to the db. // This will reduce the time it takes for an instance // to be reported as `Stopped`. - match crate::app::instance::process_vmm_update( + match instance::process_vmm_update( &self.datastore, opctx, *id, From bc3cb0fd57cae7abeb4f16e8f6052defbbb59e33 Mon Sep 17 00:00:00 2001 From: karencfv Date: Mon, 19 Oct 2026 16:32:19 +1300 Subject: [PATCH 24/44] typos and small fixes --- .../background/tasks/vmm_stop_for_update.rs | 24 +++++++++++++------ smf/nexus/multi-sled/config-partial.toml | 6 ----- smf/nexus/single-sled/config-partial.toml | 6 ----- 3 files changed, 17 insertions(+), 19 deletions(-) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index c8383b33584..894ec4647d0 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -265,6 +265,7 @@ impl VmmStopForUpdate { InlineErrorChain::new(&err), ); result.errors.push(err); + continue; } Ok(Some((_, saga))) => { match self.sagas.saga_run(saga).await { @@ -279,6 +280,7 @@ impl VmmStopForUpdate { InlineErrorChain::new(&err), ); result.errors.push(err); + continue; } } Err(err) => { @@ -291,6 +293,7 @@ impl VmmStopForUpdate { InlineErrorChain::new(&err), ); result.errors.push(err); + continue; } } } @@ -388,6 +391,13 @@ impl VmmStopForUpdate { "vmm_generation" => ?generation, "time_updated" => ?time_updated, ); + result.errors.push(Error::InternalError { + internal_message: format!( + "failed to mark VMM {} on sled {} as \ + Stopped; unexpected state reported {}", + id, sled_id, state + ), + }); } } } @@ -527,9 +537,9 @@ mod tests { // In order to stop a VMM, it needs to be registered with the simulated // sled agent. We keep a record of the states of the real VMMs we will be - // stopping. Even though `Created` is a "stoppable" state, we do not include - // it in this list as a VMM is only in this state for a short period. This - // means we cannot reliably test a VMM in the `Creating` state. + // stopping. Even though `Creating` is a "stoppable" state, we do not + // include it in this list as a VMM is only in this state for a short + // period. This means we cannot reliably test a VMM in the `Creating` state. // // The label is used to name instances and groups. const REAL_VMM_STATES: [(&str, DbVmmState); 3] = [ @@ -724,7 +734,7 @@ mod tests { create_default_ip_pools(client).await; create_project(client, PROJECT_NAME).await; - // The simulated agent can only stopp VMMs that are registered with it. + // The simulated agent can only stop VMMs that are registered with it. // So, for each VMM that we intend to stop, we create a real instance // via the external API. // @@ -745,9 +755,9 @@ mod tests { ) .await; match state { - // A new VMM in a simulatated environemnt stays in - // `Starting` until the "poke" API is called, so we can - // confidently test a VMM in this state. + // A new VMM in a simulated environemnt stays in `Starting` + // until the "poke" API is called, so we can confidently + // test a VMM in this state. DbVmmState::Starting => {} DbVmmState::Running => { instance_simulate(&cptestctx, &id).await; diff --git a/smf/nexus/multi-sled/config-partial.toml b/smf/nexus/multi-sled/config-partial.toml index 0f91d71a1df..5c8b58bebbc 100644 --- a/smf/nexus/multi-sled/config-partial.toml +++ b/smf/nexus/multi-sled/config-partial.toml @@ -67,12 +67,6 @@ service_firewall_propagation.period_secs = 300 v2p_mapping_propagation.period_secs = 30 instance_watcher.period_secs = 30 abandoned_vmm_reaper.period_secs = 60 -# How frequently to attempt to stop VMMs that have been marked to stop for an -# update. -# The task that marks VMMs as needing to be stopped runs every 300s. In -# addition, every time that task runs and marks any VMMs, this task is also -# activated. We set this task to run every minute to ensure all VMMs are stopped -# in a timely manner. vmm_stop_for_update.period_secs = 60 saga_recovery.period_secs = 600 lookup_region_port.period_secs = 60 diff --git a/smf/nexus/single-sled/config-partial.toml b/smf/nexus/single-sled/config-partial.toml index 77d9060fbdc..34b0badd2d8 100644 --- a/smf/nexus/single-sled/config-partial.toml +++ b/smf/nexus/single-sled/config-partial.toml @@ -67,12 +67,6 @@ service_firewall_propagation.period_secs = 300 v2p_mapping_propagation.period_secs = 30 instance_watcher.period_secs = 30 abandoned_vmm_reaper.period_secs = 60 -# How frequently to attempt to stop VMMs that have been marked to stop for an -# update. -# The task that marks VMMs as needing to be stopped runs every 300s. In -# addition, every time that task runs and marks any VMMs, this task is also -# activated. We set this task to run every minute to ensure all VMMs are stopped -# in a timely manner. vmm_stop_for_update.period_secs = 60 saga_recovery.period_secs = 600 lookup_region_port.period_secs = 60 From 14a620f24d1b8269f6b0e962986c1c71e11769cb Mon Sep 17 00:00:00 2001 From: karencfv Date: Mon, 19 Oct 2026 20:25:24 +1300 Subject: [PATCH 25/44] use paginated_multicolumn --- nexus/db-queries/src/db/datastore/vmm.rs | 14 ++-- .../background/tasks/vmm_stop_for_update.rs | 64 +++++++++++-------- 2 files changed, 46 insertions(+), 32 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/vmm.rs b/nexus/db-queries/src/db/datastore/vmm.rs index 628aa87d43e..cfb5d295042 100644 --- a/nexus/db-queries/src/db/datastore/vmm.rs +++ b/nexus/db-queries/src/db/datastore/vmm.rs @@ -11,6 +11,7 @@ use crate::db::model; use crate::db::model::Vmm; use crate::db::model::VmmState as DbVmmState; use crate::db::pagination::paginated; +use crate::db::pagination::paginated_multicolumn; use crate::db::update_and_check::UpdateAndCheck; use crate::db::update_and_check::UpdateAndQueryResult; use crate::db::update_and_check::UpdateStatus; @@ -572,12 +573,15 @@ impl DataStore { /// Lists VMMs that have been marked as needing to be stopped for a sled /// update. - pub async fn vmm_list_marked_stop_for_update( + /// + /// The query performed by this function is paginated by the sled and + /// VMM UUIDs, in that order. + pub async fn vmm_list_marked_stop_for_update_by_sled_id( &self, opctx: &OpContext, - pagparams: &DataPageParams<'_, Uuid>, + pagparams: &DataPageParams<'_, (Uuid, Uuid)>, ) -> ListResultVec { - paginated(dsl::vmm, dsl::id, pagparams) + paginated_multicolumn(dsl::vmm, (dsl::sled_id, dsl::id), pagparams) .filter(dsl::time_deleted.is_null()) .filter(dsl::stop_for_update_disposition_generation.is_not_null()) .filter(dsl::state.eq_any(DbVmmState::SHOULD_STOP_FOR_EVACUATION)) @@ -1344,7 +1348,7 @@ mod tests { } #[tokio::test] - async fn test_vmm_list_marked_stop_for_update() { + async fn test_vmm_list_marked_stop_for_update_by_sled_id() { // Setup let logctx = dev::test_setup_log("test_vmm_list_marked_stop_for_update"); @@ -1428,7 +1432,7 @@ mod tests { // Check the retrieved ids for stoppable VMMs are the same as the // expected ones let listed = datastore - .vmm_list_marked_stop_for_update( + .vmm_list_marked_stop_for_update_by_sled_id( &opctx, &DataPageParams::max_page(), ) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 894ec4647d0..df13e8f8c43 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -11,6 +11,7 @@ use crate::app::instance::SledAgentInstanceError; use crate::app::saga::StartSaga; use futures::future::BoxFuture; use iddqd::IdOrdMap; +use nexus_db_model::Vmm; use nexus_db_queries::context::OpContext; use nexus_db_queries::db::DataStore; use nexus_db_queries::db::datastore::SQL_BATCH_SIZE; @@ -94,7 +95,10 @@ impl VmmStopForUpdate { while let Some(p) = paginator.next() { let vmms = match self .datastore - .vmm_list_marked_stop_for_update(opctx, &p.current_pagparams()) + .vmm_list_marked_stop_for_update_by_sled_id( + opctx, + &p.current_pagparams(), + ) .await { Ok(vmms) => vmms, @@ -103,21 +107,11 @@ impl VmmStopForUpdate { break; } }; - paginator = p.found_batch(&vmms, &|vmm| vmm.id); - - let mut vmms_by_sled = IdOrdMap::new(); - for vmm in vmms { - let sled_id = vmm.sled_id(); - vmms_by_sled - .entry(sled_id) - .or_insert_with(|| VmmsBySled { - sled_id, - vmm_ids: Vec::new(), - }) - .vmm_ids - .push(PropolisUuid::from_untyped_uuid(vmm.id)); - } - let batch_result = self.stop_batch(vmms_by_sled, opctx).await; + paginator = p.found_batch(&vmms, &|vmm| { + (vmm.sled_id().into_untyped_uuid(), vmm.id) + }); + + let batch_result = self.stop_batch(&vmms, opctx).await; result.merge(batch_result); } @@ -127,7 +121,7 @@ impl VmmStopForUpdate { /// Stops a batch of VMMs async fn stop_batch( &self, - vmms_by_sled: IdOrdMap, + vmms: &[Vmm], opctx: &OpContext, ) -> VmmStopForUpdateResult { let mut result = VmmStopForUpdateResult::new(); @@ -157,7 +151,22 @@ impl VmmStopForUpdate { } }; - 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()) { + // We only retrieve the first VMM in order to get the sled id. + // The slice is constructed by VMMs with the same sled id, so it's + // fine to just check the first one + let Some(first_vmm) = vmms_per_sled.first() else { + // `chunk_by` never returns an empty slice, so this should never + // be `None`. We handle the case regardless + continue; + }; + let sled_id = first_vmm.sled_id(); + + let vmm_ids: Vec = vmms_per_sled + .iter() + .map(|vmm| PropolisUuid::from_untyped_uuid(vmm.id)) + .collect(); + slog::info!( opctx.log, "Stopping VMMs for update"; @@ -165,13 +174,13 @@ impl VmmStopForUpdate { "vmms" => ?vmm_ids, ); - // Since we're using IdOrdMap, each sled id will only appear once - // per batch of VMMs. Because of this, it's ok to start a new sled - // agent client per sled id. + // Since we're chunking by sled id, each will only appear once per + // batch of VMMs. Because of this, it's ok to start a new sled agent + // client per sled id. let sled_client = match nexus_networking::sled_client_ext( &self.datastore, &opctx, - *sled_id, + sled_id, &opctx.log, reqwest_client.clone(), ) @@ -192,7 +201,8 @@ impl VmmStopForUpdate { } }; - for id in vmm_ids { + // TODO-K: pass the whole VMM not just the ID so I can thread in the instance ID + for id in &vmm_ids { let response = match sled_client .vmm_put_state( id, @@ -215,9 +225,9 @@ impl VmmStopForUpdate { result.errors.push(err.into()); result .vmms_failed_by_sled - .entry(*sled_id) + .entry(sled_id) .or_insert_with(|| VmmsBySled { - sled_id: *sled_id, + sled_id, vmm_ids: Vec::new(), }) .vmm_ids @@ -333,9 +343,9 @@ impl VmmStopForUpdate { | VmmState::Destroyed => { result .vmms_stopped_by_sled - .entry(*sled_id) + .entry(sled_id) .or_insert_with(|| VmmsBySled { - sled_id: *sled_id, + sled_id, vmm_ids: Vec::new(), }) .vmm_ids From c0154d46735b1832919837560a19e9eb2342b2d6 Mon Sep 17 00:00:00 2001 From: karencfv Date: Mon, 19 Oct 2026 20:33:49 +1300 Subject: [PATCH 26/44] put the client where it should go --- .../background/tasks/vmm_stop_for_update.rs | 56 ++++++++++--------- 1 file changed, 30 insertions(+), 26 deletions(-) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index df13e8f8c43..5a54590c265 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -24,6 +24,7 @@ use nexus_types::internal_api::background::VmmsBySled; use omicron_common::api::external::Error; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::PropolisUuid; +use reqwest::Client as ReqwestClient; use serde_json::json; use sled_agent_client::types::VmmPutStateBody; use sled_agent_client::types::VmmStateRequested; @@ -111,7 +112,34 @@ impl VmmStopForUpdate { (vmm.sled_id().into_untyped_uuid(), vmm.id) }); - let batch_result = self.stop_batch(&vmms, opctx).await; + // Build the reqwest client beforehand to avoid building a new one per + // sled below. + let reqwest_client = + match nexus_networking::default_reqwest_client_builder().build() + { + Ok(client) => client, + Err(e) => { + let err = InlineErrorChain::new(&e); + + slog::error!( + opctx.log, + "Failed to build the HTTP client"; + &err, + ); + + result.errors.push(Error::InternalError { + internal_message: format!( + "failed to build the HTTP client: {}", + err + ), + }); + + return result; + } + }; + + let batch_result = + self.stop_batch(&vmms, reqwest_client, opctx).await; result.merge(batch_result); } @@ -122,35 +150,11 @@ impl VmmStopForUpdate { async fn stop_batch( &self, vmms: &[Vmm], + reqwest_client: ReqwestClient, opctx: &OpContext, ) -> VmmStopForUpdateResult { let mut result = VmmStopForUpdateResult::new(); - // Build the reqwest client beforehand to avoid building a new one per - // sled below. - let reqwest_client = - match nexus_networking::default_reqwest_client_builder().build() { - Ok(client) => client, - Err(e) => { - let err = InlineErrorChain::new(&e); - - slog::error!( - opctx.log, - "Failed to build the HTTP client"; - &err, - ); - - result.errors.push(Error::InternalError { - internal_message: format!( - "failed to build the HTTP client: {}", - err - ), - }); - - return result; - } - }; - for vmms_per_sled in vmms.chunk_by(|a, b| a.sled_id() == b.sled_id()) { // We only retrieve the first VMM in order to get the sled id. // The slice is constructed by VMMs with the same sled id, so it's From 9f418e3e3084a15243fd81df698e38d6c2256d16 Mon Sep 17 00:00:00 2001 From: karencfv Date: Mon, 19 Oct 2026 21:58:32 +1300 Subject: [PATCH 27/44] parallel tasks --- .../background/tasks/vmm_stop_for_update.rs | 454 ++++++++++-------- 1 file changed, 253 insertions(+), 201 deletions(-) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 5a54590c265..d73e01fa194 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -24,13 +24,23 @@ use nexus_types::internal_api::background::VmmsBySled; use omicron_common::api::external::Error; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::PropolisUuid; +use omicron_uuid_kinds::SledUuid; +use parallel_task_set::ParallelTaskSet; use reqwest::Client as ReqwestClient; use serde_json::json; use sled_agent_client::types::VmmPutStateBody; use sled_agent_client::types::VmmStateRequested; use slog_error_chain::InlineErrorChain; +use std::collections::BTreeMap; use std::sync::Arc; +/// Determines the number of attempts to stop VMMs (sled agent PUT request and +/// subsequent update saga per VMM) that can execute concurrently. +/// +/// Like the instance-watcher task, we keep this number low to make sure we +/// don't starve the system of resources. +const MAX_CONCURRENT_STOPS: usize = 16; + struct VmmStopForUpdateResult { vmms_stopped_by_sled: IdOrdMap, vmms_failed_by_sled: IdOrdMap, @@ -89,6 +99,11 @@ impl VmmStopForUpdate { /// stops them in batches. async fn stop_all(&self, opctx: &OpContext) -> VmmStopForUpdateResult { let mut result = VmmStopForUpdateResult::new(); + + let mut tasks = + ParallelTaskSet::new_with_parallelism(MAX_CONCURRENT_STOPS); + let task_opctx = Arc::new(opctx.child(BTreeMap::new())); + let mut paginator = Paginator::new( SQL_BATCH_SIZE, dropshot::PaginationOrder::Ascending, @@ -134,13 +149,26 @@ impl VmmStopForUpdate { ), }); - return result; + // Instead of returning here, we break the loop. We want + // to make sure that the operations to stop VMMs that + // are already in-flight don't get canceled by dropping + // `tasks`. + break; } }; - let batch_result = - self.stop_batch(&vmms, reqwest_client, opctx).await; - result.merge(batch_result); + self.stop_batch( + &vmms, + reqwest_client, + &task_opctx, + &mut tasks, + &mut result, + ) + .await; + } + + while let Some(vmm_result) = tasks.join_next().await { + result.merge(vmm_result); } result @@ -151,10 +179,10 @@ impl VmmStopForUpdate { &self, vmms: &[Vmm], reqwest_client: ReqwestClient, - opctx: &OpContext, - ) -> VmmStopForUpdateResult { - let mut result = VmmStopForUpdateResult::new(); - + opctx: &Arc, + tasks: &mut ParallelTaskSet, + result: &mut VmmStopForUpdateResult, + ) { for vmms_per_sled in vmms.chunk_by(|a, b| a.sled_id() == b.sled_id()) { // We only retrieve the first VMM in order to get the sled id. // The slice is constructed by VMMs with the same sled id, so it's @@ -206,220 +234,244 @@ impl VmmStopForUpdate { }; // TODO-K: pass the whole VMM not just the ID so I can thread in the instance ID - for id in &vmm_ids { - let response = match sled_client - .vmm_put_state( - id, - &VmmPutStateBody { state: VmmStateRequested::Stopped }, - ) - .await - { - Ok(res) => res.into_inner().updated_runtime.map(Into::into), - Err(e) => { - let err = SledAgentInstanceError(e); + for vmm_id in vmm_ids { + let stop = + self.stop_vmm(opctx, sled_client.clone(), sled_id, vmm_id); + if let Some(vmm_result) = tasks.spawn(stop).await { + result.merge(vmm_result); + } + } + } + } - slog::error!( - opctx.log, - "Failed to mark VMM as Stopped"; - "sled_id" => %sled_id, - "vmm_id" => %id, - InlineErrorChain::new(&err), - ); + /// Stops a single VMM. + fn stop_vmm( + &self, + opctx: &Arc, + sled_client: sled_agent_client::Client, + sled_id: SledUuid, + vmm_id: PropolisUuid, + ) -> impl Future + Send + 'static + use<> + { + let datastore = self.datastore.clone(); + let sagas = self.sagas.clone(); + let opctx = Arc::clone(opctx); + + async move { + let mut result = VmmStopForUpdateResult::new(); + let id = &vmm_id; + let response = match sled_client + .vmm_put_state( + id, + &VmmPutStateBody { state: VmmStateRequested::Stopped }, + ) + .await + { + Ok(res) => res.into_inner().updated_runtime.map(Into::into), + Err(e) => { + let err = SledAgentInstanceError(e); - result.errors.push(err.into()); - result - .vmms_failed_by_sled - .entry(sled_id) - .or_insert_with(|| VmmsBySled { - sled_id, - vmm_ids: Vec::new(), - }) - .vmm_ids - .push(*id); - - continue; - } - }; + slog::error!( + opctx.log, + "Failed to mark VMM as Stopped"; + "sled_id" => %sled_id, + "vmm_id" => %id, + InlineErrorChain::new(&err), + ); - match response { - None => { - slog::debug!( - opctx.log, - "VMM already stopped, no changes to state"; - "sled_id" => %sled_id, - "vmm_id" => %id - ); - } - Some(sled_vmm_state) => { - // We attempt to write the new VMM state to the db. - // This will reduce the time it takes for an instance - // to be reported as `Stopped`. - match instance::process_vmm_update( - &self.datastore, - opctx, - *id, - &sled_vmm_state, - ) - .await - { - Err(err) => { - // Sled agent will attempt to publish the new - // state to Nexus when the VMM process exits, - // and we also have the instance watcher - // background task which is constantly trying to - // read instances' states. So, it's not a huge - // deal if we fail here. The instance state will - // be updated eventually. - slog::warn!( - opctx.log, - "Failed to record new VMM state"; - "sled_id" => %sled_id, - "vmm_id" => %id, - "state" => ?sled_vmm_state, - InlineErrorChain::new(&err), - ); - result.errors.push(err); - continue; - } - Ok(Some((_, saga))) => { - match self.sagas.saga_run(saga).await { - Ok((saga_id, completed)) => { - if let Err(err) = completed.await { - slog::warn!( - opctx.log, - "Instance update saga failed"; - "sled_id" => %sled_id, - "vmm_id" => %id, - "saga_id" => %saga_id, - InlineErrorChain::new(&err), - ); - result.errors.push(err); - continue; - } - } - Err(err) => { + result.errors.push(err.into()); + result + .vmms_failed_by_sled + .entry(sled_id) + .or_insert_with(|| VmmsBySled { + sled_id, + vmm_ids: Vec::new(), + }) + .vmm_ids + .push(*id); + + return result; + } + }; + + match response { + None => { + slog::debug!( + opctx.log, + "VMM already stopped, no changes to state"; + "sled_id" => %sled_id, + "vmm_id" => %id + ); + } + Some(sled_vmm_state) => { + // We attempt to write the new VMM state to the db. + // This will reduce the time it takes for an instance + // to be reported as `Stopped`. + match instance::process_vmm_update( + &datastore, + &opctx, + *id, + &sled_vmm_state, + ) + .await + { + Err(err) => { + // Sled agent will attempt to publish the new + // state to Nexus when the VMM process exits, + // and we also have the instance watcher + // background task which is constantly trying to + // read instances' states. So, it's not a huge + // deal if we fail here. The instance state will + // be updated eventually. + slog::warn!( + opctx.log, + "Failed to record new VMM state"; + "sled_id" => %sled_id, + "vmm_id" => %id, + "state" => ?sled_vmm_state, + InlineErrorChain::new(&err), + ); + result.errors.push(err); + return result; + } + Ok(Some((_, saga))) => { + match sagas.saga_run(saga).await { + Ok((saga_id, completed)) => { + if let Err(err) = completed.await { slog::warn!( opctx.log, - "Instance update saga could not \ - be started"; + "Instance update saga failed"; "sled_id" => %sled_id, "vmm_id" => %id, + "saga_id" => %saga_id, InlineErrorChain::new(&err), ); result.errors.push(err); - continue; + return result; } } + Err(err) => { + slog::warn!( + opctx.log, + "Instance update saga could not \ + be started"; + "sled_id" => %sled_id, + "vmm_id" => %id, + InlineErrorChain::new(&err), + ); + result.errors.push(err); + return result; + } } - Ok(None) => {} } + Ok(None) => {} + } - let SledVmmState { - vmm_state, - migration_in: _, - migration_out: _, - } = sled_vmm_state; - - let VmmRuntimeState { state, generation, time_updated } = - vmm_state; - - match state { - // If we succeeded, one of three things happened: - // - // - The VMM has not stopped yet, but propolis has - // accepted the `put_state` request and reports - // the VMM's state as `Stopping`. - // - A propolis zone does not exist yet. In this - // scenario a VMM could have been in the - // `Creating` state but had not reached the point - // where the propolis zone was created. In this - // case the request succeeded, but the VMM is - // reported as `Destroyed`. - // - A VMM should technically never be reported as - // `Stopped` since - // propolis_client::types::InstanceState::Stopped - // is always reported as `VmmState::Stopping`. - // But, if for some reason we get this state, - // it's what we want, so we consider it a - // success. - VmmState::Stopped - | VmmState::Stopping - | VmmState::Destroyed => { - result - .vmms_stopped_by_sled - .entry(sled_id) - .or_insert_with(|| VmmsBySled { - sled_id, - vmm_ids: Vec::new(), - }) - .vmm_ids - .push(*id); - - slog::debug!( - opctx.log, - "Stopped VMM for sled evacuation during \ - an update"; - "sled_id" => %sled_id, - "vmm_id" => %id, - "vmm_generation" => ?generation, - "time_updated" => ?time_updated, - ); - } - // 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. - // - // While a `VmmState::Failed` could also be due to a - // sled agent or sled restart, `vmm_put_state` would - // report those cases as an `Error`, so we don't - // need to handle those specific cases here. - VmmState::Failed(reason) => { - let description = reason.description(); - slog::info!( - opctx.log, - "Unable to find VMM, possibly due to a \ - restart. No need to stop it"; - "sled_id" => %sled_id, - "vmm_id" => %id, - "reason" => description, - "state" => ?state, - "vmm_generation" => ?generation, - "time_updated" => ?time_updated, - ); - } - VmmState::Migrating - | VmmState::Rebooting - | VmmState::Starting - | VmmState::Running - | VmmState::Creating - | VmmState::SagaUnwound => { - slog::error!( - opctx.log, - "Failed to mark VMM as Stopped, \ - unexpected state reported"; - "sled_id" => %sled_id, - "vmm_id" => %id, - "state" => ?state, - "vmm_generation" => ?generation, - "time_updated" => ?time_updated, - ); - result.errors.push(Error::InternalError { - internal_message: format!( - "failed to mark VMM {} on sled {} as \ + let SledVmmState { + vmm_state, + migration_in: _, + migration_out: _, + } = sled_vmm_state; + + let VmmRuntimeState { state, generation, time_updated } = + vmm_state; + + match state { + // If we succeeded, one of three things happened: + // + // - The VMM has not stopped yet, but propolis has + // accepted the `put_state` request and reports + // the VMM's state as `Stopping`. + // - A propolis zone does not exist yet. In this + // scenario a VMM could have been in the + // `Creating` state but had not reached the point + // where the propolis zone was created. In this + // case the request succeeded, but the VMM is + // reported as `Destroyed`. + // - A VMM should technically never be reported as + // `Stopped` since + // propolis_client::types::InstanceState::Stopped + // is always reported as `VmmState::Stopping`. + // But, if for some reason we get this state, + // it's what we want, so we consider it a + // success. + VmmState::Stopped + | VmmState::Stopping + | VmmState::Destroyed => { + result + .vmms_stopped_by_sled + .entry(sled_id) + .or_insert_with(|| VmmsBySled { + sled_id, + vmm_ids: Vec::new(), + }) + .vmm_ids + .push(*id); + + slog::debug!( + opctx.log, + "Stopped VMM for sled evacuation during \ + an update"; + "sled_id" => %sled_id, + "vmm_id" => %id, + "vmm_generation" => ?generation, + "time_updated" => ?time_updated, + ); + } + // 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. + // + // While a `VmmState::Failed` could also be due to a + // sled agent or sled restart, `vmm_put_state` would + // report those cases as an `Error`, so we don't + // need to handle those specific cases here. + VmmState::Failed(reason) => { + let description = reason.description(); + slog::info!( + opctx.log, + "Unable to find VMM, possibly due to a \ + restart. No need to stop it"; + "sled_id" => %sled_id, + "vmm_id" => %id, + "reason" => description, + "state" => ?state, + "vmm_generation" => ?generation, + "time_updated" => ?time_updated, + ); + } + VmmState::Migrating + | VmmState::Rebooting + | VmmState::Starting + | VmmState::Running + | VmmState::Creating + | VmmState::SagaUnwound => { + slog::error!( + opctx.log, + "Failed to mark VMM as Stopped, \ + unexpected state reported"; + "sled_id" => %sled_id, + "vmm_id" => %id, + "state" => ?state, + "vmm_generation" => ?generation, + "time_updated" => ?time_updated, + ); + result.errors.push(Error::InternalError { + internal_message: format!( + "failed to mark VMM {} on sled {} as \ Stopped; unexpected state reported {}", - id, sled_id, state - ), - }); - } + id, sled_id, state + ), + }); } } } } - } - result + result + } } pub(crate) async fn actually_activate( From 6d41c219eee2a8fd055f602c4c959d4432d9ed1a Mon Sep 17 00:00:00 2001 From: karencfv Date: Mon, 19 Oct 2026 22:20:20 +1300 Subject: [PATCH 28/44] use BTreeSet and leave TODOs for tomorrow --- nexus/db-model/src/instance.rs | 5 ---- .../background/tasks/vmm_stop_for_update.rs | 24 +++++++++++++------ nexus/types/src/internal_api/background.rs | 3 ++- 3 files changed, 19 insertions(+), 13 deletions(-) diff --git a/nexus/db-model/src/instance.rs b/nexus/db-model/src/instance.rs index bd652c0a74f..b7298886736 100644 --- a/nexus/db-model/src/instance.rs +++ b/nexus/db-model/src/instance.rs @@ -521,11 +521,6 @@ impl InstanceAutoRestart { let now = diesel::dsl::now.into_sql::(); - // TODO-K: For a follow up PR. Do we want to restart instances we are - // evacuating regardless of the auto-restart policy? - // - // Tracking issue: https://github.com/oxidecomputer/omicron/issues/11169 - // The instance's auto-restart policy must allow the control plane // to restart it automatically. // diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index d73e01fa194..344b2ca928d 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -32,6 +32,7 @@ use sled_agent_client::types::VmmPutStateBody; use sled_agent_client::types::VmmStateRequested; use slog_error_chain::InlineErrorChain; use std::collections::BTreeMap; +use std::collections::BTreeSet; use std::sync::Arc; /// Determines the number of attempts to stop VMMs (sled agent PUT request and @@ -67,14 +68,20 @@ impl VmmStopForUpdateResult { for VmmsBySled { sled_id, vmm_ids } in vmms_stopped_by_sled { self.vmms_stopped_by_sled .entry(sled_id) - .or_insert_with(|| VmmsBySled { sled_id, vmm_ids: Vec::new() }) + .or_insert_with(|| VmmsBySled { + sled_id, + vmm_ids: BTreeSet::new(), + }) .vmm_ids .extend(vmm_ids); } for VmmsBySled { sled_id, vmm_ids } in vmms_failed_by_sled { self.vmms_failed_by_sled .entry(sled_id) - .or_insert_with(|| VmmsBySled { sled_id, vmm_ids: Vec::new() }) + .or_insert_with(|| VmmsBySled { + sled_id, + vmm_ids: BTreeSet::new(), + }) .vmm_ids .extend(vmm_ids); } @@ -233,7 +240,8 @@ impl VmmStopForUpdate { } }; - // TODO-K: pass the whole VMM not just the ID so I can thread in the instance ID + // TODO-K: pass the whole VMM not just the ID so I can thread in the + // instance ID for logs for vmm_id in vmm_ids { let stop = self.stop_vmm(opctx, sled_client.clone(), sled_id, vmm_id); @@ -268,6 +276,8 @@ impl VmmStopForUpdate { .await { Ok(res) => res.into_inner().updated_runtime.map(Into::into), + // TODO-K: check if SledAgentInstanceError::vmm_gone returns + // true, and if so, synthesize a Failed instance state update Err(e) => { let err = SledAgentInstanceError(e); @@ -285,10 +295,10 @@ impl VmmStopForUpdate { .entry(sled_id) .or_insert_with(|| VmmsBySled { sled_id, - vmm_ids: Vec::new(), + vmm_ids: BTreeSet::new(), }) .vmm_ids - .push(*id); + .insert(*id); return result; } @@ -403,10 +413,10 @@ impl VmmStopForUpdate { .entry(sled_id) .or_insert_with(|| VmmsBySled { sled_id, - vmm_ids: Vec::new(), + vmm_ids: BTreeSet::new(), }) .vmm_ids - .push(*id); + .insert(*id); slog::debug!( opctx.log, diff --git a/nexus/types/src/internal_api/background.rs b/nexus/types/src/internal_api/background.rs index 75605d33c94..63ae7246975 100644 --- a/nexus/types/src/internal_api/background.rs +++ b/nexus/types/src/internal_api/background.rs @@ -28,6 +28,7 @@ use serde::Serialize; use sled_agent_types::early_networking::SwitchSlot; use slog::Key; use std::collections::BTreeMap; +use std::collections::BTreeSet; use std::collections::HashMap; use std::collections::VecDeque; use std::net::Ipv6Addr; @@ -111,7 +112,7 @@ pub struct AbandonedVmmReaperStatus { #[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Eq)] pub struct VmmsBySled { pub sled_id: SledUuid, - pub vmm_ids: Vec, + pub vmm_ids: BTreeSet, } impl IdOrdItem for VmmsBySled { From e42049bd0a39cb28ce78e4cd97fe5aea976305c9 Mon Sep 17 00:00:00 2001 From: karencfv Date: Mon, 19 Oct 2026 22:29:32 +1300 Subject: [PATCH 29/44] omfg --- .../background/tasks/vmm_stop_for_update.rs | 59 +++++++++---------- 1 file changed, 27 insertions(+), 32 deletions(-) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 344b2ca928d..61e5e303099 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -107,6 +107,31 @@ impl VmmStopForUpdate { async fn stop_all(&self, opctx: &OpContext) -> VmmStopForUpdateResult { let mut result = VmmStopForUpdateResult::new(); + // Build the reqwest client beforehand to avoid building a new one per + // page or sled below. + let reqwest_client = + match nexus_networking::default_reqwest_client_builder().build() { + Ok(client) => client, + Err(e) => { + let err = InlineErrorChain::new(&e); + + slog::error!( + opctx.log, + "Failed to build the HTTP client"; + &err, + ); + + result.errors.push(Error::InternalError { + internal_message: format!( + "failed to build the HTTP client: {}", + err + ), + }); + + return result; + } + }; + let mut tasks = ParallelTaskSet::new_with_parallelism(MAX_CONCURRENT_STOPS); let task_opctx = Arc::new(opctx.child(BTreeMap::new())); @@ -134,39 +159,9 @@ impl VmmStopForUpdate { (vmm.sled_id().into_untyped_uuid(), vmm.id) }); - // Build the reqwest client beforehand to avoid building a new one per - // sled below. - let reqwest_client = - match nexus_networking::default_reqwest_client_builder().build() - { - Ok(client) => client, - Err(e) => { - let err = InlineErrorChain::new(&e); - - slog::error!( - opctx.log, - "Failed to build the HTTP client"; - &err, - ); - - result.errors.push(Error::InternalError { - internal_message: format!( - "failed to build the HTTP client: {}", - err - ), - }); - - // Instead of returning here, we break the loop. We want - // to make sure that the operations to stop VMMs that - // are already in-flight don't get canceled by dropping - // `tasks`. - break; - } - }; - self.stop_batch( &vmms, - reqwest_client, + &reqwest_client, &task_opctx, &mut tasks, &mut result, @@ -185,7 +180,7 @@ impl VmmStopForUpdate { async fn stop_batch( &self, vmms: &[Vmm], - reqwest_client: ReqwestClient, + reqwest_client: &ReqwestClient, opctx: &Arc, tasks: &mut ParallelTaskSet, result: &mut VmmStopForUpdateResult, From 179a4416d8c58d379077a891f5e64c51e0a02948 Mon Sep 17 00:00:00 2001 From: karencfv Date: Tue, 20 Oct 2026 10:02:24 +1300 Subject: [PATCH 30/44] log the instance id --- .../background/tasks/vmm_stop_for_update.rs | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 61e5e303099..a244704d547 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -23,8 +23,8 @@ use nexus_types::internal_api::background::VmmStopForUpdateStatus; use nexus_types::internal_api::background::VmmsBySled; use omicron_common::api::external::Error; use omicron_uuid_kinds::GenericUuid; +use omicron_uuid_kinds::InstanceUuid; use omicron_uuid_kinds::PropolisUuid; -use omicron_uuid_kinds::SledUuid; use parallel_task_set::ParallelTaskSet; use reqwest::Client as ReqwestClient; use serde_json::json; @@ -235,11 +235,9 @@ impl VmmStopForUpdate { } }; - // TODO-K: pass the whole VMM not just the ID so I can thread in the - // instance ID for logs - for vmm_id in vmm_ids { + for vmm in vmms_per_sled { let stop = - self.stop_vmm(opctx, sled_client.clone(), sled_id, vmm_id); + self.stop_vmm(opctx, sled_client.clone(), vmm.clone()); if let Some(vmm_result) = tasks.spawn(stop).await { result.merge(vmm_result); } @@ -252,13 +250,15 @@ impl VmmStopForUpdate { &self, opctx: &Arc, sled_client: sled_agent_client::Client, - sled_id: SledUuid, - vmm_id: PropolisUuid, + vmm: Vmm, ) -> impl Future + Send + 'static + use<> { let datastore = self.datastore.clone(); let sagas = self.sagas.clone(); let opctx = Arc::clone(opctx); + let vmm_id = PropolisUuid::from_untyped_uuid(vmm.id); + let instance_id = InstanceUuid::from_untyped_uuid(vmm.instance_id); + let sled_id = vmm.sled_id(); async move { let mut result = VmmStopForUpdateResult::new(); @@ -281,6 +281,7 @@ impl VmmStopForUpdate { "Failed to mark VMM as Stopped"; "sled_id" => %sled_id, "vmm_id" => %id, + "instance_id" => %instance_id, InlineErrorChain::new(&err), ); @@ -305,7 +306,8 @@ impl VmmStopForUpdate { opctx.log, "VMM already stopped, no changes to state"; "sled_id" => %sled_id, - "vmm_id" => %id + "vmm_id" => %id, + "instance_id" => %instance_id, ); } Some(sled_vmm_state) => { @@ -334,6 +336,7 @@ impl VmmStopForUpdate { "sled_id" => %sled_id, "vmm_id" => %id, "state" => ?sled_vmm_state, + "instance_id" => %instance_id, InlineErrorChain::new(&err), ); result.errors.push(err); @@ -349,6 +352,7 @@ impl VmmStopForUpdate { "sled_id" => %sled_id, "vmm_id" => %id, "saga_id" => %saga_id, + "instance_id" => %instance_id, InlineErrorChain::new(&err), ); result.errors.push(err); @@ -362,6 +366,7 @@ impl VmmStopForUpdate { be started"; "sled_id" => %sled_id, "vmm_id" => %id, + "instance_id" => %instance_id, InlineErrorChain::new(&err), ); result.errors.push(err); @@ -420,6 +425,7 @@ impl VmmStopForUpdate { "sled_id" => %sled_id, "vmm_id" => %id, "vmm_generation" => ?generation, + "instance_id" => %instance_id, "time_updated" => ?time_updated, ); } @@ -441,6 +447,7 @@ impl VmmStopForUpdate { restart. No need to stop it"; "sled_id" => %sled_id, "vmm_id" => %id, + "instance_id" => %instance_id, "reason" => description, "state" => ?state, "vmm_generation" => ?generation, @@ -461,6 +468,7 @@ impl VmmStopForUpdate { "vmm_id" => %id, "state" => ?state, "vmm_generation" => ?generation, + "instance_id" => %instance_id, "time_updated" => ?time_updated, ); result.errors.push(Error::InternalError { From 29db8ef7a1003e347728b3be28ee7ec24d572cf9 Mon Sep 17 00:00:00 2001 From: karencfv Date: Tue, 20 Oct 2026 13:10:15 +1300 Subject: [PATCH 31/44] handle vmm_gone --- .../background/tasks/vmm_stop_for_update.rs | 198 ++++++++++++------ 1 file changed, 134 insertions(+), 64 deletions(-) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index a244704d547..1b279400099 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -17,6 +17,7 @@ use nexus_db_queries::db::DataStore; use nexus_db_queries::db::datastore::SQL_BATCH_SIZE; use nexus_db_queries::db::pagination::Paginator; use nexus_types::instance::SledVmmState; +use nexus_types::instance::VmmFailureReason; use nexus_types::instance::VmmRuntimeState; use nexus_types::instance::VmmState; use nexus_types::internal_api::background::VmmStopForUpdateStatus; @@ -271,32 +272,56 @@ impl VmmStopForUpdate { .await { Ok(res) => res.into_inner().updated_runtime.map(Into::into), - // TODO-K: check if SledAgentInstanceError::vmm_gone returns - // true, and if so, synthesize a Failed instance state update Err(e) => { let err = SledAgentInstanceError(e); - slog::error!( - opctx.log, - "Failed to mark VMM as Stopped"; - "sled_id" => %sled_id, - "vmm_id" => %id, - "instance_id" => %instance_id, - InlineErrorChain::new(&err), - ); - - result.errors.push(err.into()); - result - .vmms_failed_by_sled - .entry(sled_id) - .or_insert_with(|| VmmsBySled { - sled_id, - vmm_ids: BTreeSet::new(), + // 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() { + slog::error!( + opctx.log, + "Failed to mark VMM as Stopped; VMM was already \ + gone. Marking as Failed"; + "sled_id" => %sled_id, + "vmm_id" => %id, + "instance_id" => %instance_id, + InlineErrorChain::new(&err), + ); + + result.errors.push(err.into()); + Some(SledVmmState { + vmm_state: vmm.runtime().transition( + VmmState::Failed( + VmmFailureReason::NoSuchInstance, + ), + ), + migration_in: None, + migration_out: None, }) - .vmm_ids - .insert(*id); - - return result; + } else { + slog::error!( + opctx.log, + "Failed to mark VMM as Stopped"; + "sled_id" => %sled_id, + "vmm_id" => %id, + "instance_id" => %instance_id, + InlineErrorChain::new(&err), + ); + + result.errors.push(err.into()); + result + .vmms_failed_by_sled + .entry(sled_id) + .or_insert_with(|| VmmsBySled { + sled_id, + vmm_ids: BTreeSet::new(), + }) + .vmm_ids + .insert(*id); + + return result; + } } }; @@ -582,6 +607,7 @@ mod tests { use diesel::QueryDsl; use diesel::SelectableHelper; use nexus_db_lookup::AsyncConnection; + use nexus_db_lookup::LookupPath; use nexus_db_model::Generation; use nexus_db_model::InstanceState; use nexus_db_model::Vmm; @@ -589,6 +615,8 @@ mod tests { use nexus_db_model::VmmFailureReason; use nexus_db_model::VmmState as DbVmmState; use nexus_db_model::to_db_typed_generation; + use nexus_db_queries::authz; + use nexus_db_queries::db::pub_test_utils::helpers::create_instance_with_vmm; use nexus_test_utils::ControlPlaneBuilder; use nexus_test_utils::resource_helpers::create_anti_affinity_group; use nexus_test_utils::resource_helpers::create_default_ip_pools; @@ -811,7 +839,12 @@ mod tests { let generation = UpdateDispositionGeneration::from(1); create_default_ip_pools(client).await; - create_project(client, PROJECT_NAME).await; + let project = create_project(client, PROJECT_NAME).await; + let (.., authz_project) = LookupPath::new(&opctx, &datastore) + .project_id(project.identity.id) + .lookup_for(authz::Action::CreateChild) + .await + .expect("project should exist"); // The simulated agent can only stop VMMs that are registered with it. // So, for each VMM that we intend to stop, we create a real instance @@ -880,15 +913,41 @@ mod tests { vmm_ids.push(unmarked.id); if !REAL_VMM_STATES.iter().any(|(_, real)| *real == state) { - let marked = insert_vmm_in_state( - &datastore, - &opctx, - sled_id, - state, - Some(generation), - ) - .await; - vmm_ids.push(marked.id); + let marked_id = if state == DbVmmState::Creating { + // TODO-K: Fix comment The marked `Creating` VMM is the + // active VMM of a real instance, as it would be while + // the instance is starting. No sled agent knows about + // it, so the task records it as `Failed`, and the + // instance-update saga needs the instance to exist. + + // For each desired VMM in the Creating state , we + // create a "real" instance as it would be while no + // sled agent knows about it yet. + let (_, vmm_id) = create_instance_with_vmm( + &opctx, + &datastore, + &authz_project, + &format!("creating-{sled_id}"), + sled_id, + ) + .await; + let vmm_id = vmm_id.into_untyped_uuid(); + mark_vmm_stopped_for_update(&conn, vmm_id, generation) + .await; + vmm_id + } else { + insert_vmm_in_state( + &datastore, + &opctx, + sled_id, + state, + Some(generation), + ) + .await + .id + }; + + vmm_ids.push(marked_id); } } } @@ -911,21 +970,22 @@ mod tests { // to stop for the purposes of this test && vmm.state != DbVmmState::Creating }; - let (expected_rows_to_be_stopped, expected_rows_to_be_unchanged): ( + let (expected_rows_to_be_stopped, expected_rows_to_not_be_stopped): ( Vec, Vec, ) = vmms.values().cloned().partition(should_be_stopped); // Collect the ids of the VMMs in `Creating` state, which are the ones - // we expect to turn up as failed. - let expected_failed_ids: BTreeSet = expected_rows_to_be_unchanged - .iter() - .filter(|vmm| { - vmm.stop_for_update_disposition_generation.is_some() - && vmm.state == DbVmmState::Creating - }) - .map(|vmm| vmm.id) - .collect(); + // we expect to turn up as `Failed`. + let expected_failed_ids: BTreeSet = + expected_rows_to_not_be_stopped + .iter() + .filter(|vmm| { + vmm.stop_for_update_disposition_generation.is_some() + && vmm.state == DbVmmState::Creating + }) + .map(|vmm| vmm.id) + .collect(); assert_eq!(expected_rows_to_be_stopped.len(), 6); assert_eq!(expected_failed_ids.len(), 2); @@ -967,18 +1027,19 @@ mod tests { expected_stopped_ids ); - // The VMMs we expected to fail to stop, failed. - let failed_count: usize = status + // No VMMs are expected to fail to stop + let failed_to_stop_count: usize = status .vmms_failed_by_sled .iter() .map(|sled| sled.vmm_ids.len()) .sum(); - assert_eq!(failed_count, expected_failed_ids.len()); - assert_eq!( - vmm_ids_in_all_sleds(&status.vmms_failed_by_sled), - expected_failed_ids - ); - assert_eq!(status.error_messages.len(), expected_failed_ids.len()); + assert_eq!(failed_to_stop_count, 0); + + // The VMMs that were in the `Creating` state will not have had a VMM + // registered with a sled agent yet. The task will then force a `Failed` + // state which means the VMMs will not be included in the + // `vmms_failed_by_sled` field. But, we still collect the "not found" + // errors. assert_eq!( status.error_messages, vec![ @@ -1021,8 +1082,27 @@ mod tests { ); } + // The initial marked `Creating` VMMs should now be `Failed`. Also, the + // instance-update saga should have marked them as deleted. + for id in &expected_failed_ids { + let actual = &vmms_after[id]; + assert_eq!( + actual.state, + DbVmmState::Failed, + "VMM {id} was not marked as failed", + ); + assert_eq!( + actual.failure_reason, + Some(VmmFailureReason::NoSuchInstance), + ); + assert!(actual.time_deleted.is_some(), "VMM {id} was not deleted"); + } + // Make sure all other rows are unchanged - for expected in &expected_rows_to_be_unchanged { + for expected in expected_rows_to_not_be_stopped + .iter() + .filter(|vmm| !expected_failed_ids.contains(&vmm.id)) + { assert_eq!( &vmms_after[&expected.id], expected, @@ -1039,19 +1119,9 @@ mod tests { // No new VMMs were marked to be stopped so nothing is stopped assert!(status_second_run.vmms_stopped_by_sled.is_empty()); - // The VMMs in `Creating` state still fail because no sled agent is - // aware of them - let failed_count_second_run: usize = status_second_run - .vmms_failed_by_sled - .iter() - .map(|sled| sled.vmm_ids.len()) - .sum(); - assert_eq!(failed_count_second_run, expected_failed_ids.len()); - assert_eq!( - vmm_ids_in_all_sleds(&status_second_run.vmms_failed_by_sled), - expected_failed_ids - ); - assert_eq!(status_second_run.error_messages, status.error_messages); + // Nothing changed, so we expect no errors or failures + assert!(status_second_run.vmms_failed_by_sled.is_empty()); + assert!(status_second_run.error_messages.is_empty()); // The second run makes no changes to the VMMs let vmms_after_second_run = fetch_vmms(&conn, &vmm_ids).await; From f56309cdd3859f3572c642f37f69d3a9fc7b777b Mon Sep 17 00:00:00 2001 From: karencfv Date: Tue, 20 Oct 2026 16:32:33 +1300 Subject: [PATCH 32/44] optimise the optimisation --- .../background/tasks/vmm_stop_for_update.rs | 198 +++++++++++------- 1 file changed, 124 insertions(+), 74 deletions(-) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 1b279400099..f3acb8f65a2 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -26,6 +26,7 @@ use omicron_common::api::external::Error; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::InstanceUuid; use omicron_uuid_kinds::PropolisUuid; +use omicron_uuid_kinds::SledUuid; use parallel_task_set::ParallelTaskSet; use reqwest::Client as ReqwestClient; use serde_json::json; @@ -263,10 +264,9 @@ impl VmmStopForUpdate { async move { let mut result = VmmStopForUpdateResult::new(); - let id = &vmm_id; let response = match sled_client .vmm_put_state( - id, + &vmm_id, &VmmPutStateBody { state: VmmStateRequested::Stopped }, ) .await @@ -284,7 +284,7 @@ impl VmmStopForUpdate { "Failed to mark VMM as Stopped; VMM was already \ gone. Marking as Failed"; "sled_id" => %sled_id, - "vmm_id" => %id, + "vmm_id" => %vmm_id, "instance_id" => %instance_id, InlineErrorChain::new(&err), ); @@ -304,7 +304,7 @@ impl VmmStopForUpdate { opctx.log, "Failed to mark VMM as Stopped"; "sled_id" => %sled_id, - "vmm_id" => %id, + "vmm_id" => %vmm_id, "instance_id" => %instance_id, InlineErrorChain::new(&err), ); @@ -318,7 +318,7 @@ impl VmmStopForUpdate { vmm_ids: BTreeSet::new(), }) .vmm_ids - .insert(*id); + .insert(vmm_id); return result; } @@ -331,76 +331,51 @@ impl VmmStopForUpdate { opctx.log, "VMM already stopped, no changes to state"; "sled_id" => %sled_id, - "vmm_id" => %id, + "vmm_id" => %vmm_id, "instance_id" => %instance_id, ); } Some(sled_vmm_state) => { - // We attempt to write the new VMM state to the db. - // This will reduce the time it takes for an instance - // to be reported as `Stopped`. - match instance::process_vmm_update( - &datastore, + // Updating the VMM as we have done above is, in practice, + // enough to stop the VMM itself. The instance on the other + // hand, is not updated instantaneously to report the new + // state the VMM is in. + // + // There are a few ways in which the instance will know the + // state of the VMM has changed, and therefore its own state + // should change to reflect the VMM's new state: + // + // - The instance watcher backgraound task continuously + // makes sure the instances' states are correct in + // relation to their VMMs. + // - Sled agent will attempt to publish the new state to + // Nexus when the VMM process exits. + // + // Both of these methods are not immediate, and it will take + // some time for the instance to report itself as `Stopped`. + // To reduce the latency from the time we change the Vmm's + // state to the time this is reflected in the instance, we + // do two things: + // + // - Attempt to write the new VMM state to the db with + // `instance::process_vmm_update`. + // - Execute the instance update saga that + // `instance::process_vmm_update` returns to reconcile + // the instance to the new VMM state. + // + // We don't care if either of the previous two steps fails. + // We are running these steps as an _optimisation_ only. The + // instance state will be updated eventually by either the + // instance watcher task, or sled agent. + optimise_instance_state_update( + &mut result, + datastore, + sagas, &opctx, - *id, - &sled_vmm_state, + AssociatedIds { vmm_id, instance_id, sled_id }, + sled_vmm_state.clone(), ) - .await - { - Err(err) => { - // Sled agent will attempt to publish the new - // state to Nexus when the VMM process exits, - // and we also have the instance watcher - // background task which is constantly trying to - // read instances' states. So, it's not a huge - // deal if we fail here. The instance state will - // be updated eventually. - slog::warn!( - opctx.log, - "Failed to record new VMM state"; - "sled_id" => %sled_id, - "vmm_id" => %id, - "state" => ?sled_vmm_state, - "instance_id" => %instance_id, - InlineErrorChain::new(&err), - ); - result.errors.push(err); - return result; - } - Ok(Some((_, saga))) => { - match sagas.saga_run(saga).await { - Ok((saga_id, completed)) => { - if let Err(err) = completed.await { - slog::warn!( - opctx.log, - "Instance update saga failed"; - "sled_id" => %sled_id, - "vmm_id" => %id, - "saga_id" => %saga_id, - "instance_id" => %instance_id, - InlineErrorChain::new(&err), - ); - result.errors.push(err); - return result; - } - } - Err(err) => { - slog::warn!( - opctx.log, - "Instance update saga could not \ - be started"; - "sled_id" => %sled_id, - "vmm_id" => %id, - "instance_id" => %instance_id, - InlineErrorChain::new(&err), - ); - result.errors.push(err); - return result; - } - } - } - Ok(None) => {} - } + .await; let SledVmmState { vmm_state, @@ -441,14 +416,14 @@ impl VmmStopForUpdate { vmm_ids: BTreeSet::new(), }) .vmm_ids - .insert(*id); + .insert(vmm_id); slog::debug!( opctx.log, "Stopped VMM for sled evacuation during \ an update"; "sled_id" => %sled_id, - "vmm_id" => %id, + "vmm_id" => %vmm_id, "vmm_generation" => ?generation, "instance_id" => %instance_id, "time_updated" => ?time_updated, @@ -471,7 +446,7 @@ impl VmmStopForUpdate { "Unable to find VMM, possibly due to a \ restart. No need to stop it"; "sled_id" => %sled_id, - "vmm_id" => %id, + "vmm_id" => %vmm_id, "instance_id" => %instance_id, "reason" => description, "state" => ?state, @@ -490,7 +465,7 @@ impl VmmStopForUpdate { "Failed to mark VMM as Stopped, \ unexpected state reported"; "sled_id" => %sled_id, - "vmm_id" => %id, + "vmm_id" => %vmm_id, "state" => ?state, "vmm_generation" => ?generation, "instance_id" => %instance_id, @@ -500,7 +475,7 @@ impl VmmStopForUpdate { internal_message: format!( "failed to mark VMM {} on sled {} as \ Stopped; unexpected state reported {}", - id, sled_id, state + vmm_id, sled_id, state ), }); } @@ -593,6 +568,81 @@ impl BackgroundTask for VmmStopForUpdate { } } +// This struct exists solely to avoid a clippy "too_many_arguments" error +struct AssociatedIds { + vmm_id: PropolisUuid, + instance_id: InstanceUuid, + sled_id: SledUuid, +} + +// Attempt to update the instance state +async fn optimise_instance_state_update( + result: &mut VmmStopForUpdateResult, + datastore: Arc, + sagas: Arc, + opctx: &Arc, + ids: AssociatedIds, + sled_vmm_state: SledVmmState, +) { + let AssociatedIds { vmm_id, instance_id, sled_id } = ids; + + // Attempt to write the new VMM state to the db. + match instance::process_vmm_update( + &datastore, + opctx, + vmm_id, + &sled_vmm_state, + ) + .await + { + Err(err) => { + // As mentioned earlier, it's not a huge deal if we + // fail here. The instance state will be updated + // eventually by the instance watcher task or sled + // agent. + slog::warn!( + opctx.log, + "Failed to record new VMM state"; + "sled_id" => %sled_id, + "vmm_id" => %vmm_id, + "state" => ?sled_vmm_state, + "instance_id" => %instance_id, + InlineErrorChain::new(&err), + ); + result.errors.push(err); + } + Ok(Some((_, saga))) => match sagas.saga_run(saga).await { + Ok((saga_id, completed)) => { + if let Err(err) = completed.await { + slog::warn!( + opctx.log, + "Instance update saga failed"; + "sled_id" => %sled_id, + "vmm_id" => %vmm_id, + "saga_id" => %saga_id, + "instance_id" => %instance_id, + InlineErrorChain::new(&err), + ); + result.errors.push(err); + } + } + Err(err) => { + slog::warn!( + opctx.log, + "Instance update saga could not \ + be started"; + "sled_id" => %sled_id, + "vmm_id" => %vmm_id, + "instance_id" => %instance_id, + InlineErrorChain::new(&err), + ); + result.errors.push(err); + } + }, + Ok(None) => {} + } +} + #[cfg(test)] mod tests { use super::*; From a603bfa51f279b1760db1ca4c01c8dfff9a77db6 Mon Sep 17 00:00:00 2001 From: karencfv Date: Tue, 20 Oct 2026 16:48:08 +1300 Subject: [PATCH 33/44] clean up --- nexus/src/app/background/tasks/vmm_stop_for_update.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index f3acb8f65a2..5c8035a57be 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -951,7 +951,7 @@ mod tests { // task skips it entirely. // // Also, we are including the VMMs in `Creating` state. As mentioned - // earlier, these cannot be realiably tested to stop, but we can + // earlier, these cannot be reliably tested to stop, but we can // conveniently use them to simulate VMMs that failed to stop. We insert // them and mark them as needing to be stopped. for sled_id in [sled_a, sled_b] { @@ -964,12 +964,6 @@ mod tests { if !REAL_VMM_STATES.iter().any(|(_, real)| *real == state) { let marked_id = if state == DbVmmState::Creating { - // TODO-K: Fix comment The marked `Creating` VMM is the - // active VMM of a real instance, as it would be while - // the instance is starting. No sled agent knows about - // it, so the task records it as `Failed`, and the - // instance-update saga needs the instance to exist. - // For each desired VMM in the Creating state , we // create a "real" instance as it would be while no // sled agent knows about it yet. From 80eafe6c93f29786d9f742342329508fd3346210 Mon Sep 17 00:00:00 2001 From: karencfv Date: Tue, 20 Oct 2026 17:20:31 +1300 Subject: [PATCH 34/44] improve documentation --- .../background/tasks/vmm_stop_for_update.rs | 22 +++++++++++++------ 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 5c8035a57be..a8fb0e9bc1b 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -429,22 +429,30 @@ impl VmmStopForUpdate { "time_updated" => ?time_updated, ); } - // 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. + // `vmm_put_state` only reports the `VmmState::Failed` + // state when Propolis is alive but does not find a VMM. + // This implies that it restarted and lost the + // previously created VMM. + // + // Additionally, When `vmm_put_state` returns a + // `NO_SUCH_INSTANCE`` error, we transition the runtime + // state to `VmmState::Failed` above. // // While a `VmmState::Failed` could also be due to a // sled agent or sled restart, `vmm_put_state` would // report those cases as an `Error`, so we don't // need to handle those specific cases here. + // + // Because of this, it's not necessary to stop it + // anymore, so we don't add it to the + // `vmms_failed_by_sled` list. The instance + // reincarnation task will automatically attempt to + // restart any instance that is in a failed state. VmmState::Failed(reason) => { let description = reason.description(); slog::info!( opctx.log, - "Unable to find VMM, possibly due to a \ - restart. No need to stop it"; + "Unable to find VMM. No need to stop it"; "sled_id" => %sled_id, "vmm_id" => %vmm_id, "instance_id" => %instance_id, From a30797d5674c1b351df929ba6090d79920995f1d Mon Sep 17 00:00:00 2001 From: karencfv Date: Tue, 20 Oct 2026 17:26:21 +1300 Subject: [PATCH 35/44] make field more explicit --- dev-tools/omdb/src/bin/omdb/nexus.rs | 14 ++++++---- .../background/tasks/vmm_stop_for_update.rs | 28 +++++++++---------- nexus/types/src/internal_api/background.rs | 2 +- 3 files changed, 23 insertions(+), 21 deletions(-) diff --git a/dev-tools/omdb/src/bin/omdb/nexus.rs b/dev-tools/omdb/src/bin/omdb/nexus.rs index 5da1ff76334..5ab76a3c880 100644 --- a/dev-tools/omdb/src/bin/omdb/nexus.rs +++ b/dev-tools/omdb/src/bin/omdb/nexus.rs @@ -1523,7 +1523,7 @@ fn print_task_vmm_stop_for_update(details: &serde_json::Value) { ), Ok(VmmStopForUpdateStatus { vmms_stopped_by_sled, - vmms_failed_by_sled, + vmms_failed_to_stop_by_sled, error_messages, }) => { let total: usize = @@ -1536,13 +1536,15 @@ fn print_task_vmm_stop_for_update(details: &serde_json::Value) { sled.vmm_ids.len() ); } - if !vmms_failed_by_sled.is_empty() { - let total_failed: usize = - vmms_failed_by_sled.iter().map(|s| s.vmm_ids.len()).sum(); + if !vmms_failed_to_stop_by_sled.is_empty() { + let total_failed: usize = vmms_failed_to_stop_by_sled + .iter() + .map(|s| s.vmm_ids.len()) + .sum(); println!(" VMMs that failed to stop: {total_failed} total"); - for sled in vmms_failed_by_sled { + for sled in vmms_failed_to_stop_by_sled { println!( - " sled_id={} n_failed={}", + " sled_id={} n_failed_to_stop={}", sled.sled_id, sled.vmm_ids.len() ); diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index a8fb0e9bc1b..e366cbdb6fe 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -46,7 +46,7 @@ const MAX_CONCURRENT_STOPS: usize = 16; struct VmmStopForUpdateResult { vmms_stopped_by_sled: IdOrdMap, - vmms_failed_by_sled: IdOrdMap, + vmms_failed_to_stop_by_sled: IdOrdMap, errors: Vec, } @@ -54,7 +54,7 @@ impl VmmStopForUpdateResult { fn new() -> Self { Self { vmms_stopped_by_sled: IdOrdMap::new(), - vmms_failed_by_sled: IdOrdMap::new(), + vmms_failed_to_stop_by_sled: IdOrdMap::new(), errors: Vec::new(), } } @@ -63,7 +63,7 @@ impl VmmStopForUpdateResult { fn merge(&mut self, other: VmmStopForUpdateResult) { let VmmStopForUpdateResult { vmms_stopped_by_sled, - vmms_failed_by_sled, + vmms_failed_to_stop_by_sled, errors, } = other; @@ -77,8 +77,8 @@ impl VmmStopForUpdateResult { .vmm_ids .extend(vmm_ids); } - for VmmsBySled { sled_id, vmm_ids } in vmms_failed_by_sled { - self.vmms_failed_by_sled + for VmmsBySled { sled_id, vmm_ids } in vmms_failed_to_stop_by_sled { + self.vmms_failed_to_stop_by_sled .entry(sled_id) .or_insert_with(|| VmmsBySled { sled_id, @@ -311,7 +311,7 @@ impl VmmStopForUpdate { result.errors.push(err.into()); result - .vmms_failed_by_sled + .vmms_failed_to_stop_by_sled .entry(sled_id) .or_insert_with(|| VmmsBySled { sled_id, @@ -445,7 +445,7 @@ impl VmmStopForUpdate { // // Because of this, it's not necessary to stop it // anymore, so we don't add it to the - // `vmms_failed_by_sled` list. The instance + // `vmms_failed_to_stop_by_sled` list. The instance // reincarnation task will automatically attempt to // restart any instance that is in a failed state. VmmState::Failed(reason) => { @@ -501,7 +501,7 @@ impl VmmStopForUpdate { ) -> VmmStopForUpdateStatus { let VmmStopForUpdateResult { vmms_stopped_by_sled, - vmms_failed_by_sled, + vmms_failed_to_stop_by_sled, errors, } = self.stop_all(opctx).await; @@ -528,8 +528,8 @@ impl VmmStopForUpdate { } } - if !vmms_failed_by_sled.is_empty() { - for sled in &vmms_failed_by_sled { + if !vmms_failed_to_stop_by_sled.is_empty() { + for sled in &vmms_failed_to_stop_by_sled { let VmmsBySled { sled_id, vmm_ids } = sled; slog::info!( &opctx.log, @@ -551,7 +551,7 @@ impl VmmStopForUpdate { VmmStopForUpdateStatus { vmms_stopped_by_sled, - vmms_failed_by_sled, + vmms_failed_to_stop_by_sled, error_messages, } } @@ -1081,7 +1081,7 @@ mod tests { // No VMMs are expected to fail to stop let failed_to_stop_count: usize = status - .vmms_failed_by_sled + .vmms_failed_to_stop_by_sled .iter() .map(|sled| sled.vmm_ids.len()) .sum(); @@ -1090,7 +1090,7 @@ mod tests { // The VMMs that were in the `Creating` state will not have had a VMM // registered with a sled agent yet. The task will then force a `Failed` // state which means the VMMs will not be included in the - // `vmms_failed_by_sled` field. But, we still collect the "not found" + // `vmms_failed_to_stop_by_sled` field. But, we still collect the "not found" // errors. assert_eq!( status.error_messages, @@ -1172,7 +1172,7 @@ mod tests { assert!(status_second_run.vmms_stopped_by_sled.is_empty()); // Nothing changed, so we expect no errors or failures - assert!(status_second_run.vmms_failed_by_sled.is_empty()); + assert!(status_second_run.vmms_failed_to_stop_by_sled.is_empty()); assert!(status_second_run.error_messages.is_empty()); // The second run makes no changes to the VMMs diff --git a/nexus/types/src/internal_api/background.rs b/nexus/types/src/internal_api/background.rs index 63ae7246975..b6f63e6fc26 100644 --- a/nexus/types/src/internal_api/background.rs +++ b/nexus/types/src/internal_api/background.rs @@ -133,7 +133,7 @@ pub struct VmmStopForUpdateStatus { pub vmms_stopped_by_sled: IdOrdMap, /// Number of VMMs that failed to stop in this activation, keyed by the sled /// they are running on. - pub vmms_failed_by_sled: IdOrdMap, + pub vmms_failed_to_stop_by_sled: IdOrdMap, /// Errors encountered during this activation, if any. pub error_messages: Vec, } From 2533d117bafc8496b62cc48e0f3131d5d0e9df07 Mon Sep 17 00:00:00 2001 From: karencfv Date: Wed, 21 Oct 2026 12:57:27 +1300 Subject: [PATCH 36/44] cache the reqwest client --- .../background/tasks/vmm_stop_for_update.rs | 49 +++++++++++-------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index e366cbdb6fe..3fcc89ec3e2 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -94,6 +94,7 @@ impl VmmStopForUpdateResult { pub struct VmmStopForUpdate { datastore: Arc, sagas: Arc, + reqwest_client: Option, } impl VmmStopForUpdate { @@ -101,38 +102,46 @@ impl VmmStopForUpdate { datastore: Arc, sagas: Arc, ) -> Self { - Self { datastore, sagas } + Self { datastore, sagas, reqwest_client: None } } /// Retrieves a list of VMMs marked to be stopped for update by sled, and /// stops them in batches. - async fn stop_all(&self, opctx: &OpContext) -> VmmStopForUpdateResult { + async fn stop_all(&mut self, opctx: &OpContext) -> VmmStopForUpdateResult { let mut result = VmmStopForUpdateResult::new(); // Build the reqwest client beforehand to avoid building a new one per // page or sled below. - let reqwest_client = - match nexus_networking::default_reqwest_client_builder().build() { - Ok(client) => client, - Err(e) => { - let err = InlineErrorChain::new(&e); + 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); - slog::error!( - opctx.log, - "Failed to build the HTTP client"; - &err, - ); + slog::error!( + opctx.log, + "Failed to build the HTTP client"; + &err, + ); - result.errors.push(Error::InternalError { - internal_message: format!( - "failed to build the HTTP client: {}", - err - ), - }); + result.errors.push(Error::InternalError { + internal_message: format!( + "failed to build the HTTP client: {}", + err + ), + }); - return result; + return result; + } } - }; + } + }; let mut tasks = ParallelTaskSet::new_with_parallelism(MAX_CONCURRENT_STOPS); From 73529c986fb285fbf9cedd83397b79a18f2d57d6 Mon Sep 17 00:00:00 2001 From: karencfv Date: Wed, 21 Oct 2026 13:55:44 +1300 Subject: [PATCH 37/44] fix logs --- .../app/background/tasks/vmm_stop_for_update.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 3fcc89ec3e2..5a55353c467 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -288,7 +288,7 @@ impl VmmStopForUpdate { // 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() { - slog::error!( + slog::info!( opctx.log, "Failed to mark VMM as Stopped; VMM was already \ gone. Marking as Failed"; @@ -309,7 +309,7 @@ impl VmmStopForUpdate { migration_out: None, }) } else { - slog::error!( + slog::info!( opctx.log, "Failed to mark VMM as Stopped"; "sled_id" => %sled_id, @@ -477,7 +477,7 @@ impl VmmStopForUpdate { | VmmState::Running | VmmState::Creating | VmmState::SagaUnwound => { - slog::error!( + slog::warn!( opctx.log, "Failed to mark VMM as Stopped, \ unexpected state reported"; @@ -522,14 +522,14 @@ impl VmmStopForUpdate { if vmms_stopped_by_sled.is_empty() { slog::info!( &opctx.log, - "no VMMs were stopped for a sled update"; + "No VMMs were stopped for a sled update"; ); } else { for sled in &vmms_stopped_by_sled { let VmmsBySled { sled_id, vmm_ids } = sled; slog::info!( &opctx.log, - "stopped {} VMMs marked to stop for a sled update", + "Stopped {} VMMs marked to stop for a sled update", vmm_ids.len(); "sled_id" => %sled_id, "vmms" => ?vmm_ids, @@ -542,7 +542,7 @@ impl VmmStopForUpdate { let VmmsBySled { sled_id, vmm_ids } = sled; slog::info!( &opctx.log, - "failed to stop {} VMMs marked to stop for a sled update", + "Failed to stop {} VMMs marked to stop for a sled update", vmm_ids.len(); "sled_id" => %sled_id, "vmms" => ?vmm_ids, @@ -551,9 +551,10 @@ impl VmmStopForUpdate { } if !error_messages.is_empty() { - slog::error!( + slog::info!( &opctx.log, - "failed to stop VMMs marked to stop for a sled update"; + "Attempting to stop VMMs marked to stop for a sled update \ + returned errors"; "errors" => ?error_messages, ); } From 1e77105f4a5c8e0fa81e74f2258caa9508cc80f5 Mon Sep 17 00:00:00 2001 From: karencfv Date: Wed, 21 Oct 2026 14:10:50 +1300 Subject: [PATCH 38/44] fix typos --- nexus/src/app/background/tasks/vmm_stop_for_update.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 5a55353c467..242d928eb91 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -354,7 +354,7 @@ impl VmmStopForUpdate { // state of the VMM has changed, and therefore its own state // should change to reflect the VMM's new state: // - // - The instance watcher backgraound task continuously + // - The instance watcher background task continuously // makes sure the instances' states are correct in // relation to their VMMs. // - Sled agent will attempt to publish the new state to @@ -444,7 +444,7 @@ impl VmmStopForUpdate { // previously created VMM. // // Additionally, When `vmm_put_state` returns a - // `NO_SUCH_INSTANCE`` error, we transition the runtime + // `NO_SUCH_INSTANCE` error, we transition the runtime // state to `VmmState::Failed` above. // // While a `VmmState::Failed` could also be due to a @@ -935,7 +935,7 @@ mod tests { ) .await; match state { - // A new VMM in a simulated environemnt stays in `Starting` + // A new VMM in a simulated environment stays in `Starting` // until the "poke" API is called, so we can confidently // test a VMM in this state. DbVmmState::Starting => {} From 7355d30a9771be2309302219acb424a7292ff509 Mon Sep 17 00:00:00 2001 From: karencfv Date: Thu, 22 Oct 2026 13:11:15 +1300 Subject: [PATCH 39/44] do not stop Creating VMMs --- nexus/db-model/src/vmm_state.rs | 61 +++++++++- nexus/db-queries/src/db/datastore/vmm.rs | 11 +- .../background/tasks/vmm_stop_for_update.rs | 115 +++--------------- 3 files changed, 80 insertions(+), 107 deletions(-) diff --git a/nexus/db-model/src/vmm_state.rs b/nexus/db-model/src/vmm_state.rs index 51718113920..fa978c6a92a 100644 --- a/nexus/db-model/src/vmm_state.rs +++ b/nexus/db-model/src/vmm_state.rs @@ -107,16 +107,29 @@ impl VmmState { self.to_nexus_state().exists_on_sled() } + /// The states in which a VMM should be marked to be stopped during sled + /// evacuation. + pub const SHOULD_MARK_TO_STOP_FOR_EVACUATION: &'static [Self] = &[ + // A VMM in one of these states is on its way, or is already running, + // and can be marked to 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 marked. + ]; + /// The states in which a VMM should be stopped during sled evacuation. pub const SHOULD_STOP_FOR_EVACUATION: &'static [Self] = &[ // A VMM in one of these states is on its way, or is already running, - // and can be stopped. - VmmState::Creating, + // is registered with a sled agent, and can be stopped. 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. + // migrating, is not registered with a sled agent yet, or has + // terminated, so it does not need to be stopped. ]; } @@ -451,7 +464,7 @@ mod tests { } #[test] - fn test_should_stop_for_update_states_consistent() { + fn test_should_mark_to_stop_for_update_states_consistent() { // 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 @@ -469,12 +482,52 @@ mod tests { VmmState::Creating | VmmState::Starting | VmmState::Running + | VmmState::Rebooting => { + assert!( + VmmState::SHOULD_MARK_TO_STOP_FOR_EVACUATION + .contains(state) + ); + } + VmmState::Failed + | VmmState::Stopping + | VmmState::Stopped + | VmmState::Migrating + | VmmState::Destroyed + | VmmState::SagaUnwound => { + assert!( + !VmmState::SHOULD_MARK_TO_STOP_FOR_EVACUATION + .contains(state) + ); + } + } + } + } + + #[test] + fn test_should_stop_for_update_states_consistent() { + // This test exists to make sure that when a new state is added, we + // consider whether it is a state where reconfigurator should stop a VMM + // 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 stopped. Do *not* change the match + // here to be non-exhaustive! + for state in VmmState::ALL_STATES.iter() { + match state { + VmmState::Starting + | VmmState::Running | VmmState::Rebooting => { assert!( VmmState::SHOULD_STOP_FOR_EVACUATION.contains(state) ); } + VmmState::Failed + | VmmState::Creating | VmmState::Stopping | VmmState::Stopped | VmmState::Migrating diff --git a/nexus/db-queries/src/db/datastore/vmm.rs b/nexus/db-queries/src/db/datastore/vmm.rs index cfb5d295042..9243533cb31 100644 --- a/nexus/db-queries/src/db/datastore/vmm.rs +++ b/nexus/db-queries/src/db/datastore/vmm.rs @@ -216,7 +216,7 @@ impl DataStore { model::DbSledBpAvailability::Unavailable, ) .bind::, _>( - DbVmmState::SHOULD_STOP_FOR_EVACUATION.to_vec(), + DbVmmState::SHOULD_MARK_TO_STOP_FOR_EVACUATION.to_vec(), ) .bind::(i64::from(batch_size.get())) .execute_async(&*conn) @@ -576,6 +576,8 @@ impl DataStore { /// /// The query performed by this function is paginated by the sled and /// VMM UUIDs, in that order. + // TODO-K: Probably change the name here? It's retrieving stoppable states, + // not all of the VMMs marked to stop pub async fn vmm_list_marked_stop_for_update_by_sled_id( &self, opctx: &OpContext, @@ -1362,8 +1364,7 @@ mod tests { let is_stoppable = |state: DbVmmState| { matches!( state, - DbVmmState::Creating - | DbVmmState::Starting + DbVmmState::Starting | DbVmmState::Running | DbVmmState::Rebooting ) @@ -1418,7 +1419,7 @@ mod tests { .expect("VMMs should be counted"); assert_eq!(row_count, 40); - // Check there are 8 stoppable VMMs (4 per sled) + // Check there are 6 stoppable VMMs (3 per sled) let expected_ids: BTreeSet = vmms .iter() .filter(|vmm| { @@ -1427,7 +1428,7 @@ mod tests { }) .map(|vmm| vmm.id) .collect(); - assert_eq!(expected_ids.len(), 8); + assert_eq!(expected_ids.len(), 6); // Check the retrieved ids for stoppable VMMs are the same as the // expected ones diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 242d928eb91..654e7c3a4d7 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -675,7 +675,6 @@ mod tests { use diesel::QueryDsl; use diesel::SelectableHelper; use nexus_db_lookup::AsyncConnection; - use nexus_db_lookup::LookupPath; use nexus_db_model::Generation; use nexus_db_model::InstanceState; use nexus_db_model::Vmm; @@ -683,8 +682,6 @@ mod tests { use nexus_db_model::VmmFailureReason; use nexus_db_model::VmmState as DbVmmState; use nexus_db_model::to_db_typed_generation; - use nexus_db_queries::authz; - use nexus_db_queries::db::pub_test_utils::helpers::create_instance_with_vmm; use nexus_test_utils::ControlPlaneBuilder; use nexus_test_utils::resource_helpers::create_anti_affinity_group; use nexus_test_utils::resource_helpers::create_default_ip_pools; @@ -712,9 +709,7 @@ mod tests { // In order to stop a VMM, it needs to be registered with the simulated // sled agent. We keep a record of the states of the real VMMs we will be - // stopping. Even though `Creating` is a "stoppable" state, we do not - // include it in this list as a VMM is only in this state for a short - // period. This means we cannot reliably test a VMM in the `Creating` state. + // stopping. // // The label is used to name instances and groups. const REAL_VMM_STATES: [(&str, DbVmmState); 3] = [ @@ -907,12 +902,7 @@ mod tests { let generation = UpdateDispositionGeneration::from(1); create_default_ip_pools(client).await; - let project = create_project(client, PROJECT_NAME).await; - let (.., authz_project) = LookupPath::new(&opctx, &datastore) - .project_id(project.identity.id) - .lookup_for(authz::Action::CreateChild) - .await - .expect("project should exist"); + create_project(client, PROJECT_NAME).await; // The simulated agent can only stop VMMs that are registered with it. // So, for each VMM that we intend to stop, we create a real instance @@ -963,15 +953,9 @@ mod tests { // rows directly in the DB. The task should skip them entirely, so they // don't need to be registered with sled agent. // - // Even though this shouldn't happen in practice, we insert marked - // non-stoppable VMMs. This ensures that even if the task to mark VMMs - // marks a VMM that is in a state that it shouldn't be stopped in, this - // task skips it entirely. - // - // Also, we are including the VMMs in `Creating` state. As mentioned - // earlier, these cannot be reliably tested to stop, but we can - // conveniently use them to simulate VMMs that failed to stop. We insert - // them and mark them as needing to be stopped. + // We insert marked non-stoppable VMMs. This ensures that even if the + // task to mark VMMs marks a VMM that is in a state that it shouldn't be + // stopped in, this task skips it entirely. for sled_id in [sled_a, sled_b] { for &state in DbVmmState::ALL_STATES { let unmarked = insert_vmm_in_state( @@ -981,35 +965,15 @@ mod tests { vmm_ids.push(unmarked.id); if !REAL_VMM_STATES.iter().any(|(_, real)| *real == state) { - let marked_id = if state == DbVmmState::Creating { - // For each desired VMM in the Creating state , we - // create a "real" instance as it would be while no - // sled agent knows about it yet. - let (_, vmm_id) = create_instance_with_vmm( - &opctx, - &datastore, - &authz_project, - &format!("creating-{sled_id}"), - sled_id, - ) - .await; - let vmm_id = vmm_id.into_untyped_uuid(); - mark_vmm_stopped_for_update(&conn, vmm_id, generation) - .await; - vmm_id - } else { - insert_vmm_in_state( - &datastore, - &opctx, - sled_id, - state, - Some(generation), - ) - .await - .id - }; - - vmm_ids.push(marked_id); + let marked = insert_vmm_in_state( + &datastore, + &opctx, + sled_id, + state, + Some(generation), + ) + .await; + vmm_ids.push(marked.id); } } } @@ -1028,29 +992,13 @@ mod tests { let should_be_stopped = |vmm: &Vmm| { vmm.stop_for_update_disposition_generation.is_some() && DbVmmState::SHOULD_STOP_FOR_EVACUATION.contains(&vmm.state) - // As mentioned above, we don't expect VMMs in `Creating` state - // to stop for the purposes of this test - && vmm.state != DbVmmState::Creating }; let (expected_rows_to_be_stopped, expected_rows_to_not_be_stopped): ( Vec, Vec, ) = vmms.values().cloned().partition(should_be_stopped); - // Collect the ids of the VMMs in `Creating` state, which are the ones - // we expect to turn up as `Failed`. - let expected_failed_ids: BTreeSet = - expected_rows_to_not_be_stopped - .iter() - .filter(|vmm| { - vmm.stop_for_update_disposition_generation.is_some() - && vmm.state == DbVmmState::Creating - }) - .map(|vmm| vmm.id) - .collect(); - assert_eq!(expected_rows_to_be_stopped.len(), 6); - assert_eq!(expected_failed_ids.len(), 2); // Confirm there is one of each stoppable state per sled in // `expected_rows_to_be_stopped` @@ -1097,18 +1045,8 @@ mod tests { .sum(); assert_eq!(failed_to_stop_count, 0); - // The VMMs that were in the `Creating` state will not have had a VMM - // registered with a sled agent yet. The task will then force a `Failed` - // state which means the VMMs will not be included in the - // `vmms_failed_to_stop_by_sled` field. But, we still collect the "not found" - // errors. - assert_eq!( - status.error_messages, - vec![ - "Invalid Request: Not Found".to_string(), - "Invalid Request: Not Found".to_string() - ] - ); + // No errors are expected + assert!(status.error_messages.is_empty()); // Poke each simulated instance so each sled agent reports back to Nexus // and wait until each one reports `NoVmm` @@ -1144,27 +1082,8 @@ mod tests { ); } - // The initial marked `Creating` VMMs should now be `Failed`. Also, the - // instance-update saga should have marked them as deleted. - for id in &expected_failed_ids { - let actual = &vmms_after[id]; - assert_eq!( - actual.state, - DbVmmState::Failed, - "VMM {id} was not marked as failed", - ); - assert_eq!( - actual.failure_reason, - Some(VmmFailureReason::NoSuchInstance), - ); - assert!(actual.time_deleted.is_some(), "VMM {id} was not deleted"); - } - // Make sure all other rows are unchanged - for expected in expected_rows_to_not_be_stopped - .iter() - .filter(|vmm| !expected_failed_ids.contains(&vmm.id)) - { + for expected in &expected_rows_to_not_be_stopped { assert_eq!( &vmms_after[&expected.id], expected, From a0b22ca38deb879f01468cc5f6371c8afe67381e Mon Sep 17 00:00:00 2001 From: karencfv Date: Thu, 22 Oct 2026 14:23:32 +1300 Subject: [PATCH 40/44] only retrieve vmms in in-service sleds --- nexus/db-queries/src/db/datastore/vmm.rs | 28 +++++++++++++------ .../background/tasks/vmm_stop_for_update.rs | 2 +- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/vmm.rs b/nexus/db-queries/src/db/datastore/vmm.rs index 9243533cb31..b900bb2b532 100644 --- a/nexus/db-queries/src/db/datastore/vmm.rs +++ b/nexus/db-queries/src/db/datastore/vmm.rs @@ -25,10 +25,12 @@ use nexus_db_errors::ErrorHandler; use nexus_db_errors::OptionalError; use nexus_db_errors::public_error_from_diesel; use nexus_db_lookup::DbConnection; +use nexus_db_model::ApplySledFilterExt; use nexus_db_schema::enums::SledBpAvailabilityEnum; use nexus_db_schema::enums::VmmStateEnum; use nexus_db_schema::schema::vmm; use nexus_db_schema::schema::vmm::dsl; +use nexus_types::deployment::SledFilter; use nexus_types::instance::Migrations; use nexus_types::instance::VmmRuntimeState; use omicron_common::api::external::CreateResult; @@ -572,21 +574,23 @@ impl DataStore { } /// Lists VMMs that have been marked as needing to be stopped for a sled - /// update. + /// update, and that *can* be stopped. /// /// The query performed by this function is paginated by the sled and /// VMM UUIDs, in that order. - // TODO-K: Probably change the name here? It's retrieving stoppable states, - // not all of the VMMs marked to stop - pub async fn vmm_list_marked_stop_for_update_by_sled_id( + pub async fn vmm_list_stoppable_for_update_by_sled_id( &self, opctx: &OpContext, pagparams: &DataPageParams<'_, (Uuid, Uuid)>, ) -> ListResultVec { + use nexus_db_schema::schema::sled::dsl as sled_dsl; + paginated_multicolumn(dsl::vmm, (dsl::sled_id, dsl::id), pagparams) + .inner_join(sled_dsl::sled.on(sled_dsl::id.eq(dsl::sled_id))) .filter(dsl::time_deleted.is_null()) .filter(dsl::stop_for_update_disposition_generation.is_not_null()) .filter(dsl::state.eq_any(DbVmmState::SHOULD_STOP_FOR_EVACUATION)) + .sled_filter(SledFilter::InService) .select(Vmm::as_select()) .load_async(&*self.pool_connection_authorized(opctx).await?) .await @@ -601,6 +605,7 @@ mod tests { use crate::db::model::Generation; use crate::db::model::Migration; use crate::db::pub_test_utils::TestDatabase; + use crate::db::pub_test_utils::helpers::SledUpdateBuilder; use crate::db::pub_test_utils::helpers::create_vmm_for_instance; use iddqd::IdOrdMap; use nexus_db_model::ActiveSledBpAvailability; @@ -1350,17 +1355,17 @@ mod tests { } #[tokio::test] - async fn test_vmm_list_marked_stop_for_update_by_sled_id() { + async fn test_vmm_list_stoppable_for_update_by_sled_id() { // Setup let logctx = - dev::test_setup_log("test_vmm_list_marked_stop_for_update"); + dev::test_setup_log("test_vmm_list_stoppable_for_update_by_sled_id"); let db = TestDatabase::new_with_datastore(&logctx.log).await; let (opctx, datastore) = (db.opctx(), db.datastore()); let conn = datastore.pool_connection_for_tests().await.unwrap(); // The states from which a VMM can still be stopped for an update. Only // marked VMMs in these states should be listed by - // `vmm_list_marked_stop_for_update`. + // `vmm_list_stoppable_for_update_by_sled_id`. let is_stoppable = |state: DbVmmState| { matches!( state, @@ -1377,6 +1382,13 @@ mod tests { let sled_b = SledUuid::new_v4(); let mut vmms = Vec::new(); for sled_id in [sled_a, sled_b] { + // The query only returns VMMs on in-service sleds. We store the two + // sleds in the db. New sleds are in-service by default. + datastore + .sled_upsert(SledUpdateBuilder::new().sled_id(sled_id).build()) + .await + .expect("sled should be upserted successfully"); + for &state in DbVmmState::ALL_STATES { for marker in [Some(generation), None] { let failure_reason = (state == DbVmmState::Failed) @@ -1433,7 +1445,7 @@ mod tests { // Check the retrieved ids for stoppable VMMs are the same as the // expected ones let listed = datastore - .vmm_list_marked_stop_for_update_by_sled_id( + .vmm_list_stoppable_for_update_by_sled_id( &opctx, &DataPageParams::max_page(), ) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 654e7c3a4d7..f2ba60504ad 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -154,7 +154,7 @@ impl VmmStopForUpdate { while let Some(p) = paginator.next() { let vmms = match self .datastore - .vmm_list_marked_stop_for_update_by_sled_id( + .vmm_list_stoppable_for_update_by_sled_id( opctx, &p.current_pagparams(), ) From 6e18f90b166feaf83e7ef5ba082cf79a26883bed Mon Sep 17 00:00:00 2001 From: karencfv Date: Thu, 22 Oct 2026 15:00:15 +1300 Subject: [PATCH 41/44] adapt tests --- nexus/db-queries/src/db/datastore/vmm.rs | 39 ++++++++++++++++++------ 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/vmm.rs b/nexus/db-queries/src/db/datastore/vmm.rs index b900bb2b532..cb3da23f08f 100644 --- a/nexus/db-queries/src/db/datastore/vmm.rs +++ b/nexus/db-queries/src/db/datastore/vmm.rs @@ -601,6 +601,7 @@ impl DataStore { #[cfg(test)] mod tests { use super::*; + use crate::authz; use crate::db; use crate::db::model::Generation; use crate::db::model::Migration; @@ -608,6 +609,7 @@ mod tests { use crate::db::pub_test_utils::helpers::SledUpdateBuilder; use crate::db::pub_test_utils::helpers::create_vmm_for_instance; use iddqd::IdOrdMap; + use nexus_db_lookup::LookupPath; use nexus_db_model::ActiveSledBpAvailability; use nexus_db_model::SledBlueprintAvailabilityInput; use nexus_db_model::SledBpAvailabilityState; @@ -1357,8 +1359,9 @@ mod tests { #[tokio::test] async fn test_vmm_list_stoppable_for_update_by_sled_id() { // Setup - let logctx = - dev::test_setup_log("test_vmm_list_stoppable_for_update_by_sled_id"); + let logctx = dev::test_setup_log( + "test_vmm_list_stoppable_for_update_by_sled_id", + ); let db = TestDatabase::new_with_datastore(&logctx.log).await; let (opctx, datastore) = (db.opctx(), db.datastore()); let conn = datastore.pool_connection_for_tests().await.unwrap(); @@ -1376,19 +1379,34 @@ mod tests { }; // Insert 2 VMMs (one marked to stop, one not) in each state per sled - // (2 sleds only). + // (2 in-service and 1 expunged). let generation = UpdateDispositionGeneration::from(1); let sled_a = SledUuid::new_v4(); let sled_b = SledUuid::new_v4(); + let sled_expunged = SledUuid::new_v4(); let mut vmms = Vec::new(); - for sled_id in [sled_a, sled_b] { - // The query only returns VMMs on in-service sleds. We store the two - // sleds in the db. New sleds are in-service by default. + for sled_id in [sled_a, sled_b, sled_expunged] { + // The query only returns VMMs on in-service sleds. We store the + // three sleds in the db. datastore .sled_upsert(SledUpdateBuilder::new().sled_id(sled_id).build()) .await .expect("sled should be upserted successfully"); + // New sleds are in-service by default. We expunge one of them to + // test that no VMMs from that sled are returned. + if sled_id == sled_expunged { + let (.., authz_sled) = LookupPath::new(&opctx, datastore) + .sled_id(sled_id) + .lookup_for(authz::Action::Modify) + .await + .expect("sled should exist"); + datastore + .sled_set_policy_to_expunged(&opctx, &authz_sled) + .await + .expect("sled should be expunged"); + } + for &state in DbVmmState::ALL_STATES { for marker in [Some(generation), None] { let failure_reason = (state == DbVmmState::Failed) @@ -1420,7 +1438,7 @@ mod tests { } } - // There should be 40 VMM rows. 2 VMMs (one marked, one not) per state + // There should be 60 VMM rows. 2 VMMs (one marked, one not) per state // per sled let row_count: i64 = dsl::vmm // Add a filter to ensure there is no full table scan @@ -1429,13 +1447,14 @@ mod tests { .get_result_async(&*conn) .await .expect("VMMs should be counted"); - assert_eq!(row_count, 40); + assert_eq!(row_count, 60); - // Check there are 6 stoppable VMMs (3 per sled) + // Check there are 6 stoppable VMMs (3 per in-service sled). let expected_ids: BTreeSet = vmms .iter() .filter(|vmm| { - vmm.stop_for_update_disposition_generation.is_some() + vmm.sled_id() != sled_expunged + && vmm.stop_for_update_disposition_generation.is_some() && is_stoppable(vmm.state) }) .map(|vmm| vmm.id) From 63d36780af141df19929ff09a3918e648e224c48 Mon Sep 17 00:00:00 2001 From: karencfv Date: Thu, 22 Oct 2026 15:06:31 +1300 Subject: [PATCH 42/44] fix typos before John finds them --- nexus/src/app/background/tasks/vmm_stop_for_update.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index f2ba60504ad..976e2d3e6e5 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -362,7 +362,7 @@ impl VmmStopForUpdate { // // Both of these methods are not immediate, and it will take // some time for the instance to report itself as `Stopped`. - // To reduce the latency from the time we change the Vmm's + // To reduce the latency from the time we change the `Vmm`'s // state to the time this is reflected in the instance, we // do two things: // @@ -370,7 +370,7 @@ impl VmmStopForUpdate { // `instance::process_vmm_update`. // - Execute the instance update saga that // `instance::process_vmm_update` returns to reconcile - // the instance to the new VMM state. + // the instance to the new VMM state. // // We don't care if either of the previous two steps fails. // We are running these steps as an _optimisation_ only. The @@ -443,7 +443,7 @@ impl VmmStopForUpdate { // This implies that it restarted and lost the // previously created VMM. // - // Additionally, When `vmm_put_state` returns a + // Additionally, when `vmm_put_state` returns a // `NO_SUCH_INSTANCE` error, we transition the runtime // state to `VmmState::Failed` above. // @@ -806,7 +806,7 @@ mod tests { } // Inserts a VMM row without sled agent knowing about it. This is used for - // VMM in states that we don't expect to stop. + // VMMs in states that we don't expect to stop. async fn insert_vmm_in_state( datastore: &DataStore, opctx: &OpContext, From 54357d1012bb7680c7789a54d615229839ceec2f Mon Sep 17 00:00:00 2001 From: karencfv Date: Tue, 27 Oct 2026 17:20:42 +1300 Subject: [PATCH 43/44] address comments --- nexus/db-queries/src/db/datastore/vmm.rs | 6 +- nexus/examples/config-second.toml | 2 +- nexus/examples/config.toml | 2 +- .../background/tasks/vmm_stop_for_update.rs | 93 +++++++++---------- 4 files changed, 51 insertions(+), 52 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/vmm.rs b/nexus/db-queries/src/db/datastore/vmm.rs index cb3da23f08f..8efe06e8a8e 100644 --- a/nexus/db-queries/src/db/datastore/vmm.rs +++ b/nexus/db-queries/src/db/datastore/vmm.rs @@ -178,9 +178,9 @@ impl DataStore { /// Marks VMMs sleds that are evacuating as needing to be stopped in order /// to update the sled. /// - /// VMMs that are not in one of the `SHOULD_STOP_FOR_EVACUATION` 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. + /// VMMs that are not in one of the `SHOULD_MARK_TO_STOP_FOR_EVACUATION` + /// 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. /// /// The task to stop instances only needs to know whether a VMM should be /// stopped or not, so a boolean would be enough. But, we mark the VMMs diff --git a/nexus/examples/config-second.toml b/nexus/examples/config-second.toml index fae85043cfd..9f16dc2fc06 100644 --- a/nexus/examples/config-second.toml +++ b/nexus/examples/config-second.toml @@ -162,7 +162,7 @@ instance_reincarnation.period_secs = 60 service_firewall_propagation.period_secs = 300 v2p_mapping_propagation.period_secs = 30 abandoned_vmm_reaper.period_secs = 60 -vmm_stop_for_update.period_secs = 300 +vmm_stop_for_update.period_secs = 60 saga_recovery.period_secs = 600 lookup_region_port.period_secs = 60 region_snapshot_replacement_start.period_secs = 30 diff --git a/nexus/examples/config.toml b/nexus/examples/config.toml index aad8cecca1d..17bdf0ffc8b 100644 --- a/nexus/examples/config.toml +++ b/nexus/examples/config.toml @@ -153,7 +153,7 @@ instance_reincarnation.period_secs = 60 service_firewall_propagation.period_secs = 300 v2p_mapping_propagation.period_secs = 30 abandoned_vmm_reaper.period_secs = 60 -vmm_stop_for_update.period_secs = 300 +vmm_stop_for_update.period_secs = 60 saga_recovery.period_secs = 600 lookup_region_port.period_secs = 60 region_snapshot_replacement_start.period_secs = 30 diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index 976e2d3e6e5..df46b7fa7dc 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -376,7 +376,7 @@ impl VmmStopForUpdate { // We are running these steps as an _optimisation_ only. The // instance state will be updated eventually by either the // instance watcher task, or sled agent. - optimise_instance_state_update( + try_instance_state_update( &mut result, datastore, sagas, @@ -401,11 +401,7 @@ impl VmmStopForUpdate { // - The VMM has not stopped yet, but propolis has // accepted the `put_state` request and reports // the VMM's state as `Stopping`. - // - A propolis zone does not exist yet. In this - // scenario a VMM could have been in the - // `Creating` state but had not reached the point - // where the propolis zone was created. In this - // case the request succeeded, but the VMM is + // - A propolis zone does not exist; the VMM is // reported as `Destroyed`. // - A VMM should technically never be reported as // `Stopped` since @@ -447,11 +443,6 @@ impl VmmStopForUpdate { // `NO_SUCH_INSTANCE` error, we transition the runtime // state to `VmmState::Failed` above. // - // While a `VmmState::Failed` could also be due to a - // sled agent or sled restart, `vmm_put_state` would - // report those cases as an `Error`, so we don't - // need to handle those specific cases here. - // // Because of this, it's not necessary to stop it // anymore, so we don't add it to the // `vmms_failed_to_stop_by_sled` list. The instance @@ -593,8 +584,12 @@ struct AssociatedIds { sled_id: SledUuid, } -// Attempt to update the instance state -async fn optimise_instance_state_update( +// Attempt to update the instance state. +// +// This function is intended to be run by a caller that does not care whether +// the instance state is actually successfully updated or not. It does not +// Errors are not returned, but rather logged and saved in `VmmStopForUpdateResult`. +async fn try_instance_state_update( result: &mut VmmStopForUpdateResult, datastore: Arc, sagas: Arc, @@ -605,7 +600,7 @@ async fn optimise_instance_state_update( let AssociatedIds { vmm_id, instance_id, sled_id } = ids; // Attempt to write the new VMM state to the db. - match instance::process_vmm_update( + let saga = match instance::process_vmm_update( &datastore, opctx, vmm_id, @@ -613,11 +608,10 @@ async fn optimise_instance_state_update( ) .await { + // As mentioned earlier, it's not a huge deal if we fail here. The + // instance state will be updated eventually by the instance watcher + // task or sled agent. Err(err) => { - // As mentioned earlier, it's not a huge deal if we - // fail here. The instance state will be updated - // eventually by the instance watcher task or sled - // agent. slog::warn!( opctx.log, "Failed to record new VMM state"; @@ -628,36 +622,41 @@ async fn optimise_instance_state_update( InlineErrorChain::new(&err), ); result.errors.push(err); + return; } - Ok(Some((_, saga))) => match sagas.saga_run(saga).await { - Ok((saga_id, completed)) => { - if let Err(err) = completed.await { - slog::warn!( - opctx.log, - "Instance update saga failed"; - "sled_id" => %sled_id, - "vmm_id" => %vmm_id, - "saga_id" => %saga_id, - "instance_id" => %instance_id, - InlineErrorChain::new(&err), - ); - result.errors.push(err); - } - } - Err(err) => { - slog::warn!( - opctx.log, - "Instance update saga could not \ - be started"; - "sled_id" => %sled_id, - "vmm_id" => %vmm_id, - "instance_id" => %instance_id, - InlineErrorChain::new(&err), - ); - result.errors.push(err); - } - }, - Ok(None) => {} + // The new state would like an instance-update saga, so we'll try to + // run it. + Ok(Some((_, saga))) => saga, + // The new state does not require an update saga, nothing left to do + // here. + Ok(None) => return, + }; + let (saga_id, completed) = match sagas.saga_run(saga).await { + Ok(s) => s, + Err(err) => { + slog::warn!( + opctx.log, + "Instance update saga could not be started"; + "sled_id" => %sled_id, + "vmm_id" => %vmm_id, + "instance_id" => %instance_id, + InlineErrorChain::new(&err), + ); + result.errors.push(err); + return; + } + }; + if let Err(err) = completed.await { + slog::warn!( + opctx.log, + "Instance update saga failed"; + "sled_id" => %sled_id, + "vmm_id" => %vmm_id, + "saga_id" => %saga_id, + "instance_id" => %instance_id, + InlineErrorChain::new(&err), + ); + result.errors.push(err); } } From 304c2f8486584e37729d9b28a6eea68db5186b9a Mon Sep 17 00:00:00 2001 From: karencfv Date: Tue, 27 Oct 2026 17:58:01 +1300 Subject: [PATCH 44/44] improve testing on failures --- .../app/background/tasks/vmm_stop_for_update.rs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/nexus/src/app/background/tasks/vmm_stop_for_update.rs b/nexus/src/app/background/tasks/vmm_stop_for_update.rs index df46b7fa7dc..0f020aa530f 100644 --- a/nexus/src/app/background/tasks/vmm_stop_for_update.rs +++ b/nexus/src/app/background/tasks/vmm_stop_for_update.rs @@ -1021,6 +1021,14 @@ mod tests { VmmStopForUpdate::new(datastore.clone(), nexus.sagas.clone()); let status = task.actually_activate(&opctx).await; + // No errors are expected + assert_eq!( + status.error_messages.len(), + 0, + "unexpected errors: {:?}", + status.error_messages + ); + // We stopped the VMMs we expected to stop let stopped_count: usize = status .vmms_stopped_by_sled @@ -1045,7 +1053,12 @@ mod tests { assert_eq!(failed_to_stop_count, 0); // No errors are expected - assert!(status.error_messages.is_empty()); + assert_eq!( + status.error_messages.len(), + 0, + "unexpected errors: {:?}", + status.error_messages + ); // Poke each simulated instance so each sled agent reports back to Nexus // and wait until each one reports `NoVmm`