[update] add missing sleds to contact_support checks by karencfv · Pull Request #10476 · oxidecomputer/omicron

karencfv · 2026-05-21T09:15:40Z

Follow up to #10271

karencfv · 2026-05-21T09:40:48Z

-    /// Build a map of version strings to the number of components on that
-    /// version
-    async fn component_version_counts(
+    async fn get_internal_update_status(


Most of the work was already done here, I just extracted the bit that was creating internal_views::UpdateStatus and used that to determine missing sleds.

sunshowers

Looks good overall! Just a few questions and comments.

sunshowers · 2026-05-26T18:34:58Z

+            .iter()
+            .filter(|sled| {
+                // `unknown()` returns the represenation of the update status
+                // for a given sled ID that isn't present in inventory.


Is this case also hit when a sled is present in inventory but there's no last reconciliation status?

yep

omicron/nexus/types/src/internal_api/views.rs

Lines 1010 to 1012 in bca885a

let Some(inv) = sa.last_reconciliation.as_ref() else {

return Some(SledAgentUpdateStatus::unknown(sa.sled_id));

};

I can update the comment to explain this

That would be great -- thanks!

sunshowers · 2026-05-26T18:38:50Z

+                **sled
+                    == internal_views::SledAgentUpdateStatus::unknown(
+                        sled.sled_id,
+                    )
+            })


Hmm, looking at SledAgentUpdateStatus, the unknown state isn't recorded as an enum variant. This is pre-existing but I'm a little worried about the fragility of things like string equality here:

omicron/nexus/types/src/internal_api/views.rs

Lines 677 to 681 in 2fb3663

host_phase_2: HostPhase2Status {

boot_disk: Err("unknown".to_string()),

slot_a_version: TufRepoVersion::Unknown,

slot_b_version: TufRepoVersion::Unknown,

},

I fear that this fix may be a bit more involved than it seems at a first glance #10271 (comment)

Do you mind if I open up a follow up issue for this work?

Sounds good.

sunshowers · 2026-05-26T18:55:40Z

                }
            };

+        let missing_sleds: BTreeSet<SledUuid> = self


Does omdb need to show this information? Seems like for support it would be better than grepping through logs.

Yeah, that'd be really nice. There was a discussion about that here -> #9412 (comment)

I think this work may need its own issue though. Thoughts?

Yeah, a followup sounds good.

Added #7242 (comment)

sunshowers · 2026-05-26T19:00:58Z

        let expected_sleds = self
            .datastore()
            .sled_list_all_batched(
                opctx,
                SledFilter::SpsUpdatedByReconfigurator,
            )
            .await?


Hmm so this will do a live db query, but inventory is cached. Does this mean that when a sled is newly commissioned, contact_support might be true for a short period of time?

Yeah, probably. This is also true for other checks in various circumstances. I'll add these caveats to the support runbooks. It's worth mentioning that users would only hit this case if they wish to perform an update or check the update status immediately after a sled is newly commissioned, which is unlikely?

Yeah, that sounds good. I do feel like in the limit this is worth fixing, but it's pretty far down the list.

sunshowers · 2026-05-26T19:02:18Z

+    async fn component_version_counts(
+        &self,
+        status: internal_views::UpdateStatus,
+    ) -> Result<BTreeMap<String, usize>, Error> {


Two notes:

This doesn't need to either be async or take &self, I think.

Small nit: thoughts on reworking this to take a reference to UpdateStatus?

Yeah, you're right. I extracted the bits that I needed but didn't check the signature to see if it still made sense. I'll fix this

sunshowers · 2026-05-26T19:03:44Z

+            .sleds
+            .iter()
+            .filter(|sled| {
+                // `unknown()` returns the represenation of the update status


nit:

Suggested change

// `unknown()` returns the represenation of the update status

// `unknown()` returns the representation of the update status

karencfv

Thanks for taking a look! I think I've addressed all your comments, let me know if I missed anything :)

karencfv · 2026-05-28T04:58:38Z

+            .iter()
+            .filter(|sled| {
+                // `unknown()` returns the represenation of the update status
+                // for a given sled ID that isn't present in inventory.


yep

omicron/nexus/types/src/internal_api/views.rs

Lines 1010 to 1012 in bca885a

let Some(inv) = sa.last_reconciliation.as_ref() else {

return Some(SledAgentUpdateStatus::unknown(sa.sled_id));

};

I can update the comment to explain this

karencfv · 2026-05-28T05:01:39Z

                }
            };

+        let missing_sleds: BTreeSet<SledUuid> = self


Yeah, that'd be really nice. There was a discussion about that here -> #9412 (comment)

I think this work may need its own issue though. Thoughts?

karencfv · 2026-05-28T05:05:39Z

+                **sled
+                    == internal_views::SledAgentUpdateStatus::unknown(
+                        sled.sled_id,
+                    )
+            })


I fear that this fix may be a bit more involved than it seems at a first glance #10271 (comment)

Do you mind if I open up a follow up issue for this work?

karencfv · 2026-05-28T05:07:06Z

+    async fn component_version_counts(
+        &self,
+        status: internal_views::UpdateStatus,
+    ) -> Result<BTreeMap<String, usize>, Error> {


Yeah, you're right. I extracted the bits that I needed but didn't check the signature to see if it still made sense. I'll fix this

karencfv added 2 commits May 21, 2026 21:03

[update] add missing sleds to contact_support checks

2eef2e2

clean up

a3874ea

karencfv marked this pull request as draft May 21, 2026 09:15

add some comments

426815c

karencfv commented May 21, 2026

View reviewed changes

karencfv marked this pull request as ready for review May 21, 2026 09:41

karencfv requested review from davepacheco and jgallagher May 21, 2026 09:41

karencfv mentioned this pull request May 25, 2026

Tracking issue for update related health checks #10488

Open

sunshowers reviewed May 26, 2026

View reviewed changes

karencfv added 2 commits May 28, 2026 17:15

address comments

8b2e24c

add more comments

8df7de9

karencfv commented May 28, 2026

View reviewed changes

sunshowers approved these changes May 28, 2026

View reviewed changes

karencfv merged commit c6af9c9 into oxidecomputer:main May 28, 2026
18 checks passed

karencfv deleted the find-missing-sleds branch May 28, 2026 20:38

karencfv mentioned this pull request May 28, 2026

HostPhase2Status could use an enum when boot disk is unknown #10504

Open

	let Some(inv) = sa.last_reconciliation.as_ref() else {
	return Some(SledAgentUpdateStatus::unknown(sa.sled_id));
	};

	host_phase_2: HostPhase2Status {
	boot_disk: Err("unknown".to_string()),
	slot_a_version: TufRepoVersion::Unknown,
	slot_b_version: TufRepoVersion::Unknown,
	},

	// `unknown()` returns the represenation of the update status
	// `unknown()` returns the representation of the update status

Conversation

karencfv commented May 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants