Skip to content

fm: refactor (SitrepVersion, Sitrep) into a type #11053

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Draft
hawkw wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fm: refactor (SitrepVersion, Sitrep) into a type #11053

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 16 additions & 11 deletions nexus/db-queries/src/db/datastore/fm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ impl DataStore {
pub async fn fm_sitrep_read_current(
&self,
opctx: &OpContext,
) -> Result<Option<(fm::SitrepVersion, Sitrep)>, Error> {
) -> Result<Option<fm::CommittedSitrep>, Error> {
let conn = self.pool_connection_authorized(opctx).await?;
loop {
let version =
Expand All @@ -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
Expand Down Expand Up @@ -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!(
Expand Down Expand Up @@ -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());

Expand Down Expand Up @@ -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());

Expand Down
41 changes: 23 additions & 18 deletions nexus/fm/src/analysis_input.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -38,7 +38,7 @@ pub use nexus_types::fm::analysis_reports::InputReport as Report;
/// [`Input::builder`].
#[derive(Debug)]
pub struct Input {
parent_sitrep: Option<Arc<(SitrepVersion, Sitrep)>>,
parent_sitrep: Option<Arc<fm::CommittedSitrep>>,
inv: Arc<inventory::Collection>,
/// Ereports which are new and should be input to analysis in the next
/// sitrep.
Expand All @@ -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 {
Expand Down Expand Up @@ -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<Arc<(SitrepVersion, Sitrep)>>,
parent_sitrep: Option<Arc<fm::CommittedSitrep>>,
inv: Arc<inventory::Collection>,
in_service_disks: Arc<IdOrdMap<InServiceDisk>>,
) -> Result<Builder, InvalidInputs> {
// 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
Expand Down Expand Up @@ -159,7 +159,7 @@ pub enum InvalidInputs {

#[must_use]
pub struct Builder {
parent_sitrep: Option<Arc<(SitrepVersion, Sitrep)>>,
parent_sitrep: Option<Arc<fm::CommittedSitrep>>,
inv: Arc<inventory::Collection>,
in_service_disks: Arc<IdOrdMap<InServiceDisk>>,
/// Ereports which are new and should be input to analysis in the next
Expand Down Expand Up @@ -212,7 +212,7 @@ impl Builder {
&mut self,
ereports: impl IntoIterator<Item = fm::Ereport>,
) {
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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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()),
)
Expand Down
12 changes: 9 additions & 3 deletions nexus/fm/src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
)
Expand Down Expand Up @@ -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()),
)
Expand Down Expand Up @@ -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()),
)
Expand Down
19 changes: 11 additions & 8 deletions nexus/fm/src/diagnosis/physical_disk.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,14 +411,17 @@ mod tests {
in_service: IdOrdMap<InServiceDisk>,
) -> 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))
Expand Down
14 changes: 6 additions & 8 deletions nexus/fm/src/diagnosis/power_shelf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1016,14 +1016,12 @@ mod tests {
new_ereports: impl IntoIterator<Item = Ereport>,
) -> 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()))
Expand Down
4 changes: 2 additions & 2 deletions nexus/fm/src/test_util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -66,7 +66,7 @@ impl FmTest {
// somehow...
pub fn input_builder(
&self,
parent_sitrep: Option<Arc<(SitrepVersion, Sitrep)>>,
parent_sitrep: Option<Arc<fm::CommittedSitrep>>,
inv: Arc<inventory::Collection>,
in_service_disks: Arc<IdOrdMap<InServiceDisk>>,
) -> Result<Builder, InvalidInputs> {
Expand Down
59 changes: 31 additions & 28 deletions nexus/src/app/background/tasks/fm_analysis.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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);
Expand Down
Loading
Loading