Skip to content

[nexus] Don't re-fetch a disk after committing attach/detach (fixes #10695) #10706

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
MavenRain wants to merge 1 commit into
base: main
Choose a base branch
from
+47 −19
Open

[nexus] Don't re-fetch a disk after committing attach/detach (fixes #10695) #10706

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 34 additions & 17 deletions nexus/db-queries/src/db/datastore/disk.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -940,7 +940,7 @@ impl DataStore {
authz_instance: &authz::Instance,
authz_disk: &authz::Disk,
max_disks: u32,
) -> Result<(Instance, Disk), Error> {
) -> Result<(Instance, model::Disk), Error> {
use nexus_db_schema::schema::disk;
use nexus_db_schema::schema::instance;
use nexus_db_schema::schema::sled_resource_vmm;
Expand Down Expand Up @@ -1095,12 +1095,11 @@ impl DataStore {

let conn = self.pool_connection_authorized(opctx).await?;

let instance = match query.attach_and_get_result_async(&conn).await {
Ok((instance, _db_disk)) => {
// We'll re-fetch the datastore::Disk later, so ignore the
// model::Disk here
instance
}
let (instance, db_disk) = match query
.attach_and_get_result_async(&conn)
.await
{
Ok((instance, db_disk)) => (instance, db_disk),

Err(e) => match e {
AttachError::CollectionNotFound => {
Expand Down Expand Up @@ -1128,12 +1127,12 @@ impl DataStore {
api::external::DiskState::Attached(id)
if id == authz_instance.id() =>
{
collection
(collection, resource)
}
api::external::DiskState::Attaching(id)
if id == authz_instance.id() =>
{
collection
(collection, resource)
}
// Ok-to-attach disk states: Inspect the state to infer
// why we did not attach.
Expand Down Expand Up @@ -1239,17 +1238,29 @@ impl DataStore {
},
};

// Re-fetch the disk to get the updates
let disk = self.disk_get(&opctx, authz_disk.id()).await?;
Ok((instance, disk))
// NB: The attach mutation above is the *last fallible operation* in this
// method, and we return the `model::Disk` produced by the CTE directly.
//
// We deliberately do NOT re-fetch here to build a richer
// `datastore::Disk`: a re-fetch acquires an additional pool connection
// *after* the attach has already committed, and if that acquisition
// fails (e.g. under connection-pool exhaustion) the caller observes an
// error even though the disk is now attached. In a saga that is fatal:
// steno never runs the undo of a failed forward node, so the committed
// attach would leak and later wedge the unwind (see omicron#10695).
//
// Callers that need a `datastore::Disk` should upgrade the returned
// `model::Disk` via `disk_get`, in a context where a post-commit failure
// is recoverable (i.e. not from a saga action).
Ok((instance, db_disk))
}

pub async fn instance_detach_disk(
&self,
opctx: &OpContext,
authz_instance: &authz::Instance,
authz_disk: &authz::Disk,
) -> Result<Disk, Error> {
) -> Result<model::Disk, Error> {
use nexus_db_schema::schema::{disk, instance};

opctx.authorize(authz::Action::Modify, authz_instance).await?;
Expand All @@ -1275,7 +1286,7 @@ impl DataStore {

let conn = self.pool_connection_authorized(opctx).await?;

let _db_disk = Instance::detach_resource(
let db_disk = Instance::detach_resource(
authz_instance.id(),
authz_disk.id(),
instance::table
Expand Down Expand Up @@ -1390,9 +1401,15 @@ impl DataStore {
}
})?;

let disk = self.disk_get(&opctx, authz_disk.id()).await?;

Ok(disk)
// NB: As with `instance_attach_disk`, the detach mutation above is the
// *last fallible operation* in this method: we return the `model::Disk`
// produced by the CTE directly rather than re-fetching. A post-commit
// re-fetch here would be especially dangerous because `instance_detach_disk`
// is used as a saga *undo* action (`sic_attach_disk_to_instance_undo`);
// steno gives undo actions a single attempt with no retry, so a failure
// after the detach commits would permanently wedge the unwind even
// though the detach succeeded (see omicron#10695).
Ok(db_disk)
}

pub async fn disk_update_runtime(
Expand Down
15 changes: 13 additions & 2 deletions nexus/src/app/instance.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1924,7 +1924,7 @@ impl super::Nexus {
// - Update the DB if the request succeeded (hopefully to "Attached").
// - If the instance is not running...
// - Update the disk state in the DB to "Attached".
let (_instance, disk) = self
let (_instance, _db_disk) = self
.db_datastore
.instance_attach_disk(
&opctx,
Expand All @@ -1934,6 +1934,14 @@ impl super::Nexus {
)
.await?;

// `instance_attach_disk` returns a `model::Disk` and deliberately does
// not re-fetch a `datastore::Disk` internally (see omicron#10695): a
// re-fetch after the attach commits can fail (e.g. pool exhaustion) and
// would leak the committed attach if it ran inside a saga action. It is
// safe to upgrade to a `datastore::Disk` here, outside the mutation,
// because a failure in this API path is recoverable (the client can
// retry; attach is idempotent).
let disk = self.db_datastore.disk_get(&opctx, authz_disk.id()).await?;
Ok(disk)
}

Expand Down Expand Up @@ -1972,10 +1980,13 @@ impl super::Nexus {
// - Update the DB if the request succeeded (hopefully to "Detached").
// - If the instance is not running...
// - Update the disk state in the DB to "Detached".
let disk = self
let _db_disk = self
.db_datastore
.instance_detach_disk(&opctx, &authz_instance, &authz_disk)
.await?;
// As in `instance_attach_disk`, upgrade the returned `model::Disk` to a
// `datastore::Disk` outside the datastore mutation (see omicron#10695).
let disk = self.db_datastore.disk_get(&opctx, authz_disk.id()).await?;
Ok(disk)
}

Expand Down