Repository navigation
[fm] Add disk diagnoser with typed fact tables #10541
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,158 @@ | ||
| // This Source Code Form is subject to the terms of the Mozilla Public | ||
| // License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| // file, You can obtain one at https://mozilla.org/MPL/2.0/. | ||
|
|
||
| //! Database representation of the physical-disk diagnosis engine's facts. | ||
| //! | ||
| //! Each physical-disk fact is stored as typed columns in the | ||
| //! `fm_fact_physical_disk` table. The `kind` discriminant selects which payload | ||
| //! columns are populated; a CHECK constraint | ||
| //! (`zpool_unhealthy_columns_present`) enforces that the right columns are | ||
| //! non-NULL for each kind. See [`nexus_types::fm::DiskFact`] for semantics. | ||
|
|
||
| use crate::DbTypedUuid; | ||
| use crate::impl_enum_type; | ||
| use crate::inventory::InvZpoolHealth; | ||
| use chrono::{DateTime, Utc}; | ||
| use nexus_db_schema::schema::fm_fact_physical_disk; | ||
| use nexus_types::fm; | ||
| use nexus_types::fm::{DiskFact, FactPayload, ZpoolUnhealthyFactPayload}; | ||
| use omicron_common::api::external::Error; | ||
| use omicron_uuid_kinds::{ | ||
| CaseKind, CollectionKind, FactKind, PhysicalDiskKind, SitrepKind, ZpoolKind, | ||
| }; | ||
|
|
||
| impl_enum_type!( | ||
| FmFactPhysicalDiskKindEnum: | ||
|
|
||
| #[derive(Clone, Copy, Debug, AsExpression, FromSqlRow, PartialEq, Eq)] | ||
| pub enum FmFactPhysicalDiskKind; | ||
|
|
||
| ZpoolUnhealthy => b"zpool_unhealthy" | ||
| ); | ||
|
|
||
| /// Diesel row for the `fm_fact_physical_disk` table. | ||
| /// | ||
| /// The payload columns are populated according to `kind`: a column is `Some` | ||
| /// if it belongs to that `kind`'s payload, and `None` otherwise (by | ||
| /// convention; the per-kind CHECK constraints, e.g. | ||
| /// `zpool_unhealthy_columns_present`, enforce only the "present for the | ||
| /// matching kind" direction so that future kinds may share columns). | ||
| #[derive(Queryable, Insertable, Clone, Debug, Selectable)] | ||
| #[diesel(table_name = fm_fact_physical_disk)] | ||
| pub struct FmFactPhysicalDisk { | ||
| pub id: DbTypedUuid<FactKind>, | ||
| /// The sitrep to which this fact belongs. | ||
| /// | ||
| /// This will change as the fact is carried forward from one sitrep to the | ||
| /// next. | ||
| pub sitrep_id: DbTypedUuid<SitrepKind>, | ||
| pub case_id: DbTypedUuid<CaseKind>, | ||
| /// Sitrep in which this fact was first added. | ||
| /// | ||
| /// Preserved unchanged when the fact is carried forward; debug-only. | ||
| pub created_sitrep_id: DbTypedUuid<SitrepKind>, | ||
| pub comment: String, | ||
|
Comment on lines
+40
to
+51
Member
There was a problem hiding this comment. extremely not a blocker for this PR, but it occurs to me that these fields are going to be present on every fact table and also on the preexisting non-DE-specific "case objects" (i.e. alert requests, support bundle requests, and so on). I wonder if we might eventually want to have a macro or something similar to the existing For one thing, this could generate a I may try to hack something like that together if I have some spare time, or someone else might want to.
Collaborator
Author
There was a problem hiding this comment. I think this is reasonable idea - also, prioritization-wise, I do really want to get this merged with some DB schema first. I really want "many DEs, some of which have multiple facts" before investing the time in macros, just to have extra validation that we won't need to churn. I do think those macros will make it easier for developers to add facts, but the "total number of DEs and facts" seems very small right now, so I'd prefer to "wait a little, and do it with high confidence" rather than do it too early. |
||
|
|
||
| /// The physical disk this fact is about. Common to every `kind`, so it is | ||
| /// always present (the column is `NOT NULL`). | ||
| pub physical_disk_id: DbTypedUuid<PhysicalDiskKind>, | ||
| pub kind: FmFactPhysicalDiskKind, | ||
|
|
||
| // Columns for the `ZpoolUnhealthy` kind. | ||
| pub zpool_id: Option<DbTypedUuid<ZpoolKind>>, | ||
| pub last_seen_health: Option<InvZpoolHealth>, | ||
| pub observed_in_inv: Option<DbTypedUuid<CollectionKind>>, | ||
| pub time_observed: Option<DateTime<Utc>>, | ||
|
Comment on lines
+55
to
+61
Member
There was a problem hiding this comment. As I understand it, the design here is that we would allow a DE to record multiple distinct fact types in a case by creating multiple records in the fact table for that DE associated with the case, and each of those records would map to exactly one variant of a Rust enum when the sitrep is loaded. I'm starting to wonder whether we really need to do this, or if there might be a simpler approach. Do we really anticipate that we will need to have this one-to-many relationship between a case and its facts, or could we do something where instead of having a "fact record", we have a "facts record" for each case? Here, if we wanted to add a different kind of fact to the "physical disk" diagnosis engine, we would do something like adding a new variant of impl FmFactsPhysicalDisk {
pub fn as_zpool_unhealthy(&self) -> Option<ZpoolUnhealthy> {
let Self {
zpool_id: Some(zpool_id),
last_seen_health: Some(last_seen_health),
unhealthy_zpool_observed_in_inv: Some(observed_in_inv),
unhealthy_zpool_time_observed: Some(time_observed),
... // ignore unrelated fields
} = self else {
return None;
};
Some(ZpoolUnhealthy {
zpool_id,
last_seen_health,
observed_in_inv,
time_observed,
})
}
pub fn as_pcie_link_down(&self) -> Option<PcieLinkDown> {
let Self {
link_down_whatever_fields_this_has: Some(whatever),
link_down_other_field: Some(other_field),
... // ignore unrelated fields
} = self else {
return None;
};
Some(PcieLinkDown {
whatever,
other_field,
})
}
}We could still have an enum with variants for all those structs, too, if we wanted to, and could do something like having an iterator over each kind of fact the case contains, but I'm not sure if we would need that. And, in some cases, the facts we want to represent may be simple enough that we don't actually need all this ceremony and they're just single columns that may or may not be present in a record for a particular case. This is the most attractive part of this approach, IMO --- if a DE just wants to be able to store a few different facts that are each representable by a single column, it can just add them to the schema for its table without having to go through all the ceremony of messing around with enum variants and stuff. This might end up making things a bit easier, and prevents us from being able to represent some of the nonsensical cases we currently can (such as a case with several different On the other hand, there may also be future DEs that do need the one-to-many fact relationship --- for example, you might want to be able to record that various crucible volumes were all impacted by an unhealthy physical disk. In that case, we might be able to represent that by using SQL arrays to store the multiple instances of the same fact in one record, like Basically, I think the crux of what I'm getting at here is whether we actually need "fact tables" to represent a case's DE-specific facts, or if all we really need are "case tables" that extend a case with one record in a DE-specific table. This isn't a strongly-held belief and I'm very open to being convinced we do need the additional expressiveness of multiple fact records per case, but I wanted to bring it up so we could weigh some of the pros and cons.
Member
There was a problem hiding this comment. Per a Matrix conversation with @smklein, I think we've determined that SQL arrays are not going to be sufficiently expressive for a lot of the multiple fact cases we may want to be able to represent, so we're going to continue with the current thing.
Collaborator
Author
There was a problem hiding this comment. I think my motivation towards "have a one to many relationship between cases and facts" is that I suspect, especially for more complex cases, we'll need to build up a variety of facts over time to actually determine whether the case should be transformed into an active problem. I don't really know, yet, how complex this is going to get, but IMO the flexibility of: "you want to be able to add arbitrary data points as facts in the DB, and then re-assemble them in memory to analyze the current case status" seems like it should be possible. There are also some limitations with
Which doesn't mean they're always bad! just that... there may be footguns we're dropping for ourselves if we want them to support arbitrary data this way. For example, they cannot support nesting, and not all types can be used in arrays.
Comment on lines
+57
to
+61
Member
There was a problem hiding this comment. much as it pains me to suggest this, I wonder somewhat if we might want to begin a convention of prefixing all of these with the fact kind name (so here,
Collaborator
Author
There was a problem hiding this comment. asdf I'm on the fence about this. I understand "why", but this could result in some obnoxiously long column names, where we're already guarded by the |
||
| } | ||
|
|
||
| impl FmFactPhysicalDisk { | ||
| /// Build a row from a fact's shared metadata (`fact`) and its | ||
| /// already-dispatched physical-disk payload (`disk_fact`). | ||
| /// | ||
| /// Callers route each fact to its engine's table by matching on | ||
| /// [`fact.payload`](fm::case::Fact::payload) and pass the matched payload | ||
| /// here, so this never has to interpret another engine's payload. | ||
| pub fn from_sitrep( | ||
|
Collaborator
Author
There was a problem hiding this comment. I'm sorta following the convention of the other fm-based models here (they all use IDK; might change this in a follow-up. |
||
| sitrep_id: impl Into<DbTypedUuid<SitrepKind>>, | ||
| case_id: impl Into<DbTypedUuid<CaseKind>>, | ||
| fact: &fm::case::Fact, | ||
| disk_fact: &DiskFact, | ||
| ) -> Self { | ||
| let base = Self { | ||
| id: fact.id.into(), | ||
| sitrep_id: sitrep_id.into(), | ||
| case_id: case_id.into(), | ||
| created_sitrep_id: fact.created_sitrep_id.into(), | ||
| comment: fact.comment.clone(), | ||
| physical_disk_id: disk_fact.physical_disk_id().into(), | ||
| kind: FmFactPhysicalDiskKind::ZpoolUnhealthy, | ||
| zpool_id: None, | ||
| last_seen_health: None, | ||
| observed_in_inv: None, | ||
| time_observed: None, | ||
| }; | ||
| match disk_fact { | ||
| DiskFact::ZpoolUnhealthy(p) => Self { | ||
| kind: FmFactPhysicalDiskKind::ZpoolUnhealthy, | ||
| zpool_id: Some(p.zpool_id.into()), | ||
| last_seen_health: Some(p.last_seen_health.into()), | ||
| observed_in_inv: Some(p.observed_in_inv.into()), | ||
| time_observed: Some(p.time_observed), | ||
| ..base | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| /// Reconstruct an in-memory fact from a row. | ||
| /// | ||
| /// The payload columns the database's CHECK constraint guarantees are | ||
| /// non-NULL for this `kind` are unwrapped; a NULL where one is required | ||
| /// indicates a corrupt row (e.g. hand-edited) and yields an internal | ||
| /// error rather than a panic. | ||
| pub fn into_fact(self) -> Result<fm::case::Fact, Error> { | ||
| let kind = self.kind; | ||
| let payload = match kind { | ||
| FmFactPhysicalDiskKind::ZpoolUnhealthy => { | ||
| FactPayload::PhysicalDisk(DiskFact::ZpoolUnhealthy( | ||
| ZpoolUnhealthyFactPayload { | ||
| physical_disk_id: self.physical_disk_id.into(), | ||
| zpool_id: self | ||
| .zpool_id | ||
| .ok_or_else(|| missing_column(kind, "zpool_id"))? | ||
| .into(), | ||
| last_seen_health: self | ||
| .last_seen_health | ||
| .ok_or_else(|| { | ||
| missing_column(kind, "last_seen_health") | ||
| })? | ||
| .into(), | ||
| observed_in_inv: self | ||
| .observed_in_inv | ||
| .ok_or_else(|| { | ||
| missing_column(kind, "observed_in_inv") | ||
| })? | ||
| .into(), | ||
| time_observed: self.time_observed.ok_or_else(|| { | ||
| missing_column(kind, "time_observed") | ||
| })?, | ||
| }, | ||
| )) | ||
| } | ||
| }; | ||
| Ok(fm::case::Fact { | ||
| id: self.id.into(), | ||
| created_sitrep_id: self.created_sitrep_id.into(), | ||
| payload, | ||
| comment: self.comment, | ||
| }) | ||
| } | ||
|
Comment on lines
+103
to
+145
Member
There was a problem hiding this comment. someday, i bet we'll be able to figure out a way to derive all this from an enum, but let's save that for later. :) |
||
| } | ||
|
|
||
| fn missing_column(kind: FmFactPhysicalDiskKind, column: &str) -> Error { | ||
| Error::internal_error(&format!( | ||
| "fm_fact_physical_disk row of kind {kind:?} has a NULL {column}, \ | ||
| violating the CHECK constraint requiring it to be non-NULL for \ | ||
| this kind" | ||
| )) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
urgh, I wanted to suggest we wrap this line, but it's a raw string, so that's hard. never mind.