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

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

No description provided.

jgallagher · 2026-01-28T18:09:01Z

nexus/db-model/src/deployment.rs


+#[derive(Queryable, Clone, Debug, Selectable, Insertable)]
+#[diesel(table_name = bp_single_measurements)]
+//#[diesel(check_for_backend(diesel::pg::Pg))]


Leftover debugging?

jgallagher · 2026-01-28T21:01:07Z

nexus/db-model/src/deployment.rs

+            },
+            hash: *self
+                .image_artifact_sha256
+                .expect("this should always be set"),


Can the field be non-optional (and the corresponding db row have a NOT NULL constraint)?

jgallagher · 2026-01-28T21:01:48Z

nexus/db-model/src/deployment.rs

+    pub id: DbTypedUuid<MeasurementKind>,
+
+    pub image_artifact_sha256: Option<ArtifactHash>,
+    pub prune: bool,


Discussed in chat but noting for completeness - I think we can drop the prune field since the planner has access to both the current and previous TUF repo.

jgallagher · 2026-01-28T21:07:47Z

nexus/types/src/deployment.rs

+    }
+}
+
+impl From<BTreeSet<BlueprintSingleMeasurement>> for BlueprintMeasurements {


These Froms seem fine-ish but surprising - could wherever we're currently using BTreeSet<BlueprintSingleMeasurement> use BlueprintMeasurements instead?

jgallagher · 2026-01-28T21:10:18Z

nexus/types/src/deployment.rs

+        self.measurements.insert(single);
+    }
+
+    // An empty measurement set here corresponds to the install dataset


Is this property guaranteed? Wondering if there are edge cases where we could have an empty set of measurements but that didn't imply "use the install dataset" - maybe dev/test systems? Or if this is a surprising security-related property in the future sometime if someone assumes "empty set" means "won't be able to talk" but it actually means "falls back to whatever was most recently written to the install dataset (possibly months/years ago)".

Yeah this is a bit of a strange edge case. I did a first pass with this being an enum like BlueprintZoneImageSource but I'm trying to remember why doing that here was ugly. It's possible I went a layer too far in getting rid of indirection. I'll play around with this more.

That said, there eventually should be no case in the future when this is empty past the initial install. As we get further along and more confident I expect we'll want to make that a stronger assertion.

This turned out to be more doable than I remembered, perhaps now that things are matured

labbott · 2026-02-03T19:40:31Z

nexus/types/src/deployment.rs

+        match measurement {
+            BlueprintMeasurements::Artifacts { artifacts } => artifacts
+                .iter()
+                .map(move |d| {
+                    BpTableRow::from_strings(
+                        state,
+                        vec![d.hash.to_string(), d.version.to_string()],
+                    )
+                })
+                .collect::<Vec<BpTableRow>>()
+                .into_iter(),
+            BlueprintMeasurements::InstallDataset => {
+                vec![BpTableRow::from_strings(
+                    state,
+                    vec![
+                        "install dataset".to_string(),
+                        "(no version)".to_string(),
+                    ],
+                )]
+                .into_iter()
+                .collect::<Vec<BpTableRow>>()
+                .into_iter()


I need to fix this up to use Either

labbott · 2026-02-03T19:40:51Z

nexus/types/src/deployment.rs

+        match self {
+            BlueprintMeasurements::InstallDataset => {
+                std::iter::empty::<&BlueprintSingleMeasurement>()
+                    .collect::<Vec<&BlueprintSingleMeasurement>>()
+                    .into_iter()
+            }
+            BlueprintMeasurements::Artifacts { artifacts } => artifacts
+                .iter()
+                .collect::<Vec<&BlueprintSingleMeasurement>>()
+                .into_iter(),
+        }


Fix this to use Either

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

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?

jgallagher · 2026-02-04T16:29:04Z

nexus/types/src/deployment.rs

    pub zones: IdOrdMap<BlueprintZoneConfig>,
    pub remove_mupdate_override: Option<MupdateOverrideUuid>,
    pub host_phase_2: BlueprintHostPhase2DesiredSlots,
+    #[serde(default = "BlueprintMeasurements::default")]


Do we need a #[serde(default)] here? I don't think we need to parse old BlueprintSledConfigs.

This was something I added when I thought I had a different problem. I can remove it.

jgallagher · 2026-02-04T16:29:49Z

nexus/types/src/deployment/blueprint_display.rs

    }
 }

+/// The [`BpTable`] schema for desired host phase 2 contents


Copy/pasta typo

jgallagher · 2026-02-04T16:30:22Z

schema/crdb/dbinit.sql

+    id UUID NOT NULL,
+
+     image_artifact_sha256 STRING(64) NOT NULL,
+     PRIMARY KEY (blueprint_id, id)


Turbo-nit - these two lines are indented differently from the columns above

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

Add measurement blueprints

bc950ef

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

jgallagher reviewed Feb 4, 2026

View reviewed changes

Conversation

labbott commented Jan 25, 2026

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!

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!

2 participants