Add support for re-adopting physical disks by andrewjstone · Pull Request #10221 · oxidecomputer/omicron

andrewjstone · 2026-04-04T00:42:17Z

This change implements the determinations in RFD 663. It allows
re-adopting physical disks in the control plane after the control plane
level disk in the physical_disk table is expunged.

It does this by forcing manual adoption of disks by an operator, where
requests are placed in the physical_disk_adoption_request table.
A disk will now only be adopted or re-adopted by the disk adoption
background task if its physical vendor/model/serial information is
present in a physical_disk_adoption_request row.

The typical flow for an operator is to list uninitialized disks and then
issue an adoption request via the external API.

This change implements the determinations in RFD 693. It allows re-adopting physical disks in the control plane after the control plane level disk in the `physical_disk` table is expunged. It does this by forcing manual adoption of disks by an operator, where requests are placed in the `physical_disk_adoption_request` table. A disk will now only be adopted or re-adopted by the disk adoption background task if its physical vendor/model/serial information is present in a `physical_disk_adoption_request` row. The typical flow for an operator is to list unitialized disks and then issue an adoption request via the external API.

smklein · 2026-04-07T21:23:09Z

This change implements the determinations in RFD 693. It allows re-adopting physical disks in the control plane after the control plane level disk in the physical_disk table is expunged.

Nit: 663

andrewjstone · 2026-04-07T21:25:19Z

This change implements the determinations in RFD 693. It allows re-adopting physical disks in the control plane after the control plane level disk in the physical_disk table is expunged.

Nit: 663

Whoops. I even have it open in a tab. Thanks!

ahl

To the best of my understanding, we have a couple of metaphors in here. Disks that are unknown to the control plane are "uninitialized" and appear in that list. The verb we're using is "adopt", as in "this uninitialized disk is adopted by the control plane". Is the term "adopt" intended to be the opposite of "expunge"?

I'm not clear on the how the operator would use this. Presumably there's supposed to be some step (e.g. of exploration or cognition) between "list uninit disks" and "approve disk(s)", but I'm not sure what it is.

We don’t want to allow automatic disk adoption due to the risk of the insertion of malicious hardware during casual physical access. This is especially problematic before we have disk attestation support, and in the case of existing sleds with empty disk bays.

Presumably I want to make sure the hardware I just put into the U.2 bay is the same as what I'm about to adopt. How do I do that?

The API changes look fine; I'd ask you think about nomenclature ("uninitialized" "adopt").

ahl · 2026-04-07T22:33:06Z

+        query: Query<PaginationParams<EmptyScanParams, String>>,
+    ) -> Result<
+        HttpResponseOk<
+            ResultsPage<latest::physical_disk::UninitializedPhysicalDisk>,


if I expunge a disk, I think that changes both the policy and state properties (I'm not sure why there are both of these--is one intent and the other is status?), does the disk immediately show up in this list?

Do disks show up in only one place or the other? Do some show up in both?

Expunge happens immediately, and is a terminating enum variant. It's the intention of the desired state. Once it occurs we can assume it will never change and most things just look at expunge. Decommissioned state occurs after other steps, when the intended state is realized. So there is some delay there.

hawkw · 2026-04-07T22:56:27Z

We don’t want to allow automatic disk adoption due to the risk of the insertion of malicious hardware during casual physical access. This is especially problematic before we have disk attestation support, and in the case of existing sleds with empty disk bays.

Presumably I want to make sure the hardware I just put into the U.2 bay is the same as what I'm about to adopt. How do I do that?

Is the idea that one would do this by comparing the manufacturer/model number/serial listed by the API endpoint with those physically printed on the actual disk, and also based on foreknowledge that disks have or have not been inserted in specific locations at the current point in time?

hawkw · 2026-04-07T22:52:23Z

+    pub async fn physical_disk_adoptable_list(
+        &self,
+        opctx: &OpContext,
+        inventory_collection_id: CollectionUuid,
+    ) -> ListResultVec<InvPhysicalDisk> {


i thought about suggesting that this be paginated, but...do we expect the maximum number of rows to be limited by the physical fact that 32 sleds * 10 U.2s - 320 disks maximum?

There's not really a good way to paginate this right now AFAIK.

hawkw · 2026-04-07T22:57:56Z

+/// A physical disk that has not yet been adopted by the control plane
+#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize, JsonSchema)]
+pub struct UninitializedPhysicalDisk {
+    pub sled_id: SledUuid,


do we expect the client to hydrate this sled UUID into a sled's physical location? it seems like it would be desirable for a UI listing physical disks that need to be adopted to be able to say which sled they are in as well as the slot within that sled...

Good question. I suppose we could also provide the sled cubby. That would help operators out a bit probably.

It's not guaranteed we have this, right? Uninitalized physical disks come from sled-agent inventory, and sled-agent doesn't know its own cubby number. We could try to match up the sled serial against the SP inventory contents to identify a cubby, and that will usually work, but we'd still need to be able to represent "physical disk for sled X for cubby I Dunno Ask Again Later".

andrewjstone · 2026-04-07T22:59:21Z

We don’t want to allow automatic disk adoption due to the risk of the insertion of malicious hardware during casual physical access. This is especially problematic before we have disk attestation support, and in the case of existing sleds with empty disk bays.

Presumably I want to make sure the hardware I just put into the U.2 bay is the same as what I'm about to adopt. How do I do that?

Is the idea that one would do this by comparing the manufacturer/model number/serial listed by the API endpoint with those physically printed on the actual disk, and also based on foreknowledge that disks have or have not been inserted in specific locations at the current point in time?

Yes, basically, if they actually cared. I think the larger security issue mitigated is that any disk inserted can only be activated by an operator and not just adopted automatically for usage. Checking the serial is how they would know which disk they were validating against.

smklein · 2026-04-07T22:31:13Z

+                        ),
+                ),
+            )
+            // Ensure that each inventory disk has a valid adoption request


Is there a precondition that "you cannot have an adoption request for an already in-service disk"?

(Having already-in-service disks show up here seems wrong, just confirming what prevents that. I think the answer is "yes, a non-deleted adoption request means you have no live disks here")

Yes, that's a precondition.

andrewjstone · 2026-04-07T23:21:31Z

To the best of my understanding, we have a couple of metaphors in here. Disks that are unknown to the control plane are "uninitialized" and appear in that list. The verb we're using is "adopt", as in "this uninitialized disk is adopted by the control plane". Is the term "adopt" intended to be the opposite of "expunge"?

It's not necessarily the opposite of expunge. A user can insert a disk in an empty bay and it would be adopted for use. It's idle / uninitialized before that.

I'm not clear on the how the operator would use this. Presumably there's supposed to be some step (e.g. of exploration or cognition) between "list uninit disks" and "approve disk(s)", but I'm not sure what it is.

We don’t want to allow automatic disk adoption due to the risk of the insertion of malicious hardware during casual physical access. This is especially problematic before we have disk attestation support, and in the case of existing sleds with empty disk bays.

Presumably I want to make sure the hardware I just put into the U.2 bay is the same as what I'm about to adopt. How do I do that?

As Eliza noted, an operator would have to look at the vendor/model/serial on the drive and compare it to what shows up in the API.

The API changes look fine; I'd ask you think about nomenclature ("uninitialized" "adopt").

I'm open to changing this. adoption has been our internal de-facto term for a while. WE could say 'initializeoractivate` or something else.

jgallagher · 2026-04-08T17:22:39Z

+/// A physical disk that has not yet been adopted by the control plane
+#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize, JsonSchema)]
+pub struct UninitializedPhysicalDisk {
+    pub sled_id: SledUuid,


It's not guaranteed we have this, right? Uninitalized physical disks come from sled-agent inventory, and sled-agent doesn't know its own cubby number. We could try to match up the sled serial against the SP inventory contents to identify a cubby, and that will usually work, but we'd still need to be able to represent "physical disk for sled X for cubby I Dunno Ask Again Later".

hawkw · 2026-04-08T21:08:43Z

I'm not clear on the how the operator would use this. Presumably there's supposed to be some step (e.g. of exploration or cognition) between "list uninit disks" and "approve disk(s)", but I'm not sure what it is.

We don’t want to allow automatic disk adoption due to the risk of the insertion of malicious hardware during casual physical access. This is especially problematic before we have disk attestation support, and in the case of existing sleds with empty disk bays.

Presumably I want to make sure the hardware I just put into the U.2 bay is the same as what I'm about to adopt. How do I do that?

As Eliza noted, an operator would have to look at the vendor/model/serial on the drive and compare it to what shows up in the API.

@smklein and I have been talking through disk replacement scenarios a bit from the fault management context, and I think the adoption requests will eventually be part of the service flow for disk replacements. I think in particular, we would really like it if the adoption requests could easily include the physical location of the disk as part of a "you just replaced the disk in sled 19 slot 3, okay yeah it's that one" kinda spot check.

andrewjstone · 2026-04-08T22:48:04Z

To the best of my understanding, we have a couple of metaphors in here. Disks that are unknown to the control plane are "uninitialized" and appear in that list. The verb we're using is "adopt", as in "this uninitialized disk is adopted by the control plane". Is the term "adopt" intended to be the opposite of "expunge"?

@ahl @smklein @hawkw

I just talked about this with John a bit and if you don't like the term adopt, we could always use the word import. Then instead of uninitialized we would say unimported. We could also not mix metaphors by saying adopt and unadopted.

ahl · 2026-04-08T22:55:36Z

I'm good with whatever of those you choose. I appreciate you discussing.

andrewjstone · 2026-04-08T23:08:24Z

@smklein and I have been talking through disk replacement scenarios a bit from the fault management context, and I think the adoption requests will eventually be part of the service flow for disk replacements. I think in particular, we would really like it if the adoption requests could easily include the physical location of the disk as part of a "you just replaced the disk in sled 19 slot 3, okay yeah it's that one" kinda spot check.

As @jgallagher pointed out earlier today, we don't actually have this information in sled-agent inventory. What happens if the sp inventory doesn't exist for this collection? Then we can't list the sled as uninitialized or we always need to carry around an option here. Would it make sense to tell a customer: "Hey, actually I don't know what cubby this disk is in right now." or "This sled is not uninitialized" event though the customer just inserted it into the rack and expects to see it?

andrewjstone · 2026-04-09T00:06:04Z

One thing I just realized was that with the current code, we will no longer automatically adopt disks when sleds are added to a rack. I confirmed with @rmustacc that this was not what he intended. Unfortunately, I'm not immediately sure how to fit this behavior in with the current one. Disks are detected asynchronously after a sled is added and currently adopted by the background task automatically. The new behavior ensures that a disk adoption request is made by a user to allow adoption by the background task, but the adoption itself is still done asynchronously in the background task and is separate from the sled add request.

What we would want is a state attached to the sled that says automatically adopt disks that were present when the sled was added to the rack. But we don't really have any mechanism to discover that information. I think the best we can do with the current code base is say something like "automatically adopt disks for this sled for 5 minutes" after it has been created. Given inventory delays this is also problematic as a disk the customer expected to be adopted that was in the sled when it was added may not actually get adopted. That's a terrible user experience. We could lessen the likelihood of non-adoption by increasing this window, but that now lengthens the time when casual physical attack is possible by inserting arbitrary disks.

The only other thing I can think of doing is adding disk information to the SledAgentInfo that gets published to the nexus internal api when the sled-agent starts up. But this is a client-versioned API currently, which could be problematic if a sled needs to be added during an upgrade.

hawkw · 2026-04-09T00:21:17Z

I just talked about this with John a bit and if you don't like the term adopt, we could always use the word import. Then instead of uninitialized we would say unimported. We could also not mix metaphors by saying adopt and unadopted.

NOT TO BE ANNOYING BUT: I really don't like "import" in this context, it feels like it is too easily misconstrued as "import the data that was on this disk", which is not what one would expect to be offered but which is a somewhat conceivable thing that might occur.

andrewjstone · 2026-04-09T00:29:10Z

I just talked about this with John a bit and if you don't like the term adopt, we could always use the word import. Then instead of uninitialized we would say unimported. We could also not mix metaphors by saying adopt and unadopted.

NOT TO BE ANNOYING BUT: I really don't like "import" in this context, it feels like it is too easily misconstrued as "import the data that was on this disk", which is not what one would expect to be offered but which is a somewhat conceivable thing that might occur.

NOT ANNOYING AT ALL!!! I appreciate the feedback.

I'm not a huge fan of import either. I still like adopt. FWIW, claude does too, but it's a sycophant that hallucinated usage in both zfs and kubernetes, so do with that what you will.

I think to make the metaphors less mixed I'd switch to listing unadopted rather than uninitialized disks.

andrewjstone · 2026-04-10T17:55:42Z

Thanks for the reviews @smklein @hawkw @jgallagher

I believe I've addressed everything. Unfortunately, I discovered an issue that should probably be resolved before merge or explicitly decided not to implement: #10221 (comment)

andrewjstone · 2026-04-10T17:58:28Z

One thing I just realized was that with the current code, we will no longer automatically adopt disks when sleds are added to a rack. I confirmed with @rmustacc that this was not what he intended. Unfortunately, I'm not immediately sure how to fit this behavior in with the current one. Disks are detected asynchronously after a sled is added and currently adopted by the background task automatically. The new behavior ensures that a disk adoption request is made by a user to allow adoption by the background task, but the adoption itself is still done asynchronously in the background task and is separate from the sled add request.

What we would want is a state attached to the sled that says automatically adopt disks that were present when the sled was added to the rack. But we don't really have any mechanism to discover that information. I think the best we can do with the current code base is say something like "automatically adopt disks for this sled for 5 minutes" after it has been created. Given inventory delays this is also problematic as a disk the customer expected to be adopted that was in the sled when it was added may not actually get adopted. That's a terrible user experience. We could lessen the likelihood of non-adoption by increasing this window, but that now lengthens the time when casual physical attack is possible by inserting arbitrary disks.

The only other thing I can think of doing is adding disk information to the SledAgentInfo that gets published to the nexus internal api when the sled-agent starts up. But this is a client-versioned API currently, which could be problematic if a sled needs to be added during an upgrade.

There are further problems that make the implementation next to impossible to do in an ideal manner.

Before the sled-agent is up, it is not on the underlay network and so we can't ask it for the disks that are currently inserted.
Even if we include those disks in the client-side versioned put to nexus from sled-agent, the disks themselves are loaded asynchronously by the hardware manager. It's possible that some of them haven't made themselves known yet.

We really seem to be restricted to a time based setup, or forcing manual disk adoption at all times.

smklein · 2026-04-10T18:54:12Z

@andrewjstone and I chatted about this a bit offline. Recording some of our thoughts here:

In the short-term, it may make sense to keep the old behavior of "auto-adopt disks that haven't been part of the control plane before". We can make that old pathway create adoption requests, to unify the disk setup process. This still would allow an operator to re-add an expunged disk. The "auto-adoption" conditions could also be turned into a toggle, or turned off, at some point in the future.
We could read disk information from uninitalized sleds over the bootstrap network, and present that information as a part of "sled add" - basically, "sled add" could become "sled add AND create these disk adoption requests". We suspect this would not be a small task, but it's theoretically possible?

andrewjstone · 2026-04-20T23:16:15Z

@andrewjstone and I chatted about this a bit offline. Recording some of our thoughts here:

* In the short-term, it may make sense to keep the old behavior of "auto-adopt disks that haven't been part of the control plane before". We can make that old pathway create adoption requests, to unify the disk setup process. This still would allow an operator to re-add an expunged disk. The "auto-adoption" conditions could also be turned into a toggle, or turned off, at some point in the future.

* We could read disk information from uninitalized sleds over the bootstrap network, and present that information as a part of "sled add" - basically, "sled add" could become "sled add AND create these disk adoption requests". We suspect this would not be a small task, but it's theoretically possible?

Based on discussion in update huddle last week, we decided that to move forward we would auto-adopt disks that haven't been part of the control plane before. c9618fc makes this change. Importantly it makes this change by inserting new disks into the physical_disk_adoption_request table and not changing the method that determines which disks are adoptable. This makes it easier to remove in the future. Thanks to @smklein for the suggestion.

andrewjstone · 2026-04-21T00:01:50Z

This is ready for a re-review @jgallagher @smklein. I still need to test it on hardware which I'll do after feedback. I'd like to test either Thursday afternoon or Friday. Thanks!

andrewjstone commented Apr 4, 2026

View reviewed changes

Comment thread nexus/src/app/sled.rs Outdated

andrewjstone force-pushed the manual-disk-adoption branch from 67bdc40 to ce4b577 Compare April 7, 2026 20:35

andrewjstone changed the title ~~WIP: Manual disk adoption~~ Add support for re-adopting physical disks Apr 7, 2026

andrewjstone force-pushed the manual-disk-adoption branch from ce4b577 to edc851b Compare April 7, 2026 20:42

andrewjstone marked this pull request as ready for review April 7, 2026 20:42

andrewjstone requested review from ahl, jgallagher and smklein April 7, 2026 20:42

andrewjstone commented Apr 7, 2026

View reviewed changes

Comment thread nexus/src/external_api/http_entrypoints.rs Outdated

andrewjstone added 2 commits April 7, 2026 21:47

fix tests

3ee2cc1

expectorate

260a348

ahl reviewed Apr 7, 2026

View reviewed changes

hawkw reviewed Apr 7, 2026

View reviewed changes

smklein reviewed Apr 7, 2026

View reviewed changes

jgallagher reviewed Apr 8, 2026

View reviewed changes

andrewjstone added 2 commits April 9, 2026 01:06

minor fixes

b4d48fe

A few more review fixes

2c79dd0

andrewjstone added 7 commits April 9, 2026 20:30

more small fixes

af22bb6

Do not error bg task for not found

b246e49

Fix my fix

df9afa6

Try to fix some queries

1fe99e0

fix unique constraint handling

b448340

Use typed uuid

daa5057

add test and fix fn name

dcd1a06

AlejandroME added this to the 20 milestone Apr 10, 2026

Add delete endpoint and fix a bunch of stuff

8bce6a0

andrewjstone added 2 commits April 10, 2026 19:02

openapi

87bc5a0

Auto adopt newly seen physical disks

c9618fc

Merge branch 'main' into manual-disk-adoption

e695804

Conversation

andrewjstone commented Apr 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

smklein commented Apr 7, 2026

Uh oh!

andrewjstone commented Apr 7, 2026

Uh oh!

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

Uh oh!

hawkw commented Apr 7, 2026

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 commented Apr 7, 2026

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!

Uh oh!

Uh oh!

andrewjstone commented Apr 7, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hawkw commented Apr 8, 2026

Uh oh!

andrewjstone commented Apr 8, 2026

Uh oh!

ahl commented Apr 8, 2026

Uh oh!

andrewjstone commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewjstone commented Apr 9, 2026

Uh oh!

hawkw commented Apr 9, 2026

Uh oh!

andrewjstone commented Apr 9, 2026

Uh oh!

andrewjstone commented Apr 10, 2026

Uh oh!

andrewjstone commented Apr 10, 2026

Uh oh!

smklein commented Apr 10, 2026

Uh oh!

andrewjstone commented Apr 20, 2026

Uh oh!

andrewjstone commented Apr 21, 2026

Uh oh!

andrewjstone commented Apr 4, 2026 •

edited

Loading

andrewjstone commented Apr 8, 2026 •

edited

Loading