diff --git a/nexus/db-queries/src/db/datastore/fm.rs b/nexus/db-queries/src/db/datastore/fm.rs index f86e3362100..3a47bdea7d0 100644 --- a/nexus/db-queries/src/db/datastore/fm.rs +++ b/nexus/db-queries/src/db/datastore/fm.rs @@ -249,7 +249,7 @@ impl DataStore { pub async fn fm_sitrep_read_current( &self, opctx: &OpContext, - ) -> Result, Error> { + ) -> Result, Error> { let conn = self.pool_connection_authorized(opctx).await?; loop { let version = @@ -263,7 +263,9 @@ impl DataStore { None => return Ok(None), }; match self.fm_sitrep_read_on_conn(version.id, &conn).await { - Ok(sitrep) => return Ok(Some((version, sitrep))), + Ok(sitrep) => { + return fm::CommittedSitrep::new(version, sitrep).map(Some); + } // If `fm_sitrep_read_on_conn` returns `NotFound` for a sitrep // ID that was returned by `fm_current_sitrep_version_on_conn`, // this means that the sitrep we were attempting to read is no @@ -2268,9 +2270,10 @@ mod tests { .fm_sitrep_read_current(&opctx) .await .expect("should successfully read current sitrep"); - let (version, current_sitrep) = current.expect("sitrep should be Some"); - assert_eq!(version.id, sitrep.metadata.id); - assert_eq!(version.version, 1); + let committed = current.expect("sitrep should be Some"); + let current_sitrep = &committed.sitrep; + assert_eq!(current_sitrep.metadata.id, sitrep.metadata.id); + assert_eq!(committed.version, 1); assert_eq!(sitrep.id(), current_sitrep.id()); assert_eq!(sitrep.parent_id(), current_sitrep.parent_id()); assert_eq!( @@ -2340,13 +2343,14 @@ mod tests { ); // Verify the second sitrep is now current - let (version, current_sitrep) = datastore + let committed = datastore .fm_sitrep_read_current(&opctx) .await .unwrap() .expect("current sitrep should be Some"); - assert_eq!(version.id, sitrep2.id()); - assert_eq!(version.version, 2); + let current_sitrep = &committed.sitrep; + assert_eq!(current_sitrep.metadata.id, sitrep2.id()); + assert_eq!(committed.version, 2); assert_eq!(sitrep2.id(), current_sitrep.id()); assert_eq!(sitrep2.parent_id(), current_sitrep.parent_id()); @@ -2494,13 +2498,14 @@ mod tests { } // Verify sitrep2 is still current - let (version, current_sitrep) = datastore + let committed = datastore .fm_sitrep_read_current(&opctx) .await .unwrap() .expect("current sitrep should be Some"); - assert_eq!(version.id, sitrep2.id()); - assert_eq!(version.version, 2); + let current_sitrep = &committed.sitrep; + assert_eq!(current_sitrep.metadata.id, sitrep2.id()); + assert_eq!(committed.version, 2); assert_eq!(sitrep2.id(), current_sitrep.id()); assert_eq!(sitrep2.parent_id(), current_sitrep.parent_id()); diff --git a/nexus/fm/src/analysis_input.rs b/nexus/fm/src/analysis_input.rs index b3fec133d38..218f96fefce 100644 --- a/nexus/fm/src/analysis_input.rs +++ b/nexus/fm/src/analysis_input.rs @@ -8,7 +8,7 @@ use chrono::{DateTime, Utc}; use iddqd::IdOrdMap; use nexus_db_model::EreporterRestart; use nexus_types::fm::analysis_reports::ClosedCaseReport; -use nexus_types::fm::{self, Sitrep, SitrepVersion}; +use nexus_types::fm::{self, Sitrep}; use nexus_types::in_service_disk::InServiceDisk; use nexus_types::inventory; use omicron_uuid_kinds::AlertUuid; @@ -38,7 +38,7 @@ pub use nexus_types::fm::analysis_reports::InputReport as Report; /// [`Input::builder`]. #[derive(Debug)] pub struct Input { - parent_sitrep: Option>, + parent_sitrep: Option>, inv: Arc, /// Ereports which are new and should be input to analysis in the next /// sitrep. @@ -64,7 +64,7 @@ pub struct Input { impl Input { pub fn parent_sitrep(&self) -> Option<&Sitrep> { - self.parent_sitrep.as_ref().map(|s| &s.1) + self.parent_sitrep.as_ref().map(|s| &s.sitrep) } pub fn inventory(&self) -> &inventory::Collection { @@ -107,15 +107,15 @@ impl Input { /// Returns a [`Builder`] for constructing a new `Input` from the provided /// `parent_sitrep`, inventory collection, and in-service disks. pub fn builder( - parent_sitrep: Option>, + parent_sitrep: Option>, inv: Arc, in_service_disks: Arc>, ) -> Result { // Before preparing analysis inputs, check that the proposed input // inventory collection is at least as new as the parent sitrep's // inventory collection. - if let Some((_, ref parent)) = parent_sitrep.as_deref() { - let parent = &parent.metadata; + if let Some(ref committed) = parent_sitrep.as_deref() { + let parent = &committed.sitrep.metadata; // It is always okay to produce a new sitrep based on the same // inventory collection as the parent sitrep... if parent.inv_collection_id != inv.id @@ -159,7 +159,7 @@ pub enum InvalidInputs { #[must_use] pub struct Builder { - parent_sitrep: Option>, + parent_sitrep: Option>, inv: Arc, in_service_disks: Arc>, /// Ereports which are new and should be input to analysis in the next @@ -212,7 +212,7 @@ impl Builder { &mut self, ereports: impl IntoIterator, ) { - let parent_sitrep = self.parent_sitrep.as_ref().map(|s| &s.1); + let parent_sitrep = self.parent_sitrep.as_ref().map(|s| &s.sitrep); self.new_ereports.extend(ereports.into_iter().filter_map(|ereport| { if let Some(sitrep) = parent_sitrep { let id = ereport.id; @@ -272,7 +272,7 @@ impl Builder { /// that provides a human-readable summary of how the inputs were /// constructed. pub fn build(self) -> (Input, Report) { - let parent_sitrep = self.parent_sitrep.as_ref().map(|s| &s.1); + let parent_sitrep = self.parent_sitrep.as_ref().map(|s| &s.sitrep); let (parent_sitrep_id, parent_inv_id) = match parent_sitrep { Some(sitrep) => { let id = sitrep.id(); @@ -621,14 +621,17 @@ mod tests { cases, ereports_by_id, }; - Arc::new(( - SitrepVersion { - id: parent_sitrep_id, - version: 420, - time_made_current: chrono::Utc::now(), - }, - sitrep, - )) + Arc::new( + fm::CommittedSitrep::new( + SitrepVersion { + id: parent_sitrep_id, + version: 420, + time_made_current: chrono::Utc::now(), + }, + sitrep, + ) + .unwrap(), + ) }; // Build analysis input @@ -893,7 +896,9 @@ mod tests { time_made_current: chrono::Utc::now(), }; Input::builder( - Some(Arc::new((parent_version, parent))), + Some(Arc::new( + fm::CommittedSitrep::new(parent_version, parent).unwrap(), + )), inv, Arc::new(IdOrdMap::new()), ) diff --git a/nexus/fm/src/builder.rs b/nexus/fm/src/builder.rs index ffb184aa2a5..c8ec41beba7 100644 --- a/nexus/fm/src/builder.rs +++ b/nexus/fm/src/builder.rs @@ -295,7 +295,9 @@ mod tests { time_made_current: chrono::Utc::now(), }; let (input, _) = Input::builder( - Some(Arc::new((parent_version, parent))), + Some(Arc::new( + fm::CommittedSitrep::new(parent_version, parent).unwrap(), + )), inv, Arc::new(IdOrdMap::new()), ) @@ -521,7 +523,9 @@ mod tests { time_made_current: chrono::Utc::now(), }; let mut builder_inputs = crate::analysis_input::Input::builder( - Some(Arc::new((parent_version, parent))), + Some(Arc::new( + fm::CommittedSitrep::new(parent_version, parent).unwrap(), + )), inv, Arc::new(IdOrdMap::new()), ) @@ -596,7 +600,9 @@ mod tests { time_made_current: chrono::Utc::now(), }; let mut builder_inputs = crate::analysis_input::Input::builder( - Some(Arc::new((parent_version, parent))), + Some(Arc::new( + fm::CommittedSitrep::new(parent_version, parent).unwrap(), + )), inv, Arc::new(IdOrdMap::new()), ) diff --git a/nexus/fm/src/diagnosis/physical_disk.rs b/nexus/fm/src/diagnosis/physical_disk.rs index 8a0519ac165..f23bc243068 100644 --- a/nexus/fm/src/diagnosis/physical_disk.rs +++ b/nexus/fm/src/diagnosis/physical_disk.rs @@ -411,14 +411,17 @@ mod tests { in_service: IdOrdMap, ) -> Input { let parent = parent_sitrep.map(|s| { - Arc::new(( - SitrepVersion { - id: s.id(), - version: 0, - time_made_current: Utc::now(), - }, - s, - )) + Arc::new( + fm::CommittedSitrep::new( + SitrepVersion { + id: s.id(), + version: 0, + time_made_current: Utc::now(), + }, + s, + ) + .unwrap(), + ) }); let builder = Input::builder(parent, Arc::new(collection), Arc::new(in_service)) diff --git a/nexus/fm/src/diagnosis/power_shelf.rs b/nexus/fm/src/diagnosis/power_shelf.rs index 666afcfb343..12243c08da2 100644 --- a/nexus/fm/src/diagnosis/power_shelf.rs +++ b/nexus/fm/src/diagnosis/power_shelf.rs @@ -1016,14 +1016,12 @@ mod tests { new_ereports: impl IntoIterator, ) -> Input { let parent = parent_sitrep.map(|s| { - Arc::new(( - SitrepVersion { - id: s.id(), - version: 1, - time_made_current: Utc::now(), - }, - s, - )) + let version = SitrepVersion { + id: s.id(), + version: 1, + time_made_current: Utc::now(), + }; + Arc::new(fm::CommittedSitrep::new(version, s).unwrap()) }); let mut builder = fmtest .input_builder(parent, collection.into(), Arc::new(IdOrdMap::new())) diff --git a/nexus/fm/src/test_util.rs b/nexus/fm/src/test_util.rs index 2eced035034..a0776e44f38 100644 --- a/nexus/fm/src/test_util.rs +++ b/nexus/fm/src/test_util.rs @@ -9,10 +9,10 @@ use chrono::Utc; use iddqd::IdOrdMap; use nexus_db_model::EreporterRestart; use nexus_reconfigurator_planning::example; +use nexus_types::fm; use nexus_types::fm::ereport::{ Ena, Ereport, EreportData, EreportId, Reporter, }; -use nexus_types::fm::{Sitrep, SitrepVersion}; use nexus_types::in_service_disk::InServiceDisk; use nexus_types::inventory; use omicron_test_utils::dev; @@ -66,7 +66,7 @@ impl FmTest { // somehow... pub fn input_builder( &self, - parent_sitrep: Option>, + parent_sitrep: Option>, inv: Arc, in_service_disks: Arc>, ) -> Result { diff --git a/nexus/src/app/background/tasks/fm_analysis.rs b/nexus/src/app/background/tasks/fm_analysis.rs index 3ce33ff7f4f..5144b3c4a55 100644 --- a/nexus/src/app/background/tasks/fm_analysis.rs +++ b/nexus/src/app/background/tasks/fm_analysis.rs @@ -126,7 +126,7 @@ impl FmAnalysis { .collect(); let parent_sitrep = self.sitrep_rx.borrow_and_update().clone(); - let parent_sitrep_id = parent_sitrep.as_ref().map(|s| s.1.id()); + let parent_sitrep_id = parent_sitrep.as_ref().map(|s| s.sitrep.id()); let cfg = { let cfg_view = self.cfg_rx.borrow_and_update(); @@ -313,14 +313,14 @@ impl FmAnalysis { .context("failed to load new ereports")?; self.load_existing_alert_markers( opctx, - parent_sitrep.as_ref().map(|s| &s.1), + parent_sitrep.as_ref().map(|s| &s.sitrep), &mut builder, ) .await .context("failed to load existing alert markers")?; self.load_existing_support_bundle_markers( opctx, - parent_sitrep.as_ref().map(|s| &s.1), + parent_sitrep.as_ref().map(|s| &s.sitrep), &mut builder, ) .await @@ -867,24 +867,24 @@ mod tests { /// inventory collection. fn make_current_sitrep(inv: &inventory::Collection) -> CurrentSitrep { let id = SitrepUuid::new_v4(); - Arc::new(( - SitrepVersion { id, version: 1, time_made_current: Utc::now() }, - Sitrep { - metadata: SitrepMetadata { - id, - parent_sitrep_id: None, - inv_collection_id: inv.id, - next_inv_min_time_started: inv.time_done, - creator_id: OmicronZoneUuid::new_v4(), - comment: "test sitrep".to_string(), - time_created: Utc::now(), - alert_generation: Generation::new(), - support_bundle_generation: Generation::new(), - }, - cases: Default::default(), - ereports_by_id: Default::default(), + let version = + SitrepVersion { id, version: 1, time_made_current: Utc::now() }; + let sitrep = Sitrep { + metadata: SitrepMetadata { + id, + parent_sitrep_id: None, + inv_collection_id: inv.id, + next_inv_min_time_started: inv.time_done, + creator_id: OmicronZoneUuid::new_v4(), + comment: "test sitrep".to_string(), + time_created: Utc::now(), + alert_generation: Generation::new(), + support_bundle_generation: Generation::new(), }, - )) + cases: Default::default(), + ereports_by_id: Default::default(), + }; + Arc::new(fm::CommittedSitrep::new(version, sitrep).unwrap()) } #[tokio::test] @@ -1196,14 +1196,17 @@ mod tests { .await .expect("created the satisfied case's alert"); - let parent: CurrentSitrep = Arc::new(( - SitrepVersion { - id: sitrep_id, - version: 1, - time_made_current: Utc::now(), - }, - sitrep, - )); + let parent: CurrentSitrep = Arc::new( + fm::CommittedSitrep::new( + SitrepVersion { + id: sitrep_id, + version: 1, + time_made_current: Utc::now(), + }, + sitrep, + ) + .unwrap(), + ); let (_sitrep_tx, sitrep_rx) = watch::channel(None); let (_inv_tx, inv_rx) = watch::channel(None); diff --git a/nexus/src/app/background/tasks/fm_rendezvous.rs b/nexus/src/app/background/tasks/fm_rendezvous.rs index e91de5ee080..46554fdc3ba 100644 --- a/nexus/src/app/background/tasks/fm_rendezvous.rs +++ b/nexus/src/app/background/tasks/fm_rendezvous.rs @@ -127,7 +127,7 @@ impl FmRendezvous { "; let status = Status { - sitrep_id: Some(sitrep.1.id()), + sitrep_id: Some(sitrep.sitrep.id()), alerts: alerts.await.expect(TASKS_SHOULDNT_FAIL), support_bundles: support_bundles.await.expect(TASKS_SHOULDNT_FAIL), ereport_marking: marking.await.expect(TASKS_SHOULDNT_FAIL), @@ -164,7 +164,7 @@ impl FmRendezvous { let sitrep = sitrep.clone(); let opctx = opctx.child( [ - ("sitrep_id".to_string(), sitrep.1.id().to_string()), + ("sitrep_id".to_string(), sitrep.sitrep.id().to_string()), ("rendezvous_op".to_string(), opname.to_string()), ] .into_iter() @@ -184,7 +184,7 @@ impl FmRendezvous { sitrep: CurrentSitrep, opctx: OpContext, ) -> AlertCreationStatus { - let (_, ref sitrep) = *sitrep; + let sitrep = &sitrep.sitrep; let mut status = AlertCreationStatus::default(); let expected_alert_generation = sitrep.metadata.alert_generation; @@ -317,7 +317,7 @@ impl FmRendezvous { ) -> EreportMarkingStatus { const BATCH_SIZE: usize = 1000; - let (_, ref sitrep) = *sitrep; + let sitrep = &sitrep.sitrep; let mut status = EreportMarkingStatus { batch_size: BATCH_SIZE, total_ereports_in_sitrep: sitrep.ereports_by_id.len(), @@ -390,7 +390,7 @@ impl FmRendezvous { sitrep: CurrentSitrep, opctx: OpContext, ) -> SupportBundleCreationStatus { - let (_, ref sitrep) = *sitrep; + let sitrep = &sitrep.sitrep; let mut status = SupportBundleCreationStatus::default(); let expected_support_bundle_generation = @@ -565,7 +565,7 @@ impl FmRendezvous { sitrep: CurrentSitrep, opctx: OpContext, ) -> MarkerGcStatus { - let (_, ref sitrep) = *sitrep; + let sitrep = &sitrep.sitrep; let result = self .datastore .fm_rendezvous_alert_marker_gc( @@ -584,7 +584,7 @@ impl FmRendezvous { sitrep: CurrentSitrep, opctx: OpContext, ) -> MarkerGcStatus { - let (_, ref sitrep) = *sitrep; + let sitrep = &sitrep.sitrep; let result = self .datastore .fm_rendezvous_support_bundle_marker_gc( @@ -770,14 +770,17 @@ mod tests { .expect("inserted sitrep1"); sitrep_tx - .send(Some(Arc::new(( - fm::SitrepVersion { - id: sitrep1_id, - version: 1, - time_made_current: Utc::now(), - }, - sitrep1, - )))) + .send(Some(Arc::new( + fm::CommittedSitrep::new( + fm::SitrepVersion { + id: sitrep1_id, + version: 1, + time_made_current: Utc::now(), + }, + sitrep1, + ) + .unwrap(), + ))) .unwrap(); let Status { sitrep_id, alerts, alert_marker_gc, .. } = @@ -883,14 +886,17 @@ mod tests { .expect("inserted sitrep2"); sitrep_tx - .send(Some(Arc::new(( - fm::SitrepVersion { - id: sitrep2_id, - version: 2, - time_made_current: Utc::now(), - }, - sitrep2, - )))) + .send(Some(Arc::new( + fm::CommittedSitrep::new( + fm::SitrepVersion { + id: sitrep2_id, + version: 2, + time_made_current: Utc::now(), + }, + sitrep2, + ) + .unwrap(), + ))) .unwrap(); let status = dbg!(task.actually_activate(opctx).await); @@ -1072,14 +1078,16 @@ mod tests { // of date, and rendezvous execution should be aborted before // attempting to insert the other alert. sitrep_tx - .send(Some(Arc::new(( - fm::SitrepVersion { - id: stale_sitrep_id, - version: 1, - time_made_current: Utc::now(), - }, - stale_sitrep, - )))) + .send(Some(Arc::new( + fm::CommittedSitrep::new( + fm::SitrepVersion { + id: stale_sitrep_id, + version: 1, + time_made_current: Utc::now(), + }, + stale_sitrep, + ).unwrap() + ))) .unwrap(); let status = dbg!(task.actually_activate(opctx).await); @@ -1248,14 +1256,16 @@ mod tests { // and breaks out of the bundle loop before the second request is // attempted. sitrep_tx - .send(Some(Arc::new(( - fm::SitrepVersion { - id: stale_sitrep_id, - version: 1, - time_made_current: Utc::now(), - }, - stale_sitrep, - )))) + .send(Some(Arc::new( + fm::CommittedSitrep::new( + fm::SitrepVersion { + id: stale_sitrep_id, + version: 1, + time_made_current: Utc::now(), + }, + stale_sitrep, + ).unwrap() + ))) .unwrap(); let status = dbg!(task.actually_activate(opctx).await); @@ -1531,14 +1541,16 @@ mod tests { }; sitrep_tx - .send(Some(Arc::new(( - fm::SitrepVersion { - id: sitrep1_id, - version: 1, - time_made_current: Utc::now(), - }, - sitrep1, - )))) + .send(Some(Arc::new( + fm::CommittedSitrep::new( + fm::SitrepVersion { + id: sitrep1_id, + version: 1, + time_made_current: Utc::now(), + }, + sitrep1, + ).unwrap() + ))) .unwrap(); // First activation should mark ereport1 and ereport2 as seen @@ -1744,14 +1756,16 @@ mod tests { }; sitrep_tx - .send(Some(Arc::new(( - fm::SitrepVersion { - id: sitrep1_id, - version: 1, - time_made_current: Utc::now(), - }, - sitrep1, - )))) + .send(Some(Arc::new( + fm::CommittedSitrep::new( + fm::SitrepVersion { + id: sitrep1_id, + version: 1, + time_made_current: Utc::now(), + }, + sitrep1, + ).unwrap() + ))) .unwrap(); // Activate with sitrep 1 --- should mark only ereport1. @@ -1856,14 +1870,16 @@ mod tests { }; sitrep_tx - .send(Some(Arc::new(( - fm::SitrepVersion { - id: sitrep2_id, - version: 2, - time_made_current: Utc::now(), - }, - sitrep2, - )))) + .send(Some(Arc::new( + fm::CommittedSitrep::new( + fm::SitrepVersion { + id: sitrep2_id, + version: 2, + time_made_current: Utc::now(), + }, + sitrep2, + ).unwrap() + ))) .unwrap(); // Activate with sitrep 2 --- should mark ereport2 and ereport3, but @@ -2064,14 +2080,16 @@ mod tests { .expect("inserted sitrep1"); sitrep_tx - .send(Some(Arc::new(( - fm::SitrepVersion { - id: sitrep1_id, - version: 1, - time_made_current: Utc::now(), - }, - sitrep1, - )))) + .send(Some(Arc::new( + fm::CommittedSitrep::new( + fm::SitrepVersion { + id: sitrep1_id, + version: 1, + time_made_current: Utc::now(), + }, + sitrep1, + ).unwrap() + ))) .unwrap(); // First activation: should create the support bundle. @@ -2157,14 +2175,16 @@ mod tests { .expect("inserted sitrep2"); sitrep_tx - .send(Some(Arc::new(( - fm::SitrepVersion { - id: sitrep2_id, - version: 2, - time_made_current: Utc::now(), - }, - sitrep2, - )))) + .send(Some(Arc::new( + fm::CommittedSitrep::new( + fm::SitrepVersion { + id: sitrep2_id, + version: 2, + time_made_current: Utc::now(), + }, + sitrep2, + ).unwrap() + ))) .unwrap(); // Activation with sitrep2: should create only the new bundle. The @@ -2274,14 +2294,16 @@ mod tests { .expect("inserted sitrep"); sitrep_tx - .send(Some(Arc::new(( - fm::SitrepVersion { - id: sitrep_id, - version: 1, - time_made_current: Utc::now(), - }, - sitrep, - )))) + .send(Some(Arc::new( + fm::CommittedSitrep::new( + fm::SitrepVersion { + id: sitrep_id, + version: 1, + time_made_current: Utc::now(), + }, + sitrep, + ).unwrap() + ))) .unwrap(); let status = dbg!(task.actually_activate(opctx).await); diff --git a/nexus/src/app/background/tasks/fm_sitrep_load.rs b/nexus/src/app/background/tasks/fm_sitrep_load.rs index b3733c8c852..ca8ac342a76 100644 --- a/nexus/src/app/background/tasks/fm_sitrep_load.rs +++ b/nexus/src/app/background/tasks/fm_sitrep_load.rs @@ -10,21 +10,20 @@ use chrono::Utc; use futures::future::BoxFuture; use nexus_db_queries::context::OpContext; use nexus_db_queries::db::DataStore; -use nexus_types::fm::Sitrep; -use nexus_types::fm::SitrepVersion; +use nexus_types::fm; use nexus_types::internal_api::background::SitrepLoadStatus as Status; use serde_json::json; use slog_error_chain::InlineErrorChain; use std::sync::Arc; use tokio::sync::watch; +pub type CurrentSitrep = Arc; + pub struct SitrepLoader { datastore: Arc, - tx: watch::Sender>, + tx: watch::Sender>>, } -pub type CurrentSitrep = Arc<(SitrepVersion, Sitrep)>; - impl BackgroundTask for SitrepLoader { fn activate<'a>( &'a mut self, @@ -65,7 +64,11 @@ impl SitrepLoader { let (old, log) = match &*self.tx.borrow() { None => (None, opctx.log.clone()), Some(old) => { - let (ref old_version, _) = **old; + let old_version = fm::SitrepVersion { + id: old.sitrep.id(), + version: old.version, + time_made_current: old.time_made_current, + }; let log = opctx.log.new(slog::o!( // since this is a TypedUuid, use `Debug` to avoid // including () @@ -73,20 +76,20 @@ impl SitrepLoader { "original_made_current" => old_version.time_made_current.to_string(), "original_version" => old_version.version, )); - (Some(old_version.clone()), log) + (Some(old_version), log) } }; // Get the ID of the current sitrep. let time_loaded = Utc::now(); - let current_version: SitrepVersion = match self + let current_version: fm::SitrepVersion = match self .datastore .fm_current_sitrep_version(opctx) .await { Ok(Some(version)) => version, Ok(None) => match old { - Some(SitrepVersion { version, id, .. }) => { + Some(fm::SitrepVersion { version, id, .. }) => { // We should never go from "some sitrep" to "no sitrep"; // pruning should always keep a small number of old sitreps // around until we have new ones to replace them. @@ -138,7 +141,7 @@ impl SitrepLoader { current_version.version, old.version, )); } - Some(SitrepVersion { version, id, .. }) + Some(fm::SitrepVersion { version, id, .. }) if version == current_version.version && id != current_version.id => { @@ -178,7 +181,9 @@ impl SitrepLoader { } }; - let sitrep = Arc::new((current_version.clone(), sitrep)); + let sitrep = Arc::new(fm::CommittedSitrep::new(current_version.clone(), sitrep).expect( + "version ID should match sitrep ID since we just loaded the sitrep by version ID" + )); self.tx.send_modify(|s| { *s = Some(sitrep); }); @@ -192,6 +197,7 @@ mod test { use super::*; use crate::app::background::BackgroundTask; use nexus_db_queries::db::pub_test_utils::TestDatabase; + use nexus_types::fm::Sitrep; use nexus_types::fm::SitrepMetadata; use omicron_common::api::external::Generation; use omicron_test_utils::dev; @@ -247,16 +253,21 @@ mod test { .borrow_and_update() .clone() .expect("the new sitrep should have been loaded"); - let (ref loaded_version1, ref loaded_sitrep) = *snapshot; + let loaded_version1 = fm::SitrepVersion { + id: snapshot.sitrep.id(), + version: snapshot.version, + time_made_current: snapshot.time_made_current, + }; + let loaded_sitrep = &snapshot.sitrep; // N.B.: we just compare the IDs here as comparing the whole struct may // not be equal, since the `time_created` field may have been rounded in // CRDB. Which is a shame, but whatever. :/ assert_eq!(loaded_sitrep.metadata.id, sitrep1.metadata.id); - dbg!(loaded_version1); + dbg!(&loaded_version1); let status = serde_json::from_value::(status).unwrap(); match status { Status::Loaded { version, .. } => { - assert_eq!(&version, loaded_version1); + assert_eq!(version, loaded_version1); } status => panic!("expected Status::Loaded, got {status:?}",), }; @@ -272,13 +283,18 @@ mod test { .borrow_and_update() .clone() .expect("the same should have been loaded"); - let (ref loaded_version2, ref loaded_sitrep) = *snapshot; + let loaded_version2 = fm::SitrepVersion { + id: snapshot.sitrep.id(), + version: snapshot.version, + time_made_current: snapshot.time_made_current, + }; + let loaded_sitrep = &snapshot.sitrep; assert_eq!(loaded_sitrep.metadata.id, sitrep1.metadata.id); - dbg!(loaded_version1, loaded_version2); + dbg!(&loaded_version1, &loaded_version2); let status = serde_json::from_value::(status).unwrap(); match status { Status::Loaded { version, .. } => { - assert_eq!(&version, loaded_version2); + assert_eq!(version, loaded_version2); } status => panic!("expected Status::Loaded, got {status:?}",), }; @@ -316,14 +332,19 @@ mod test { .borrow_and_update() .clone() .expect("the new sitrep should have been loaded"); - let (ref loaded_version3, ref loaded_sitrep) = *snapshot; + let loaded_version3 = fm::SitrepVersion { + id: snapshot.sitrep.id(), + version: snapshot.version, + time_made_current: snapshot.time_made_current, + }; + let loaded_sitrep = &snapshot.sitrep; assert_eq!(loaded_sitrep.metadata.id, sitrep2.metadata.id); - dbg!(loaded_version3); - assert_ne!(loaded_version3, loaded_version2); + dbg!(&loaded_version3); + assert_ne!(&loaded_version3, &loaded_version2); let status = serde_json::from_value::(status).unwrap(); match status { Status::Loaded { version, .. } => { - assert_eq!(&version, loaded_version3); + assert_eq!(version, loaded_version3); } status => panic!("expected Status::Loaded, got {status:?}",), }; diff --git a/nexus/types/src/fm.rs b/nexus/types/src/fm.rs index f673f7ff751..46e000ac943 100644 --- a/nexus/types/src/fm.rs +++ b/nexus/types/src/fm.rs @@ -24,6 +24,7 @@ pub mod display; use case::AlertRequest; use chrono::{DateTime, Utc}; use iddqd::IdOrdMap; +use omicron_common::api::external::Error; use omicron_common::api::external::Generation; use omicron_uuid_kinds::{ CaseUuid, CollectionUuid, OmicronZoneUuid, SitrepUuid, @@ -125,6 +126,42 @@ impl Sitrep { } } +/// Represents a sitrep that has been committed to the sitrep history, and which +/// was read from the sitrep history. This is a wrapper for a [`Sitrep`] along +/// with its version metadata. +#[derive(Clone, Debug, Eq, PartialEq, Deserialize, Serialize)] +pub struct CommittedSitrep { + pub version: u32, + pub time_made_current: DateTime, + pub sitrep: Sitrep, +} + +impl CommittedSitrep { + pub fn new(version: SitrepVersion, sitrep: Sitrep) -> Result { + let SitrepVersion { version, time_made_current, id } = version; + if id != sitrep.id() { + return Err(Error::invalid_value( + "version", + format!( + "version v{version} refers to sitrep {id}, not sitrep {}", + sitrep.id() + ), + )); + } + Ok(Self { version, time_made_current, sitrep }) + } +} + +impl TryFrom<(SitrepVersion, Sitrep)> for CommittedSitrep { + type Error = Error; + + fn try_from( + (version, sitrep): (SitrepVersion, Sitrep), + ) -> Result { + Self::new(version, sitrep) + } +} + /// Metadata describing a sitrep. /// /// This corresponds to the records stored in the `fm_sitrep` database table.