[fm] only you can prevent inventory collection time travel (#10266)

Depends on #10258.

As described in #10260 (and [§8.2 RFD 603]), **only _you_ can prevent
inventory collection time travel**: because each Nexus that might
participate in FM analysis has its own copy of the inventory which it
considers to be current (loaded by its `inventory_load` background
task). Since we must allow for the possibility of a particular Nexus
being arbitrarily behind, we can easily imagine a scenario where a Nexus
attempts to perform FM analysis and generate a sitrep while operating on
a copy of the inventory that is actually *older* than the inventory that
was used to produce the current sitrep. If this happens, we might
incorrectly think that the state of the system has _changed_ from the
state in the current sitrep, while actually, we are just operating on an
outdated understanding. This branch fixes that.

In #10260, I proposed addressing this by making the CTE that inserts a
sitrep into the sitrep history only succeed if the inventory collection
UUID in that sitrep's metadata is newer than the one in the current
sitrep's metadata. This would probably have worked, but putting that
check in the already somewhat hairy CTE was not actually necessary.
Instead, we can just perform this check *before* actually doing any
analysis, which feels a lot nicer. In order to do this, it was necessary
to add a query to just go and fetch only the timestamps from an
inventory by its UUID, which was not a huge deal. I think it is
plausible that the "is strictly newer than" check for inventory
collections may be useful elsewhere as well.

Fixes #10260 

[§8.2 RFD 603]: https://rfd.shared.oxide.computer/rfd/0603#_inventory

Author: hawkw

Cargo.lock

@@ -246,6 +246,7 @@ async fn cmd_db_sitrep_show(
         parent_sitrep_id,
         inv_collection_id,
         comment,
+        next_inv_min_time_started,
     } = metadata;
 
     const ID: &'static str = "ID";
@@ -259,6 +260,8 @@ async fn cmd_db_sitrep_show(
     const INV_COLLECTION_ID: &'static str = "inventory collection ID";
     const INV_STARTED_AT: &'static str = "  started at";
     const INV_FINISHED_AT: &'static str = "  finished at";
+    const NEXT_INV_MIN_START: &'static str =
+        "  next inventory minimum start time";
     const TOTAL_EREPORTS: &'static str = "ereports in this sitrep";
 
     const WIDTH: usize = const_max_len(&[
@@ -273,6 +276,7 @@ async fn cmd_db_sitrep_show(
         INV_COLLECTION_ID,
         INV_STARTED_AT,
         INV_FINISHED_AT,
+        NEXT_INV_MIN_START,
         TOTAL_EREPORTS,
     ]);
 
@@ -346,6 +350,8 @@ async fn cmd_db_sitrep_show(
             )
         }
     }
+    println!("    {NEXT_INV_MIN_START:>WIDTH$}: {next_inv_min_time_started}");
+    println!("    ");
     println!("    {TOTAL_EREPORTS}: {}", ereports_by_id.len());
     // TODO(eliza): perhaps display a table summarizing those ereports? possibly
     // behind a verbose flag?

dev-tools/omdb/src/bin/omdb/db/sitrep.rs

@@ -3485,6 +3485,7 @@ fn print_task_fm_analysis(details: &serde_json::Value) {
     use nexus_types::internal_api::background::fm_analysis::{
         AnalysisOutcome, AnalysisStatus, Outcome, PreparationStatus,
     };
+
     let FmAnalysisStatus { parent_sitrep_id, inv_collection_id, outcome } =
         match serde_json::from_value::<FmAnalysisStatus>(details.clone()) {
             Err(error) => {
@@ -3512,6 +3513,30 @@ fn print_task_fm_analysis(details: &serde_json::Value) {
             );
             return;
         }
+        Outcome::WaitingForNewerInventory {
+            parent_inv_id,
+            next_inv_min_time_started,
+            input_inv_time_started,
+        } => {
+            const PARENT_INV: &str = "parent sitrep's inventory ID:";
+            const MIN_STARTED: &str = "earliest start time for next inventory:";
+            const LOADED_STARTED: &str = "loaded inventory started at:";
+            const WIDTH: usize =
+                const_max_len(&[PARENT_INV, MIN_STARTED, LOADED_STARTED]) + 1;
+            println!(
+                "    waiting for a newer inventory collection than the one \
+                 in the parent sitrep"
+            );
+            let min_started = humantime::format_rfc3339_millis(
+                next_inv_min_time_started.into(),
+            );
+            let loaded_started =
+                humantime::format_rfc3339_millis(input_inv_time_started.into());
+            println!("      {PARENT_INV:<WIDTH$}{parent_inv_id}");
+            println!("      {MIN_STARTED:<WIDTH$}{min_started}");
+            println!("      {LOADED_STARTED:<WIDTH$}{loaded_started}");
+            return;
+        }
         Outcome::PreparationError(error) => {
             println!(
                 "{ERRICON} failed to prepare analysis inputs:\n    {error}"

dev-tools/omdb/src/bin/omdb/nexus.rs

@@ -38,6 +38,7 @@ pub struct SitrepMetadata {
     pub time_created: DateTime<Utc>,
     pub creator_id: DbTypedUuid<OmicronZoneKind>,
     pub comment: String,
+    pub next_inv_min_time_started: DateTime<Utc>,
 }
 
 impl From<SitrepMetadata> for nexus_types::fm::SitrepMetadata {
@@ -49,12 +50,14 @@ impl From<SitrepMetadata> for nexus_types::fm::SitrepMetadata {
             creator_id,
             comment,
             time_created,
+            next_inv_min_time_started,
         } = db_meta;
         Self {
             id: id.into(),
             parent_sitrep_id: parent_sitrep_id.map(Into::into),
             inv_collection_id: inv_collection_id.into(),
             creator_id: creator_id.into(),
+            next_inv_min_time_started,
             comment,
             time_created,
         }
@@ -70,6 +73,7 @@ impl From<nexus_types::fm::SitrepMetadata> for SitrepMetadata {
             creator_id,
             comment,
             time_created,
+            next_inv_min_time_started,
         } = db_meta;
         Self {
             id: id.into(),
@@ -78,6 +82,7 @@ impl From<nexus_types::fm::SitrepMetadata> for SitrepMetadata {
             creator_id: creator_id.into(),
             comment,
             time_created,
+            next_inv_min_time_started,
         }
     }
 }

nexus/db-model/src/fm.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(250, 0, 0);
+pub const SCHEMA_VERSION: Version = Version::new(251, 0, 0);
 
 /// List of all past database schema versions, in *reverse* order
 ///
@@ -28,6 +28,7 @@ pub static KNOWN_VERSIONS: LazyLock<Vec<KnownVersion>> = 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(251, "fm-sitrep-next-inv-min-time-started"),
         KnownVersion::new(250, "inv-svc-enabled-not-online"),
         KnownVersion::new(249, "fm-support-bundle-request"),
         KnownVersion::new(248, "cleanup-orphaned-subnet-pool-silo-links"),

nexus/db-model/src/schema_versions.rs

@@ -1779,6 +1779,7 @@ mod tests {
                 comment: "TEST SITREP PLEASE IGNORE".to_string(),
                 time_created: Utc::now(),
                 parent_sitrep_id: None,
+                next_inv_min_time_started: Utc::now(),
             },
             cases: Default::default(),
             ereports_by_id: Default::default(),
@@ -1829,6 +1830,7 @@ mod tests {
                 comment: "TEST SITREP 1".to_string(),
                 time_created: Utc::now(),
                 parent_sitrep_id: None,
+                next_inv_min_time_started: Utc::now(),
             },
             cases: Default::default(),
             ereports_by_id: Default::default(),
@@ -1844,6 +1846,7 @@ mod tests {
                 comment: "TEST SITREP 2".to_string(),
                 time_created: Utc::now(),
                 parent_sitrep_id: Some(sitrep1.id()),
+                next_inv_min_time_started: Utc::now(),
             },
             cases: Default::default(),
             ereports_by_id: Default::default(),
@@ -1886,6 +1889,7 @@ mod tests {
                 comment: "TEST SITREP 1".to_string(),
                 time_created: Utc::now(),
                 parent_sitrep_id: None,
+                next_inv_min_time_started: Utc::now(),
             },
             cases: Default::default(),
             ereports_by_id: Default::default(),
@@ -1902,6 +1906,7 @@ mod tests {
                 comment: "TEST SITREP WITH BAD PARENT".to_string(),
                 time_created: Utc::now(),
                 parent_sitrep_id: Some(nonexistent_id),
+                next_inv_min_time_started: Utc::now(),
             },
             cases: Default::default(),
             ereports_by_id: Default::default(),
@@ -1938,6 +1943,7 @@ mod tests {
                 comment: "TEST SITREP 1".to_string(),
                 time_created: Utc::now(),
                 parent_sitrep_id: None,
+                next_inv_min_time_started: Utc::now(),
             },
             cases: Default::default(),
             ereports_by_id: Default::default(),
@@ -1953,6 +1959,7 @@ mod tests {
                 comment: "TEST SITREP 2".to_string(),
                 time_created: Utc::now(),
                 parent_sitrep_id: Some(sitrep1.id()),
+                next_inv_min_time_started: Utc::now(),
             },
             cases: Default::default(),
             ereports_by_id: Default::default(),
@@ -1969,6 +1976,7 @@ mod tests {
                 comment: "TEST SITREP 3 WITH OUTDATED PARENT".to_string(),
                 time_created: Utc::now(),
                 parent_sitrep_id: Some(sitrep1.id()),
+                next_inv_min_time_started: Utc::now(),
             },
             cases: Default::default(),
             ereports_by_id: Default::default(),
@@ -2296,6 +2304,7 @@ mod tests {
                     .to_string(),
                 time_created: Utc::now(),
                 parent_sitrep_id: None,
+                next_inv_min_time_started: Utc::now(),
             },
             cases,
             ereports_by_id,
@@ -2405,6 +2414,7 @@ mod tests {
                 comment: "sitrep with support bundle requests".to_string(),
                 time_created: Utc::now(),
                 parent_sitrep_id: None,
+                next_inv_min_time_started: Utc::now(),
             },
             cases,
             ereports_by_id: Default::default(),
@@ -2456,6 +2466,7 @@ mod tests {
                         creator_id: OmicronZoneUuid::new_v4(),
                         comment: "my cool sitrep".to_string(),
                         inv_collection_id: CollectionUuid::new_v4(),
+                        next_inv_min_time_started: Utc::now(),
                     },
                     cases: Default::default(),
                     ereports_by_id: Default::default(),
@@ -2612,6 +2623,7 @@ mod tests {
                         creator_id: OmicronZoneUuid::new_v4(),
                         comment: "my cool sitrep".to_string(),
                         inv_collection_id: CollectionUuid::new_v4(),
+                        next_inv_min_time_started: Utc::now(),
                     },
                     cases: Default::default(),
                     ereports_by_id: Default::default(),
@@ -2745,6 +2757,7 @@ mod tests {
                 comment: "test sitrep".to_string(),
                 time_created: Utc::now(),
                 parent_sitrep_id: None,
+                next_inv_min_time_started: Utc::now(),
             },
             cases: Default::default(),
             ereports_by_id: Default::default(),
@@ -2979,6 +2992,7 @@ mod tests {
                 comment: "test sitrep".to_string(),
                 time_created: Utc::now(),
                 parent_sitrep_id: None,
+                next_inv_min_time_started: Utc::now(),
             },
             cases: Default::default(),
             ereports_by_id: Default::default(),
@@ -3105,6 +3119,7 @@ mod tests {
                 comment: "test sitrep".to_string(),
                 time_created: Utc::now(),
                 parent_sitrep_id: None,
+                next_inv_min_time_started: Utc::now(),
             },
             cases: Default::default(),
             ereports_by_id: Default::default(),
@@ -3399,6 +3414,7 @@ mod tests {
                         creator_id: OmicronZoneUuid::new_v4(),
                         comment: "child sitrep".to_string(),
                         inv_collection_id: CollectionUuid::new_v4(),
+                        next_inv_min_time_started: Utc::now(),
                     },
                     cases: Default::default(),
                     ereports_by_id: Default::default(),

nexus/db-queries/src/db/datastore/fm.rs

@@ -3135,6 +3135,7 @@ table! {
         time_created -> Timestamptz,
         creator_id -> Uuid,
         comment -> Text,
+        next_inv_min_time_started -> Timestamptz,
     }
 }
 

nexus/db-schema/src/schema.rs

@@ -24,6 +24,7 @@ serde.workspace = true
 serde_json.workspace = true
 slog.workspace = true
 slog-error-chain.workspace = true
+thiserror.workspace = true
 typed-rng.workspace = true
 
 # deps for test utils

nexus/fm/Cargo.toml

@@ -4,10 +4,12 @@
 
 //! Inputs to fault management analysis.
 
+use chrono::{DateTime, Utc};
 use iddqd::IdOrdMap;
 use nexus_types::fm::analysis_reports::ClosedCaseReport;
 use nexus_types::fm::{self, Sitrep, SitrepVersion};
 use nexus_types::inventory;
+use omicron_uuid_kinds::CollectionUuid;
 use std::collections::BTreeMap;
 use std::collections::BTreeSet;
 use std::sync::Arc;
@@ -65,16 +67,49 @@ impl Input {
     pub fn builder(
         parent_sitrep: Option<Arc<(SitrepVersion, Sitrep)>>,
         inv: Arc<inventory::Collection>,
-    ) -> Builder {
-        Builder {
+    ) -> 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;
+            // 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
+            // ...but if they are not equal, the new inventory collection must
+            // have started after the minimum start time to be considered
+            // newer than the parent's.
+                && inv.time_started < parent.next_inv_min_time_started
+            {
+                return Err(InvalidInputs::InventoryStale {
+                    parent_inv_id: parent.inv_collection_id,
+                    next_inv_min_time_started: parent.next_inv_min_time_started,
+                    input_inv_time_started: inv.time_started,
+                });
+            }
+        }
+        Ok(Builder {
             parent_sitrep,
             inv,
             new_ereports: IdOrdMap::default(),
             unmarked_seen_ereports: BTreeSet::default(),
-        }
+        })
     }
 }
 
+#[derive(Debug, Eq, PartialEq, thiserror::Error)]
+pub enum InvalidInputs {
+    #[error(
+        "inventory collection from {input_inv_time_started} is not new \
+         enough: must have started at {next_inv_min_time_started} or later"
+    )]
+    InventoryStale {
+        parent_inv_id: CollectionUuid,
+        next_inv_min_time_started: DateTime<Utc>,
+        input_inv_time_started: DateTime<Utc>,
+    },
+}
+
 #[must_use]
 pub struct Builder {
     parent_sitrep: Option<Arc<(SitrepVersion, Sitrep)>>,
@@ -213,7 +248,7 @@ mod tests {
     use nexus_types::fm::{DiagnosisEngineKind, SitrepVersion};
     use nexus_types::inventory::SpType;
     use omicron_uuid_kinds::{
-        CaseEreportUuid, CaseUuid, CollectionUuid, OmicronZoneUuid, SitrepUuid,
+        CaseEreportUuid, CaseUuid, OmicronZoneUuid, SitrepUuid,
     };
     use std::sync::Arc;
 
@@ -404,10 +439,11 @@ mod tests {
                 metadata: fm::SitrepMetadata {
                     id: parent_sitrep_id,
                     parent_sitrep_id: Some(SitrepUuid::new_v4()),
-                    inv_collection_id: CollectionUuid::new_v4(),
+                    inv_collection_id: inv.id,
                     creator_id: OmicronZoneUuid::new_v4(),
                     comment: "parent sitrep for test".to_string(),
                     time_created: chrono::Utc::now(),
+                    next_inv_min_time_started: inv.time_done,
                 },
                 cases,
                 ereports_by_id,
@@ -424,7 +460,8 @@ mod tests {
 
         // Build analysis input
         let (input, report) = {
-            let mut builder = Input::builder(Some(parent_sitrep), inv);
+            let mut builder = Input::builder(Some(parent_sitrep), inv)
+                .expect("collection start time check should always pass");
             // Pass in four ereports:
             //  - two that are in the open cases of the parent sitrep
             //  - one that is in the (to-be-copied-forward) closed case

nexus/fm/src/analysis_input.rs

@@ -124,6 +124,11 @@ impl<'a> SitrepBuilder<'a> {
                 creator_id,
                 comment: self.comment,
                 time_created,
+                // When creating a new sitrep that is a child of this sitrep,
+                // the input inventory collection must either be the same
+                // inventory as this sitrep, or have started after this sitrep's
+                // inventory collection ended.
+                next_inv_min_time_started: self.inventory.time_done,
             },
             cases,
             ereports_by_id,