Add measurements to blueprints by labbott · Pull Request #9718 · oxidecomputer/omicron

labbott · 2026-01-25T18:40:10Z

No description provided.

nexus/db-model/src/deployment.rs

nexus/types/src/deployment.rs

jgallagher · 2026-02-04T16:26:50Z

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

            );
        }

+        let raw_measurements: Vec<(BpSingleMeasurement, Option<TufArtifact>)> = {


I think this is in the wrong place within this function, after the changes @smklein made to ensure we don't read partially-deleted blueprints (#9703). About 50 lines above this is:

// ================================================================ // STAGE 3: Parse and validate all loaded data // // At this point, we know the blueprint exists (wasn't deleted). // Any validation errors from here indicate real database // corruption, not a torn read from concurrent deletion. // ================================================================

and we're not expected to do any more reads from the DB in this stage, so I think this needs to go above into "STAGE 1". (Maybe we should break the stages up into three methods to make this mistake more difficult? But that probably has its own annoyances.)

I think this may also mean we don't need the

// We start with measurements from the install dataset, if // there's anything in the database this will get overwritten

comment above - if we've already read raw_measurements before we get to that point, hopefully we can fill in the correct value immediately?

Good catch. I'll see about making this change and mayyybe see about refactoring but I'm not great with diesel. Maybe I should just file a bug instead?

nexus/types/src/deployment.rs

nexus/types/src/deployment/blueprint_display.rs

schema/crdb/dbinit.sql

jgallagher

LGTM, just a handful of nits plus the "can't merge yet, R18.1" note :-/

jgallagher · 2026-02-18T15:38:01Z

nexus/db-model/src/schema_versions.rs

 /// 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(231, 0, 0);
+pub const SCHEMA_VERSION: Version = Version::new(232, 0, 0);


We have to hold off on merging this until 18.1 is cut (should be soon...)

jgallagher · 2026-02-18T15:42:53Z

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

+        for (id, artifacts) in omicron_measurements {
+            let sled_config = sled_configs.get_mut(&id).ok_or_else(|| {
+                // This means we found a measurement without
+                // the associated sled


This confirms we don't have unexpected measurement rows that don't match any sled configs (good) - do we need to check the other way around (i.e., that any sled config has measurements)? Or is "no measurement rows" the implicit db representation for BlueprintMeasurements::InstallDataset?

no measurement rows implies BlueprintMeasurements::InstallDataset. This gets set as the default entry and gets overwritten when we pull things out.

We have some checks in the external API when an operator tries to start an update for "has a mupdate taken place". Do we need to update those checks to look for this value too? That doesn't seem quite right if we're going to default existing blueprints to InstallDataset.

jgallagher · 2026-02-18T15:46:13Z

nexus/types/src/deployment.rs

+                        "(no version)".to_string(),
+                    ],
+                )]
+                .into_iter(),


Tiny nit - I think this outer vec![...].into_iter() could be std::iter::once(...) instead, both to save a small heap allocation and for clarity?

jgallagher · 2026-02-18T15:49:31Z

nexus/types/src/deployment.rs

+    fn len(&self) -> usize {
+        match self {
+            BlueprintMeasurements::InstallDataset => 0,
+            BlueprintMeasurements::Artifacts { artifacts } => artifacts.len(),


This is maybe overly paranoid, but: is it legal to have BlueprintMeasurements::Artifacts { artifacts } where the inner artifacts set is itself empty? (If it's not, I'm wondering if it's worth the ergonomics pain to change this to some kind of NonEmptyBTreeSet<BlueprintSingleMeasurement> instead.)

It shouldn't be legal. This is one of the awkward spots about the enum modeling vs just a struct that I don't love. I see https://docs.rs/nonempty/latest/nonempty/ for vec but I don't see anything like that for sets. I guess this could be an opaque inner type and we could check at creation time and panic if we try and create something with zero length? It's also worth considering the cases where we could end up constructing this right now are pretty narrow.

I guess this could be an opaque inner type and we could check at creation time and panic if we try and create something with zero length?

Yeah, this is the direction I'd go (although I wouldn't panic: I'd either return an error or make it return an Option like the NonZero* integer types in the std lib, since there's exactly one input that can possibly fail).

It's also worth considering the cases where we could end up constructing this right now are pretty narrow.

If there aren't many ways to construct this, it seems like it might be worth the work to enforce that construction guarantees at least one artifact?

nexus/types/src/deployment.rs

jgallagher · 2026-02-18T15:57:40Z

schema/crdb/dbinit.sql

+    -- foreign key into the `blueprint` table
+    blueprint_id UUID NOT NULL,
+    sled_id UUID NOT NULL,
+    -- id solely for database purposes


Two questions:

What are the database purposes?

(Maybe depends on the details of the former) What prevents id from being the sole primary key?

This was me both doing some copy/paste and running into diesel pain. Most other parts of the system like disks/datasets already have an id. I ran into some diesel headaches when I tried it without the id. I think we always want to be looking up for a specific blueprint so having that be part of the primary key made sense to me.

Should we revisit this? Either address whatever diesel pain caused this column to exist and figure out how to drop it, or expand the comment to explain why it's here?

andrewjstone

Looks great!

andrewjstone · 2026-02-18T15:58:39Z

nexus/db-model/src/deployment.rs

+        Self {
+            blueprint_id: blueprint_id.into(),
+            sled_id: sled_id.into(),
+            id: omicron_uuid_kinds::MeasurementUuid::new_v4().into(),


Nit, I'd probably import MeasurementUuid like the other typed UUIDs.

andrewjstone · 2026-02-18T16:13:39Z

nexus/types/src/deployment.rs

+impl<'a> BlueprintMeasurementsTableData<'a> {
+    fn diff_rows<'b>(
+        diffs: &'b daft::BTreeSetDiff<BlueprintSingleMeasurement>,
+    ) -> impl Iterator<Item = BpTableRow> + 'b + use<'b> {


TIL about use syntax. I don't think you need the 'b if you already have the use<'b>.

andrewjstone · 2026-02-18T16:17:21Z

nexus/types/src/deployment.rs

+impl<'a> BlueprintMeasurementsTableData<'a> {
+    fn diff_rows<'b>(
+        diffs: &'b daft::BTreeSetDiff<BlueprintSingleMeasurement>,
+    ) -> impl Iterator<Item = BpTableRow> + 'b + use<'b> {


I'm slightly weirded out by the use of Either here to return an interator rather than just returning the underlying vecs.

Oh, this is the same as #9718 (comment). For the Left case, there isn't an underlying vec - it should be using std::iter::once() instead.

nexus/types/src/deployment.rs

jgallagher · 2026-02-24T16:45:39Z

dev-tools/reconfigurator-cli/tests/output/cmds-add-sled-no-disks-stdout

+    ---------------------------
+    hash      version          
+    ---------------------------
+    unknown   (unknown version)


I only skimmed all these expectorate outputs, but just checking: it's expected that some will have unknown version and some will have no version?

I think I forgot to update a few tests but install gets no version vs unknown version, I'll double check to make sure these are consistent

jgallagher · 2026-02-24T16:58:08Z

nexus/types/src/deployment.rs

+                        "(no version)".to_string(),
+                    ],
+                ))
+                .collect::<Vec<_>>()


I don't think collecting an iter::once() into a single-element Vec is necessary; we can construct the vec directly:

vec![BpTableRow::from_strings(...args...)].into_iter()

(same note in the next branch)

jgallagher · 2026-02-24T16:58:47Z

nexus/types/src/deployment.rs

+                BpDiffState::Added,
+                vec!["install dataset".to_string(), "(no version)".to_string()],
+            )))
+            .collect::<Vec<_>>()


Same note on this one - this would be clearer as

vec![ BpTableRow::from_strings(...the removed row...), BpTableRow::from_strings(...the added row...), ].into_iter()

jgallagher · 2026-02-24T17:06:27Z

nexus/types/src/deployment.rs

+
+#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq, Eq)]
+#[serde(tag = "type", rename_all = "snake_case")]
+pub enum BlueprintMeasurementsInternal {


We shouldn't derive Deserialize - that would allow construction of a BlueprintMeasurementsInternal::Artifacts with an empty set of artifacts via a JSON blob with the right tag/contents. We need to manually implement it to reject those. (We have various examples of this floating around - usually we have a deserialize impl that internally has a type with the same shape as this type, but then validates the cases that need extra validation.)

That said, I don't think this is how I'd split the public/private details. It means callers are forced to do an if is_install { ... } else if is_unknown { ... } else { ... } chain instead of matching; what if we split it like this:

// clients can match and not use if/else chains pub enum BlueprintMeasurements { Unknown, InstallDataset, Artifacts { artifacts: BlueprintArtifactMeasurements }, } // private inner field; construction guarantees at least one measurement in the inner set pub struct BlueprintArtifactMeasurements(BTreeSet<BlueprintSingleMeasurement>);

which pushes the type safety down as far as it can go. (The BlueprintArtifactMeasurements struct here would still need a non-derived Deserialized for the same reasons noted above.)

ah! not being able to match was very annoying. let me play around with this.

jgallagher · 2026-02-24T17:48:55Z

schema/crdb/dbinit.sql

        NOT NULL,

+    -- the measurements for this sled
+    measurements omicron.public.bp_sled_measurements NOT NULL DEFAULT 'unknown',


We generally stay away from DEFAULTs on columns. We need it in the migration to populate existing rows, but then a followup migration step can alter the column and drop DEFAULT.

jgallagher · 2026-02-24T17:51:08Z

schema/crdb/dbinit.sql

+    -- foreign key into the `blueprint` table
+    blueprint_id UUID NOT NULL,
+    sled_id UUID NOT NULL,
+    -- id solely for database purposes


Should we revisit this? Either address whatever diesel pain caused this column to exist and figure out how to drop it, or expand the comment to explain why it's here?

We don't actually update the measuremenst right now, that will come in follow on work.

labbott marked this pull request as draft January 25, 2026 18:40

labbott requested review from andrewjstone and jgallagher January 26, 2026 12:56

labbott force-pushed the labbott/measurement_blueprints branch from 03bf716 to e6a722e Compare January 27, 2026 21:15

labbott marked this pull request as ready for review January 27, 2026 21:15

labbott changed the title ~~WIP: Add measurements to blueprints~~ Add measurements to blueprints Jan 27, 2026

labbott force-pushed the labbott/measurement_blueprints branch from e6a722e to 9d27ff4 Compare January 27, 2026 21:16

jgallagher reviewed Jan 28, 2026

View reviewed changes

labbott force-pushed the labbott/measurement_blueprints branch from ae2e508 to bc950ef Compare February 3, 2026 19:37

labbott commented Feb 3, 2026

View reviewed changes

nexus/types/src/deployment.rs Outdated Show resolved Hide resolved

labbott commented Feb 3, 2026

View reviewed changes

nexus/types/src/deployment.rs Outdated Show resolved Hide resolved

jgallagher reviewed Feb 4, 2026

View reviewed changes

labbott force-pushed the labbott/measurement_blueprints branch from bc950ef to b14debb Compare February 9, 2026 15:11

labbott force-pushed the labbott/measurement_blueprints branch from 6729e5c to c20ae5c Compare February 17, 2026 16:10

jgallagher reviewed Feb 18, 2026

View reviewed changes

andrewjstone approved these changes Feb 18, 2026

View reviewed changes

labbott force-pushed the labbott/measurement_blueprints branch from 7e9cdd0 to 03268dd Compare February 23, 2026 23:43

jgallagher reviewed Feb 24, 2026

View reviewed changes

labbott force-pushed the labbott/measurement_blueprints branch from af33521 to 0494af6 Compare February 24, 2026 23:35

labbott added 3 commits February 25, 2026 01:18

Add measurement blueprints

bca4d6d

We don't actually update the measuremenst right now, that will come in follow on work.

fixup tests

c2257b3

tweak that

0494af6

Comments

Conversation

labbott commented Jan 25, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jgallagher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewjstone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants