Do not allocate local storage for deleted disks by jmpesp · Pull Request #10754 · oxidecomputer/omicron

jmpesp · 2026-07-06T21:30:25Z

During the sled resource insertion query, do not perform local storage allocations if the higher level disk has been deleted. It is very unlikely that an instance reservation and disk delete would race this way (as disk delete requires a detach first), but we did see it in the field! Amazing really...

Fixes oxidecomputer/customer-support#1321

During the sled resource insertion query, do not perform local storage allocations if the higher level disk has been deleted. It is very unlikely that an instance reservation and disk delete would race this way (as disk delete requires a detach first), but we _did_ see it in the field! Amazing really... Fixes oxidecomputer#1321

hawkw · 2026-07-06T23:01:17Z

+                    .first_async(&*conn)
+                    .await
+                    .map_err(|e| {
+                        public_error_from_diesel(e, ErrorHandler::Server)
+                    })?;


this will error out if the disk has been hard-deleted. is that the behavior we want, or should we instead treat a NotFound as being equivalent to the time_deleted being set?

We're currently only soft-deleting disk records so I'm happy leaving this as is.

hawkw · 2026-07-06T23:02:15Z

+                    .first_async(&*conn)
+                    .await
+                    .map_err(|e| {
+                        public_error_from_diesel(e, ErrorHandler::Server)


it might be worth adding something to the error here to say what failed, like:

Suggested change

public_error_from_diesel(e, ErrorHandler::Server)

public_error_from_diesel(e, ErrorHandler::Server).internal_context(format!("failed to look up disk {disk_id}")

k, done in 5383be7

hawkw · 2026-07-06T23:02:33Z

+
+        let zpools_for_sled = datastore
+            .zpool_get_for_sled_reservation(opctx, self.sled_target)
+            .await?;


similarly, might be nice to slap an internal_context on this so we know what failed?

hawkw · 2026-07-06T23:03:25Z

+        }
+
+        let zpools_for_sled = datastore
+            .zpool_get_for_sled_reservation(opctx, self.sled_target)


it could be worth adding a version of this that takes a &conn so we can reuse the connection we already checked out of the pool --- especially since we will keep holding onto it for the entire duration of this funcion.

Definitely, and even more so due to the Backends exist, and appear online, but all claims are used errors showing up in CI. Done in 0a9df40

hawkw · 2026-07-06T23:04:29Z

+    // Allow `transaction_async`; this is a test, and does not need to retry
+    #[allow(clippy::disallowed_methods)]


take it or leave it: it might be a little nicer to put this specifically on the transaction_async call, rather than allowing it for the entire function?

Yeah, I originally did this wrong and saw

error[E0658]: attributes on expressions are experimental --> nexus/db-queries/src/db/datastore/sled.rs:4752:13 | 4752 | #[allow(clippy::disallowed_methods)] | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: see issue #15701 <https://github.com/rust-lang/rust/issues/15701> for more information For more information about this error, try `rustc --explain E0658`.

because I tried to put it over the call to transaction_async instaed of over the let .... Changed in 6fa00f7

hawkw · 2026-07-06T23:05:27Z

+            .await
+            .unwrap();
+
+        assert!(disks_with_zombie_allocations.is_empty());


take it or leave it: maybe

Suggested change

assert!(disks_with_zombie_allocations.is_empty());

assert_eq!(disks_with_zombie_allocations, &[]);

so that the panic message will also include the list of disks with zombie allocations, if this assert fails?

I like this idea. Turns out though that you have to be very explicit about the type of the empty Vec. Done in c9d158d

smklein · 2026-07-21T22:35:47Z

+        // Between when the instance allocation was requested and now, any disk
+        // backed by local storage could have been detached and deleted. Check
+        // for that here, and prune the entire search space if this happened.


I'm looking at how disk deletion is implemented here:

https://github.com/jmpesp/omicron/blob/c9d158dd5f25c5cdcd3a7d68748cc9721532fe93/nexus/src/app/disk.rs#L369-L379

It looks like it does the following:

Reads disk state from DB

Shoves disk state into disk_delete saga parameters

Actually deletes disk (setting time deleted = now()).

Meanwhile, this new validation code looks like it's doing the following:

Read disk state from DB

Check that disk hasn't been deleted (otherwise we bail)

Proceed with allocation

So it seems like we could hit the following case with concurrent requests R1/R2:

R1: Sled reservation starts, wants attached disk D

R2: Detach disk D

R2: Start the deletion saga of disk D (storing the disk_delete info into saga parameters)

R1: Reservation INSERT commits using a disk

R2: Disk actually gets deleted. But it's actually attached to an instance that's getting created!

Basically: this check seems "necessary but insufficient" - should we also be validating that the disk is still attached (attach_instance_id = <requesting instance>) at the moment we commit the instance reservation record to the database?

smklein · 2026-07-21T22:44:53Z

+                query
+                    .sql("(SELECT time_deleted IS NULL FROM DISK WHERE ID = ")
+                    .param()
+                    .bind::<sql_types::Uuid, _>(allocation.disk_id);


See my other comment about raciness - I think we could still cause a disk to be deleted after passing this condition.

But also: are we testing this behavior anywhere? In local_storage_allocation_no_zombies, it looks like we're deleting the disk and then relying on the "pruning" check to remove it, rather than relying on this insertion-time backstop

jmpesp requested review from hawkw and smklein July 6, 2026 21:30

hawkw reviewed Jul 6, 2026

View reviewed changes

jmpesp added 4 commits July 7, 2026 16:05

put specifically on transaction_async call

6fa00f7

zpool_get_for_sled_reservation accepts a Connection now

0a9df40

internal context

5383be7

show zombies for assert failure

c9d158d

smklein reviewed Jul 21, 2026

View reviewed changes

	public_error_from_diesel(e, ErrorHandler::Server)
	public_error_from_diesel(e, ErrorHandler::Server).internal_context(format!("failed to look up disk {disk_id}")

		// Allow `transaction_async`; this is a test, and does not need to retry
		#[allow(clippy::disallowed_methods)]

	assert!(disks_with_zombie_allocations.is_empty());
	assert_eq!(disks_with_zombie_allocations, &[]);

Uh oh!

Conversation

jmpesp commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

jmpesp commented Jul 6, 2026 •

edited

Loading