schema + db-model: support bundle request data selection tables

[mergeconflict]

Adds two tables:

• fm_support_bundle_request (the primary table representing nexus_types::fm::case::SupportBundleRequest
• fm_sb_req_data_selection (the join table representing the nexus_types::support_bundle::BundleDataSelections in the support bundle request).

... and their corresponding models.

This is for persisting fault-management-initiated support bundle requests as part of a sitrep. Each request belongs to a case and tracks which sitrep originally requested it. Context: #10062.

This supersedes #10091 and #10095, in which the schema had a different table for each BundleDataSelection variant. That got hairy because we were joining 6 tables together and Diesel got mad about it. (Then I got mad.)

[jgallagher]

This should be okay, but since the consequence of being wrong is crashing Nexus, we usually make these runtime errors; e.g.,

omicron/nexus/db-model/src/deployment.rs

Lines 382 to 388 in 72b5433

	(DbBpPhysicalDiskDisposition::InService, Some(_))
	| (DbBpPhysicalDiskDisposition::Expunged, None) => Err(anyhow!(
	"illegal database state (CHECK constraint broken?!): \
	disposition {:?}, disposition_expunged_as_of_generation {:?}",
	value.disposition,
	value.expunged_as_of_generation,
	)),

[mergeconflict]

Fixed in ef06169. Question about this, though: should we basically never use expect in production code? In the past, I tend to use Result-like things only for expected errors, and prefer to panic for cases that we think really shouldn't happen (M-PANIC-ON-BUG). Thoughts?

[smklein]

I generally think "expect/unwrap" is fine if you're writing code that is truly impossible to trigger, under any circumstances. My metric for this is basically self-consistency: within the Nexus binary, is this always impossible?

This does not pass the bar - we mention the CHECK constraint here, but CockroachDB is an external process, which could be modified independently of Nexus. It could return whatever data, the version could skew, etc. Under that lens, data returned here should be treated as "arbitrary input".

With that lens: It's totally fine to throw an error if the data from CockroachDB looks unexpected, but that seems very different to me from "panic the whole process".

[mergeconflict]

@smklein Makes sense, that's a good explanation. Mismatched expectations due to version skew or whatever are super subtle and I've definitely been burned by them within the last year.

To be clear: when you say "throw an error" you mean "return an Err result", yeah?

[smklein]

To be clear: when you say "throw an error" you mean "return an Err result", yeah?

Yeah, exactly - we could treat this as a serialization error ("I refuse to read this from the database!") or as a parsing error ("I can read the raw data from the database, but I'm preventing us from transforming this into a usable type"), or something else -- as long as it doesn't crash the Nexus process

[jgallagher]

Fixed in ef06169. Question about this, though: should we basically never use expect in production code? In the past, I tend to use Result-like things only for expected errors, and prefer to panic for cases that we think really shouldn't happen (M-PANIC-ON-BUG). Thoughts?

+1 on all of Sean's comments - sorry, I should have gone into more details. I agree with panicking on bugs. Some cases that I know we use expect() that I think are fine are:

• Serializing a type to JSON in memory (this can only panic if the type isn't representable in JSON, which would be a pretty severe bug if we expect it to be JSON-able)
• Pulling items out of maps by keys that we know exist, because we locally created the map or already checked that the key exists or somesuch
• Calling fallible functions with data that obviously can't fail (e.g., NonZeroU8::new(123).unwrap(), because NonZeroU8::new() only fails if passed 0)

Some cases we've used expect() in the past (and maybe still do) but shouldn't:

• Reading from the DB, as in this case for the reasons Sean noted
• Pulling items out of maps by keys that we're pretty sure ought to exist (Nexus panic inside sync_switch_configuration background task #8579 is a specific example of this: we run two different but closely related db queries in separate transactions and assume they have the same contents - that's almost always true, unless someone happens to have changed them in between the two queries)
• Assuming some invariant is always upheld when it's only upheld sometimes (Large silo quota amount could crash nexus #5732 is a specific example of this, where we reasonably assumed the ByteCount newtype could only have valid contents, but because we #[derive(Deserialize)]'d it, it could be constructed via deserialize without upholding the invariants). This case is a little fiddly - I think more often than not the expect() is fine and we need to ensure the invariants are always upheld (e.g. in fixing Large silo quota amount could crash nexus #5732, we manually implemented Deserialize to enforce the invariant).

[davepacheco]

Agree with all of the above. I've filed #10118 on an idea we've discussed before to be able to report operational errors that we believe should be impossible.