[reconfigurator-cli] RoT bootloader support #8620

karencfv · 2025-07-17T08:51:19Z

Equivalent of #8273 for RoT bootloader

sunshowers

Thanks -- a few questions.

nexus/reconfigurator/planning/src/system.rs

sunshowers · 2025-07-18T03:07:03Z

nexus/reconfigurator/planning/src/system.rs

+    pub fn sled_stage0_version(
+        &self,
+        sled_id: SledUuid,
+    ) -> anyhow::Result<Option<&str>> {


What does Option mean here -- i.e. under what circumstances is it possible to return None? And can this return an &ArtifactVersion to match what sled_update_rot_bootloader_versions accepts?

Being honest, I was just copying what @davepacheco had done for the SP

omicron/nexus/reconfigurator/planning/src/system.rs

Lines 518 to 526 in 2b32fc6

pub fn sled_sp_active_version(

&self,

sled_id: SledUuid,

) -> anyhow::Result<Option<&str>> {

let sled = self.sleds.get(&sled_id).with_context(|| {

format!("attempted to access sled {} not found in system", sled_id)

})?;

Ok(sled.sp_active_caboose().map(|c| c.version.as_ref()))

}

That said, I think I have an idea as to why he did it this way. These methods are only used for the cmd_sled_show function. If I were to return ArtifactVersion, then I would have to parse the result back to a string within the cmd_sled_show function, otherwise the output would look like this:

RoT bootloader stage 0 version: Some(ArtifactVersion("0.0.1"))

instead of:

omicron/dev-tools/reconfigurator-cli/tests/output/cmds-example-stdout

Lines 43 to 44 in 2b32fc6

SP active version: Some("0.0.1")

SP inactive version: None

As to why he used an option, I'm not entirely sure. I'd like to keep it like this for now, just so it matches what the SP does. When @davepacheco comes back, I'll ask him why it was an option in the first place, and happy to change it if it'd be better to not have the Option. WDYT?

Summarizing for myself (tell me if this sounds wrong): this structure is a model of the state of a simulated system (though it may have been seeded from state from a real system). The only things it does with SP versions is (1) initialize them based on whatever the caller provides, (2) allow the caller to change them via a simulated SP firmware update, and (3) report the current version.

sled_sp_active_version() returns None if the underlying sp_active_caboose field is None. I believe the only way this can happen is if this simulated system was seeded from a reconfigurator state file using an inventory collection that's missing information about this SP. I think the only way you can get here today is if you load a state file with reconfigurator-cli and tell it to use an inventory collection that's missing this SP, though there's no reason another code path couldn't hit it. (This code path winds up going through sled_full().)

I don't remember if there's a particularly important reason to support this aside from the fact that inventory collections can always be missing stuff due to transient failures and it could be annoying if reconfigurator-cli couldn't do anything with such a collection. It may have been just that it seemed pretty easy to support, given how little we do with this field.

sunshowers · 2025-07-18T03:08:07Z

nexus/reconfigurator/planning/src/system.rs

@@ -596,6 +633,43 @@ impl SystemDescription {
                    part_number: sp_state.model.clone(),
                    serial_number: sp_state.serial_number.clone(),
                };
+                if let Some(stage0) = &s.stage0_caboose() {


Is the & required here and in stage0_next_caboose below?

aha! it is not 😄

sunshowers · 2025-07-18T03:08:59Z

nexus/reconfigurator/planning/src/system.rs

+    pub stage0: Option<Arc<nexus_types::inventory::Caboose>>,
+    pub stage0_next: Option<Arc<nexus_types::inventory::Caboose>>,


same q here I guess -- can we prefill these values to something reasonable? In production we always have these filled out, right?

Same as above, I'd like to ask @davepacheco why it was done this way with the SPs to begin with

karencfv

Thanks for taking the time to review!

karencfv · 2025-07-18T03:37:19Z

nexus/reconfigurator/planning/src/system.rs

+    pub fn sled_stage0_version(
+        &self,
+        sled_id: SledUuid,
+    ) -> anyhow::Result<Option<&str>> {


Being honest, I was just copying what @davepacheco had done for the SP

omicron/nexus/reconfigurator/planning/src/system.rs

Lines 518 to 526 in 2b32fc6

pub fn sled_sp_active_version(

&self,

sled_id: SledUuid,

) -> anyhow::Result<Option<&str>> {

let sled = self.sleds.get(&sled_id).with_context(|| {

format!("attempted to access sled {} not found in system", sled_id)

})?;

Ok(sled.sp_active_caboose().map(|c| c.version.as_ref()))

}

That said, I think I have an idea as to why he did it this way. These methods are only used for the cmd_sled_show function. If I were to return ArtifactVersion, then I would have to parse the result back to a string within the cmd_sled_show function, otherwise the output would look like this:

RoT bootloader stage 0 version: Some(ArtifactVersion("0.0.1"))

instead of:

omicron/dev-tools/reconfigurator-cli/tests/output/cmds-example-stdout

Lines 43 to 44 in 2b32fc6

SP active version: Some("0.0.1")

SP inactive version: None

As to why he used an option, I'm not entirely sure. I'd like to keep it like this for now, just so it matches what the SP does. When @davepacheco comes back, I'll ask him why it was an option in the first place, and happy to change it if it'd be better to not have the Option. WDYT?

karencfv · 2025-07-18T03:41:29Z

nexus/reconfigurator/planning/src/system.rs

@@ -596,6 +633,43 @@ impl SystemDescription {
                    part_number: sp_state.model.clone(),
                    serial_number: sp_state.serial_number.clone(),
                };
+                if let Some(stage0) = &s.stage0_caboose() {


aha! it is not 😄

karencfv · 2025-07-18T03:42:24Z

nexus/reconfigurator/planning/src/system.rs

+    pub stage0: Option<Arc<nexus_types::inventory::Caboose>>,
+    pub stage0_next: Option<Arc<nexus_types::inventory::Caboose>>,


Same as above, I'd like to ask @davepacheco why it was done this way with the SPs to begin with

karencfv added 2 commits July 17, 2025 20:50

[reconfigurator-cli] RoT bootloader support

f2f44ac

small fix

22fdb16

karencfv requested review from sunshowers, jgallagher and andrewjstone July 18, 2025 01:23

karencfv marked this pull request as ready for review July 18, 2025 01:23

sunshowers reviewed Jul 18, 2025

View reviewed changes

address comments

d4c9251

karencfv commented Jul 18, 2025

View reviewed changes

karencfv requested a review from sunshowers July 21, 2025 00:55

karencfv added 2 commits July 23, 2025 12:39

merge main

5db9f78

expectorate

a498b86

karencfv mentioned this pull request Jul 23, 2025

[reconfigurator] RoT bootloader planner support #8664

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[reconfigurator-cli] RoT bootloader support #8620

[reconfigurator-cli] RoT bootloader support #8620

karencfv commented Jul 17, 2025 •

edited

Loading

Uh oh!

sunshowers left a comment

Uh oh!

Uh oh!

sunshowers Jul 18, 2025

Uh oh!

karencfv Jul 18, 2025

Uh oh!

davepacheco Jul 23, 2025

Uh oh!

sunshowers Jul 18, 2025

Uh oh!

karencfv Jul 18, 2025

Uh oh!

sunshowers Jul 18, 2025

Uh oh!

karencfv Jul 18, 2025

Uh oh!

karencfv left a comment

Uh oh!

karencfv Jul 18, 2025

Uh oh!

karencfv Jul 18, 2025

Uh oh!

karencfv Jul 18, 2025

Uh oh!

Uh oh!

	pub fn sled_sp_active_version(
	&self,
	sled_id: SledUuid,
	) -> anyhow::Result<Option<&str>> {
	let sled = self.sleds.get(&sled_id).with_context(\|\| {
	format!("attempted to access sled {} not found in system", sled_id)
	})?;
	Ok(sled.sp_active_caboose().map(\|c\| c.version.as_ref()))
	}

		pub stage0: Option<Arc<nexus_types::inventory::Caboose>>,
		pub stage0_next: Option<Arc<nexus_types::inventory::Caboose>>,

[reconfigurator-cli] RoT bootloader support #8620

Are you sure you want to change the base?

[reconfigurator-cli] RoT bootloader support #8620

Conversation

karencfv commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sunshowers left a comment

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!

karencfv 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!

Uh oh!

karencfv commented Jul 17, 2025 •

edited

Loading