[fm] Add saga diagnosis engine

[smklein]

Adds the a fault management diagnosis engine: the problematic saga diagnoser.

What it does

Opens a case (keyed by saga ID) for any unfinished saga that needs attention.

This PR uses three fact kinds:

• NotProgressing: a live (running or unwinding) saga has recorded no node event for longer than STALE_SAGA_THRESHOLD (this is 30 minutes, but we can set it to whatever).
• OwnerNotCurrentGeneration: the saga's current_sec is a Nexus that will never advance it, either quiesced (an older generation that handed off) or expunged (no db_metadata_nexus record).
• Abandoned: Nexus permanently gave up on recovering the saga.

Inputs

The preparation phase builds an ObservedSaga projection, the saga analog of #10541's InServiceDisk: all unfinished sagas (running, unwinding, or abandoned), joined with each saga's latest node-event time and its owner's classification against db_metadata_nexus. This is the executed view read directly from the database, and it is included in the analysis input report so "why did FM (not) flag this saga" is answerable from a sitrep.

Case lifecycle

The two live-saga facts may coexist on one case; as conditions change, facts are removed and re-added under the same case (with stable fact UUIDs when the observation is unchanged). The case closes when the saga completes, or when every condition clears (progress resumes, owner re-adopted), and a recurrence later opens a fresh case. An abandoned saga's case never closes on its own; it persists, carrying the Abandoned fact, until remediation removes the saga row. Actually remediating an Abandoned saga seems like a case-by-case resolution, so I'm not doing it in this PR!

Context

• health check reported hung sagas that aren't actually running #10531: dogfood had sagas stuck in running/unwinding with no Nexus actually executing them. The determination there was that these should be surfaced by FM ([fm] File a case, create active problem for "sagas which are stuck" #10530), decoupled from the update health check. This engine is that surfacing mechanism.
• Nexus should explicitly abandon sagas that it fails to recover for non-transient reasons #10581: Nexus will explicitly abandon sagas it cannot recover for non-transient reasons. The Abandoned fact kind exists so that landing Nexus should explicitly abandon sagas that it fails to recover for non-transient reasons #10581 escalates these cases instead of silently closing them.

Fixes #10530

[mergeconflict]

Nit: this is selecting everything including saga_dag (the JSON blob), which isn't used in prepare_observed_sagas. Is it easy enough to just project the columns that are needed for ObservedSaga?

[smklein]

Sure thing, done in 77a6442. Good idea to not pull out all the JSON that we don't need!

[mergeconflict]

Do you need #[serde(default)] here to make preexisting serialized instances parseable?

[smklein]

good catch, added, and also adding a warning comment in 0f6b14e

(also went ahead and added this to in_service_disks, which probably should have had the same treatment)

[hawkw]

<3 thanks for adding the warning

[mergeconflict]

Would it make sense to add a test that running the DE twice doesn't change anything? Something like:

// run once with no parent sitrep
let input1 = build_input(collection.clone(), None, observed.clone());
let (sitrep1, _) = run_analyze(&logctx.log, &input1);

// run again with the sitrep1 as the parent
let input2 = build_input(collection, Some(sitrep1.clone()), observed);
let (sitrep2, _) = run_analyze(&logctx.log, &input2);

assert_eq!(
    sitrep1.compare_state(), sitrep2,
    "no changes must produce an identical sitrep to the parent",
);

This should really be a property of all DEs (cc @hawkw), not just this one.

[smklein]

Added in f070699. I do think that there is a bit of DE-specific machinery here though, we need to tickle the input in such a way that "the DE fires". If we passed "no sagas" to this DE and tried to validate the same thing, it'd be a little less interesting.

[hawkw]

Yeah, I think in the eventual future, if we have a proptest thing for generating arbitrary inputs, we could maybe help ensure a test like this is interesting by rejecting any inputs where the first analysis step makes a sitrep that has nothing interesting in it, but I dunno. That might run for a long time.

[mergeconflict]

Nit: the comment at the top of the file describing STALE_SAGA_THRESHOLD says "at least this long", but this is strict greater-than.

[smklein]

Updated in ec0536a

[mergeconflict]

When we see SagaOwnerState::Absent in orphaned_reason, we map that to "expunged". That's mostly right, but I think it's not correct when nexus_states is empty. Should we skip this in that case? It'd sort of be analogous to DataStore::check_nexus_access in db_metadata, where we bail if the table is empty.

[smklein]

I don't think nexus_states should ever be empty - this metadata is necessary for Nexuses to perform handoff, and therefore, it's necessary for them to boot. The only time it's empty is during rack initialization, when general database access is not active yet (and the FM subsystem isn't running yet).

(So, in another way, if we see a saga pointing to an owner without a nexus_state, it's a lot more likely that it has been expunged than that we have no nexus_states)

[mergeconflict]

Similar to the comment I left on @hawkw's PSU DE (#10593 (comment)), I feel like there's some (future!) opportunity for us to extract the state-munging logic that all DEs need into some shared component. That is, I think the essence of this DE is roughly the three desired_* functions, and the rest is plumbing. Once the dust settles, we should try squinting at the DEs we've ended up with and see what we can pull out.

[smklein]

Agreed. I definitely think case-parsing + "assigning case-specific facts" could be shared, as well as the "re-evaluation of existing vs new cases flow". but yeah, i do still want to punt for now, because generalizing too early seems painful in a different way

[hawkw]

Yeah. I think once we have more than 2 DEs that care about facts, and more than 2 that care about ereports, we might be starting to exit the zone where trying to pull out abstractions is premature!

[mergeconflict]

What's the expected worst-case gap between a Nexus's db_metadata_nexus row going away and sagas_reassign_sec re-adopting that Nexus's sagas? If it can span an analysis pass, we might open a case thinking the saga is orphaned, and then close it again when the saga is reassigned.

[smklein]

I think your take here is correct: even within one blueprint execution, the metadata-row deletion and reassignment are different steps, but it could also span blueprints.

Either way, I think the DE is doing the correct thing by labeling "what it sees right now". If the DE sees a saga that currently has no owner, it might be undergoing re-assignment momentarily, or it might never happen.

So:

• If we see a saga which deleted the db_metadata_nexus row but hasn't re-assigned yet, it does look (briefly!) orphaned
• If we later see that saga which has been re-assigned, we'd close the case

My two cents: I think it's okay to open a case here, but you're right that we might not want to actually trigger an active problem here without some hysteresis. This case was originally motivated by #10530 (comment) , where an issue with steno prevented re-assignment.

So, TL;DR, I think the clarification for "is this suspect saga actually a problem" is answered by "is the issue temporary or permanent", which we could use as our case -> active problem promotion criteria.

[mergeconflict]

Quick q: I think there isn't a partial index filtered for WHERE saga_state != 'done' (or however you want to express "unfinished states"), so I think this query effectively has to scan all sagas. Would it be worth adding an index?

[smklein]

There is an index to catch this:

omicron/schema/crdb/dbinit.sql

Lines 2994 to 3000 in 007c97f

	/*
	* For recovery (and probably takeover), we need to be able to list running
	* sagas by SEC. We need to paginate this list by the id.
	*/
	CREATE UNIQUE INDEX IF NOT EXISTS lookup_saga_by_sec ON omicron.public.saga (
	current_sec, id
	) WHERE saga_state != 'done';

I can phrase this as the inverse (saga_state.ne(done)) to match the index in a more obvious way?

[mergeconflict]

Nit: is there a way to derive this vec from the definition of DbMetadataNexusState so they don't get out of sync if somebody adds a variant later? Or do you mean to be explicit about these states being the only ones you care about?

[hawkw]

we could perhaps use strum to derive a DbMetadataNexusState::ALL_VARIANTS --- i've found that useful for other DB enum types where you have a query for looking them up by state but also want to sometimes say "don't care"

[smklein]

Yeah, strum seems reasonable. I'll use the "all variants" approach here.

[mergeconflict]

Nit: I think this constraint could be tighter - saga_state has to be running or unwinding, it can't be done or abandoned. Or we should just use a different enum type here.

[smklein]

I'll make the check tighter. I'm trying to follow the convention we agreed upon for per-DE fact types (one DB table, distinguished by kind), but I'll make the saga_state check more explicit.

[hawkw]

hm, i wonder if we might want to start moving the different fact types into submodules for the different DEs, since there's starting to be a lot of fact types in here. This has the advantage of not needing to prefix every type with the name of the DE that cares about it, and the DE can blanket import its submodule if it wants to. not a blocker, just a thought.

[smklein]

I'm down to do this, but I'd prefer to do it as a follow-up PR to make it a little more obvious what is "refactoring only".

[smklein]

#10869

[hawkw]

this comment feels a bit like claude gibberish, could we maybe rewrite this in a way that's a bit easier to read?

[smklein]

Sure, updated the comment.

TL;DR, "only record what's relevant and might change (state + timestamp), everything else can be looked up by ID later".

[hawkw]

this is maybe obnoxious but i really don't like constructing errors with &format!, because it allocates a string, passes it by reference, and then the internal_error constructor ToStrings the &String, allocating a new string and throwing away the old one. we could write:

Suggested change

	Err(Error::internal_error(&format!(
	"fm_fact_saga row has saga_state {state:?}, which is never \
	recorded on a NotProgressing saga fact"
	)))
	Err(Error::InternalError { internal_message: format!(
	"fm_fact_saga row has saga_state {state:?}, which is never \
	recorded on a NotProgressing saga fact"
	)})

to avoid that...or, eventually, we should really get around to changing the function constructors for error types to take impl ToString, so you can just pass the string you already formatted and not have it formatted into a new string and thrown away...

[smklein]

ah, yeah, I can avoid the extra allocation by dodging the constructor function. I'll do that for the other callsites in this PR invoking Error::internal_error(&format!(...)) too.

[hawkw]

it occurs to me that someday we could maybe make this pattern a bit less odious if we had a macro like

macro_rules! expect_column {
    ($row:ident, $colname:ident) => $row.$colname.ok_or_else(|| missing_column(kind, stringify!($colname))
}

except written in some kinda way where it could be used in every fact module. not a blocker just a crazy idea i had

[smklein]

Filing #10869 for this, and to generalize it. hopefully the lambda + function approach is readable enough for now?

[hawkw]

did we check whether there are other places where we might also be doing this, and could benefit from using the smaller type?

[smklein]

In this PR, no, but outside this PR: saga_list_running_or_unwinding_older_than and get_all_sagas_in_state could read "not the whole saga row"?

That said, reading #10866 , we might change this implementation after that merges?

I'm not sure. The intent of that PR seems like it's: "try to make it harder to observe invalid saga states, reading from the database (e.g., accessing abandoned metadata in its entirety when it exists) but we are already catching that in the try_from implementation below? Idk

[hawkw]

ultra nitpicky, but "parsed" feels a bit odd to call something that is really being interpreted from data in the database...

[smklein]

mmm I used the same naming convention in the disk DE. Would you prefer "InterpretedSagaCase"?

I am taking the DB data, validating it, and shoving it into a new type that signifies the validation. IMO that's parsing. but I can call it something else?

[hawkw]

now that you've called me out on this, i am not sure if i care about this enough to actually think of an alternative name or not..."parsing" makes me think of "turning bytes/characters into structured data" rather than "turning structured data into more structured data", but I am not sure if I care. this is fine!

[hawkw]

occurs to me we might have the various as_$foo() methods return a ForeignFact { fact_id } error of their own, and the UninterpretableCase error could just #[from] ForeignFact here and elsewhere

[smklein]

Sure, I'll make the as_ methods return Result<_, ForeignFact> and both DEs can use the #[from] magic for conversion. This can also identify which DE the fact actually does belong to.

[hawkw]

Yeah. I think once we have more than 2 DEs that care about facts, and more than 2 that care about ereports, we might be starting to exit the zone where trying to pull out abstractions is premature!

[hawkw]

oh, this is such a good call that I think we maybe ought to enshrine it in convention for other DEs moving forwards --- I had been wondering "hmm I wonder where it will make sense to get a 'now' time for stuff like 'wait at least so long before closing a case'...". Despite the fact that I just wrote something about "premature abstraction" in my last comment, I kinda want to say that we should have an AnalysisInputs::now() or something that just does this in order to encourage future DE code to also do this.

[smklein]

I think I'd prefer to call this reference_time instead of now, but sure, I can refactor this out now!

[hawkw]

should this mayhaps also be a log_warning? since it represents something unexpected rather than just closing the case for a normal reason

[smklein]

Sure, I can do that. I'll also update the disk DE for parity.

[hawkw]

i kinda want to make the various maps of queried inputs be added to the builder by methods, so that we don't have to write Arc::new(IdOrdMap::new()) four or five times in every test. but on the other hand it does increase the risk of just forgetting to ever add it, i dunno. on the third hand using a builder method would at least allow the reader to see a name for what that Arc::new(IdOrdMap::new()) is supposed to be...

[smklein]

I'm taking a swing at this, trying to balance a couple competing interests:

1. In prod, we really want each of these fields to be supplied explicitly. "Forgetting to set something" is problematic, because an Arc::new(IdOrdMap::new()) has meaning - e.g., for sagas, it means "there aren't any running!". So... I don't want to JUST do setters, since the omission of a callsite could be bad.
2. We want the names of these fields to be visible at builder call-site.
3. Under test, it would be nice to not need to re-state all the inputs we aren't using.

Here's my angle:

• Change the builder to allow these as `Option<...>" by default
• Inputs are supplied via named setters
• build() throws an error if inputs are missing
• Tests can use a with_empty_defaults() function to populate all the fields with a default value (Arc::new(...) here, but can work for whatever).

[hawkw]

we could perhaps use strum to derive a DbMetadataNexusState::ALL_VARIANTS --- i've found that useful for other DB enum types where you have a query for looking them up by state but also want to sometimes say "don't care"

[hawkw]

i wonder if the map of Nexii and their states will ever want to be a top-level analysis input of its own...

[smklein]

Perhaps! I could split this out if we have a DE somewhere else that wants it.

[hawkw]

part of me wonders if we ought to have a different heuristic for which case to prefer than "which one happened to have the highest UUID" but it's fine I guess...

[smklein]

technically it's lowest UUID, because we're iterating through them in Btree order, but...

IDK, I'm personally not super concerned here, I think I just want to prioritize (a) determinism, and (b) graceful handling of impossible states, which is sorta our follow-up goal with slippy

[hawkw]

now that we can log warnings to the sitrep analysis report, let's use that here!

[hawkw]

Yeah, I think in the eventual future, if we have a proptest thing for generating arbitrary inputs, we could maybe help ensure a test like this is interesting by rejecting any inputs where the first analysis step makes a sitrep that has nothing interesting in it, but I dunno. That might run for a long time.

[hawkw]

idly wondering if this is loading enough stuff that we might want to start running some of these "load thing" steps in parallel. i realize that the way i did ereport loading (which requires stuffing things into the builder) is hard to factor out to run in a child task, but we could change it to return a vec that gets stuffed into the builder or whatever.

[smklein]

Agreed we should do this, and it doesn't seem bad. Added to #10869 for now

[hawkw]

I would kind of prefer it if these were like:

  * saga 5a9a0001-5a9a-45a9-85a9-5a9a5a9a5a9a (fake-saga): 
    state: Unwinding
    last event: 1970-01-01 00:00:00 UTC
    owner: Quiesced
  * saga 5a9a0002-5a9a-45a9-85a9-5a9a5a9a5a9a (another-fake-saga):
    state: Abandoned
    last event: <none recorded>
    owner: <no current SEC>

but i will admit that i'm kind of a fascist about 80-column terminals and you can ignore me

[smklein]

Sure, I'll patch this up. Happy to be consistent/make this more readable

[hawkw]

<3 thanks for adding the warning

[hawkw]

a high level question is that, IIRC, there's also an update health check and possibly other stuff that also cares about not-running sagas, might this code be shareable with it? okay to address as a follow up

[smklein]

So from what I can tell the health check exists in nexus/src/app/update.rs , but it's currently test-only, until #10538 gets fixed.

I do think it would be reasonable for that check to look at: "are there cases open for sagas", or perhaps "are there active problems open for sagas" (if we're trying to consider #10592 (comment) ).

But yeah, I do think it makes sense to de-duplicate these