Do not allocate local storage for deleted disks#10754
Conversation
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
| .first_async(&*conn) | ||
| .await | ||
| .map_err(|e| { | ||
| public_error_from_diesel(e, ErrorHandler::Server) | ||
| })?; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
We're currently only soft-deleting disk records so I'm happy leaving this as is.
| .first_async(&*conn) | ||
| .await | ||
| .map_err(|e| { | ||
| public_error_from_diesel(e, ErrorHandler::Server) |
There was a problem hiding this comment.
it might be worth adding something to the error here to say what failed, like:
| public_error_from_diesel(e, ErrorHandler::Server) | |
| public_error_from_diesel(e, ErrorHandler::Server).internal_context(format!("failed to look up disk {disk_id}") |
|
|
||
| let zpools_for_sled = datastore | ||
| .zpool_get_for_sled_reservation(opctx, self.sled_target) | ||
| .await?; |
There was a problem hiding this comment.
similarly, might be nice to slap an internal_context on this so we know what failed?
| } | ||
|
|
||
| let zpools_for_sled = datastore | ||
| .zpool_get_for_sled_reservation(opctx, self.sled_target) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| // Allow `transaction_async`; this is a test, and does not need to retry | ||
| #[allow(clippy::disallowed_methods)] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| .await | ||
| .unwrap(); | ||
|
|
||
| assert!(disks_with_zombie_allocations.is_empty()); |
There was a problem hiding this comment.
take it or leave it: maybe
| 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?
There was a problem hiding this comment.
I like this idea. Turns out though that you have to be very explicit about the type of the empty Vec. Done in c9d158d
2 participants
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