[2/n][fm] factor restart_id and time_created out of EreportData#10631
Conversation
hawkw
added
the
fault-management
hawkw
force-pushed
the
eliza/restart-order-ereport-refactor
branch
from
6242a48 to
0b4a9d2
Compare
There was a problem hiding this comment.
Pull request overview
Refactors FM ereport ingestion/types to avoid duplicating restart_id and time_collected across each per-ereport payload, ensuring those values cannot diverge within a single insertion batch (aligning with the ereporter_restart “first-seen” table approach introduced in #10618).
Changes:
- Move
id,time_collected, andcollector_idfromEreportDataonto the top-levelEreport. - Change
EreportData::from_sp_ereportto return(Ena, EreportData)and updateDataStore::ereports_insertto accept(Ena, EreportData)pairs plus a singlecollector_id. - Update affected call sites and tests across Nexus and support-bundle collection.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| support-bundle-collection/src/steps/ereports.rs | Switches path construction to use ereport.id after moving ID out of EreportData. |
| nexus/types/src/fm/ereport.rs | Restructures FM ereport types: lifts id/time_collected/collector_id to Ereport; adjusts SP conversion helper return type. |
| nexus/types/src/fm/case.rs | Updates FM case tests to construct Ereport with top-level ID/time/collector fields. |
| nexus/src/app/background/tasks/support_bundle_collector.rs | Updates test fixture inserts to the new ereports_insert API and tuple-based ereport inputs. |
| nexus/src/app/background/tasks/fm_rendezvous.rs | Updates rendezvous task tests to the new ereport construction and insert API (IDs now separate from data). |
| nexus/src/app/background/tasks/ereport_ingester.rs | Adapts ingester to new SP conversion return type and passes collector_id explicitly to ereports_insert. |
| nexus/fm/src/test_util.rs | Updates test utility to build Ereport with top-level ID/time/collector fields; adapts to SP conversion tuple return. |
| nexus/db-queries/src/db/datastore/fm.rs | Updates FM datastore tests for new Ereport::new signature and new ereports_insert tuple input format. |
| nexus/db-queries/src/db/datastore/ereport.rs | Updates datastore insert API signature, call sites, and tests to use a single collector_id and (Ena, EreportData) items. |
| nexus/db-model/src/ereport.rs | Updates DB model constructors/conversions to match the new types::Ereport / types::EreportData split. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -19,6 +19,10 @@ pub use ereport_types::{Ena, EreportId}; | |||
|
|
|||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | |||
| pub struct Ereport { | |||
There was a problem hiding this comment.
It might be useful to document what fields make sense to appear on Ereport vs. on EreportData. If I understand correctly, the idea is that EreportData is the actual "payload" of the ereport, created by the ereport's producer, whereas Ereports other fields are specific to Nexus's ingestion and bookkeeping of ereports. Is that right?
| pub fn id(&self) -> &EreportId { | ||
| &self.data.id | ||
| &self.id | ||
| } |
There was a problem hiding this comment.
Nit: maybe this function is less useful now?
Depends on #10618
This is a small refactor which I discussed in this comment on #10618. As part of that branch, I changed the
DataStore::ereports_insertmethod to take the restart ID andtime_collectedtimestamp as arguments to the method, since they are used for populating the restart entry. This results in the same values also being passed as part of theEreportDatastructs that represent the individual ereports to insert. This is unfortunate because it results in having to duplicate those values, but more importantly, because it allows them to differ when they should always be the same. This branch fixes that.