Skip to content

Commit

Permalink
Fix for deleted volumes during region replacement
Browse files Browse the repository at this point in the history
Volumes can be deleted at any time, but the tasks and sagas that perform
region replacement did not account for this. This commit adds checks in
a few places for if a volume is soft-deleted or hard-deleted, and bails
out of any affected region replacement accordingly:

- If the replacement request is in the Requested state and the volume
  was seen to be soft-deleted or hard-deleted in the "region
  replacement" background task, then transition the region replacement
  request to Complete

- If the replacement request is in the Running state, and the volume was
  seen to be soft-deleted or hard-deleted in the region replacement
  drive saga, then skip any operations on that volume in that saga and
  allow that saga to transition the region replacement request to
  ReplacementDone. Later the rest of the region replacement machinery
  will transition the request to Complete and clean up resources as
  appropriate.

Testing this required fleshing out the simulated Crucible Pantry with
support for the new endpoints that the region replacement drive saga
queries. Full parity is left for future work, and the endpoints required
were left in but commented out.

This commit was peeled off work in progress to address oxidecomputer#6353.
  • Loading branch information
jmpesp committed Sep 24, 2024
1 parent eb4d5a5 commit 4c83c6d
Show file tree
Hide file tree
Showing 12 changed files with 1,173 additions and 13 deletions.
8 changes: 8 additions & 0 deletions dev-tools/omdb/src/bin/omdb/nexus.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1129,6 +1129,14 @@ fn print_task_details(bgtask: &BackgroundTask, details: &serde_json::Value) {
println!(" > {line}");
}

println!(
" region replacement requests set to completed ok: {}",
status.requests_completed_ok.len()
);
for line in &status.requests_completed_ok {
println!(" > {line}");
}

println!(" errors: {}", status.errors.len());
for line in &status.errors {
println!(" > {line}");
Expand Down
7 changes: 7 additions & 0 deletions nexus/db-model/src/region_replacement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,13 @@ impl std::str::FromStr for RegionReplacementState {
/// "finish" notification is seen by the region replacement drive background
/// task. This check is done before invoking the region replacement drive saga.
///
/// If the volume whose region is being replaced is soft-deleted or
/// hard-deleted, then the replacement request will be transitioned along the
/// states to Complete while avoiding operations that are meant to operate on
/// that volume. If the volume is soft-deleted or hard-deleted while the
/// replacement request is in the "Requested" state, the replacement request
/// will transition straight to Complete, and no operations will be performed.
///
/// See also: RegionReplacementStep records
#[derive(
Queryable,
Expand Down
71 changes: 70 additions & 1 deletion nexus/db-queries/src/db/datastore/region_replacement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -657,7 +657,7 @@ impl DataStore {

/// Transition a RegionReplacement record from Completing to Complete,
/// clearing the operating saga id. Also removes the `volume_repair` record
/// that is taking a "lock" on the Volume.
/// that is taking a lock on the Volume.
pub async fn set_region_replacement_complete(
&self,
opctx: &OpContext,
Expand Down Expand Up @@ -723,6 +723,75 @@ impl DataStore {
})
}

/// Transition a RegionReplacement record from Requested to Complete, which
/// occurs when the associated volume is soft or hard deleted. Also removes
/// the `volume_repair` record that is taking a lock on the Volume.
pub async fn set_region_replacement_complete_from_requested(
&self,
opctx: &OpContext,
request: RegionReplacement,
) -> Result<(), Error> {
type TxnError = TransactionError<Error>;

assert_eq!(
request.replacement_state,
RegionReplacementState::Requested,
);

self.pool_connection_authorized(opctx)
.await?
.transaction_async(|conn| async move {
Self::volume_repair_delete_query(
request.volume_id,
request.id,
)
.execute_async(&conn)
.await?;

use db::schema::region_replacement::dsl;

let result = diesel::update(dsl::region_replacement)
.filter(dsl::id.eq(request.id))
.filter(
dsl::replacement_state.eq(RegionReplacementState::Requested),
)
.filter(dsl::operating_saga_id.is_null())
.set((
dsl::replacement_state.eq(RegionReplacementState::Complete),
))
.check_if_exists::<RegionReplacement>(request.id)
.execute_and_check(&conn)
.await?;

match result.status {
UpdateStatus::Updated => Ok(()),

UpdateStatus::NotUpdatedButExists => {
let record = result.found;

if record.replacement_state == RegionReplacementState::Complete {
Ok(())
} else {
Err(TxnError::CustomError(Error::conflict(format!(
"region replacement {} set to {:?} (operating saga id {:?})",
request.id,
record.replacement_state,
record.operating_saga_id,
))))
}
}
}
})
.await
.map_err(|e| match e {
TxnError::CustomError(error) => error,

TxnError::Database(error) => {
public_error_from_diesel(error, ErrorHandler::Server)
}
})
}

/// Nexus has been notified by an Upstairs (or has otherwised determined)
/// that a region replacement is done, so update the record. Filter on the
/// following:
Expand Down
8 changes: 8 additions & 0 deletions nexus/db-queries/src/db/datastore/volume.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1572,6 +1572,14 @@ impl DataStore {
.optional()
.map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))
}

/// Return true if a volume was soft-deleted or hard-deleted
pub async fn volume_deleted(&self, volume_id: Uuid) -> Result<bool, Error> {
match self.volume_get(volume_id).await? {
Some(v) => Ok(v.time_deleted.is_some()),
None => Ok(true),
}
}
}

#[derive(Default, Clone, Debug, Serialize, Deserialize)]
Expand Down
71 changes: 71 additions & 0 deletions nexus/src/app/background/tasks/region_replacement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,8 +170,79 @@ impl BackgroundTask for RegionReplacementDetector {
};

for request in requests {
// If the replacement request is in the `requested` state and
// the request's volume was soft-deleted or hard-deleted, avoid
// sending the start request and instead transition the request
// to completed

let volume_deleted = match self
.datastore
.volume_deleted(request.volume_id)
.await
{
Ok(volume_deleted) => volume_deleted,

Err(e) => {
let s = format!(
"error checking if volume id {} was deleted: {e}",
request.volume_id,
);
error!(&log, "{s}");

status.errors.push(s);
continue;
}
};

let request_id = request.id;

if volume_deleted {
// Volume was soft or hard deleted, so proceed with clean
// up, which if this is in state Requested there won't be
// any additional associated state, so transition the record
// to Completed.

info!(
&log,
"request {} volume {} was soft or hard deleted!",
request_id,
request.volume_id,
);

let result = self
.datastore
.set_region_replacement_complete_from_requested(
opctx, request,
)
.await;

match result {
Ok(()) => {
let s = format!(
"request {} transitioned from requested to \
complete",
request_id,
);

info!(&log, "{s}");
status.requests_completed_ok.push(s);
}

Err(e) => {
let s = format!(
"error transitioning {} from requested to \
complete: {e}",
request_id,
);

error!(&log, "{s}");
status.errors.push(s);
}
}

continue;
}

let result = self
.send_start_request(
authn::saga::Serialized::for_opctx(opctx),
Expand Down
26 changes: 26 additions & 0 deletions nexus/src/app/sagas/region_replacement_drive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,32 @@ async fn srrd_drive_region_replacement_check(
&params.serialized_authn,
);

// It doesn't make sense to perform any of this saga if the volume was soft
// or hard deleted: for example, this happens if the higher level resource
// like the disk was deleted. Volume deletion potentially results in the
// clean-up of Crucible resources, so it wouldn't even be valid to attempt
// to drive forward any type of live repair or reconciliation.
//
// Setting Done here will cause this saga to transition the replacement
// request to ReplacementDone.

let volume_deleted = osagactx
.datastore()
.volume_deleted(params.request.volume_id)
.await
.map_err(ActionError::action_failed)?;

if volume_deleted {
info!(
log,
"volume was soft or hard deleted!";
"region replacement id" => %params.request.id,
"volume id" => %params.request.volume_id,
);

return Ok(DriveCheck::Done);
}

let last_request_step = osagactx
.datastore()
.current_region_replacement_request_step(&opctx, params.request.id)
Expand Down
Loading

0 comments on commit 4c83c6d

Please sign in to comment.