From 17cda03354c5bbc69c14b62f23a7d433aa182f4d Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 6 Jun 2025 11:59:16 -0700 Subject: [PATCH 01/18] first cut --- nexus/db-model/src/deployment.rs | 50 +++- nexus/db-model/src/lib.rs | 1 + .../db-queries/src/db/datastore/deployment.rs | 271 +++++++++++++++++- nexus/db-schema/src/schema.rs | 13 + schema/crdb/dbinit.sql | 21 ++ 5 files changed, 347 insertions(+), 9 deletions(-) diff --git a/nexus/db-model/src/deployment.rs b/nexus/db-model/src/deployment.rs index b427ba6cc20..be8c47e140f 100644 --- a/nexus/db-model/src/deployment.rs +++ b/nexus/db-model/src/deployment.rs @@ -5,12 +5,13 @@ //! Types for representing the deployed software and configuration in the //! database -use crate::inventory::ZoneType; +use crate::inventory::{SpMgsSlot, SpType, ZoneType}; use crate::omicron_zone_config::{self, OmicronZoneNic}; use crate::typed_uuid::DbTypedUuid; use crate::{ - ArtifactHash, ByteCount, DbOximeterReadMode, Generation, MacAddr, Name, - SledState, SqlU8, SqlU16, SqlU32, TufArtifact, impl_enum_type, ipv6, + ArtifactHash, ByteCount, DbArtifactVersion, DbOximeterReadMode, Generation, + MacAddr, Name, SledState, SqlU8, SqlU16, SqlU32, TufArtifact, + impl_enum_type, ipv6, }; use anyhow::{Context, Result, anyhow, bail}; use chrono::{DateTime, Utc}; @@ -21,10 +22,10 @@ use nexus_db_schema::schema::{ bp_clickhouse_keeper_zone_id_to_node_id, bp_clickhouse_server_zone_id_to_node_id, bp_omicron_dataset, bp_omicron_physical_disk, bp_omicron_zone, bp_omicron_zone_nic, - bp_oximeter_read_policy, bp_sled_metadata, bp_target, + bp_oximeter_read_policy, bp_pending_mgs_update_sp, bp_sled_metadata, + bp_target, }; use nexus_sled_agent_shared::inventory::OmicronZoneDataset; -use nexus_types::deployment::BlueprintDatasetDisposition; use nexus_types::deployment::BlueprintPhysicalDiskConfig; use nexus_types::deployment::BlueprintPhysicalDiskDisposition; use nexus_types::deployment::BlueprintTarget; @@ -33,14 +34,18 @@ use nexus_types::deployment::BlueprintZoneDisposition; use nexus_types::deployment::BlueprintZoneType; use nexus_types::deployment::ClickhouseClusterConfig; use nexus_types::deployment::CockroachDbPreserveDowngrade; +use nexus_types::deployment::PendingMgsUpdate; +use nexus_types::deployment::PendingMgsUpdateDetails; use nexus_types::deployment::{ BlueprintDatasetConfig, BlueprintZoneImageVersion, OximeterReadMode, }; +use nexus_types::deployment::{BlueprintDatasetDisposition, ExpectedVersion}; use nexus_types::deployment::{BlueprintZoneImageSource, blueprint_zone_type}; use nexus_types::deployment::{ OmicronZoneExternalFloatingAddr, OmicronZoneExternalFloatingIp, OmicronZoneExternalSnatIp, }; +use nexus_types::inventory::BaseboardId; use omicron_common::api::internal::shared::NetworkInterface; use omicron_common::disk::DiskIdentity; use omicron_common::zpool_name::ZpoolName; @@ -50,6 +55,7 @@ use omicron_uuid_kinds::{ PhysicalDiskKind, SledKind, SledUuid, ZpoolKind, ZpoolUuid, }; use std::net::{IpAddr, SocketAddrV6}; +use std::sync::Arc; use uuid::Uuid; /// See [`nexus_types::deployment::Blueprint`]. @@ -1239,3 +1245,37 @@ impl BpOximeterReadPolicy { } } } + +#[derive(Queryable, Clone, Debug, Selectable, Insertable)] +#[diesel(table_name = bp_pending_mgs_update_sp)] +pub struct BpPendingMgsUpdateSp { + pub blueprint_id: DbTypedUuid, + pub hw_baseboard_id: Uuid, + pub sp_type: SpType, + pub sp_slot: SpMgsSlot, + pub artifact_sha256: ArtifactHash, + pub artifact_version: DbArtifactVersion, + pub expected_active_version: DbArtifactVersion, + pub expected_inactive_version: Option, +} + +impl BpPendingMgsUpdateSp { + pub fn into_generic(self, baseboard_id: Arc) -> PendingMgsUpdate { + PendingMgsUpdate { + baseboard_id, + sp_type: self.sp_type.into(), + slot_id: u32::from(**self.sp_slot), + artifact_hash: self.artifact_sha256.into(), + artifact_version: (*self.artifact_version).clone(), + details: PendingMgsUpdateDetails::Sp { + expected_active_version: (*self.expected_active_version) + .clone(), + expected_inactive_version: match self.expected_inactive_version + { + Some(v) => ExpectedVersion::Version((*v).clone()), + None => ExpectedVersion::NoValidVersion, + }, + }, + } + } +} diff --git a/nexus/db-model/src/lib.rs b/nexus/db-model/src/lib.rs index e0a0910f2aa..31d1161355c 100644 --- a/nexus/db-model/src/lib.rs +++ b/nexus/db-model/src/lib.rs @@ -233,6 +233,7 @@ pub use switch_interface::*; pub use switch_port::*; pub use target_release::*; pub use tuf_repo::*; +pub use typed_uuid::DbTypedUuid; pub use typed_uuid::to_db_typed_uuid; pub use upstairs_repair::*; pub use user_builtin::*; diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index 1df74355b8f..ae4fe5a53fb 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -26,6 +26,7 @@ use diesel::NullableExpressionMethods; use diesel::OptionalExtension; use diesel::QueryDsl; use diesel::RunQueryDsl; +use diesel::Table; use diesel::expression::SelectableHelper; use diesel::pg::Pg; use diesel::query_builder::AstPass; @@ -34,6 +35,7 @@ use diesel::query_builder::QueryId; use diesel::result::DatabaseErrorKind; use diesel::result::Error as DieselError; use diesel::sql_types; +use diesel::sql_types::Nullable; use futures::FutureExt; use id_map::IdMap; use nexus_db_errors::ErrorHandler; @@ -41,6 +43,7 @@ use nexus_db_errors::OptionalError; use nexus_db_errors::TransactionError; use nexus_db_errors::public_error_from_diesel; use nexus_db_lookup::DbConnection; +use nexus_db_model::ArtifactHash; use nexus_db_model::Blueprint as DbBlueprint; use nexus_db_model::BpClickhouseClusterConfig; use nexus_db_model::BpClickhouseKeeperZoneIdToNodeId; @@ -50,18 +53,29 @@ use nexus_db_model::BpOmicronPhysicalDisk; use nexus_db_model::BpOmicronZone; use nexus_db_model::BpOmicronZoneNic; use nexus_db_model::BpOximeterReadPolicy; +use nexus_db_model::BpPendingMgsUpdateSp; use nexus_db_model::BpSledMetadata; use nexus_db_model::BpTarget; +use nexus_db_model::DbArtifactVersion; +use nexus_db_model::DbTypedUuid; +use nexus_db_model::HwBaseboardId; +use nexus_db_model::SpMgsSlot; +use nexus_db_model::SpType; +use nexus_db_model::SqlU16; use nexus_db_model::TufArtifact; use nexus_db_model::to_db_typed_uuid; +use nexus_db_schema::enums::SpTypeEnum; use nexus_types::deployment::Blueprint; use nexus_types::deployment::BlueprintMetadata; use nexus_types::deployment::BlueprintSledConfig; use nexus_types::deployment::BlueprintTarget; use nexus_types::deployment::ClickhouseClusterConfig; use nexus_types::deployment::CockroachDbPreserveDowngrade; +use nexus_types::deployment::ExpectedVersion; use nexus_types::deployment::OximeterReadMode; +use nexus_types::deployment::PendingMgsUpdateDetails; use nexus_types::deployment::PendingMgsUpdates; +use nexus_types::inventory::BaseboardId; use omicron_common::api::external::DataPageParams; use omicron_common::api::external::Error; use omicron_common::api::external::Generation; @@ -74,6 +88,9 @@ use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::OmicronZoneUuid; use omicron_uuid_kinds::SledUuid; use std::collections::BTreeMap; +use std::collections::BTreeSet; +use std::sync::Arc; +use thiserror::Error; use tufaceous_artifact::KnownArtifactKind; use uuid::Uuid; @@ -299,6 +316,22 @@ impl DataStore { &blueprint.oximeter_read_mode, ); + #[derive(Debug, Error)] + enum TxnError { + #[error("database error")] + Diesel(#[from] DieselError), + + #[error( + "aborting transaction after unexpectedly inserting {count} rows \ + for baseboard {baseboard_id:?} into {table_name}" + )] + BadInsertCount { + table_name: &'static str, + baseboard_id: Arc, + count: usize, + }, + } + // This implementation inserts all records associated with the // blueprint in one transaction. This is required: we don't want // any planner or executor to see a half-inserted blueprint, nor do we @@ -410,11 +443,132 @@ impl DataStore { .await?; } + // Insert pending MGS updates for service processors for this + // blueprint. These include foreign keys into the hw_baseboard_id + // table that we don't have handy. To achieve this, we use the same + // pattern used during inventory insertion: + // INSERT INTO bp_pending_mgs_update_sp + // SELECT + // id + // [other column values as literals] + // FROM hw_baseboard_id + // WHERE part_number = ... AND serial_number = ...; + // + // This way, we don't need to know the id. The database looks it up + // for us as it does the INSERT. + for update in &blueprint.pending_mgs_updates + { + // Right now, we only implement support for storing SP updates. + let PendingMgsUpdateDetails::Sp { + expected_active_version, + expected_inactive_version + } = &update.details else { + continue; + }; + + use nexus_db_schema::schema::hw_baseboard_id::dsl as baseboard_dsl; + use nexus_db_schema::schema::bp_pending_mgs_update_sp::dsl as update_dsl; + let selection = nexus_db_schema::schema::hw_baseboard_id::table + .select(( + DbTypedUuid::from(blueprint_id).into_sql::(), + baseboard_dsl::id, + SpType::from(update.sp_type).into_sql::(), + // XXX-dap size of int + SpMgsSlot::from(SqlU16::from(u16::try_from(update.slot_id).unwrap())).into_sql::(), + ArtifactHash::from(update.artifact_hash).into_sql::(), + DbArtifactVersion::from(update.artifact_version.clone()).into_sql::(), + DbArtifactVersion::from(expected_active_version.clone()).into_sql::(), + match expected_inactive_version { + ExpectedVersion::NoValidVersion => None, + ExpectedVersion::Version(v) => Some(DbArtifactVersion::from(v.clone())), + } + .into_sql::>(), + )) + .filter( + baseboard_dsl::part_number + .eq(update.baseboard_id.part_number.clone()), + ) + .filter( + baseboard_dsl::serial_number + .eq(update.baseboard_id.serial_number.clone()), + ); + let count = + diesel::insert_into(update_dsl::bp_pending_mgs_update_sp) + .values(selection) + .into_columns(( + update_dsl::blueprint_id, + update_dsl::hw_baseboard_id, + update_dsl::sp_type, + update_dsl::sp_slot, + update_dsl::artifact_sha256, + update_dsl::artifact_version, + update_dsl::expected_active_version, + update_dsl::expected_inactive_version, + )) + .execute_async(&conn) + .await?; + if count != 1 { + // This should be impossible in practice. We will insert + // however many rows matched the `baseboard_id` parts of + // the query above. It can't be more than one 1 because + // we've filtered on a pair of columns that are unique + // together. It could only be 0 if the baseboard id had + // never been seen before in an inventory collection. But + // in that case, how did we manage to construct a blueprint + // with it? + // + // This could happen in the test suite or with + // `reconfigurator-cli`, which both let you create any + // blueprint you like. In the test suite, the test just has + // to deal with this behavior (e.g., by inserting an + // inventory collection containing this SP). With + // `reconfigurator-cli`, this amounts to user error. + error!(&opctx.log, + "blueprint insertion: unexpectedly tried to insert wrong number of + rows into bp_pending_mgs_update_sp (aborting transaction)"; + "count" => count, + &update.baseboard_id, + ); + return Err(TxnError::BadInsertCount { + table_name: "bp_pending_mgs_update_sp", + count, + baseboard_id: update.baseboard_id.clone(), + }); + } + + // This statement is just here to force a compilation error + // if the set of columns in `bp_pending_mgs_update_sp` + // changes because that will affect the correctness of the + // above statement. + // + // If you're here because of a compile error, you might be + // changing the `bp_pending_mgs_update_sp` table. Update + // the statement below and be sure to update the code above, + // too! + let ( + _blueprint_id, + _hw_baseboard_id, + _sp_type, + _sp_slot, + _artifact_sha256, + _artifact_version, + _expected_active_version, + _expected_inactive_version, + ) = update_dsl::bp_pending_mgs_update_sp::all_columns(); + } + Ok(()) }) .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + .map_err(|e| match e { + TxnError::Diesel(e) => public_error_from_diesel(e, ErrorHandler::Server), + e @ TxnError::BadInsertCount { .. } => { + // This variant is always an internal error and has no causes so + // we don't need to use InlineErrorChain here. + Error::internal_error(&e.to_string()) + } + })?; info!( &opctx.log, @@ -940,11 +1094,96 @@ impl DataStore { } }; + // Load all pending SP updates. + // + // Pagination is a little silly here because we will only allow one at a + // time in practice for a while, but it's easy enough to do. + let mut pending_updates_sp = Vec::new(); + { + use nexus_db_schema::schema::bp_pending_mgs_update_sp::dsl; + + let mut paginator = Paginator::new(SQL_BATCH_SIZE); + while let Some(p) = paginator.next() { + let batch = paginated( + dsl::bp_pending_mgs_update_sp, + dsl::hw_baseboard_id, + &p.current_pagparams(), + ) + .filter(dsl::blueprint_id.eq(to_db_typed_uuid(blueprint_id))) + .select(BpPendingMgsUpdateSp::as_select()) + .load_async(&*conn) + .await + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + })?; + + paginator = p.found_batch(&batch, &|d| d.hw_baseboard_id); + for row in batch { + pending_updates_sp.push(row); + } + } + } + + // Collect the unique baseboard ids referenced by pending updates. + let baseboard_id_ids: BTreeSet<_> = + pending_updates_sp.iter().map(|s| s.hw_baseboard_id).collect(); + // XXX-dap could commonize with inventory + // Fetch the corresponding baseboard records. + let baseboards_by_id: BTreeMap<_, _> = { + use nexus_db_schema::schema::hw_baseboard_id::dsl; + + let mut bbs = BTreeMap::new(); + + let mut paginator = Paginator::new(SQL_BATCH_SIZE); + while let Some(p) = paginator.next() { + let batch = paginated( + dsl::hw_baseboard_id, + dsl::id, + &p.current_pagparams(), + ) + .filter(dsl::id.eq_any(baseboard_id_ids.clone())) + .select(HwBaseboardId::as_select()) + .load_async(&*conn) + .await + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + })?; + paginator = p.found_batch(&batch, &|row| row.id); + bbs.extend( + batch + .into_iter() + .map(|bb| (bb.id, Arc::new(BaseboardId::from(bb)))), + ); + } + + bbs + }; + + // Combine this information to assemble the set of pending MGS updates. + let mut pending_mgs_updates = PendingMgsUpdates::new(); + for row in pending_updates_sp { + let Some(baseboard) = baseboards_by_id.get(&row.hw_baseboard_id) + else { + // This should be impossible. + return Err(Error::internal_error(&format!( + "loading blueprint {}: missing baseboard that we should have fetched: {}", + blueprint_id, row.hw_baseboard_id + ))); + }; + + let update = row.into_generic(baseboard.clone()); + if let Some(previous) = pending_mgs_updates.insert(update) { + // This should be impossible. + return Err(Error::internal_error(&format!( + "blueprint {}: found multiple pending updates for baseboard {:?}", + blueprint_id, previous.baseboard_id + ))); + } + } + Ok(Blueprint { id: blueprint_id, - // TODO these need to be serialized to the database. - // See oxidecomputer/omicron#7981. - pending_mgs_updates: PendingMgsUpdates::new(), + pending_mgs_updates, sleds: sled_configs, parent_blueprint_id, internal_dns_version, @@ -1870,6 +2109,7 @@ mod tests { use nexus_types::deployment::BlueprintZoneImageVersion; use nexus_types::deployment::BlueprintZoneType; use nexus_types::deployment::OmicronZoneExternalFloatingIp; + use nexus_types::deployment::PendingMgsUpdate; use nexus_types::deployment::PlanningInput; use nexus_types::deployment::PlanningInputBuilder; use nexus_types::deployment::SledDetails; @@ -2314,6 +2554,21 @@ mod tests { .unwrap(); } + // Configure an SP update. + let (baseboard_id, sp) = + collection.sps.iter().next().expect("at least one SP"); + builder.pending_mgs_update_insert(PendingMgsUpdate { + baseboard_id: baseboard_id.clone(), + sp_type: sp.sp_type, + slot_id: u32::from(sp.sp_slot), + details: PendingMgsUpdateDetails::Sp { + expected_active_version: "1.0.0".parse().unwrap(), + expected_inactive_version: ExpectedVersion::NoValidVersion, + }, + artifact_hash: ArtifactHash([72; 32]), + artifact_version: "2.0.0".parse().unwrap(), + }); + let num_new_ntp_zones = 1; let num_new_crucible_zones = new_sled_zpools.len(); let num_new_sled_zones = num_new_ntp_zones + num_new_crucible_zones; @@ -2346,6 +2601,14 @@ mod tests { assert_all_zones_in_service(&blueprint2); assert_eq!(blueprint2.parent_blueprint_id, Some(blueprint1.id)); + // This blueprint contains a PendingMgsUpdate that references an SP from + // `collection`. This must already be present in the database for + // blueprint insertion to work. + datastore + .inventory_insert_collection(&opctx, &collection) + .await + .expect("failed to insert inventory collection"); + // Check that we can write it to the DB and read it back. datastore .blueprint_insert(&opctx, &blueprint2) diff --git a/nexus/db-schema/src/schema.rs b/nexus/db-schema/src/schema.rs index 42f0cf191c3..1ef5f69c11e 100644 --- a/nexus/db-schema/src/schema.rs +++ b/nexus/db-schema/src/schema.rs @@ -1960,6 +1960,19 @@ table! { } } +table! { + bp_pending_mgs_update_sp (blueprint_id, hw_baseboard_id) { + blueprint_id -> Uuid, + hw_baseboard_id -> Uuid, + sp_type -> crate::enums::SpTypeEnum, + sp_slot -> Int4, + artifact_sha256 -> Text, + artifact_version -> Text, + expected_active_version -> Text, + expected_inactive_version -> Nullable, + } +} + table! { bootstore_keys (key, generation) { key -> Text, diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 6e2dd8502f8..3829a3b9cc7 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -4412,6 +4412,27 @@ CREATE TABLE IF NOT EXISTS omicron.public.bp_oximeter_read_policy ( oximeter_read_mode omicron.public.oximeter_read_mode NOT NULL ); +-- Blueprint information related to pending SP upgrades. +CREATE TABLE IF NOT EXISTS omicron.public.bp_pending_mgs_update_sp ( + -- Foreign key into the `blueprint` table + blueprint_id UUID, + -- identify of the device to be updated + -- (foreign key into the `hw_baseboard_id` table) + hw_baseboard_id UUID NOT NULL, + -- location of this device according to MGS + sp_type omicron.public.sp_type NOT NULL, + sp_slot INT4 NOT NULL, + -- artifact to be deployed to this device + artifact_sha256 STRING(64) NOT NULL, + artifact_version STRING(64) NOT NULL, + + -- SP-specific details + expected_active_version STRING NOT NULL, + expected_inactive_version STRING, -- NULL means invalid (no version expected) + + PRIMARY KEY(blueprint_id, hw_baseboard_id) +); + -- Mapping of Omicron zone ID to CockroachDB node ID. This isn't directly used -- by the blueprint tables above, but is used by the more general Reconfigurator -- system along with them (e.g., to decommission expunged CRDB nodes). From 86f4605e7780a4cef444f2ddf449a5b6a0ef8639 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 6 Jun 2025 14:25:15 -0700 Subject: [PATCH 02/18] rustfmt --- nexus/db-model/src/deployment.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/nexus/db-model/src/deployment.rs b/nexus/db-model/src/deployment.rs index be8c47e140f..28443427ada 100644 --- a/nexus/db-model/src/deployment.rs +++ b/nexus/db-model/src/deployment.rs @@ -1260,7 +1260,10 @@ pub struct BpPendingMgsUpdateSp { } impl BpPendingMgsUpdateSp { - pub fn into_generic(self, baseboard_id: Arc) -> PendingMgsUpdate { + pub fn into_generic( + self, + baseboard_id: Arc, + ) -> PendingMgsUpdate { PendingMgsUpdate { baseboard_id, sp_type: self.sp_type.into(), From f538adfefba0b6616bd526c55e398a8f2b70455d Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 18 Jun 2025 14:38:52 -0700 Subject: [PATCH 03/18] clean up blueprint insert style --- .../db-queries/src/db/datastore/deployment.rs | 451 +++++++++++------- 1 file changed, 267 insertions(+), 184 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index 9123d652571..a5a252beca0 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -353,222 +353,305 @@ impl DataStore { #[allow(clippy::disallowed_methods)] self.transaction_non_retry_wrapper("blueprint_insert") .transaction(&conn, |conn| async move { - // Insert the row for the blueprint. - { - use nexus_db_schema::schema::blueprint::dsl; - let _: usize = diesel::insert_into(dsl::blueprint) - .values(row_blueprint) - .execute_async(&conn) - .await?; - } - - // Insert all the sled states for this blueprint. - { - use nexus_db_schema::schema::bp_sled_metadata::dsl as sled_metadata; + // Insert the row for the blueprint. + { + use nexus_db_schema::schema::blueprint::dsl; + let _: usize = diesel::insert_into(dsl::blueprint) + .values(row_blueprint) + .execute_async(&conn) + .await?; + } - let _ = diesel::insert_into(sled_metadata::bp_sled_metadata) - .values(sled_metadatas) - .execute_async(&conn) - .await?; - } + // Insert all the sled states for this blueprint. + { + // Skip formatting this line to prevent rustfmt bailing out. + #[rustfmt::skip] + use nexus_db_schema::schema::bp_sled_metadata::dsl + as sled_metadata; + let _ = + diesel::insert_into(sled_metadata::bp_sled_metadata) + .values(sled_metadatas) + .execute_async(&conn) + .await?; + } - // Insert all physical disks for this blueprint. - { - use nexus_db_schema::schema::bp_omicron_physical_disk::dsl as omicron_disk; - let _ = diesel::insert_into(omicron_disk::bp_omicron_physical_disk) + // Insert all physical disks for this blueprint. + { + // Skip formatting this line to prevent rustfmt bailing out. + #[rustfmt::skip] + use nexus_db_schema::schema::bp_omicron_physical_disk::dsl + as omicron_disk; + let _ = diesel::insert_into( + omicron_disk::bp_omicron_physical_disk, + ) .values(omicron_physical_disks) .execute_async(&conn) .await?; - } + } - // Insert all datasets for this blueprint. - { - use nexus_db_schema::schema::bp_omicron_dataset::dsl as omicron_dataset; - let _ = diesel::insert_into(omicron_dataset::bp_omicron_dataset) + // Insert all datasets for this blueprint. + { + // Skip formatting this line to prevent rustfmt bailing out. + #[rustfmt::skip] + use nexus_db_schema::schema::bp_omicron_dataset::dsl + as omicron_dataset; + let _ = diesel::insert_into( + omicron_dataset::bp_omicron_dataset, + ) .values(omicron_datasets) .execute_async(&conn) .await?; - } - - // Insert all the Omicron zones for this blueprint. - { - use nexus_db_schema::schema::bp_omicron_zone::dsl as omicron_zone; - let _ = diesel::insert_into(omicron_zone::bp_omicron_zone) - .values(omicron_zones) - .execute_async(&conn) - .await?; - } + } - { - use nexus_db_schema::schema::bp_omicron_zone_nic::dsl as omicron_zone_nic; - let _ = - diesel::insert_into(omicron_zone_nic::bp_omicron_zone_nic) - .values(omicron_zone_nics) + // Insert all the Omicron zones for this blueprint. + { + // Skip formatting this line to prevent rustfmt bailing out. + #[rustfmt::skip] + use nexus_db_schema::schema::bp_omicron_zone::dsl + as omicron_zone; + let _ = diesel::insert_into(omicron_zone::bp_omicron_zone) + .values(omicron_zones) .execute_async(&conn) .await?; - } - - // Insert all clickhouse cluster related tables if necessary - if let Some((clickhouse_cluster_config, keepers, servers)) = clickhouse_tables { - { - use nexus_db_schema::schema::bp_clickhouse_cluster_config::dsl; - let _ = diesel::insert_into(dsl::bp_clickhouse_cluster_config) - .values(clickhouse_cluster_config) - .execute_async(&conn) - .await?; } + { - use nexus_db_schema::schema::bp_clickhouse_keeper_zone_id_to_node_id::dsl; - let _ = diesel::insert_into(dsl::bp_clickhouse_keeper_zone_id_to_node_id) - .values(keepers) + // Skip formatting this line to prevent rustfmt bailing out. + #[rustfmt::skip] + use nexus_db_schema::schema::bp_omicron_zone_nic::dsl as + omicron_zone_nic; + let _ = diesel::insert_into( + omicron_zone_nic::bp_omicron_zone_nic, + ) + .values(omicron_zone_nics) .execute_async(&conn) .await?; } + + // Insert all clickhouse cluster related tables if necessary. + if let Some((clickhouse_cluster_config, keepers, servers)) = + clickhouse_tables { - use nexus_db_schema::schema::bp_clickhouse_server_zone_id_to_node_id::dsl; - let _ = diesel::insert_into(dsl::bp_clickhouse_server_zone_id_to_node_id) - .values(servers) - .execute_async(&conn) - .await?; + { + // Skip formatting this line to prevent rustfmt bailing + // out. + #[rustfmt::skip] + use nexus_db_schema::schema:: + bp_clickhouse_cluster_config::dsl; + let _ = diesel::insert_into( + dsl::bp_clickhouse_cluster_config, + ) + .values(clickhouse_cluster_config) + .execute_async(&conn) + .await?; + } + { + // Skip formatting this line to prevent rustfmt bailing + // out. + #[rustfmt::skip] + use nexus_db_schema::schema:: + bp_clickhouse_keeper_zone_id_to_node_id::dsl; + let _ = diesel::insert_into( + dsl::bp_clickhouse_keeper_zone_id_to_node_id, + ) + .values(keepers) + .execute_async(&conn) + .await?; + } + { + // Skip formatting this line to prevent rustfmt bailing + // out. + #[rustfmt::skip] + use nexus_db_schema::schema:: + bp_clickhouse_server_zone_id_to_node_id::dsl; + let _ = diesel::insert_into( + dsl::bp_clickhouse_server_zone_id_to_node_id, + ) + .values(servers) + .execute_async(&conn) + .await?; + } } - } - // Insert oximeter read policy for this blueprint - { - use nexus_db_schema::schema::bp_oximeter_read_policy::dsl; - let _ = - diesel::insert_into(dsl::bp_oximeter_read_policy) + // Insert oximeter read policy for this blueprint + { + use nexus_db_schema::schema::bp_oximeter_read_policy::dsl; + let _ = diesel::insert_into(dsl::bp_oximeter_read_policy) .values(oximeter_read_policy) .execute_async(&conn) .await?; - } + } - // Insert pending MGS updates for service processors for this - // blueprint. These include foreign keys into the hw_baseboard_id - // table that we don't have handy. To achieve this, we use the same - // pattern used during inventory insertion: - // INSERT INTO bp_pending_mgs_update_sp - // SELECT - // id - // [other column values as literals] - // FROM hw_baseboard_id - // WHERE part_number = ... AND serial_number = ...; - // - // This way, we don't need to know the id. The database looks it up - // for us as it does the INSERT. - for update in &blueprint.pending_mgs_updates - { - // Right now, we only implement support for storing SP updates. - let PendingMgsUpdateDetails::Sp { - expected_active_version, - expected_inactive_version - } = &update.details else { - continue; - }; + // Insert pending MGS updates for service processors for this + // blueprint. These include foreign keys into the hw_baseboard_id + // table that we don't have handy. To achieve this, we use the same + // pattern used during inventory insertion: + // INSERT INTO bp_pending_mgs_update_sp + // SELECT + // id + // [other column values as literals] + // FROM hw_baseboard_id + // WHERE part_number = ... AND serial_number = ...; + // + // This way, we don't need to know the id. The database looks it up + // for us as it does the INSERT. + + for update in &blueprint.pending_mgs_updates { + // Right now, we only implement support for storing SP + // updates. + let PendingMgsUpdateDetails::Sp { + expected_active_version, + expected_inactive_version, + } = &update.details + else { + continue; + }; - use nexus_db_schema::schema::hw_baseboard_id::dsl as baseboard_dsl; - use nexus_db_schema::schema::bp_pending_mgs_update_sp::dsl as update_dsl; - let selection = nexus_db_schema::schema::hw_baseboard_id::table - .select(( - DbTypedUuid::from(blueprint_id).into_sql::(), - baseboard_dsl::id, - SpType::from(update.sp_type).into_sql::(), - // XXX-dap size of int - SpMgsSlot::from(SqlU16::from(u16::try_from(update.slot_id).unwrap())).into_sql::(), - ArtifactHash::from(update.artifact_hash).into_sql::(), - DbArtifactVersion::from(update.artifact_version.clone()).into_sql::(), - DbArtifactVersion::from(expected_active_version.clone()).into_sql::(), + // slot_ids fit into a u16 in practice. This will be enforced + // at compile time shortly. See oxidecomputer/omicron#8378. + let update_slot_id = u16::try_from(update.slot_id) + .expect("slot id to fit into u16"); + + let db_blueprint_id = DbTypedUuid::from(blueprint_id) + .into_sql::( + ); + let db_sp_type = + SpType::from(update.sp_type).into_sql::(); + let db_slot_id = SpMgsSlot::from(SqlU16::from( + u16::try_from(update_slot_id).unwrap(), + )) + .into_sql::(); + let db_artifact_hash = + ArtifactHash::from(update.artifact_hash) + .into_sql::(); + let db_artifact_version = DbArtifactVersion::from( + update.artifact_version.clone(), + ) + .into_sql::(); + let db_expected_version = DbArtifactVersion::from( + expected_active_version.clone(), + ) + .into_sql::(); + let db_expected_inactive_version = match expected_inactive_version { ExpectedVersion::NoValidVersion => None, - ExpectedVersion::Version(v) => Some(DbArtifactVersion::from(v.clone())), + ExpectedVersion::Version(v) => { + Some(DbArtifactVersion::from(v.clone())) + } } - .into_sql::>(), - )) - .filter( - baseboard_dsl::part_number - .eq(update.baseboard_id.part_number.clone()), + .into_sql::>(); + + // Skip formatting several lines to prevent rustfmt bailing + // out. + #[rustfmt::skip] + use nexus_db_schema::schema::hw_baseboard_id::dsl + as baseboard_dsl; + #[rustfmt::skip] + use nexus_db_schema::schema::bp_pending_mgs_update_sp::dsl + as update_dsl; + let selection = + nexus_db_schema::schema::hw_baseboard_id::table + .select(( + db_blueprint_id, + baseboard_dsl::id, + db_slot_id, + db_artifact_hash, + db_artifact_version, + db_expected_version, + db_expected_inactive_version, + )) + .filter( + baseboard_dsl::part_number.eq(update + .baseboard_id + .part_number + .clone()), + ) + .filter( + baseboard_dsl::serial_number.eq(update + .baseboard_id + .serial_number + .clone()), + ); + let count = diesel::insert_into( + update_dsl::bp_pending_mgs_update_sp, ) - .filter( - baseboard_dsl::serial_number - .eq(update.baseboard_id.serial_number.clone()), - ); - let count = - diesel::insert_into(update_dsl::bp_pending_mgs_update_sp) - .values(selection) - .into_columns(( - update_dsl::blueprint_id, - update_dsl::hw_baseboard_id, - update_dsl::sp_type, - update_dsl::sp_slot, - update_dsl::artifact_sha256, - update_dsl::artifact_version, - update_dsl::expected_active_version, - update_dsl::expected_inactive_version, - )) - .execute_async(&conn) - .await?; - if count != 1 { - // This should be impossible in practice. We will insert - // however many rows matched the `baseboard_id` parts of - // the query above. It can't be more than one 1 because - // we've filtered on a pair of columns that are unique - // together. It could only be 0 if the baseboard id had - // never been seen before in an inventory collection. But - // in that case, how did we manage to construct a blueprint - // with it? + .values(selection) + .into_columns(( + update_dsl::blueprint_id, + update_dsl::hw_baseboard_id, + update_dsl::sp_type, + update_dsl::sp_slot, + update_dsl::artifact_sha256, + update_dsl::artifact_version, + update_dsl::expected_active_version, + update_dsl::expected_inactive_version, + )) + .execute_async(&conn) + .await?; + if count != 1 { + // This should be impossible in practice. We will + // insert however many rows matched the `baseboard_id` + // parts of the query above. It can't be more than one + // 1 because we've filtered on a pair of columns that + // are unique together. It could only be 0 if the + // baseboard id had never been seen before in an + // inventory collection. But in that case, how did we + // manage to construct a blueprint with it? + // + // This could happen in the test suite or with + // `reconfigurator-cli`, which both let you create any + // blueprint you like. In the test suite, the test just + // has to deal with this behavior (e.g., by inserting an + // inventory collection containing this SP). With + // `reconfigurator-cli`, this amounts to user error. + error!(&opctx.log, + "blueprint insertion: unexpectedly tried to insert \ + wrong number of rows into \ + bp_pending_mgs_update_sp (aborting transaction)"; + "count" => count, + &update.baseboard_id, + ); + return Err(TxnError::BadInsertCount { + table_name: "bp_pending_mgs_update_sp", + count, + baseboard_id: update.baseboard_id.clone(), + }); + } + + // This statement is just here to force a compilation error + // if the set of columns in `bp_pending_mgs_update_sp` + // changes because that will affect the correctness of the + // above statement. // - // This could happen in the test suite or with - // `reconfigurator-cli`, which both let you create any - // blueprint you like. In the test suite, the test just has - // to deal with this behavior (e.g., by inserting an - // inventory collection containing this SP). With - // `reconfigurator-cli`, this amounts to user error. - error!(&opctx.log, - "blueprint insertion: unexpectedly tried to insert wrong number of - rows into bp_pending_mgs_update_sp (aborting transaction)"; - "count" => count, - &update.baseboard_id, - ); - return Err(TxnError::BadInsertCount { - table_name: "bp_pending_mgs_update_sp", - count, - baseboard_id: update.baseboard_id.clone(), - }); + // If you're here because of a compile error, you might be + // changing the `bp_pending_mgs_update_sp` table. Update + // the statement below and be sure to update the code above, + // too! + let ( + _blueprint_id, + _hw_baseboard_id, + _sp_type, + _sp_slot, + _artifact_sha256, + _artifact_version, + _expected_active_version, + _expected_inactive_version, + ) = update_dsl::bp_pending_mgs_update_sp::all_columns(); } - // This statement is just here to force a compilation error - // if the set of columns in `bp_pending_mgs_update_sp` - // changes because that will affect the correctness of the - // above statement. - // - // If you're here because of a compile error, you might be - // changing the `bp_pending_mgs_update_sp` table. Update - // the statement below and be sure to update the code above, - // too! - let ( - _blueprint_id, - _hw_baseboard_id, - _sp_type, - _sp_slot, - _artifact_sha256, - _artifact_version, - _expected_active_version, - _expected_inactive_version, - ) = update_dsl::bp_pending_mgs_update_sp::all_columns(); - } - - Ok(()) - - }) - .await - .map_err(|e| match e { - TxnError::Diesel(e) => public_error_from_diesel(e, ErrorHandler::Server), - e @ TxnError::BadInsertCount { .. } => { - // This variant is always an internal error and has no causes so - // we don't need to use InlineErrorChain here. - Error::internal_error(&e.to_string()) - } - })?; + Ok(()) + }) + .await + .map_err(|e| match e { + TxnError::Diesel(e) => { + public_error_from_diesel(e, ErrorHandler::Server) + } + e @ TxnError::BadInsertCount { .. } => { + // This variant is always an internal error and has no + // causes so we don't need to use InlineErrorChain here. + Error::internal_error(&e.to_string()) + } + })?; info!( &opctx.log, From 31a882c6e62d058c16271766425bcf6612b7152a Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 19 Jun 2025 08:55:33 -0700 Subject: [PATCH 04/18] rustfmt bails out in blueprint_insert_on_connection() --- .../db-queries/src/db/datastore/deployment.rs | 181 +++++++++++------- 1 file changed, 110 insertions(+), 71 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index 93a71bec015..961af8aa460 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -320,101 +320,140 @@ impl DataStore { #[allow(clippy::disallowed_methods)] self.transaction_non_retry_wrapper("blueprint_insert") .transaction(&conn, |conn| async move { - // Insert the row for the blueprint. - { - use nexus_db_schema::schema::blueprint::dsl; - let _: usize = diesel::insert_into(dsl::blueprint) - .values(row_blueprint) - .execute_async(&conn) - .await?; - } - - // Insert all the sled states for this blueprint. - { - use nexus_db_schema::schema::bp_sled_metadata::dsl as sled_metadata; + // Insert the row for the blueprint. + { + use nexus_db_schema::schema::blueprint::dsl; + let _: usize = diesel::insert_into(dsl::blueprint) + .values(row_blueprint) + .execute_async(&conn) + .await?; + } - let _ = diesel::insert_into(sled_metadata::bp_sled_metadata) - .values(sled_metadatas) - .execute_async(&conn) - .await?; - } + // Insert all the sled states for this blueprint. + { + // Skip formatting this line to prevent rustfmt bailing out. + #[rustfmt::skip] + use nexus_db_schema::schema::bp_sled_metadata::dsl + as sled_metadata; + + let _ = + diesel::insert_into(sled_metadata::bp_sled_metadata) + .values(sled_metadatas) + .execute_async(&conn) + .await?; + } - // Insert all physical disks for this blueprint. - { - use nexus_db_schema::schema::bp_omicron_physical_disk::dsl as omicron_disk; - let _ = diesel::insert_into(omicron_disk::bp_omicron_physical_disk) + // Insert all physical disks for this blueprint. + { + // Skip formatting this line to prevent rustfmt bailing out. + #[rustfmt::skip] + use nexus_db_schema::schema::bp_omicron_physical_disk::dsl + as omicron_disk; + let _ = diesel::insert_into( + omicron_disk::bp_omicron_physical_disk, + ) .values(omicron_physical_disks) .execute_async(&conn) .await?; - } + } - // Insert all datasets for this blueprint. - { - use nexus_db_schema::schema::bp_omicron_dataset::dsl as omicron_dataset; - let _ = diesel::insert_into(omicron_dataset::bp_omicron_dataset) + // Insert all datasets for this blueprint. + { + // Skip formatting this line to prevent rustfmt bailing out. + #[rustfmt::skip] + use nexus_db_schema::schema::bp_omicron_dataset::dsl + as omicron_dataset; + let _ = diesel::insert_into( + omicron_dataset::bp_omicron_dataset, + ) .values(omicron_datasets) .execute_async(&conn) .await?; - } - - // Insert all the Omicron zones for this blueprint. - { - use nexus_db_schema::schema::bp_omicron_zone::dsl as omicron_zone; - let _ = diesel::insert_into(omicron_zone::bp_omicron_zone) - .values(omicron_zones) - .execute_async(&conn) - .await?; - } + } - { - use nexus_db_schema::schema::bp_omicron_zone_nic::dsl as omicron_zone_nic; - let _ = - diesel::insert_into(omicron_zone_nic::bp_omicron_zone_nic) - .values(omicron_zone_nics) + // Insert all the Omicron zones for this blueprint. + { + // Skip formatting this line to prevent rustfmt bailing out. + #[rustfmt::skip] + use nexus_db_schema::schema::bp_omicron_zone::dsl + as omicron_zone; + let _ = diesel::insert_into(omicron_zone::bp_omicron_zone) + .values(omicron_zones) .execute_async(&conn) .await?; - } - - // Insert all clickhouse cluster related tables if necessary - if let Some((clickhouse_cluster_config, keepers, servers)) = clickhouse_tables { - { - use nexus_db_schema::schema::bp_clickhouse_cluster_config::dsl; - let _ = diesel::insert_into(dsl::bp_clickhouse_cluster_config) - .values(clickhouse_cluster_config) - .execute_async(&conn) - .await?; } + { - use nexus_db_schema::schema::bp_clickhouse_keeper_zone_id_to_node_id::dsl; - let _ = diesel::insert_into(dsl::bp_clickhouse_keeper_zone_id_to_node_id) - .values(keepers) + // Skip formatting this line to prevent rustfmt bailing out. + #[rustfmt::skip] + use nexus_db_schema::schema::bp_omicron_zone_nic::dsl + as omicron_zone_nic; + let _ = diesel::insert_into( + omicron_zone_nic::bp_omicron_zone_nic, + ) + .values(omicron_zone_nics) .execute_async(&conn) .await?; } + + // Insert all clickhouse cluster related tables if necessary + if let Some((clickhouse_cluster_config, keepers, servers)) = + clickhouse_tables { - use nexus_db_schema::schema::bp_clickhouse_server_zone_id_to_node_id::dsl; - let _ = diesel::insert_into(dsl::bp_clickhouse_server_zone_id_to_node_id) - .values(servers) - .execute_async(&conn) - .await?; + { + // Skip formatting this line to prevent rustfmt bailing + // out. + #[rustfmt::skip] + use nexus_db_schema::schema:: + bp_clickhouse_cluster_config::dsl; + let _ = diesel::insert_into( + dsl::bp_clickhouse_cluster_config, + ) + .values(clickhouse_cluster_config) + .execute_async(&conn) + .await?; + } + { + // Skip formatting this line to prevent rustfmt bailing + // out. + #[rustfmt::skip] + use nexus_db_schema::schema:: + bp_clickhouse_keeper_zone_id_to_node_id::dsl; + let _ = diesel::insert_into( + dsl::bp_clickhouse_keeper_zone_id_to_node_id, + ) + .values(keepers) + .execute_async(&conn) + .await?; + } + { + // Skip formatting this line to prevent rustfmt bailing + // out. + #[rustfmt::skip] + use nexus_db_schema::schema:: + bp_clickhouse_server_zone_id_to_node_id::dsl; + let _ = diesel::insert_into( + dsl::bp_clickhouse_server_zone_id_to_node_id, + ) + .values(servers) + .execute_async(&conn) + .await?; + } } - } - // Insert oximeter read policy for this blueprint - { - use nexus_db_schema::schema::bp_oximeter_read_policy::dsl; - let _ = - diesel::insert_into(dsl::bp_oximeter_read_policy) + // Insert oximeter read policy for this blueprint + { + use nexus_db_schema::schema::bp_oximeter_read_policy::dsl; + let _ = diesel::insert_into(dsl::bp_oximeter_read_policy) .values(oximeter_read_policy) .execute_async(&conn) .await?; - } - - Ok(()) + } - }) - .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + Ok(()) + }) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; info!( &opctx.log, From dd3267aeefe58436619eb2d8793664a76ef31c5b Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 19 Jun 2025 09:08:17 -0700 Subject: [PATCH 05/18] unnecessary diffs --- nexus/db-queries/src/db/datastore/deployment.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index a5a252beca0..a8165cea340 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -368,6 +368,7 @@ impl DataStore { #[rustfmt::skip] use nexus_db_schema::schema::bp_sled_metadata::dsl as sled_metadata; + let _ = diesel::insert_into(sled_metadata::bp_sled_metadata) .values(sled_metadatas) @@ -418,8 +419,8 @@ impl DataStore { { // Skip formatting this line to prevent rustfmt bailing out. #[rustfmt::skip] - use nexus_db_schema::schema::bp_omicron_zone_nic::dsl as - omicron_zone_nic; + use nexus_db_schema::schema::bp_omicron_zone_nic::dsl + as omicron_zone_nic; let _ = diesel::insert_into( omicron_zone_nic::bp_omicron_zone_nic, ) From 0c72f01718c5887191d6f4146fe39e718454a7a5 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 19 Jun 2025 09:12:57 -0700 Subject: [PATCH 06/18] review feedback --- .../db-queries/src/db/datastore/deployment.rs | 21 ++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index a8165cea340..ef033304e68 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -500,13 +500,20 @@ impl DataStore { for update in &blueprint.pending_mgs_updates { // Right now, we only implement support for storing SP // updates. - let PendingMgsUpdateDetails::Sp { - expected_active_version, - expected_inactive_version, - } = &update.details - else { - continue; - }; + let (expected_active_version, expected_inactive_version) = + match &update.details { + PendingMgsUpdateDetails::Sp { + expected_active_version, + expected_inactive_version, + } => ( + expected_active_version, + expected_inactive_version, + ), + PendingMgsUpdateDetails::Rot { .. } + | PendingMgsUpdateDetails::RotBootloader { + .. + } => continue, + }; // slot_ids fit into a u16 in practice. This will be enforced // at compile time shortly. See oxidecomputer/omicron#8378. From e63d9bd89cc7546664183cda4d1ab71c7770c8d8 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 19 Jun 2025 09:22:43 -0700 Subject: [PATCH 07/18] fixes --- nexus/db-queries/src/db/datastore/deployment.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index ef033304e68..5dd5a1f6dee 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -525,10 +525,9 @@ impl DataStore { ); let db_sp_type = SpType::from(update.sp_type).into_sql::(); - let db_slot_id = SpMgsSlot::from(SqlU16::from( - u16::try_from(update_slot_id).unwrap(), - )) - .into_sql::(); + let db_slot_id = + SpMgsSlot::from(SqlU16::from(update_slot_id)) + .into_sql::(); let db_artifact_hash = ArtifactHash::from(update.artifact_hash) .into_sql::(); @@ -562,6 +561,7 @@ impl DataStore { .select(( db_blueprint_id, baseboard_dsl::id, + db_sp_type, db_slot_id, db_artifact_hash, db_artifact_version, From 1feb3c8c5edaa005b10c6f215e97baa493dd63de Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 19 Jun 2025 09:27:52 -0700 Subject: [PATCH 08/18] more nits --- .../db-queries/src/db/datastore/deployment.rs | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index 5dd5a1f6dee..f6711069fbe 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -484,9 +484,11 @@ impl DataStore { } // Insert pending MGS updates for service processors for this - // blueprint. These include foreign keys into the hw_baseboard_id - // table that we don't have handy. To achieve this, we use the same - // pattern used during inventory insertion: + // blueprint. These include foreign keys into the + // hw_baseboard_id table that we don't have handy. To achieve + // this, we use the same pattern used during inventory + // insertion: + // // INSERT INTO bp_pending_mgs_update_sp // SELECT // id @@ -494,8 +496,8 @@ impl DataStore { // FROM hw_baseboard_id // WHERE part_number = ... AND serial_number = ...; // - // This way, we don't need to know the id. The database looks it up - // for us as it does the INSERT. + // This way, we don't need to know the id. The database looks + // it up for us as it does the INSERT. for update in &blueprint.pending_mgs_updates { // Right now, we only implement support for storing SP @@ -515,8 +517,9 @@ impl DataStore { } => continue, }; - // slot_ids fit into a u16 in practice. This will be enforced - // at compile time shortly. See oxidecomputer/omicron#8378. + // slot_ids fit into a u16 in practice. This will be + // enforced at compile time shortly. See + // oxidecomputer/omicron#8378. let update_slot_id = u16::try_from(update.slot_id) .expect("slot id to fit into u16"); @@ -1220,7 +1223,6 @@ impl DataStore { // Collect the unique baseboard ids referenced by pending updates. let baseboard_id_ids: BTreeSet<_> = pending_updates_sp.iter().map(|s| s.hw_baseboard_id).collect(); - // XXX-dap could commonize with inventory // Fetch the corresponding baseboard records. let baseboards_by_id: BTreeMap<_, _> = { use nexus_db_schema::schema::hw_baseboard_id::dsl; @@ -1259,7 +1261,8 @@ impl DataStore { else { // This should be impossible. return Err(Error::internal_error(&format!( - "loading blueprint {}: missing baseboard that we should have fetched: {}", + "loading blueprint {}: missing baseboard that we should \ + have fetched: {}", blueprint_id, row.hw_baseboard_id ))); }; @@ -1268,7 +1271,8 @@ impl DataStore { if let Some(previous) = pending_mgs_updates.insert(update) { // This should be impossible. return Err(Error::internal_error(&format!( - "blueprint {}: found multiple pending updates for baseboard {:?}", + "blueprint {}: found multiple pending updates for \ + baseboard {:?}", blueprint_id, previous.baseboard_id ))); } From 948c6b6d6e1baf47b27733d4d8dedad7034c6261 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 19 Jun 2025 09:32:08 -0700 Subject: [PATCH 09/18] add schema migration --- nexus/db-model/src/schema_versions.rs | 3 ++- schema/crdb/add-pending-mgs-updates/up.sql | 12 ++++++++++++ schema/crdb/dbinit.sql | 2 +- 3 files changed, 15 insertions(+), 2 deletions(-) create mode 100644 schema/crdb/add-pending-mgs-updates/up.sql diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index bd270fb4c78..034c666f91e 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/nexus/db-model/src/schema_versions.rs @@ -16,7 +16,7 @@ use std::{collections::BTreeMap, sync::LazyLock}; /// /// This must be updated when you change the database schema. Refer to /// schema/crdb/README.adoc in the root of this repository for details. -pub const SCHEMA_VERSION: Version = Version::new(150, 0, 0); +pub const SCHEMA_VERSION: Version = Version::new(151, 0, 0); /// List of all past database schema versions, in *reverse* order /// @@ -28,6 +28,7 @@ static KNOWN_VERSIONS: LazyLock> = LazyLock::new(|| { // | leaving the first copy as an example for the next person. // v // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"), + KnownVersion::new(151, "add-pending-mgs-updates"), KnownVersion::new(150, "add-last-reconciliation-orphaned-datasets"), KnownVersion::new(149, "bp-add-target-release-min-gen"), KnownVersion::new(148, "clean-misplaced-m2s"), diff --git a/schema/crdb/add-pending-mgs-updates/up.sql b/schema/crdb/add-pending-mgs-updates/up.sql new file mode 100644 index 00000000000..58719a3e4cb --- /dev/null +++ b/schema/crdb/add-pending-mgs-updates/up.sql @@ -0,0 +1,12 @@ +CREATE TABLE IF NOT EXISTS omicron.public.bp_pending_mgs_update_sp ( + blueprint_id UUID, + hw_baseboard_id UUID NOT NULL, + sp_slot INT4 NOT NULL, + artifact_sha256 STRING(64) NOT NULL, + artifact_version STRING(64) NOT NULL, + + expected_active_version STRING NOT NULL, + expected_inactive_version STRING, + + PRIMARY KEY(blueprint_id, hw_baseboard_id) +); diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index b443db4902b..5949510189b 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -5781,7 +5781,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '150.0.0', NULL) + (TRUE, NOW(), NOW(), '151.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; From 94e9c2c791a942bb42a8506b5be7fb937c4d1269 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 19 Jun 2025 10:44:34 -0700 Subject: [PATCH 10/18] fix brokenness --- schema/crdb/add-pending-mgs-updates/up.sql | 1 + 1 file changed, 1 insertion(+) diff --git a/schema/crdb/add-pending-mgs-updates/up.sql b/schema/crdb/add-pending-mgs-updates/up.sql index 58719a3e4cb..28fa784d537 100644 --- a/schema/crdb/add-pending-mgs-updates/up.sql +++ b/schema/crdb/add-pending-mgs-updates/up.sql @@ -1,6 +1,7 @@ CREATE TABLE IF NOT EXISTS omicron.public.bp_pending_mgs_update_sp ( blueprint_id UUID, hw_baseboard_id UUID NOT NULL, + sp_type omicron.public.sp_type NOT NULL, sp_slot INT4 NOT NULL, artifact_sha256 STRING(64) NOT NULL, artifact_version STRING(64) NOT NULL, From a4fcd374d1fd8f101bda64718e8751bb2c55fec6 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 19 Jun 2025 13:11:00 -0700 Subject: [PATCH 11/18] `PendingMgsUpdate` slot id should be a u16 --- dev-tools/reconfigurator-cli/src/lib.rs | 2 +- .../reconfigurator-sp-updater/src/main.rs | 20 ++++++++++-------- nexus/db-model/src/deployment.rs | 2 +- .../db-queries/src/db/datastore/deployment.rs | 10 ++------- nexus/mgs-updates/src/common_sp_update.rs | 2 +- nexus/mgs-updates/src/driver_update.rs | 21 +++++++++++++------ nexus/mgs-updates/src/rot_updater.rs | 10 ++++----- nexus/mgs-updates/src/sp_updater.rs | 10 +++++---- .../src/test_util/sp_test_state.rs | 3 ++- nexus/mgs-updates/src/test_util/updates.rs | 6 +++--- .../planning/src/mgs_updates/mod.rs | 12 +++++------ nexus/types/src/deployment.rs | 4 ++-- 12 files changed, 55 insertions(+), 47 deletions(-) diff --git a/dev-tools/reconfigurator-cli/src/lib.rs b/dev-tools/reconfigurator-cli/src/lib.rs index 9f36886fd7e..92d519b0211 100644 --- a/dev-tools/reconfigurator-cli/src/lib.rs +++ b/dev-tools/reconfigurator-cli/src/lib.rs @@ -1290,7 +1290,7 @@ fn cmd_blueprint_edit( let update = PendingMgsUpdate { baseboard_id: baseboard_id.clone(), sp_type: sp.sp_type, - slot_id: u32::from(sp.sp_slot), + slot_id: sp.sp_slot, details, artifact_hash, artifact_version, diff --git a/dev-tools/reconfigurator-sp-updater/src/main.rs b/dev-tools/reconfigurator-sp-updater/src/main.rs index 2f81170ea30..c1712f5866a 100644 --- a/dev-tools/reconfigurator-sp-updater/src/main.rs +++ b/dev-tools/reconfigurator-sp-updater/src/main.rs @@ -189,7 +189,7 @@ impl Inventory { struct SpInfo { baseboard_id: Arc, sp_type: SpType, - sp_slot_id: u32, + sp_slot_id: u16, } impl Inventory { @@ -214,16 +214,21 @@ impl Inventory { }), ) .then(async move |sp_id| { + let sp_slot = u16::try_from(sp_id.slot).with_context(|| { + format!("sp slot number is out of range: {sp_id:?}") + })?; c.sp_get(sp_id.type_, sp_id.slot) .await .with_context(|| format!("fetching info about SP {:?}", sp_id)) - .map(|s| (sp_id, s)) + .map(|s| (sp_id.type_, sp_slot, s)) }) .collect::>>() .await .into_iter() .filter_map(|r| match r { - Ok((sp_id, v)) => Some((sp_id, v.into_inner())), + Ok((sp_type, sp_slot, v)) => { + Some((sp_type, sp_slot, v.into_inner())) + } Err(error) => { warn!( log, @@ -237,17 +242,14 @@ impl Inventory { let sps_by_serial = sp_infos .into_iter() - .map(|(sp_id, sp_state)| { + .map(|(sp_type, sp_slot, sp_state)| { let baseboard_id = Arc::new(BaseboardId { serial_number: sp_state.serial_number, part_number: sp_state.model, }); let serial_number = baseboard_id.serial_number.clone(); - let sp_info = SpInfo { - baseboard_id, - sp_type: sp_id.type_, - sp_slot_id: sp_id.slot, - }; + let sp_info = + SpInfo { baseboard_id, sp_type, sp_slot_id: sp_slot }; (serial_number, sp_info) }) .collect(); diff --git a/nexus/db-model/src/deployment.rs b/nexus/db-model/src/deployment.rs index a95a5da47ac..e41916bafe3 100644 --- a/nexus/db-model/src/deployment.rs +++ b/nexus/db-model/src/deployment.rs @@ -1273,7 +1273,7 @@ impl BpPendingMgsUpdateSp { PendingMgsUpdate { baseboard_id, sp_type: self.sp_type.into(), - slot_id: u32::from(**self.sp_slot), + slot_id: **self.sp_slot, artifact_hash: self.artifact_sha256.into(), artifact_version: (*self.artifact_version).clone(), details: PendingMgsUpdateDetails::Sp { diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index f6711069fbe..d96194670a6 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -517,19 +517,13 @@ impl DataStore { } => continue, }; - // slot_ids fit into a u16 in practice. This will be - // enforced at compile time shortly. See - // oxidecomputer/omicron#8378. - let update_slot_id = u16::try_from(update.slot_id) - .expect("slot id to fit into u16"); - let db_blueprint_id = DbTypedUuid::from(blueprint_id) .into_sql::( ); let db_sp_type = SpType::from(update.sp_type).into_sql::(); let db_slot_id = - SpMgsSlot::from(SqlU16::from(update_slot_id)) + SpMgsSlot::from(SqlU16::from(update.slot_id)) .into_sql::(); let db_artifact_hash = ArtifactHash::from(update.artifact_hash) @@ -2663,7 +2657,7 @@ mod tests { builder.pending_mgs_update_insert(PendingMgsUpdate { baseboard_id: baseboard_id.clone(), sp_type: sp.sp_type, - slot_id: u32::from(sp.sp_slot), + slot_id: sp.sp_slot, details: PendingMgsUpdateDetails::Sp { expected_active_version: "1.0.0".parse().unwrap(), expected_inactive_version: ExpectedVersion::NoValidVersion, diff --git a/nexus/mgs-updates/src/common_sp_update.rs b/nexus/mgs-updates/src/common_sp_update.rs index 6afc6fb7367..f7f959f2431 100644 --- a/nexus/mgs-updates/src/common_sp_update.rs +++ b/nexus/mgs-updates/src/common_sp_update.rs @@ -297,7 +297,7 @@ pub enum PrecheckError { )] WrongDevice { sp_type: SpType, - slot_id: u32, + slot_id: u16, expected_part: String, expected_serial: String, found_part: String, diff --git a/nexus/mgs-updates/src/driver_update.rs b/nexus/mgs-updates/src/driver_update.rs index 4f0ebefc48e..d8a560fdc6e 100644 --- a/nexus/mgs-updates/src/driver_update.rs +++ b/nexus/mgs-updates/src/driver_update.rs @@ -54,7 +54,7 @@ pub(crate) struct SpComponentUpdate { pub log: slog::Logger, pub component: SpComponent, pub target_sp_type: SpType, - pub target_sp_slot: u32, + pub target_sp_slot: u16, pub firmware_slot: u16, pub update_id: SpUpdateUuid, } @@ -240,7 +240,7 @@ pub(crate) async fn apply_update( client .sp_component_update( sp_type, - sp_slot, + u32::from(sp_slot), component, sp_update.firmware_slot, &sp_update.update_id.as_untyped_uuid(), @@ -445,7 +445,11 @@ async fn wait_for_delivery( let status = mgs_clients .try_all_serially(log, |client| async move { let update_status = client - .sp_component_update_status(sp_type, sp_slot, component) + .sp_component_update_status( + sp_type, + u32::from(sp_slot), + component, + ) .await?; debug!( @@ -544,7 +548,12 @@ async fn abort_update( .try_all_serially(log, |mgs_client| async move { let arg = UpdateAbortBody { id: update_id }; mgs_client - .sp_component_update_abort(sp_type, sp_slot, component, &arg) + .sp_component_update_abort( + sp_type, + u32::from(sp_slot), + component, + &arg, + ) .await }) .await @@ -707,7 +716,7 @@ mod test { gwtestctx: &GatewayTestContext, artifacts: &TestArtifacts, sp_type: SpType, - slot_id: u32, + slot_id: u16, artifact_hash: &ArtifactHash, expected_result: UpdateCompletedHow, ) { @@ -791,7 +800,7 @@ mod test { gwtestctx: &GatewayTestContext, artifacts: &TestArtifacts, sp_type: SpType, - slot_id: u32, + slot_id: u16, artifact_hash: &ArtifactHash, expected_result: UpdateCompletedHow, ) { diff --git a/nexus/mgs-updates/src/rot_updater.rs b/nexus/mgs-updates/src/rot_updater.rs index d7c5e7caf40..6e9993e0ff5 100644 --- a/nexus/mgs-updates/src/rot_updater.rs +++ b/nexus/mgs-updates/src/rot_updater.rs @@ -217,7 +217,7 @@ impl SpComponentUpdateHelper for ReconfiguratorRotUpdater { // Verify that the device is the one we think it is. let state = mgs_clients .try_all_serially(log, move |mgs_client| async move { - mgs_client.sp_get(update.sp_type, update.slot_id).await + mgs_client.sp_get(update.sp_type, u32::from(update.slot_id)).await }) .await? .into_inner(); @@ -276,7 +276,7 @@ impl SpComponentUpdateHelper for ReconfiguratorRotUpdater { mgs_client .sp_component_caboose_get( update.sp_type, - update.slot_id, + u32::from(update.slot_id), &SpComponent::ROT.to_string(), active.to_u16(), ) @@ -326,7 +326,7 @@ impl SpComponentUpdateHelper for ReconfiguratorRotUpdater { mgs_client .sp_component_caboose_get( update.sp_type, - update.slot_id, + u32::from(update.slot_id), &SpComponent::ROT.to_string(), expected_active_slot.slot().toggled().to_u16(), ) @@ -415,7 +415,7 @@ impl SpComponentUpdateHelper for ReconfiguratorRotUpdater { mgs_client .sp_component_active_slot_set( update.sp_type, - update.slot_id, + u32::from(update.slot_id), &SpComponent::ROT.to_string(), persist, &SpComponentFirmwareSlot { slot: inactive_slot } @@ -426,7 +426,7 @@ impl SpComponentUpdateHelper for ReconfiguratorRotUpdater { mgs_client .sp_component_reset( update.sp_type, - update.slot_id, + u32::from(update.slot_id), &SpComponent::ROT.to_string(), ) .await?; diff --git a/nexus/mgs-updates/src/sp_updater.rs b/nexus/mgs-updates/src/sp_updater.rs index e50b4f3061b..42e1a4918fb 100644 --- a/nexus/mgs-updates/src/sp_updater.rs +++ b/nexus/mgs-updates/src/sp_updater.rs @@ -164,7 +164,9 @@ impl SpComponentUpdateHelper for ReconfiguratorSpUpdater { // Verify that the device is the one we think it is. let state = mgs_clients .try_all_serially(log, move |mgs_client| async move { - mgs_client.sp_get(update.sp_type, update.slot_id).await + mgs_client + .sp_get(update.sp_type, u32::from(update.slot_id)) + .await }) .await? .into_inner(); @@ -188,7 +190,7 @@ impl SpComponentUpdateHelper for ReconfiguratorSpUpdater { mgs_client .sp_component_caboose_get( update.sp_type, - update.slot_id, + u32::from(update.slot_id), &SpComponent::SP_ITSELF.to_string(), 0, ) @@ -240,7 +242,7 @@ impl SpComponentUpdateHelper for ReconfiguratorSpUpdater { mgs_client .sp_component_caboose_get( update.sp_type, - update.slot_id, + u32::from(update.slot_id), &SpComponent::SP_ITSELF.to_string(), 1, ) @@ -300,7 +302,7 @@ impl SpComponentUpdateHelper for ReconfiguratorSpUpdater { mgs_client .sp_component_reset( update.sp_type, - update.slot_id, + u32::from(update.slot_id), &SpComponent::SP_ITSELF.to_string(), ) .await?; diff --git a/nexus/mgs-updates/src/test_util/sp_test_state.rs b/nexus/mgs-updates/src/test_util/sp_test_state.rs index 9c9198a4d5c..56932ea04e7 100644 --- a/nexus/mgs-updates/src/test_util/sp_test_state.rs +++ b/nexus/mgs-updates/src/test_util/sp_test_state.rs @@ -56,8 +56,9 @@ impl SpTestState { pub async fn load( mgs_client: &gateway_client::Client, sp_type: SpType, - sp_slot: u32, + sp_slot: u16, ) -> Result { + let sp_slot = u32::from(sp_slot); let caboose_sp_active = mgs_client .sp_component_caboose_get( sp_type, diff --git a/nexus/mgs-updates/src/test_util/updates.rs b/nexus/mgs-updates/src/test_util/updates.rs index f59986c6315..8ee6fefb46c 100644 --- a/nexus/mgs-updates/src/test_util/updates.rs +++ b/nexus/mgs-updates/src/test_util/updates.rs @@ -67,7 +67,7 @@ pub struct UpdateDescription<'a> { // Update parameters pub sp_type: SpType, - pub slot_id: u32, + pub slot_id: u16, pub artifact_hash: &'a ArtifactHash, // Overrides @@ -265,7 +265,7 @@ pub struct InProgressAttempt { // Parameters of the update itself sp_type: SpType, - slot_id: u32, + slot_id: u16, deployed_caboose: hubtools::Caboose, // Status of the driver @@ -385,7 +385,7 @@ pub struct FinishedUpdateAttempt { impl FinishedUpdateAttempt { async fn new( sp_type: SpType, - slot_id: u32, + slot_id: u16, sp1: SpTestState, deployed_caboose: hubtools::Caboose, result: Result, diff --git a/nexus/reconfigurator/planning/src/mgs_updates/mod.rs b/nexus/reconfigurator/planning/src/mgs_updates/mod.rs index ee3dc21f24f..66a9fee5516 100644 --- a/nexus/reconfigurator/planning/src/mgs_updates/mod.rs +++ b/nexus/reconfigurator/planning/src/mgs_updates/mod.rs @@ -241,7 +241,7 @@ fn mgs_update_status( update, ); Ok(MgsUpdateStatus::Impossible) - } else if u32::from(sp_info.sp_slot) != update.slot_id { + } else if sp_info.sp_slot != update.slot_id { warn!( log, "baseboard with in-progress SP update has moved"; @@ -466,7 +466,7 @@ fn try_make_update_sp( Some(PendingMgsUpdate { baseboard_id: baseboard_id.clone(), sp_type: sp_info.sp_type, - slot_id: u32::from(sp_info.sp_slot), + slot_id: sp_info.sp_slot, details: PendingMgsUpdateDetails::Sp { expected_active_version, expected_inactive_version, @@ -562,7 +562,7 @@ mod test { /// - switch 1: sidecar-c /// - psc 0: psc-b /// - psc 1: psc-c - fn test_config() -> BTreeMap<(SpType, u32), (&'static str, &'static str)> { + fn test_config() -> BTreeMap<(SpType, u16), (&'static str, &'static str)> { BTreeMap::from([ ((SpType::Sled, 0), ("sled_0", "gimlet-d")), ((SpType::Sled, 1), ("sled_1", "gimlet-e")), @@ -667,7 +667,7 @@ mod test { // `inactive_version` in the inactive slot. fn make_collection( active_version: ArtifactVersion, - active_version_exceptions: &BTreeMap<(SpType, u32), ArtifactVersion>, + active_version_exceptions: &BTreeMap<(SpType, u16), ArtifactVersion>, inactive_version: ExpectedVersion, ) -> Collection { let mut builder = nexus_inventory::CollectionBuilder::new( @@ -706,7 +706,7 @@ mod test { }; let baseboard_id = builder - .found_sp_state("test", sp_type, sp_slot, sp_state) + .found_sp_state("test", sp_type, u32::from(sp_slot), sp_state) .unwrap(); let active_version = active_version_exceptions .get(&(sp_type, sp_slot)) @@ -1142,7 +1142,7 @@ mod test { } fn verify_one_sp_update( - expected_updates: &mut BTreeMap<(SpType, u32), (&str, ArtifactHash)>, + expected_updates: &mut BTreeMap<(SpType, u16), (&str, ArtifactHash)>, update: &PendingMgsUpdate, ) { let sp_type = update.sp_type; diff --git a/nexus/types/src/deployment.rs b/nexus/types/src/deployment.rs index 20fb643f787..58d6a7b014d 100644 --- a/nexus/types/src/deployment.rs +++ b/nexus/types/src/deployment.rs @@ -1218,7 +1218,7 @@ pub struct PendingMgsUpdate { /// what type of baseboard this is pub sp_type: SpType, /// last known MGS slot (cubby number) of the baseboard - pub slot_id: u32, + pub slot_id: u16, /// component-specific details of the pending update pub details: PendingMgsUpdateDetails, @@ -1237,7 +1237,7 @@ impl slog::KV for PendingMgsUpdate { slog::KV::serialize(&self.baseboard_id, record, serializer)?; serializer .emit_str(Key::from("sp_type"), &format!("{:?}", self.sp_type))?; - serializer.emit_u32(Key::from("sp_slot"), self.slot_id)?; + serializer.emit_u16(Key::from("sp_slot"), self.slot_id)?; slog::KV::serialize(&self.details, record, serializer)?; serializer.emit_str( Key::from("artifact_hash"), From 5788725a1199a816e3682e5efad491cf396408fd Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Mon, 30 Jun 2025 20:25:38 -0700 Subject: [PATCH 12/18] merge: cont --- nexus/db-queries/src/db/datastore/deployment.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index 23b66d453ac..74e6fd2f205 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -517,19 +517,13 @@ impl DataStore { } => continue, }; - // slot_ids fit into a u16 in practice. This will be - // enforced at compile time shortly. See - // oxidecomputer/omicron#8378. - let update_slot_id = u16::try_from(update.slot_id) - .expect("slot id to fit into u16"); - let db_blueprint_id = DbTypedUuid::from(blueprint_id) .into_sql::( ); let db_sp_type = SpType::from(update.sp_type).into_sql::(); let db_slot_id = - SpMgsSlot::from(SqlU16::from(update_slot_id)) + SpMgsSlot::from(SqlU16::from(update.slot_id)) .into_sql::(); let db_artifact_hash = ArtifactHash::from(update.artifact_hash) From 9fb68d72d638608e824ff6c0dfe12f3e9313eb13 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Mon, 30 Jun 2025 21:01:04 -0700 Subject: [PATCH 13/18] MGS SpIdentifier slot should be a u16 --- dev-tools/omdb/src/bin/omdb/mgs.rs | 6 +- dev-tools/omdb/src/bin/omdb/mgs/sensors.rs | 6 +- gateway-test-utils/src/setup.rs | 4 +- gateway-types/src/component.rs | 24 +++---- gateway/src/management_switch.rs | 18 ++---- nexus/inventory/src/builder.rs | 29 +-------- nexus/mgs-updates/src/common_sp_update.rs | 2 +- nexus/mgs-updates/src/driver_update.rs | 6 +- nexus/mgs-updates/src/host_phase1_updater.rs | 6 +- nexus/mgs-updates/src/rot_updater.rs | 16 ++--- nexus/mgs-updates/src/sp_updater.rs | 16 +++-- .../src/test_util/sp_test_state.rs | 1 - .../planning/src/mgs_updates/mod.rs | 2 +- nexus/reconfigurator/planning/src/system.rs | 2 +- .../app/background/tasks/ereport_ingester.rs | 8 +-- openapi/gateway.json | 62 +++++++++---------- openapi/nexus-internal.json | 2 +- openapi/wicketd.json | 12 ++-- wicket-common/src/rack_setup.rs | 2 +- wicket/src/state/inventory.rs | 2 +- wicket/src/state/mod.rs | 4 +- wicket/src/wicketd.rs | 6 +- wicketd-api/src/lib.rs | 4 +- 23 files changed, 97 insertions(+), 143 deletions(-) diff --git a/dev-tools/omdb/src/bin/omdb/mgs.rs b/dev-tools/omdb/src/bin/omdb/mgs.rs index 681b3d7b3f2..65d35e9236a 100644 --- a/dev-tools/omdb/src/bin/omdb/mgs.rs +++ b/dev-tools/omdb/src/bin/omdb/mgs.rs @@ -195,7 +195,7 @@ fn show_sp_ids(sp_ids: &[SpIdentifier]) -> Result<(), anyhow::Error> { struct SpIdRow { #[tabled(rename = "TYPE")] type_: &'static str, - slot: u32, + slot: u16, } impl From<&SpIdentifier> for SpIdRow { @@ -221,7 +221,7 @@ fn show_sps_from_ignition( struct IgnitionRow { #[tabled(rename = "TYPE")] type_: &'static str, - slot: u32, + slot: u16, system_type: String, } @@ -269,7 +269,7 @@ fn show_sp_states( struct SpStateRow<'a> { #[tabled(rename = "TYPE")] type_: &'static str, - slot: u32, + slot: u16, model: String, serial: String, rev: u32, diff --git a/dev-tools/omdb/src/bin/omdb/mgs/sensors.rs b/dev-tools/omdb/src/bin/omdb/mgs/sensors.rs index d008b6f7e91..ef77fd21b8a 100644 --- a/dev-tools/omdb/src/bin/omdb/mgs/sensors.rs +++ b/dev-tools/omdb/src/bin/omdb/mgs/sensors.rs @@ -26,7 +26,7 @@ pub(crate) struct SensorsArgs { /// restrict to specified sled(s) #[clap(long, use_value_delimiter = true)] - pub sled: Vec, + pub sled: Vec, /// exclude sleds rather than include them #[clap(long, short)] @@ -256,7 +256,7 @@ struct SpInfo { async fn sp_info( mgs_client: gateway_client::Client, type_: SpType, - slot: u32, + slot: u16, ) -> Result { let mut devices = MultiMap::new(); let mut timestamps = vec![]; @@ -429,7 +429,7 @@ fn sp_info_csv( } }; - let slot = parts[1].parse::().or_else(|_| { + let slot = parts[1].parse::().or_else(|_| { bail!("invalid slot in \"{field}\""); })?; diff --git a/gateway-test-utils/src/setup.rs b/gateway-test-utils/src/setup.rs index 5937d7e13b1..dea606ab77c 100644 --- a/gateway-test-utils/src/setup.rs +++ b/gateway-test-utils/src/setup.rs @@ -151,11 +151,11 @@ pub async fn test_setup_with_config( port_description.location.get(&expected_location).unwrap(); let (sp_addr, sp_ereport_addr) = match target_sp.typ { SpType::Switch => { - let switch = &simrack.sidecars[target_sp.slot]; + let switch = &simrack.sidecars[usize::from(target_sp.slot)]; (switch.local_addr(sp_port), switch.local_ereport_addr(sp_port)) } SpType::Sled => { - let sled = &simrack.gimlets[target_sp.slot]; + let sled = &simrack.gimlets[usize::from(target_sp.slot)]; (sled.local_addr(sp_port), sled.local_ereport_addr(sp_port)) } SpType::Power => todo!(), diff --git a/gateway-types/src/component.rs b/gateway-types/src/component.rs index f0ca730b495..a1242e28846 100644 --- a/gateway-types/src/component.rs +++ b/gateway-types/src/component.rs @@ -43,8 +43,8 @@ pub enum SpType { pub struct SpIdentifier { #[serde(rename = "type")] pub typ: SpType, - #[serde(deserialize_with = "deserializer_u32_from_string")] - pub slot: u32, + #[serde(deserialize_with = "deserializer_u16_from_string")] + pub slot: u16, } impl fmt::Display for SpIdentifier { @@ -59,12 +59,12 @@ impl fmt::Display for SpIdentifier { // trying to deserialize the flattened struct as a map of strings to strings, // which breaks on `slot` (but not on `typ` for reasons I don't entirely // understand). We can work around by using an enum that allows either `String` -// or `u32` (which gets us past the serde map of strings), and then parsing the -// string into a u32 ourselves (which gets us to the `slot` we want). More +// or `u16` (which gets us past the serde map of strings), and then parsing the +// string into a u16 ourselves (which gets us to the `slot` we want). More // background: https://github.com/serde-rs/serde/issues/1346 -fn deserializer_u32_from_string<'de, D>( +fn deserializer_u16_from_string<'de, D>( deserializer: D, -) -> Result +) -> Result where D: serde::Deserializer<'de>, { @@ -72,16 +72,16 @@ where #[derive(Debug, Deserialize)] #[serde(untagged)] - enum StringOrU32 { + enum StringOrU16 { String(String), - U32(u32), + U16(u16), } - match StringOrU32::deserialize(deserializer)? { - StringOrU32::String(s) => s + match StringOrU16::deserialize(deserializer)? { + StringOrU16::String(s) => s .parse() - .map_err(|_| de::Error::invalid_type(Unexpected::Str(&s), &"u32")), - StringOrU32::U32(n) => Ok(n), + .map_err(|_| de::Error::invalid_type(Unexpected::Str(&s), &"u16")), + StringOrU16::U16(n) => Ok(n), } } diff --git a/gateway/src/management_switch.rs b/gateway/src/management_switch.rs index 15ce15adbb6..fbb4a0ce8ed 100644 --- a/gateway/src/management_switch.rs +++ b/gateway/src/management_switch.rs @@ -89,34 +89,24 @@ fn default_ereport_listen_port() -> u16 { #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Deserialize, Serialize)] pub struct SpIdentifier { pub typ: SpType, - pub slot: usize, + pub slot: u16, } impl SpIdentifier { - pub fn new(typ: SpType, slot: usize) -> Self { + pub fn new(typ: SpType, slot: u16) -> Self { Self { typ, slot } } } impl From for SpIdentifier { fn from(id: gateway_types::component::SpIdentifier) -> Self { - Self { - typ: id.typ.into(), - // id.slot may come from an untrusted source, but usize >= 32 bits - // on any platform that will run this code, so unwrap is fine - slot: usize::try_from(id.slot).unwrap(), - } + Self { typ: id.typ.into(), slot: id.slot } } } impl From for gateway_types::component::SpIdentifier { fn from(id: SpIdentifier) -> Self { - Self { - typ: id.typ.into(), - // id.slot comes from a trusted source (crate::management_switch) - // and will not exceed u32::MAX - slot: u32::try_from(id.slot).unwrap(), - } + Self { typ: id.typ.into(), slot: id.slot } } } diff --git a/nexus/inventory/src/builder.rs b/nexus/inventory/src/builder.rs index 5b5a577bdb0..7951eb5a7db 100644 --- a/nexus/inventory/src/builder.rs +++ b/nexus/inventory/src/builder.rs @@ -188,26 +188,9 @@ impl CollectionBuilder { &mut self, source: &str, sp_type: SpType, - slot: u32, + sp_slot: u16, sp_state: SpState, ) -> Option> { - // Much ado about very little: MGS reports that "slot" is a u32, though - // in practice this seems very unlikely to be bigger than a u8. (How - // many slots can there be within one rack?) The database only supports - // signed integers, so if we assumed this really could span the range of - // a u32, we'd need to store it in an i64. Instead, assume here that we - // can stick it into a u16 (which still seems generous). This will - // allow us to store it into an Int32 in the database. - let Ok(sp_slot) = u16::try_from(slot) else { - self.found_error(InventoryError::from(anyhow!( - "MGS {:?}: SP {:?} slot {}: slot number did not fit into u16", - source, - sp_type, - slot - ))); - return None; - }; - // Normalize the baseboard id: i.e., if we've seen this baseboard // before, use the same baseboard id record. Otherwise, make a new one. let baseboard = Self::normalize_item( @@ -580,7 +563,6 @@ mod test { use super::now_db_precision; use crate::examples::Representative; use crate::examples::representative; - use crate::examples::sp_state; use base64::Engine; use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use gateway_client::types::PowerState; @@ -1089,15 +1071,6 @@ mod test { .unwrap(); assert_eq!(sled1_bb, sled1_bb_dup); - // report an SP with an impossible slot number - let sled2_sp = builder.found_sp_state( - "fake MGS 1", - SpType::Sled, - u32::from(u16::MAX) + 1, - sp_state("1"), - ); - assert_eq!(sled2_sp, None); - // report SP caboose for an unknown baseboard let bogus_baseboard = BaseboardId { part_number: String::from("p1"), diff --git a/nexus/mgs-updates/src/common_sp_update.rs b/nexus/mgs-updates/src/common_sp_update.rs index f7f959f2431..7d74496d163 100644 --- a/nexus/mgs-updates/src/common_sp_update.rs +++ b/nexus/mgs-updates/src/common_sp_update.rs @@ -69,7 +69,7 @@ pub trait SpComponentUpdater { fn target_sp_type(&self) -> SpType; /// The slot number of the target SP. - fn target_sp_slot(&self) -> u32; + fn target_sp_slot(&self) -> u16; /// The target firmware slot for the component. fn firmware_slot(&self) -> u16; diff --git a/nexus/mgs-updates/src/driver_update.rs b/nexus/mgs-updates/src/driver_update.rs index b464aa05fbd..d8da1d98f19 100644 --- a/nexus/mgs-updates/src/driver_update.rs +++ b/nexus/mgs-updates/src/driver_update.rs @@ -242,7 +242,7 @@ pub(crate) async fn apply_update( client .sp_component_update( sp_type, - u32::from(sp_slot), + sp_slot, component, sp_update.firmware_slot, &sp_update.update_id.as_untyped_uuid(), @@ -452,7 +452,7 @@ async fn wait_for_delivery( let update_status = client .sp_component_update_status( sp_type, - u32::from(sp_slot), + sp_slot, component, ) .await?; @@ -555,7 +555,7 @@ async fn abort_update( mgs_client .sp_component_update_abort( sp_type, - u32::from(sp_slot), + sp_slot, component, &arg, ) diff --git a/nexus/mgs-updates/src/host_phase1_updater.rs b/nexus/mgs-updates/src/host_phase1_updater.rs index 90201ebd761..9cfe5d6551d 100644 --- a/nexus/mgs-updates/src/host_phase1_updater.rs +++ b/nexus/mgs-updates/src/host_phase1_updater.rs @@ -23,7 +23,7 @@ pub struct HostPhase1Updater { log: Logger, progress: watch::Sender>, sp_type: SpType, - sp_slot: u32, + sp_slot: u16, target_host_slot: u16, update_id: Uuid, // TODO-clarity maybe a newtype for this? TBD how we get this from @@ -34,7 +34,7 @@ pub struct HostPhase1Updater { impl HostPhase1Updater { pub fn new( sp_type: SpType, - sp_slot: u32, + sp_slot: u16, target_host_slot: u16, update_id: Uuid, phase1_data: Vec, @@ -152,7 +152,7 @@ impl SpComponentUpdater for HostPhase1Updater { self.sp_type } - fn target_sp_slot(&self) -> u32 { + fn target_sp_slot(&self) -> u16 { self.sp_slot } diff --git a/nexus/mgs-updates/src/rot_updater.rs b/nexus/mgs-updates/src/rot_updater.rs index 6e9993e0ff5..00a768f3e0d 100644 --- a/nexus/mgs-updates/src/rot_updater.rs +++ b/nexus/mgs-updates/src/rot_updater.rs @@ -35,7 +35,7 @@ pub struct RotUpdater { log: Logger, progress: watch::Sender>, sp_type: SpType, - sp_slot: u32, + sp_slot: u16, target_rot_slot: RotSlot, update_id: Uuid, // TODO-clarity maybe a newtype for this? TBD how we get this from @@ -46,7 +46,7 @@ pub struct RotUpdater { impl RotUpdater { pub fn new( sp_type: SpType, - sp_slot: u32, + sp_slot: u16, target_rot_slot: RotSlot, update_id: Uuid, rot_hubris_archive: Vec, @@ -176,7 +176,7 @@ impl SpComponentUpdater for RotUpdater { self.sp_type } - fn target_sp_slot(&self) -> u32 { + fn target_sp_slot(&self) -> u16 { self.sp_slot } @@ -217,7 +217,7 @@ impl SpComponentUpdateHelper for ReconfiguratorRotUpdater { // Verify that the device is the one we think it is. let state = mgs_clients .try_all_serially(log, move |mgs_client| async move { - mgs_client.sp_get(update.sp_type, u32::from(update.slot_id)).await + mgs_client.sp_get(update.sp_type, update.slot_id).await }) .await? .into_inner(); @@ -276,7 +276,7 @@ impl SpComponentUpdateHelper for ReconfiguratorRotUpdater { mgs_client .sp_component_caboose_get( update.sp_type, - u32::from(update.slot_id), + update.slot_id, &SpComponent::ROT.to_string(), active.to_u16(), ) @@ -326,7 +326,7 @@ impl SpComponentUpdateHelper for ReconfiguratorRotUpdater { mgs_client .sp_component_caboose_get( update.sp_type, - u32::from(update.slot_id), + update.slot_id, &SpComponent::ROT.to_string(), expected_active_slot.slot().toggled().to_u16(), ) @@ -415,7 +415,7 @@ impl SpComponentUpdateHelper for ReconfiguratorRotUpdater { mgs_client .sp_component_active_slot_set( update.sp_type, - u32::from(update.slot_id), + update.slot_id, &SpComponent::ROT.to_string(), persist, &SpComponentFirmwareSlot { slot: inactive_slot } @@ -426,7 +426,7 @@ impl SpComponentUpdateHelper for ReconfiguratorRotUpdater { mgs_client .sp_component_reset( update.sp_type, - u32::from(update.slot_id), + update.slot_id, &SpComponent::ROT.to_string(), ) .await?; diff --git a/nexus/mgs-updates/src/sp_updater.rs b/nexus/mgs-updates/src/sp_updater.rs index 42e1a4918fb..7809bc2f090 100644 --- a/nexus/mgs-updates/src/sp_updater.rs +++ b/nexus/mgs-updates/src/sp_updater.rs @@ -32,7 +32,7 @@ pub struct SpUpdater { log: Logger, progress: watch::Sender>, sp_type: SpType, - sp_slot: u32, + sp_slot: u16, update_id: Uuid, // TODO-clarity maybe a newtype for this? TBD how we get this from // wherever it's stored, which might give us a stronger type already. @@ -42,7 +42,7 @@ pub struct SpUpdater { impl SpUpdater { pub fn new( sp_type: SpType, - sp_slot: u32, + sp_slot: u16, update_id: Uuid, sp_hubris_archive: Vec, log: &Logger, @@ -123,7 +123,7 @@ impl SpComponentUpdater for SpUpdater { self.sp_type } - fn target_sp_slot(&self) -> u32 { + fn target_sp_slot(&self) -> u16 { self.sp_slot } @@ -164,9 +164,7 @@ impl SpComponentUpdateHelper for ReconfiguratorSpUpdater { // Verify that the device is the one we think it is. let state = mgs_clients .try_all_serially(log, move |mgs_client| async move { - mgs_client - .sp_get(update.sp_type, u32::from(update.slot_id)) - .await + mgs_client.sp_get(update.sp_type, update.slot_id).await }) .await? .into_inner(); @@ -190,7 +188,7 @@ impl SpComponentUpdateHelper for ReconfiguratorSpUpdater { mgs_client .sp_component_caboose_get( update.sp_type, - u32::from(update.slot_id), + update.slot_id, &SpComponent::SP_ITSELF.to_string(), 0, ) @@ -242,7 +240,7 @@ impl SpComponentUpdateHelper for ReconfiguratorSpUpdater { mgs_client .sp_component_caboose_get( update.sp_type, - u32::from(update.slot_id), + update.slot_id, &SpComponent::SP_ITSELF.to_string(), 1, ) @@ -302,7 +300,7 @@ impl SpComponentUpdateHelper for ReconfiguratorSpUpdater { mgs_client .sp_component_reset( update.sp_type, - u32::from(update.slot_id), + update.slot_id, &SpComponent::SP_ITSELF.to_string(), ) .await?; diff --git a/nexus/mgs-updates/src/test_util/sp_test_state.rs b/nexus/mgs-updates/src/test_util/sp_test_state.rs index 56932ea04e7..abdead68b5f 100644 --- a/nexus/mgs-updates/src/test_util/sp_test_state.rs +++ b/nexus/mgs-updates/src/test_util/sp_test_state.rs @@ -58,7 +58,6 @@ impl SpTestState { sp_type: SpType, sp_slot: u16, ) -> Result { - let sp_slot = u32::from(sp_slot); let caboose_sp_active = mgs_client .sp_component_caboose_get( sp_type, diff --git a/nexus/reconfigurator/planning/src/mgs_updates/mod.rs b/nexus/reconfigurator/planning/src/mgs_updates/mod.rs index 12835fb49be..3c3192c2071 100644 --- a/nexus/reconfigurator/planning/src/mgs_updates/mod.rs +++ b/nexus/reconfigurator/planning/src/mgs_updates/mod.rs @@ -711,7 +711,7 @@ mod test { }; let baseboard_id = builder - .found_sp_state("test", sp_type, u32::from(sp_slot), sp_state) + .found_sp_state("test", sp_type, sp_slot, sp_state) .unwrap(); let active_version = active_version_exceptions .get(&(sp_type, sp_slot)) diff --git a/nexus/reconfigurator/planning/src/system.rs b/nexus/reconfigurator/planning/src/system.rs index f47975820e9..dfee1ea981d 100644 --- a/nexus/reconfigurator/planning/src/system.rs +++ b/nexus/reconfigurator/planning/src/system.rs @@ -517,7 +517,7 @@ impl SystemDescription { .found_sp_state( "fake MGS 1", SpType::Sled, - u32::from(*slot), + *slot, sp_state.clone(), ) .context("recording SP state")?; diff --git a/nexus/src/app/background/tasks/ereport_ingester.rs b/nexus/src/app/background/tasks/ereport_ingester.rs index 7acddfdfa47..1e8dac927f0 100644 --- a/nexus/src/app/background/tasks/ereport_ingester.rs +++ b/nexus/src/app/background/tasks/ereport_ingester.rs @@ -136,12 +136,6 @@ impl SpEreportIngester { let mut tasks = ParallelTaskSet::new(); for gateway_client::types::SpIdentifier { type_, slot } in sps { - let Ok(slot) = u16::try_from(slot) else { - const MSG: &str = "invalid slot number received from MGS"; - error!(opctx.log, "{MSG}"; "sp_type" => %type_, "slot" => %slot); - status.errors.push(format!("{MSG}: {type_} {slot}")); - continue; - }; let sp_result = tasks .spawn({ let opctx = opctx.child(BTreeMap::from([ @@ -359,7 +353,7 @@ impl Ingester { let res = client .sp_ereports_ingest( sp_type, - u32::from(slot), + slot, committed_ena.as_ref(), LIMIT, &restart_id, diff --git a/openapi/gateway.json b/openapi/gateway.json index b103bad4101..89ece0a0e73 100644 --- a/openapi/gateway.json +++ b/openapi/gateway.json @@ -51,7 +51,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -105,7 +105,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -232,7 +232,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -277,7 +277,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -331,7 +331,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -389,7 +389,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -441,7 +441,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -507,7 +507,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -572,7 +572,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -636,7 +636,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -683,7 +683,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -736,7 +736,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -783,7 +783,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -847,7 +847,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -894,7 +894,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -941,7 +941,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -1020,7 +1020,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -1077,7 +1077,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -1122,7 +1122,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -1203,7 +1203,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -1245,7 +1245,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -1283,7 +1283,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -1328,7 +1328,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -1366,7 +1366,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -1409,7 +1409,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -1468,7 +1468,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -1513,7 +1513,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -1556,7 +1556,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -1603,7 +1603,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -1650,7 +1650,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -3124,7 +3124,7 @@ "properties": { "slot": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 }, "type": { diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index bcb7051775e..97959dfab3f 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -5928,7 +5928,7 @@ "slot_id": { "description": "last known MGS slot (cubby number) of the baseboard", "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 }, "sp_type": { diff --git a/openapi/wicketd.json b/openapi/wicketd.json index 0183c25d168..d2aa583d096 100644 --- a/openapi/wicketd.json +++ b/openapi/wicketd.json @@ -22,7 +22,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -186,7 +186,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -721,7 +721,7 @@ "required": true, "schema": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 } }, @@ -3296,7 +3296,7 @@ "type": "array", "items": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 }, "uniqueItems": true @@ -4320,12 +4320,12 @@ ] }, "SpIdentifier": { - "description": "`SpIdentifier`\n\n
JSON schema\n\n```json { \"type\": \"object\", \"required\": [ \"slot\", \"type\" ], \"properties\": { \"slot\": { \"type\": \"integer\", \"format\": \"uint32\", \"minimum\": 0.0 }, \"type\": { \"$ref\": \"#/components/schemas/SpType\" } } } ```
", + "description": "`SpIdentifier`\n\n
JSON schema\n\n```json { \"type\": \"object\", \"required\": [ \"slot\", \"type\" ], \"properties\": { \"slot\": { \"type\": \"integer\", \"format\": \"uint16\", \"minimum\": 0.0 }, \"type\": { \"$ref\": \"#/components/schemas/SpType\" } } } ```
", "type": "object", "properties": { "slot": { "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 }, "type": { diff --git a/wicket-common/src/rack_setup.rs b/wicket-common/src/rack_setup.rs index 73abf990e01..bbccad0811f 100644 --- a/wicket-common/src/rack_setup.rs +++ b/wicket-common/src/rack_setup.rs @@ -65,7 +65,7 @@ pub struct PutRssUserConfigInsensitive { /// `wicketd` will map this back to sleds with the correct `SpIdentifier` /// based on the `bootstrap_sleds` it provides in /// `CurrentRssUserConfigInsensitive`. - pub bootstrap_sleds: BTreeSet, + pub bootstrap_sleds: BTreeSet, pub ntp_servers: Vec, pub dns_servers: Vec, pub internal_services_ip_pool_ranges: Vec, diff --git a/wicket/src/state/inventory.rs b/wicket/src/state/inventory.rs index 073c6ab32e2..a558dbdf33c 100644 --- a/wicket/src/state/inventory.rs +++ b/wicket/src/state/inventory.rs @@ -280,7 +280,7 @@ impl ComponentId { Ok(Self::Psc(slot)) } - pub fn from_sp_type_and_slot(sp_type: SpType, slot: u32) -> Result { + pub fn from_sp_type_and_slot(sp_type: SpType, slot: u16) -> Result { let slot = slot.try_into().map_err(|_| { anyhow::anyhow!("invalid slot (must fit in a u8): {}", slot) })?; diff --git a/wicket/src/state/mod.rs b/wicket/src/state/mod.rs index 8cfc422bb02..1b7678025dd 100644 --- a/wicket/src/state/mod.rs +++ b/wicket/src/state/mod.rs @@ -72,7 +72,7 @@ impl State { // Do we know the wicketd sled ID? If so, we can compare // directly. (We will almost always know this.) if let Some(wicketd_sled_id) = self.wicketd_location.sled_id { - wicketd_sled_id.slot == u32::from(i) + wicketd_sled_id.slot == u16::from(i) } else { // We _could_ check and see if wicketd knows its sled's // baseboard (even though it didn't know the sled) and then @@ -88,7 +88,7 @@ impl State { // thing here for the switch. if let Some(wicketd_switch_id) = self.wicketd_location.switch_id { - wicketd_switch_id.slot == u32::from(i) + wicketd_switch_id.slot == u16::from(i) } else { false } diff --git a/wicket/src/wicketd.rs b/wicket/src/wicketd.rs index a708cc0a90a..0228b875d30 100644 --- a/wicket/src/wicketd.rs +++ b/wicket/src/wicketd.rs @@ -28,13 +28,13 @@ impl From for SpIdentifier { fn from(id: ComponentId) -> Self { match id { ComponentId::Sled(i) => { - SpIdentifier { type_: SpType::Sled, slot: u32::from(i) } + SpIdentifier { type_: SpType::Sled, slot: u16::from(i) } } ComponentId::Psc(i) => { - SpIdentifier { type_: SpType::Power, slot: u32::from(i) } + SpIdentifier { type_: SpType::Power, slot: u16::from(i) } } ComponentId::Switch(i) => { - SpIdentifier { type_: SpType::Switch, slot: u32::from(i) } + SpIdentifier { type_: SpType::Switch, slot: u16::from(i) } } } } diff --git a/wicketd-api/src/lib.rs b/wicketd-api/src/lib.rs index 03e7e798f81..467922aad86 100644 --- a/wicketd-api/src/lib.rs +++ b/wicketd-api/src/lib.rs @@ -487,7 +487,7 @@ pub struct GetArtifactsAndEventReportsResponse { /// instead. pub artifacts: Vec, - pub event_reports: BTreeMap>, + pub event_reports: BTreeMap>, } #[derive(Clone, Debug, JsonSchema, Deserialize)] @@ -535,7 +535,7 @@ pub struct GetLocationResponse { pub struct PathSpIgnitionCommand { #[serde(rename = "type")] pub type_: SpType, - pub slot: u32, + pub slot: u16, pub command: IgnitionCommand, } From 03aec8a3811601a73c89f3da7d81afbb28eab353 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Mon, 30 Jun 2025 21:06:09 -0700 Subject: [PATCH 14/18] rustfmt --- nexus/mgs-updates/src/driver_update.rs | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/nexus/mgs-updates/src/driver_update.rs b/nexus/mgs-updates/src/driver_update.rs index d8da1d98f19..05099532b06 100644 --- a/nexus/mgs-updates/src/driver_update.rs +++ b/nexus/mgs-updates/src/driver_update.rs @@ -450,11 +450,7 @@ async fn wait_for_delivery( let status = mgs_clients .try_all_serially(log, |client| async move { let update_status = client - .sp_component_update_status( - sp_type, - sp_slot, - component, - ) + .sp_component_update_status(sp_type, sp_slot, component) .await?; debug!( @@ -553,12 +549,7 @@ async fn abort_update( .try_all_serially(log, |mgs_client| async move { let arg = UpdateAbortBody { id: update_id }; mgs_client - .sp_component_update_abort( - sp_type, - sp_slot, - component, - &arg, - ) + .sp_component_update_abort(sp_type, sp_slot, component, &arg) .await }) .await From 1fafe4483ba0563e731bdf727a529d40fc825b52 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Mon, 30 Jun 2025 21:08:01 -0700 Subject: [PATCH 15/18] clippy --- dev-tools/reconfigurator-sp-updater/src/main.rs | 5 +---- gateway/src/metrics.rs | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/dev-tools/reconfigurator-sp-updater/src/main.rs b/dev-tools/reconfigurator-sp-updater/src/main.rs index f0712d3deb5..4f13caad1e2 100644 --- a/dev-tools/reconfigurator-sp-updater/src/main.rs +++ b/dev-tools/reconfigurator-sp-updater/src/main.rs @@ -213,13 +213,10 @@ impl Inventory { }), ) .then(async move |sp_id| { - let sp_slot = u16::try_from(sp_id.slot).with_context(|| { - format!("sp slot number is out of range: {sp_id:?}") - })?; c.sp_get(sp_id.type_, sp_id.slot) .await .with_context(|| format!("fetching info about SP {:?}", sp_id)) - .map(|s| (sp_id.type_, sp_slot, s)) + .map(|s| (sp_id.type_, sp_id.slot, s)) }) .collect::>>() .await diff --git a/gateway/src/metrics.rs b/gateway/src/metrics.rs index cf7b8a1f3a8..e234d11c494 100644 --- a/gateway/src/metrics.rs +++ b/gateway/src/metrics.rs @@ -648,7 +648,7 @@ impl SpPoller { hubris_archive_id: Cow::Owned( hubris_archive_id.clone(), ), - slot: self.spid.slot as u32, + slot: u32::from(self.spid.slot), component_kind: Cow::Owned(dev.device), component_id, description: Cow::Owned(dev.description), From 8f0c42f1ba9c20e70130cf9f678cf880be6b5c17 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 1 Jul 2025 07:18:59 -0700 Subject: [PATCH 16/18] fix api --- openapi/nexus-internal.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openapi/nexus-internal.json b/openapi/nexus-internal.json index bcb7051775e..97959dfab3f 100644 --- a/openapi/nexus-internal.json +++ b/openapi/nexus-internal.json @@ -5928,7 +5928,7 @@ "slot_id": { "description": "last known MGS slot (cubby number) of the baseboard", "type": "integer", - "format": "uint32", + "format": "uint16", "minimum": 0 }, "sp_type": { From f26bd3dd0adfe38a5af21493879457d4976542d3 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 1 Jul 2025 10:30:21 -0700 Subject: [PATCH 17/18] this is not an error any more --- nexus/inventory/src/builder.rs | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/nexus/inventory/src/builder.rs b/nexus/inventory/src/builder.rs index 7951eb5a7db..78beca338f9 100644 --- a/nexus/inventory/src/builder.rs +++ b/nexus/inventory/src/builder.rs @@ -1282,17 +1282,7 @@ mod test { .is_none() ); - // We should see an error. - assert_eq!( - collection - .errors - .iter() - .map(|e| format!("{:#}", e)) - .collect::>(), - vec![ - "MGS \"fake MGS 1\": SP Sled slot 65536: \ - slot number did not fit into u16" - ] - ); + // We should see no errors. + assert_eq!(collection.errors.iter().next(), None); } } From 8ab5ff1a2a0632e5beede66ebd6d60cc59735a9f Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 1 Jul 2025 10:44:50 -0700 Subject: [PATCH 18/18] clippy --- nexus/inventory/src/builder.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/inventory/src/builder.rs b/nexus/inventory/src/builder.rs index 78beca338f9..26381473c95 100644 --- a/nexus/inventory/src/builder.rs +++ b/nexus/inventory/src/builder.rs @@ -1283,6 +1283,6 @@ mod test { ); // We should see no errors. - assert_eq!(collection.errors.iter().next(), None); + assert!(collection.errors.is_empty()); } }