From dbf6de8ad466dc9012320527121cb07743f70f80 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Tue, 28 Jul 2026 15:46:54 +0000 Subject: [PATCH 1/2] Perform volume and local storage delete in tasks Disk, snapshot, and image deletes have historically been the source of a lot of problems, especially when hardware is flaky. The whole associated delete saga had to complete before any of the endpoints would return an OK message, and they would hang if the saga was in a retry loop, often until an expungement occurred that would unblock said retry loop. Even worse was if the "delete freed regions" step was the one that was blocked, as that would hang _any and every_ delete of a volume until resolved. In accordance with the guidance in RFD 704, we're looking at replacing problematic sagas with one or more background tasks. In particular, the volume delete saga _had_ to go. The whole saga was comprised of nodes that only have forward actions, so it was an ideal candidate for this conversion, and sagas that are like this don't make much sense in the first place. Now all the disk delete saga does is: - soft-delete the disk record - update the virtual provisioning accounting - if the disk was backed by crucible, soft-delete the associated volume Note each of these steps could still fail due to transient database issues, but those issues are less likely than the ones caused by either physical disk related issues or software bugs (like the recent Pantry lock-up). The snapshot and image delete sagas were similarly modified: wherever the volume delete sub saga was embedded, instead soft-delete the volume and let the background task handle it. The disk delete saga also recently grown the ability to clean up local storage backed disks, and this was also moved into a different background task. Occasionally there were issues (like if the `dataset is busy` error was returned) that would cause those local storage backed disk deletes to unwind as well, where a rerun would succeed. Note that merging this commit will cause a behaviour change: today, the disk / snapshot / image delete API calls do not return until all the associated resources are cleaned up (in the case of Crucible backed disks, cleaned up as much as possible). After this commit is merged, it will be possible to delete a disk, quickly create another one, and see an INSUFFICIENT_STORAGE error, because the background task that would have cleaned up either the regions or the local storage allocation hasn't fired yet. It would be incorrect to spin in the disk / snapshot / image delete endpoints as that would lead us into the same problems we had before, so this commit simply returns when the associated saga completes. The relevant clean-up background task is activated after each (now much shorter) delete saga in an attempt to close this window. --- dev-tools/omdb/src/bin/omdb/db.rs | 6 +- dev-tools/omdb/src/bin/omdb/nexus.rs | 83 ++ dev-tools/omdb/tests/env.out | 24 + dev-tools/omdb/tests/successes.out | 44 + nexus-config/src/nexus_config.rs | 32 + nexus/background-task-interface/src/init.rs | 2 + nexus/db-queries/src/db/datastore/disk.rs | 14 +- .../src/db/datastore/local_storage.rs | 59 + nexus/db-queries/src/db/datastore/volume.rs | 34 + nexus/examples/config-second.toml | 2 + nexus/examples/config.toml | 2 + nexus/src/app/background/init.rs | 26 + .../background/tasks/local_storage_delete.rs | 276 ++++ nexus/src/app/background/tasks/mod.rs | 2 + .../src/app/background/tasks/volume_delete.rs | 1225 +++++++++++++++++ nexus/src/app/disk.rs | 14 +- nexus/src/app/image.rs | 2 + nexus/src/app/sagas/disk_create.rs | 6 + nexus/src/app/sagas/disk_delete.rs | 200 +-- nexus/src/app/sagas/image_delete.rs | 57 +- nexus/src/app/sagas/mod.rs | 2 - .../app/sagas/region_replacement_finish.rs | 70 +- .../region_snapshot_replacement_finish.rs | 73 +- ...on_snapshot_replacement_garbage_collect.rs | 69 +- ...apshot_replacement_step_garbage_collect.rs | 73 +- nexus/src/app/sagas/snapshot_create.rs | 5 + nexus/src/app/sagas/snapshot_delete.rs | 107 +- nexus/src/app/sagas/volume_delete.rs | 541 -------- nexus/src/app/sagas/volume_remove_rop.rs | 62 +- nexus/src/app/snapshot.rs | 2 + nexus/test-utils/src/background.rs | 172 ++- nexus/tests/config.test.toml | 2 + nexus/tests/integration_tests/common.rs | 18 + .../crucible_replacements.rs | 84 +- nexus/tests/integration_tests/disks.rs | 124 +- nexus/tests/integration_tests/mod.rs | 1 + nexus/tests/integration_tests/snapshots.rs | 3 +- .../integration_tests/volume_management.rs | 50 +- nexus/types/src/internal_api/background.rs | 18 + smf/nexus/multi-sled/config-partial.toml | 2 + smf/nexus/single-sled/config-partial.toml | 2 + 41 files changed, 2477 insertions(+), 1113 deletions(-) create mode 100644 nexus/src/app/background/tasks/local_storage_delete.rs create mode 100644 nexus/src/app/background/tasks/volume_delete.rs delete mode 100644 nexus/src/app/sagas/volume_delete.rs create mode 100644 nexus/tests/integration_tests/common.rs diff --git a/dev-tools/omdb/src/bin/omdb/db.rs b/dev-tools/omdb/src/bin/omdb/db.rs index d2cdf2ddb56..717c34c4d83 100644 --- a/dev-tools/omdb/src/bin/omdb/db.rs +++ b/dev-tools/omdb/src/bin/omdb/db.rs @@ -2645,11 +2645,11 @@ async fn cmd_db_disk_info( datastore: &DataStore, args: &DiskInfoArgs, ) -> Result<(), anyhow::Error> { + let conn = datastore.pool_connection_for_tests().await?; + let disk = { use nexus_db_schema::schema::disk::dsl; - let conn = datastore.pool_connection_for_tests().await?; - dsl::disk .filter(dsl::id.eq(args.uuid)) .select(nexus_db_model::Disk::as_select()) @@ -2658,7 +2658,7 @@ async fn cmd_db_disk_info( .context("failed to find disk")? }; - match datastore.disk_get_with_model(opctx, disk).await? { + match datastore.disk_get_with_model(&conn, disk).await? { Disk::Crucible(disk) => { crucible_disk_info(opctx, datastore, disk).await } diff --git a/dev-tools/omdb/src/bin/omdb/nexus.rs b/dev-tools/omdb/src/bin/omdb/nexus.rs index a8eaeae3c91..d3078823712 100644 --- a/dev-tools/omdb/src/bin/omdb/nexus.rs +++ b/dev-tools/omdb/src/bin/omdb/nexus.rs @@ -65,6 +65,7 @@ use nexus_types::internal_api::background::IncompleteBootstoreConfigReport; use nexus_types::internal_api::background::InstanceReincarnationStatus; use nexus_types::internal_api::background::InstanceUpdaterStatus; use nexus_types::internal_api::background::InventoryLoadStatus; +use nexus_types::internal_api::background::LocalStorageDeleteStatus; use nexus_types::internal_api::background::LookupRegionPortStatus; use nexus_types::internal_api::background::PhysicalDiskAdoptionStatus; use nexus_types::internal_api::background::ProbeDistributorStatus; @@ -90,6 +91,7 @@ use nexus_types::internal_api::background::TufArtifactReplicationCounters; use nexus_types::internal_api::background::TufArtifactReplicationRequest; use nexus_types::internal_api::background::TufArtifactReplicationStatus; use nexus_types::internal_api::background::TufRepoPrunerStatus; +use nexus_types::internal_api::background::VolumeDeleteStatus; use nexus_types::internal_api::background::fm_rendezvous; use omicron_uuid_kinds::BlueprintUuid; use omicron_uuid_kinds::CollectionUuid; @@ -1400,6 +1402,12 @@ fn print_task_details( "switch_port_config_manager" => { print_task_switch_port_settings_manager(details); } + "volume_delete" => { + print_task_volume_delete(details); + } + "local_storage_delete" => { + print_task_local_storage_delete(details); + } _ => { println!( "warning: unknown background task: {:?} \ @@ -4198,6 +4206,81 @@ fn print_task_physical_disk_adoption(details: &serde_json::Value) { } } +fn print_task_volume_delete(details: &serde_json::Value) { + match serde_json::from_value::(details.clone()) { + Err(error) => eprintln!( + "warning: failed to interpret task details: {:?}: {:?}", + error, details + ), + + Ok(status) => { + let VolumeDeleteStatus { + region_results, + running_snapshot_results, + snapshot_results, + volumes_deleted, + errors, + } = &status; + + println!(" result of deleting regions:"); + for result in region_results { + println!(" > {result}"); + } + + println!(" result of deleting running snapshots:"); + for result in running_snapshot_results { + println!(" > {result}"); + } + + println!(" result of deleting snapshots:"); + for result in snapshot_results { + println!(" > {result}"); + } + + println!(" volumes deleted:"); + for id in volumes_deleted { + println!(" > {id}"); + } + + println!(" errors: {}", errors.len()); + for error in errors { + println!(" > {error}"); + } + } + } +} + +fn print_task_local_storage_delete(details: &serde_json::Value) { + match serde_json::from_value::(details.clone()) { + Err(error) => eprintln!( + "warning: failed to interpret task details: {:?}: {:?}", + error, details + ), + + Ok(status) => { + let LocalStorageDeleteStatus { + delete_results, + deallocate_results, + errors, + } = &status; + + println!(" result of deleting local storage:"); + for result in delete_results { + println!(" > {result}"); + } + + println!(" result of deallocating local storage:"); + for result in deallocate_results { + println!(" > {result}"); + } + + println!(" errors: {}", errors.len()); + for error in errors { + println!(" > {error}"); + } + } + } +} const ERRICON: &str = "/!\\"; fn warn_if_nonzero(n: usize) -> &'static str { diff --git a/dev-tools/omdb/tests/env.out b/dev-tools/omdb/tests/env.out index 4712e4f3343..b07756ed122 100644 --- a/dev-tools/omdb/tests/env.out +++ b/dev-tools/omdb/tests/env.out @@ -155,6 +155,10 @@ task: "inventory_loader" loads the latest inventory collection from the DB +task: "local_storage_delete" + delete resources for disks backed by local storage + + task: "lookup_region_port" fill in missing ports for region records @@ -268,6 +272,10 @@ task: "v2p_manager" manages opte v2p mappings for vpc networking +task: "volume_delete" + delete resources from soft-deleted volumes + + task: "vpc_route_manager" propagates updated VPC routes to all OPTE ports @@ -428,6 +436,10 @@ task: "inventory_loader" loads the latest inventory collection from the DB +task: "local_storage_delete" + delete resources for disks backed by local storage + + task: "lookup_region_port" fill in missing ports for region records @@ -541,6 +553,10 @@ task: "v2p_manager" manages opte v2p mappings for vpc networking +task: "volume_delete" + delete resources from soft-deleted volumes + + task: "vpc_route_manager" propagates updated VPC routes to all OPTE ports @@ -688,6 +704,10 @@ task: "inventory_loader" loads the latest inventory collection from the DB +task: "local_storage_delete" + delete resources for disks backed by local storage + + task: "lookup_region_port" fill in missing ports for region records @@ -801,6 +821,10 @@ task: "v2p_manager" manages opte v2p mappings for vpc networking +task: "volume_delete" + delete resources from soft-deleted volumes + + task: "vpc_route_manager" propagates updated VPC routes to all OPTE ports diff --git a/dev-tools/omdb/tests/successes.out b/dev-tools/omdb/tests/successes.out index 27dbe0059db..f0c276e9678 100644 --- a/dev-tools/omdb/tests/successes.out +++ b/dev-tools/omdb/tests/successes.out @@ -390,6 +390,10 @@ task: "inventory_loader" loads the latest inventory collection from the DB +task: "local_storage_delete" + delete resources for disks backed by local storage + + task: "lookup_region_port" fill in missing ports for region records @@ -503,6 +507,10 @@ task: "v2p_manager" manages opte v2p mappings for vpc networking +task: "volume_delete" + delete resources from soft-deleted volumes + + task: "vpc_route_manager" propagates updated VPC routes to all OPTE ports @@ -863,6 +871,14 @@ task: "inventory_loader" loaded latest inventory collection as of : collection ....................., taken at +task: "local_storage_delete" + configured period: every h m s + last completed activation: , triggered by + started at (s ago) and ran for ms + result of deleting local storage: + result of deallocating local storage: + errors: 0 + task: "lookup_region_port" configured period: every m last completed activation: , triggered by @@ -1096,6 +1112,16 @@ task: "v2p_manager" started at (s ago) and ran for ms warning: unknown background task: "v2p_manager" (don't know how to interpret details: Object {}) +task: "volume_delete" + configured period: every h m s + last completed activation: , triggered by + started at (s ago) and ran for ms + result of deleting regions: + result of deleting running snapshots: + result of deleting snapshots: + volumes deleted: + errors: 0 + task: "vpc_route_manager" configured period: every s last completed activation: , triggered by @@ -1585,6 +1611,14 @@ task: "inventory_loader" loaded latest inventory collection as of : collection ....................., taken at +task: "local_storage_delete" + configured period: every h m s + last completed activation: , triggered by + started at (s ago) and ran for ms + result of deleting local storage: + result of deallocating local storage: + errors: 0 + task: "lookup_region_port" configured period: every m last completed activation: , triggered by @@ -1818,6 +1852,16 @@ task: "v2p_manager" started at (s ago) and ran for ms warning: unknown background task: "v2p_manager" (don't know how to interpret details: Object {}) +task: "volume_delete" + configured period: every h m s + last completed activation: , triggered by + started at (s ago) and ran for ms + result of deleting regions: + result of deleting running snapshots: + result of deleting snapshots: + volumes deleted: + errors: 0 + task: "vpc_route_manager" configured period: every s last completed activation: , triggered by diff --git a/nexus-config/src/nexus_config.rs b/nexus-config/src/nexus_config.rs index 4711aeb78ba..27ef8c97e23 100644 --- a/nexus-config/src/nexus_config.rs +++ b/nexus-config/src/nexus_config.rs @@ -481,6 +481,10 @@ pub struct BackgroundTaskConfig { pub audit_log_cleanup: AuditLogCleanupConfig, /// configuration for populate switch ports task pub populate_switch_ports: PopulateSwitchPortsConfig, + /// configuration for volume delete task + pub volume_delete: VolumeDeleteConfig, + /// configuration for local storage delete task + pub local_storage_delete: LocalStorageDeleteConfig, } #[serde_as] @@ -1108,6 +1112,22 @@ pub struct TrustQuorumConfig { pub period_secs: Duration, } +#[serde_as] +#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] +pub struct VolumeDeleteConfig { + /// period (in seconds) for periodic activations of this background task + #[serde_as(as = "DurationSeconds")] + pub period_secs: Duration, +} + +#[serde_as] +#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] +pub struct LocalStorageDeleteConfig { + /// period (in seconds) for periodic activations of this background task + #[serde_as(as = "DurationSeconds")] + pub period_secs: Duration, +} + /// Configuration for a nexus server #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] pub struct PackageConfig { @@ -1399,6 +1419,8 @@ mod test { audit_log_cleanup.retention_days = 90 audit_log_cleanup.max_deleted_per_activation = 10000 populate_switch_ports.period_secs = 31 + volume_delete.period_secs = 30 + local_storage_delete.period_secs = 30 [default_region_allocation_strategy] type = "random" seed = 0 @@ -1685,6 +1707,14 @@ mod test { populate_switch_ports: PopulateSwitchPortsConfig { period_secs: Duration::from_secs(31), }, + volume_delete: + VolumeDeleteConfig { + period_secs: Duration::from_secs(30), + }, + local_storage_delete: + LocalStorageDeleteConfig { + period_secs: Duration::from_secs(30), + }, }, multicast: MulticastConfig { enabled: false }, default_region_allocation_strategy: @@ -1802,6 +1832,8 @@ mod test { audit_log_cleanup.retention_days = 90 audit_log_cleanup.max_deleted_per_activation = 10000 populate_switch_ports.period_secs = 31 + volume_delete.period_secs = 30 + local_storage_delete.period_secs = 30 [default_region_allocation_strategy] type = "random" diff --git a/nexus/background-task-interface/src/init.rs b/nexus/background-task-interface/src/init.rs index 197bc057bec..8e0ee20036f 100644 --- a/nexus/background-task-interface/src/init.rs +++ b/nexus/background-task-interface/src/init.rs @@ -64,6 +64,8 @@ pub struct BackgroundTasks { pub task_attached_subnet_manager: Activator, pub task_session_cleanup: Activator, pub task_populate_switch_ports: Activator, + pub task_volume_delete: Activator, + pub task_local_storage_delete: Activator, // Handles to activate background tasks that do not get used by Nexus // at-large. These background tasks are implementation details as far as diff --git a/nexus/db-queries/src/db/datastore/disk.rs b/nexus/db-queries/src/db/datastore/disk.rs index f1fb522c526..921654a9df3 100644 --- a/nexus/db-queries/src/db/datastore/disk.rs +++ b/nexus/db-queries/src/db/datastore/disk.rs @@ -500,7 +500,9 @@ impl DataStore { let (.., disk) = LookupPath::new(opctx, self).disk_id(disk_id).fetch().await?; - self.disk_get_with_model(opctx, disk).await + let conn = self.pool_connection_authorized(opctx).await?; + + self.disk_get_with_model(&conn, disk).await } /// Return a `datastore::Disk` given a `model::Disk` @@ -508,15 +510,15 @@ impl DataStore { /// Note: basically all of Nexus should _not_ be using this, and should be /// using `disk_get` instead: this version of the function bypasses the /// LookupPath induced permissions check and should only called from omdb. + /// Code that is looking up deleted disks should also use this method, as + /// `LookupPath` will not return deleted resources. pub async fn disk_get_with_model( &self, - opctx: &OpContext, + conn: &async_bb8_diesel::Connection, disk: model::Disk, ) -> LookupResult { let disk_id = disk.id(); - let conn = self.pool_connection_authorized(opctx).await?; - let disk = match disk.disk_type { db::model::DiskType::Crucible => { use nexus_db_schema::schema::disk_type_crucible::dsl; @@ -524,7 +526,7 @@ impl DataStore { let disk_type_crucible = dsl::disk_type_crucible .filter(dsl::disk_id.eq(disk_id)) .select(DiskTypeCrucible::as_select()) - .first_async(&*conn) + .first_async(conn) .await .map_err(|e| { public_error_from_diesel(e, ErrorHandler::Server) @@ -542,7 +544,7 @@ impl DataStore { let disk_type_local_storage = dsl::disk_type_local_storage .filter(dsl::disk_id.eq(disk_id)) .select(DiskTypeLocalStorage::as_select()) - .first_async(&*conn) + .first_async(conn) .await .map_err(|e| { public_error_from_diesel(e, ErrorHandler::Server) diff --git a/nexus/db-queries/src/db/datastore/local_storage.rs b/nexus/db-queries/src/db/datastore/local_storage.rs index 7ba521d2807..0385a2e29aa 100644 --- a/nexus/db-queries/src/db/datastore/local_storage.rs +++ b/nexus/db-queries/src/db/datastore/local_storage.rs @@ -9,10 +9,12 @@ use crate::authz; use crate::context::OpContext; use crate::db::collection_insert::AsyncInsertError; use crate::db::collection_insert::DatastoreCollection; +use crate::db::datastore; use crate::db::datastore::DbConnection; use crate::db::datastore::LocalStorageAllocation; use crate::db::datastore::LocalStorageDisk; use crate::db::datastore::SQL_BATCH_SIZE; +use crate::db::model; use crate::db::model::LocalStorageDatasetAllocation; use crate::db::model::LocalStorageUnencryptedDatasetAllocation; use crate::db::model::RendezvousLocalStorageDataset; @@ -451,4 +453,61 @@ impl DataStore { }) .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } + + /// Return all deleted disks that have undeleted local storage allocations + pub async fn deleted_disks_with_undeleted_local_storage( + &self, + opctx: &OpContext, + ) -> Result, Error> { + opctx.check_complex_operations_allowed()?; + + let conn = self.pool_connection_authorized(opctx).await?; + + use nexus_db_schema::schema::disk::dsl; + use nexus_db_schema::schema::disk_type_local_storage::dsl as dtls_dsl; + use nexus_db_schema::schema::local_storage_unencrypted_dataset_allocation::dsl as lsuda_dsl; + + // Find all deleted disks where the unencrypted local storage allocation + // is not yet deleted. + let found_disks: Vec = dsl::disk + .inner_join( + dtls_dsl::disk_type_local_storage + .on(dsl::id.eq(dtls_dsl::disk_id)), + ) + .inner_join( + lsuda_dsl::local_storage_unencrypted_dataset_allocation.on( + dtls_dsl::local_storage_unencrypted_dataset_allocation_id + .eq(lsuda_dsl::id.nullable()), + ), + ) + .filter(lsuda_dsl::time_deleted.is_null()) + .filter(dsl::time_deleted.is_not_null()) + .select(model::Disk::as_select()) + .load_async(&*conn) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + + let mut disks = Vec::with_capacity(found_disks.len()); + + for found_disk in found_disks { + match self.disk_get_with_model(&conn, found_disk).await? { + datastore::Disk::Crucible(crucible_disk) => { + // The query above joins the disk table with the + // disk_type_local_storage table, meaning the higher level + // Disk can never be the Crucible type, unless there's a + // serious problem. Return an error instead of panicking. + return Err(Error::internal_error(&format!( + "disk {} should be the local storage, not crucible", + crucible_disk.id(), + ))); + } + + datastore::Disk::LocalStorage(local_storage_disk) => { + disks.push(local_storage_disk); + } + } + } + + Ok(disks) + } } diff --git a/nexus/db-queries/src/db/datastore/volume.rs b/nexus/db-queries/src/db/datastore/volume.rs index a3192f69f31..a10c708551f 100644 --- a/nexus/db-queries/src/db/datastore/volume.rs +++ b/nexus/db-queries/src/db/datastore/volume.rs @@ -1765,6 +1765,40 @@ impl DataStore { }) } + pub async fn get_soft_deleted_volumes( + &self, + opctx: &OpContext, + ) -> ListResultVec { + opctx.check_complex_operations_allowed()?; + + let mut volumes = Vec::new(); + let mut paginator = Paginator::new( + SQL_BATCH_SIZE, + dropshot::PaginationOrder::Ascending, + ); + let conn = self.pool_connection_authorized(opctx).await?; + + while let Some(p) = paginator.next() { + use nexus_db_schema::schema::volume::dsl; + + let mut page = + paginated(dsl::volume, dsl::id, &p.current_pagparams()) + .filter(dsl::time_deleted.is_not_null()) + .select(Volume::as_select()) + .get_results_async::(&*conn) + .await + .map_err(|e| { + public_error_from_diesel(e, ErrorHandler::Server) + })?; + + paginator = p.found_batch(&page, &|r| *r.id().as_untyped_uuid()); + + volumes.append(&mut page); + } + + Ok(volumes) + } + async fn volume_remove_rop_in_txn( conn: &async_bb8_diesel::Connection, err: OptionalError, diff --git a/nexus/examples/config-second.toml b/nexus/examples/config-second.toml index 166afa72ba0..c2af4bb9907 100644 --- a/nexus/examples/config-second.toml +++ b/nexus/examples/config-second.toml @@ -217,6 +217,8 @@ audit_log_cleanup.period_secs = 600 audit_log_cleanup.retention_days = 90 audit_log_cleanup.max_deleted_per_activation = 10000 populate_switch_ports.period_secs = 30 +volume_delete.period_secs = 30 +local_storage_delete.period_secs = 30 [default_region_allocation_strategy] # allocate region on 3 random distinct zpools, on 3 random distinct sleds. diff --git a/nexus/examples/config.toml b/nexus/examples/config.toml index eeb4fa52666..2848524d8e7 100644 --- a/nexus/examples/config.toml +++ b/nexus/examples/config.toml @@ -201,6 +201,8 @@ audit_log_cleanup.period_secs = 600 audit_log_cleanup.retention_days = 90 audit_log_cleanup.max_deleted_per_activation = 10000 populate_switch_ports.period_secs = 30 +volume_delete.period_secs = 30 +local_storage_delete.period_secs = 30 [default_region_allocation_strategy] # allocate region on 3 random distinct zpools, on 3 random distinct sleds. diff --git a/nexus/src/app/background/init.rs b/nexus/src/app/background/init.rs index 8233ea5db11..b990b06ed11 100644 --- a/nexus/src/app/background/init.rs +++ b/nexus/src/app/background/init.rs @@ -118,6 +118,7 @@ use super::tasks::instance_updater; use super::tasks::instance_watcher; use super::tasks::inventory_collection; use super::tasks::inventory_load; +use super::tasks::local_storage_delete::*; use super::tasks::lookup_region_port; use super::tasks::metrics_producer_gc; use super::tasks::multicast::MulticastGroupReconciler; @@ -143,6 +144,7 @@ use super::tasks::trust_quorum; use super::tasks::tuf_artifact_replication; use super::tasks::tuf_repo_pruner; use super::tasks::v2p_mappings::V2PManager; +use super::tasks::volume_delete::*; use super::tasks::vpc_routes; use super::tasks::webhook_deliverator; use crate::Nexus; @@ -282,6 +284,8 @@ impl BackgroundTasksInitializer { task_attached_subnet_manager: Activator::new(), task_session_cleanup: Activator::new(), task_populate_switch_ports: Activator::new(), + task_volume_delete: Activator::new(), + task_local_storage_delete: Activator::new(), // Handles to activate background tasks that do not get used by Nexus // at-large. These background tasks are implementation details as far as @@ -379,6 +383,8 @@ impl BackgroundTasksInitializer { task_audit_log_timeout_incomplete, task_audit_log_cleanup, task_populate_switch_ports, + task_volume_delete, + task_local_storage_delete, // Add new background tasks here. Be sure to use this binding in a // call to `Driver::register()` below. That's what actually wires // up the Activator to the corresponding background task. @@ -1328,6 +1334,26 @@ impl BackgroundTasksInitializer { activator: task_populate_switch_ports, }); + driver.register(TaskDefinition { + name: "volume_delete", + description: "delete resources from soft-deleted volumes", + period: config.volume_delete.period_secs, + task_impl: Box::new(VolumeDeleter::new(datastore.clone())), + opctx: opctx.child(BTreeMap::new()), + watchers: vec![], + activator: task_volume_delete, + }); + + driver.register(TaskDefinition { + name: "local_storage_delete", + description: "delete resources for disks backed by local storage", + period: config.local_storage_delete.period_secs, + task_impl: Box::new(LocalStorageDeleter::new(datastore.clone())), + opctx: opctx.child(BTreeMap::new()), + watchers: vec![], + activator: task_local_storage_delete, + }); + driver } } diff --git a/nexus/src/app/background/tasks/local_storage_delete.rs b/nexus/src/app/background/tasks/local_storage_delete.rs new file mode 100644 index 00000000000..7898fa8516f --- /dev/null +++ b/nexus/src/app/background/tasks/local_storage_delete.rs @@ -0,0 +1,276 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! When the higher level disks backed by local storage are deleted, this +//! background task will delete any allocated local storage, and will delete +//! local storage allocation records when those resources have been cleand up. + +use crate::app::background::BackgroundTask; +use futures::FutureExt; +use futures::future::BoxFuture; +use nexus_db_queries::context::OpContext; +use nexus_db_queries::db::DataStore; +use nexus_db_queries::db::datastore::LocalStorageAllocation; +use nexus_db_queries::db::model::LocalStorageUnencryptedDatasetAllocation; +use nexus_types::internal_api::background::LocalStorageDeleteStatus; +use serde_json::json; +use sled_agent_client::types::LocalStorageDatasetDeleteRequest; +use slog::Logger; +use std::sync::Arc; + +pub struct LocalStorageDeleter { + datastore: Arc, + reqwest_client: reqwest::Client, +} + +/// Functions that poll for an expected change will either return that the +/// change occurred, or that the current activation of this task has to wait for +/// the change to occur in a future activations of this task. +#[derive(PartialEq)] +enum DeleteResult { + Deleted, + + WaitForNextActivation, +} + +impl LocalStorageDeleter { + pub fn new(datastore: Arc) -> Self { + LocalStorageDeleter { + datastore, + reqwest_client: reqwest::Client::new(), + } + } + + async fn delete_unencrypted_allocation( + &self, + log: &Logger, + opctx: &OpContext, + allocation: &LocalStorageUnencryptedDatasetAllocation, + status: &mut LocalStorageDeleteStatus, + ) -> DeleteResult { + let sled_id = allocation.sled_id(); + let zpool_id = allocation.pool_id().upcast(); + + // Check if either the sled or disk backing the zpool was expunged. If + // we can't determine then bail and wait for the next task activation. + + let sled_in_service = + match self.datastore.check_sled_in_service(&opctx, sled_id).await { + Ok(sled_in_service) => sled_in_service, + + Err(e) => { + let s = format!( + "error calling check_sled_in_service for sled \ + {sled_id}: {e}", + ); + + error!(log, "{s}"); + status.errors.push(s); + + return DeleteResult::WaitForNextActivation; + } + }; + + if !sled_in_service { + // Sled's been expunged, so consider the local storage deleted. + return DeleteResult::Deleted; + } + + let zpool_in_service = + match self.datastore.check_zpool_in_service(&opctx, zpool_id).await + { + Ok(zpool_in_service) => zpool_in_service, + + Err(e) => { + let s = format!( + "error calling check_zpool_in_service for zpool \ + {zpool_id}: {e}", + ); + + error!(log, "{s}"); + status.errors.push(s); + + return DeleteResult::WaitForNextActivation; + } + }; + + if !zpool_in_service { + // The disk backing the zpool's been expunged, so consider the local + // storage deleted. + return DeleteResult::Deleted; + } + + // Now that all checks are done, get a sled agent client and make the + // delete request. + + let request = LocalStorageDatasetDeleteRequest { + zpool_id: allocation.pool_id(), + dataset_id: allocation.id(), + encrypted_at_rest: false, + }; + + let sled_agent_client = match nexus_networking::sled_client_ext( + &self.datastore, + opctx, + sled_id, + log, + self.reqwest_client.clone(), + ) + .await + { + Ok(client) => client, + + Err(e) => { + let s = format!( + "error calling sled_client_ext for allocation {}: {e}", + allocation.id(), + ); + + error!(log, "{s}"); + status.errors.push(s); + + return DeleteResult::WaitForNextActivation; + } + }; + + match sled_agent_client.local_storage_dataset_delete(&request).await { + Ok(_) => DeleteResult::Deleted, + + Err(e) => { + let s = + format!("error sending local_storage_dataset_delete: {e}"); + + error!(log, "{s}"); + status.errors.push(s); + + DeleteResult::WaitForNextActivation + } + } + } +} + +impl BackgroundTask for LocalStorageDeleter { + fn activate<'a>( + &'a mut self, + opctx: &'a OpContext, + ) -> BoxFuture<'a, serde_json::Value> { + async { + let log = &opctx.log; + let mut status = LocalStorageDeleteStatus::default(); + + let disks_needing_clean_up = match self + .datastore + .deleted_disks_with_undeleted_local_storage(opctx) + .await + { + Ok(v) => v, + + Err(e) => { + let s = format!( + "error calling \ + unencrypted_allocations_for_deleted_disks: {e}" + ); + + error!(log, "{s}"); + status.errors.push(s); + + return json!(status); + } + }; + + for disk in disks_needing_clean_up { + let Some(allocation) = &disk.local_storage_dataset_allocation + else { + // No allocation was made for this disk + continue; + }; + + // Attempt deleting the local storage before removing the + // database record. If the delete does not succeed, try again in + // the next task activation. + + match allocation { + LocalStorageAllocation::Unencrypted(allocation) => { + match self + .delete_unencrypted_allocation( + log, + opctx, + &allocation, + &mut status, + ) + .await + { + DeleteResult::Deleted => { + let s = format!( + "deleted disk {} allocation {}", + disk.id(), + allocation.id(), + ); + + info!(log, "{s}"); + status.delete_results.push(s); + + // Drop through to deallocation once deletion + // succeeds. + } + + DeleteResult::WaitForNextActivation => { + // Cannot deallocate the record until deletion + // succeeds. + continue; + } + } + } + + LocalStorageAllocation::Encrypted(allocation) => { + // Until encrypted local storage is supported, seeing a + // request to clean up disks of that type should be + // noted as a error. + let s = format!( + "request to delete disk {} encrypted allocation {}", + disk.id(), + allocation.id(), + ); + + error!(log, "{s}"); + status.errors.push(s); + + continue; + } + } + + match self + .datastore + .delete_local_storage_dataset_allocations(opctx, &disk) + .await + { + Ok(()) => { + let s = format!( + "deallocated disk {} allocation {}", + disk.id(), + allocation.id(), + ); + info!(log, "{s}"); + status.deallocate_results.push(s); + } + + Err(e) => { + let s = format!( + "error calling \ + delete_local_storage_dataset_allocations: {e}" + ); + + error!(log, "{s}"); + status.errors.push(s); + + continue; + } + } + } + + json!(status) + } + .boxed() + } +} diff --git a/nexus/src/app/background/tasks/mod.rs b/nexus/src/app/background/tasks/mod.rs index 1058d0cd638..9213d36e96b 100644 --- a/nexus/src/app/background/tasks/mod.rs +++ b/nexus/src/app/background/tasks/mod.rs @@ -31,6 +31,7 @@ pub mod instance_updater; pub mod instance_watcher; pub mod inventory_collection; pub mod inventory_load; +pub mod local_storage_delete; pub mod lookup_region_port; pub mod metrics_producer_gc; pub mod multicast; @@ -58,5 +59,6 @@ pub mod trust_quorum; pub mod tuf_artifact_replication; pub mod tuf_repo_pruner; pub mod v2p_mappings; +pub mod volume_delete; pub mod vpc_routes; pub mod webhook_deliverator; diff --git a/nexus/src/app/background/tasks/volume_delete.rs b/nexus/src/app/background/tasks/volume_delete.rs new file mode 100644 index 00000000000..71a2b3e10d8 --- /dev/null +++ b/nexus/src/app/background/tasks/volume_delete.rs @@ -0,0 +1,1225 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +//! Nexus is responsible for telling Crucible Agent(s) when to clean up +//! resources - those Agents do not have any idea of what volumes are +//! constructed, currently active, etc. Plus, volumes can (and will) change +//! during their lifetime. Operations like growing a disk, removing a read-only +//! parent after a scrub has completed, or re-encrypting a disk will all change +//! the volume that backs a disk. +//! +//! Nexus has to account for all the Crucible resources it is using, and count +//! how many volumes are using those resources. Only when that count drops to +//! zero is it valid to clean up the appropriate Crucible resource. +//! +//! Complicating things is the fact that ZFS datasets cannot be deleted if there +//! are snapshots of that dataset. Nexus' resource accounting must take this +//! dependency into account. Note that ZFS snapshots can layer, but any snapshot +//! can be deleted without the requirement of (for example) deleting the +//! snapshots in a certain order. +//! +//! Multiple Nexus will be running this background task and will result in many +//! identical requests being sent to the downstream Crucible agents. Each +//! Crucible agent's endpoint is idempotent and should handle this without any +//! issues. + +use crate::app::background::BackgroundTask; +use crucible_agent_client::Client as CrucibleAgentClient; +use crucible_agent_client::types::GetSnapshotResponse; +use crucible_agent_client::types::Region; +use crucible_agent_client::types::RegionId; +use crucible_agent_client::types::State as RegionState; +use futures::FutureExt; +use futures::future::BoxFuture; +use nexus_db_queries::context::OpContext; +use nexus_db_queries::db::DataStore; +use nexus_db_queries::db::datastore::CrucibleResources; +use nexus_db_queries::db::datastore::FreedCrucibleResources; +use nexus_db_queries::db::model::CrucibleDataset; +use nexus_types::identity::Asset; +use nexus_types::internal_api::background::VolumeDeleteStatus; +use omicron_common::api::external::Error; +use omicron_uuid_kinds::DatasetUuid; +use omicron_uuid_kinds::VolumeUuid; +use serde_json::json; +use slog::Logger; +use std::sync::Arc; +use uuid::Uuid; + +pub struct VolumeDeleter { + datastore: Arc, + reqwest_client: reqwest::Client, +} + +/// Almost all endpoints that the Crucible Agent exposes are requests for +/// something asynchronous to occur. Functions that poll for the expected change +/// will either return that the change occurred, or that the current activation +/// of this task has to wait for the change to occur in a future activations of +/// this task. +#[derive(PartialEq)] +enum DeleteResult { + Deleted, + + WaitForNextActivation, +} + +// The functions in this impl can roughly be separated into two categories: +// +// 1. those that aceept a `status` argument and return a `DeleteResult` +// 2. those that do not accept a `status` argument and return a Result +// +// This separation attempts to prevent duplicates in the status' errors field. +// Functions that accept a status argument are responsible for adding to it, +// particularly adding to the errors field the Err results of functions that do +// not accept a status argument. Any function can write to the log. + +impl VolumeDeleter { + pub fn new(datastore: Arc) -> Self { + VolumeDeleter { datastore, reqwest_client: reqwest::Client::new() } + } + + fn crucible_agent_client_for_dataset( + &self, + dataset: &CrucibleDataset, + ) -> CrucibleAgentClient { + CrucibleAgentClient::new_with_client( + &format!("http://{}", dataset.address()), + self.reqwest_client.clone(), + ) + } + + /// Return true if the Crucible agent is gone, and false if it's expected to + /// be there and answer Nexus. + async fn crucible_agent_is_gone( + &self, + dataset_id: DatasetUuid, + ) -> Result { + let on_in_service_physical_disk = self + .datastore + .crucible_dataset_physical_disk_in_service(dataset_id) + .await?; + + let is_gone = !on_in_service_physical_disk; + + Ok(is_gone) + } + + /// Returns a Ok(Some(Region)) if a region with id {region_id} exists, + /// Ok(None) if it does not (a 404 was seen), and Err otherwise. + async fn maybe_get_crucible_region( + &self, + log: &Logger, + dataset: &CrucibleDataset, + region_id: Uuid, + ) -> Result, Error> { + let client = self.crucible_agent_client_for_dataset(dataset); + let dataset_id = dataset.id(); + + if self.crucible_agent_is_gone(dataset_id).await? { + return Err(Error::Gone); + } + + match client.region_get(&RegionId(region_id.to_string())).await { + Ok(v) => Ok(Some(v.into_inner())), + + Err(e) => { + if is_not_found(&e) { + // A 404 Not Found is ok for this function, just return None + Ok(None) + } else { + error!( + log, + "region_get saw {:?}", + e; + "region_id" => %region_id, + "dataset_id" => %dataset_id, + ); + + Err(into_external_error(&e)) + } + } + } + } + + /// Send a region deletion request + async fn request_crucible_region_delete( + &self, + log: &Logger, + dataset: &CrucibleDataset, + region_id: Uuid, + ) -> Result<(), Error> { + let client = self.crucible_agent_client_for_dataset(dataset); + let dataset_id = dataset.id(); + + if self.crucible_agent_is_gone(dataset_id).await? { + return Err(Error::Gone); + } + + match client.region_delete(&RegionId(region_id.to_string())).await { + Ok(_) => Ok(()), + + Err(e) => { + error!( + log, + "region_delete saw {:?}", + e; + "region_id" => %region_id, + "dataset_id" => %dataset.id(), + ); + + Err(into_external_error(&e)) + } + } + } + + /// Call out to a Crucible agent to delete a region. Poll to see if that + /// region's state has changed and indicated it has been deleted. + async fn delete_crucible_region( + &self, + log: &Logger, + dataset: &CrucibleDataset, + region_id: Uuid, + ) -> Result { + // If the region never existed, then a `GET` will return 404, and so + // will a `DELETE`. Catch this case, and return Ok if the region never + // existed. This can occur if ensuring all datasets and regions in a + // region set partially fails, and the entire region set is being + // deleted. + + match self.maybe_get_crucible_region(log, dataset, region_id).await { + Ok(Some(_)) => { + // region found, proceed with deleting + } + + Ok(None) => { + // region never exited, return Ok + return Ok(DeleteResult::Deleted); + } + + // Return Ok if the dataset's agent is gone, no delete call + // is required. + Err(Error::Gone) => { + warn!( + log, + "dataset is gone"; + "dataset_id" => %dataset.id(), + ); + + return Ok(DeleteResult::Deleted); + } + + Err(e) => return Err(e), + } + + // Past here, the region exists (or existed at some point): ensure it is + // deleted. Request the deletion (which is idempotent), then query for + // the appropriate state change. + + self.request_crucible_region_delete(log, dataset, region_id).await?; + + let region = + match self.maybe_get_crucible_region(log, dataset, region_id).await + { + // Previous to the delete request, we queried and found this + // region, and now it's gone. + Ok(None) => Err(Error::internal_error(format!( + "dataset {} region {region_id} is missing now!", + dataset.id(), + ))), + + Ok(Some(region)) => Ok(region), + + // Return Ok if the dataset's agent is gone, no state check is + // required. + Err(Error::Gone) => { + warn!( + log, + "dataset is gone"; + "dataset_id" => %dataset.id(), + ); + + return Ok(DeleteResult::Deleted); + } + + Err(e) => Err(e), + }?; + + // If this state change query shows that the region was deleted, + // proceed, otherwise the overall task needs to wait. + + match region.state { + RegionState::Tombstoned => Ok(DeleteResult::WaitForNextActivation), + + RegionState::Destroyed => Ok(DeleteResult::Deleted), + + RegionState::Failed => { + // If the delete failed, Nexus can re-request that the region be + // deleted, and it will move back to Tombstoned. This will occur + // on the next invocation of this task. + + Ok(DeleteResult::WaitForNextActivation) + } + + RegionState::Requested | RegionState::Created => { + // It's unexpected that the region be in this state after a + // deletion request. We successfully requested the region + // deletion before entering this check, and the Crucible agent + // should prevent the state transition from Tombstoned to either + // of these states. + + Err(Error::internal_error(format!( + "region is {:?} after successful deletion request!", + region.state, + ))) + } + } + } + + async fn delete_crucible_regions( + &self, + log: &Logger, + crucible_resources_to_delete: &CrucibleResources, + status: &mut VolumeDeleteStatus, + ) -> DeleteResult { + let datasets_and_regions = match self + .datastore + .regions_to_delete(&crucible_resources_to_delete) + .await + { + Ok(datasets_and_regions) => datasets_and_regions, + + Err(e) => { + let s = format!("error calling regions_to_delete: {e}"); + error!(log, "{s}"); + status.errors.push(s); + + // Try again next time! + return DeleteResult::WaitForNextActivation; + } + }; + + let request_count = datasets_and_regions.len(); + if request_count == 0 { + return DeleteResult::Deleted; + } + + // Send DELETE calls to the corresponding Crucible agents + let mut all_deleted = true; + + for (dataset, region) in &datasets_and_regions { + match self.delete_crucible_region(log, &dataset, region.id()).await + { + Ok(DeleteResult::Deleted) => { + status + .region_results + .push(format!("{}: deleted", region.id())); + } + + Ok(DeleteResult::WaitForNextActivation) => { + status.region_results.push(format!( + "{}: requested deletion, waiting", + region.id() + )); + + all_deleted = false; + } + + Err(e) => { + status.errors.push(format!("{e}")); + + all_deleted = false; + } + } + } + + // If any of the operations require waiting for the next activation, + // then return that, otherwise return that all were deleted. + + if all_deleted { + // When all crucible resources are cleaned up, hard delete the + // region records. This also re-computes the crucible_dataset + // size_used column for those region's datasets. + + let region_ids_to_delete = + datasets_and_regions.iter().map(|(_, r)| r.id()).collect(); + + match self + .datastore + .regions_hard_delete(log, region_ids_to_delete) + .await + { + Ok(()) => DeleteResult::Deleted, + + Err(e) => { + let s = format!("error calling regions_hard_delete: {e}"); + error!(log, "{s}"); + status.errors.push(s); + + // More work is required next task activation + DeleteResult::WaitForNextActivation + } + } + } else { + // More work is required next task activation + DeleteResult::WaitForNextActivation + } + } + + async fn request_crucible_running_snapshot_delete( + &self, + log: &Logger, + dataset: &CrucibleDataset, + region_id: Uuid, + snapshot_id: Uuid, + ) -> Result<(), Error> { + let client = self.crucible_agent_client_for_dataset(dataset); + let dataset_id = dataset.id(); + + if self.crucible_agent_is_gone(dataset_id).await? { + return Err(Error::Gone); + } + + match client + .region_delete_running_snapshot( + &RegionId(region_id.to_string()), + &snapshot_id.to_string(), + ) + .await + { + Ok(_) => Ok(()), + + Err(e) => { + error!( + log, + "region_delete_running_snapshot saw {:?}", + e; + "dataset_id" => %dataset.id(), + "region_id" => %region_id, + "snapshot_id" => %snapshot_id, + ); + + Err(into_external_error(&e)) + } + } + } + + async fn get_crucible_region_snapshots( + &self, + log: &Logger, + dataset: &CrucibleDataset, + region_id: Uuid, + ) -> Result { + let client = self.crucible_agent_client_for_dataset(dataset); + let dataset_id = dataset.id(); + + if self.crucible_agent_is_gone(dataset_id).await? { + return Err(Error::Gone); + } + + match client + .region_get_snapshots(&RegionId(region_id.to_string())) + .await + { + Ok(v) => Ok(v.into_inner()), + + Err(e) => { + error!( + log, + "region_get_snapshots saw {:?}", + e; + "dataset_id" => %dataset.id(), + "region_id" => %region_id, + ); + + Err(into_external_error(&e)) + } + } + } + + async fn delete_crucible_running_snapshot( + &self, + log: &Logger, + dataset: &CrucibleDataset, + region_id: Uuid, + snapshot_id: Uuid, + ) -> Result { + // Request the deletion (which is idempotent), then query for the + // appropriate state change. + + match self + .request_crucible_running_snapshot_delete( + log, + dataset, + region_id, + snapshot_id, + ) + .await + { + Ok(()) => { + // ok + } + + Err(Error::Gone) => { + warn!( + log, + "dataset is gone"; + "dataset_id" => %dataset.id(), + ); + + return Ok(DeleteResult::Deleted); + } + + Err(e) => { + return Err(e); + } + } + + let response = match self + .get_crucible_region_snapshots(log, dataset, region_id) + .await + { + Ok(v) => v, + + // Return Ok if the dataset's agent is gone, no + // delete call is required. + Err(Error::Gone) => { + warn!( + log, + "dataset is gone"; + "dataset_id" => %dataset.id(), + ); + + return Ok(DeleteResult::Deleted); + } + + Err(e) => { + return Err(e); + } + }; + + match response.running_snapshots.get(&snapshot_id.to_string()) { + Some(running_snapshot) => match running_snapshot.state { + RegionState::Tombstoned => { + Ok(DeleteResult::WaitForNextActivation) + } + + RegionState::Destroyed => Ok(DeleteResult::Deleted), + + _ => { + warn!( + log, + "running_snapshot is Some, state is {}", + running_snapshot.state.to_string(); + "region_id" => %region_id, + "snapshot_id" => %snapshot_id, + ); + + Ok(DeleteResult::WaitForNextActivation) + } + }, + + None => { + // It's possible that the running snapshot record was GCed, and + // it won't come back - consider it deleted. + + info!( + log, + "running_snapshot is None"; + "region_id" => %region_id, + "snapshot_id" => %snapshot_id, + ); + + Ok(DeleteResult::Deleted) + } + } + } + + async fn delete_crucible_running_snapshots( + &self, + log: &Logger, + crucible_resources_to_delete: &CrucibleResources, + status: &mut VolumeDeleteStatus, + ) -> DeleteResult { + let datasets_and_snapshots = match self + .datastore + .snapshots_to_delete(&crucible_resources_to_delete) + .await + { + Ok(datasets_and_snapshots) => datasets_and_snapshots, + + Err(e) => { + let s = format!("error calling snapshots_to_delete: {e}"); + error!(log, "{s}"); + status.errors.push(s); + + // Try again next time! + return DeleteResult::WaitForNextActivation; + } + }; + + let request_count = datasets_and_snapshots.len(); + if request_count == 0 { + return DeleteResult::Deleted; + } + + // Send DELETE calls to the corresponding Crucible agents + let mut all_deleted = true; + + for (dataset, region_snapshot) in datasets_and_snapshots { + let region_id = region_snapshot.region_id; + let snapshot_id = region_snapshot.snapshot_id; + + match self + .delete_crucible_running_snapshot( + log, + &dataset, + region_id, + snapshot_id, + ) + .await + { + Ok(DeleteResult::Deleted) => { + status.running_snapshot_results.push(format!( + "{} / {}: deleted", + region_id, snapshot_id + )); + } + + Ok(DeleteResult::WaitForNextActivation) => { + status.running_snapshot_results.push(format!( + "{} / {}: requested deletion, waiting", + region_id, snapshot_id, + )); + + all_deleted = false; + } + + Err(e) => { + status.errors.push(format!("{e}")); + + all_deleted = false; + } + } + } + + // If any of the operations require waiting for the next activation, + // then return that, otherwise return that all were deleted. + + if all_deleted { + DeleteResult::Deleted + } else { + DeleteResult::WaitForNextActivation + } + } + + async fn request_crucible_snapshot_delete( + &self, + log: &Logger, + dataset: &CrucibleDataset, + region_id: Uuid, + snapshot_id: Uuid, + ) -> Result<(), Error> { + let client = self.crucible_agent_client_for_dataset(dataset); + let dataset_id = dataset.id(); + + if self.crucible_agent_is_gone(dataset_id).await? { + return Err(Error::Gone); + } + + match client + .region_delete_snapshot( + &RegionId(region_id.to_string()), + &snapshot_id.to_string(), + ) + .await + { + Ok(_) => Ok(()), + + Err(e) => { + error!( + log, + "region_delete_snapshot saw {:?}", + e; + "dataset_id" => %dataset_id, + "region_id" => %region_id, + "snapshot_id" => %snapshot_id, + ); + + Err(into_external_error(&e)) + } + } + } + + async fn delete_crucible_snapshot( + &self, + log: &Logger, + dataset: &CrucibleDataset, + region_id: Uuid, + snapshot_id: Uuid, + ) -> Result { + // Unlike other Crucible agent endpoints, this one is synchronous in + // that it is not only a request to the Crucible agent: `zfs destroy` is + // performed right away. However this is still a request to illumos that + // may not take effect right away. Wait until the snapshot no longer + // appears in the list of region snapshots, meaning it was not returned + // from `zfs list`. + + // Request the deletion (which is idempotent), then query for the + // appropriate state change. + + match self + .request_crucible_snapshot_delete( + log, + dataset, + region_id, + snapshot_id, + ) + .await + { + Ok(()) => { + // ok + } + + Err(Error::Gone) => { + return Ok(DeleteResult::Deleted); + } + + Err(e) => { + return Err(e); + } + } + + let response = match self + .get_crucible_region_snapshots(log, dataset, region_id) + .await + { + Ok(v) => v, + + // Return Ok if the dataset's agent is gone, no + // delete call is required. + Err(Error::Gone) => { + warn!( + log, + "dataset is gone"; + "dataset_id" => %dataset.id(), + ); + + return Ok(DeleteResult::Deleted); + } + + Err(e) => { + return Err(e); + } + }; + + // If the snapshot is still returned in the list of snapshots, wait for + // it to be deleted. + + if response.snapshots.iter().any(|x| x.name == snapshot_id.to_string()) + { + Ok(DeleteResult::WaitForNextActivation) + } else { + Ok(DeleteResult::Deleted) + } + } + + async fn delete_crucible_snapshots( + &self, + log: &Logger, + crucible_resources_to_delete: &CrucibleResources, + status: &mut VolumeDeleteStatus, + ) -> DeleteResult { + let datasets_and_snapshots = match self + .datastore + .snapshots_to_delete(&crucible_resources_to_delete) + .await + { + Ok(datasets_and_snapshots) => datasets_and_snapshots, + + Err(e) => { + let s = format!("error calling snapshots_to_delete: {e}"); + error!(log, "{s}"); + status.errors.push(s); + + // Try again next time! + return DeleteResult::WaitForNextActivation; + } + }; + + let request_count = datasets_and_snapshots.len(); + if request_count == 0 { + return DeleteResult::Deleted; + } + + // Send DELETE calls to the corresponding Crucible agents + let mut all_deleted = true; + + for (dataset, region_snapshot) in &datasets_and_snapshots { + let region_id = region_snapshot.region_id; + let snapshot_id = region_snapshot.snapshot_id; + + match self + .delete_crucible_snapshot(log, &dataset, region_id, snapshot_id) + .await + { + Ok(DeleteResult::Deleted) => { + status.snapshot_results.push(format!( + "{} / {}: deleted", + region_id, snapshot_id + )); + } + + Ok(DeleteResult::WaitForNextActivation) => { + status.snapshot_results.push(format!( + "{} / {}: requested deletion, waiting", + region_id, snapshot_id, + )); + + all_deleted = false; + } + + Err(e) => { + status.errors.push(format!("{e}")); + + all_deleted = false; + } + } + } + + // If any of the operations require waiting for the next activation, + // then return that, otherwise return that all were deleted. + + if all_deleted { + // When all crucible resources are cleaned up, hard delete the + // region snapshot records. + + let mut result = DeleteResult::Deleted; + + for (_, region_snapshot) in datasets_and_snapshots { + if let Err(e) = self + .datastore + .region_snapshot_remove( + region_snapshot.dataset_id.into(), + region_snapshot.region_id, + region_snapshot.snapshot_id, + ) + .await + { + let s = + format!("error calling region_snapshot_remove: {e}"); + error!(log, "{s}"); + status.errors.push(s); + + // More work is required next task activation + result = DeleteResult::WaitForNextActivation + } + } + + result + } else { + DeleteResult::WaitForNextActivation + } + } + + async fn clean_up_volume_resources( + &self, + log: &Logger, + crucible_resources_to_delete: &CrucibleResources, + status: &mut VolumeDeleteStatus, + ) -> DeleteResult { + // For any resource that is no longer referenced due to the soft-delete + // of a volume, do the following in order: + // + // - delete top level regions + // - delete running snapshots (read: read-only regions backed by + // snapshots) + // - delete region snapshots + // + // Running snapshots have to be deleted before snapshots are, but + // attempt deleting both regions and running snapshots each task + // activation as they are independent. + + let delete_regions_result = self + .delete_crucible_regions(log, crucible_resources_to_delete, status) + .await; + + let delete_region_snapshots_result = self + .delete_crucible_running_snapshots( + log, + crucible_resources_to_delete, + status, + ) + .await; + + if delete_regions_result == DeleteResult::Deleted + && delete_region_snapshots_result == DeleteResult::Deleted + { + // We're able to continue on with deleting snapshots + } else { + // Otherwise, more work is required from the next task activation. + return DeleteResult::WaitForNextActivation; + } + + // Past this point, running snapshots ar deleted, so delete any + // snapshots. If this is successful, then all crucible resources to + // delete have been cleaned up + + self.delete_crucible_snapshots( + log, + crucible_resources_to_delete, + status, + ) + .await + } + + async fn conditionally_hard_delete_volume_record( + &self, + log: &Logger, + volume_id: VolumeUuid, + status: &mut VolumeDeleteStatus, + ) { + // Do not hard delete the volume record if there are lingering regions + // associated with them. This occurs when a region snapshot hasn't been + // deleted, which means we can't delete the region. Later on, deleting + // the region snapshot when its reference count goes to zero will free + // up the region(s) to be deleted (by delete_freed_crucible_regions). + + let allocated_regions = + match self.datastore.get_allocated_regions(volume_id).await { + Ok(allocated_regions) => allocated_regions, + + Err(e) => { + let s = format!( + "failed to get_allocated_regions for {volume_id}: {e}" + ); + error!(log, "{s}"); + status.errors.push(s); + return; + } + }; + + if !allocated_regions.is_empty() { + info!( + &log, + "allocated regions for {volume_id} is not-empty, skipping \ + hard delete", + ); + + return; + } + + if let Err(e) = self.datastore.volume_hard_delete(volume_id).await { + let s = + format!("failed to volume_hard_delete for {volume_id}: {e}"); + error!(log, "{s}"); + status.errors.push(s); + return; + } + + status.volumes_deleted.push(volume_id.to_string()); + } + + /// Deleting region snapshots in a previous saga node may have freed up + /// regions that were deleted in the DB but couldn't be deleted by the + /// Crucible Agent because a snapshot existed. Look for those here. These + /// will be a different volume id (i.e. for a previously deleted disk) than + /// the one in this saga's params struct. + /// + /// It's insufficient to rely on the struct of CrucibleResources to clean up + /// that is returned as part of svd_decrease_crucible_resource_count. + /// Imagine a disk that is composed of three regions (a subset of + /// [`sled_agent_client::VolumeConstructionRequest`] is shown here): + /// + /// ```json + /// { + /// "type": "volume", + /// "id": "6b353c87-afac-4ee2-b71a-6fe35fcf9e46", + /// "sub_volumes": [ + /// { + /// "type": "region", + /// "opts": { + /// "targets": [ + /// "[fd00:1122:3344:101::5]:1000", + /// "[fd00:1122:3344:102::9]:1000", + /// "[fd00:1122:3344:103::2]:1000" + /// ], + /// "read_only": false + /// } + /// } + /// ], + /// "read_only_parent": null, + /// } + /// ``` + /// + /// Taking a snapshot of this will produce the following volume: + /// + /// ```json + /// { + /// "type": "volume", + /// "id": "1ef7282e-a3fb-4222-85a8-b16d3fbfd738", <-- new UUID + /// "sub_volumes": [ + /// { + /// "type": "region", + /// "opts": { + /// "targets": [ + /// "[fd00:1122:3344:101::5]:1001", <-- port changed + /// "[fd00:1122:3344:102::9]:1001", <-- port changed + /// "[fd00:1122:3344:103::2]:1001" <-- port changed + /// ], + /// "read_only": true <-- read_only now true + /// } + /// } + /// ], + /// "read_only_parent": null, + /// } + /// ``` + /// + /// The snapshot targets will use the same IP but different port: snapshots + /// are initially located on the same filesystem as their region. + /// + /// The disk's volume has no read only resources, while the snapshot's + /// volume does. The disk volume's targets are all regions (backed by + /// downstairs that are read/write) while the snapshot volume's targets are + /// all snapshots (backed by downstairs that are read-only). The two volumes + /// are linked in the sense that the snapshots from the second are contained + /// *within* the regions of the first, reflecting the resource nesting from + /// ZFS. This is also reflected in the REST endpoint that the Crucible agent + /// uses: + /// + /// /crucible/0/regions/{id}/snapshots/{name} + /// + /// If the disk is then deleted, the volume delete saga will run for the + /// first volume shown here. The CrucibleResources struct returned as part + /// of [`svd_decrease_crucible_resource_count`] will contain *nothing* to + /// clean up: the regions contain snapshots that are part of other volumes + /// and cannot be deleted, and the disk's volume doesn't reference any + /// read-only resources. + /// + /// This is expected and normal: regions are "leaked" all the time due to + /// snapshots preventing their deletion. This function detects when those + /// regions can be cleaned up. + /// + /// Note: each delete of a snapshot could trigger another delete of a + /// region, if that region's use has gone to zero. A snapshot delete will + /// never trigger another snapshot delete. + async fn delete_freed_crucible_regions( + &self, + log: &Logger, + status: &mut VolumeDeleteStatus, + ) { + // Find regions freed up for deletion by a previous delete of region + // snapshots. + let freed_datasets_regions_and_volumes = + match self.datastore.find_deleted_volume_regions().await { + Ok(freed_datasets_regions_and_volumes) => { + freed_datasets_regions_and_volumes + } + + Err(e) => { + let s = + format!("failed to find_deleted_volume_regions: {e}"); + error!(log, "{s}"); + status.errors.push(s); + + // Try again next time! + return; + } + }; + + if freed_datasets_regions_and_volumes.is_empty() { + return; + } + + let FreedCrucibleResources { datasets_and_regions, volumes } = + freed_datasets_regions_and_volumes; + + let mut all_deleted = true; + + for (dataset, region) in &datasets_and_regions { + match self.delete_crucible_region(log, &dataset, region.id()).await + { + Ok(DeleteResult::Deleted) => { + status + .region_results + .push(format!("{}: deleted", region.id())); + } + + Ok(DeleteResult::WaitForNextActivation) => { + status.region_results.push(format!( + "{}: requested deletion, waiting", + region.id() + )); + + all_deleted = false; + } + + Err(e) => { + all_deleted = false; + + let s = format!( + "failed delete_crucible_region for {}: {e}", + region.id(), + ); + error!(log, "{s}"); + status.errors.push(s); + } + } + } + + if all_deleted { + // When all crucible resources are cleaned up, hard delete the + // region records. This also re-computes the crucible_dataset + // size_used column for those region's datasets. + + let region_ids_to_delete = + datasets_and_regions.iter().map(|(_, r)| r.id()).collect(); + + match self + .datastore + .regions_hard_delete(log, region_ids_to_delete) + .await + { + Ok(()) => { + // ok + } + + Err(e) => { + let s = format!("error calling regions_hard_delete: {e}"); + error!(log, "{s}"); + status.errors.push(s); + + // More work is required next task activation + return; + } + } + + for volume_id in volumes { + // A Volume returned by `find_deleted_volume_regions` will not + // have read/write regions, so it is safe to delete without + // checking. + + if let Err(e) = + self.datastore.volume_hard_delete(volume_id).await + { + let s = format!( + "error calling volume_hard_delete for {volume_id}: {e}" + ); + error!(log, "{s}"); + status.errors.push(s); + } + } + } else { + // More work is required next task activation + } + } +} + +impl BackgroundTask for VolumeDeleter { + fn activate<'a>( + &'a mut self, + opctx: &'a OpContext, + ) -> BoxFuture<'a, serde_json::Value> { + async { + let log = &opctx.log; + let mut status = VolumeDeleteStatus::default(); + + let soft_deleted_volumes = + match self.datastore.get_soft_deleted_volumes(opctx).await { + Ok(v) => v, + Err(e) => { + let s = format!( + "error calling get_soft_deleted_volumes: {e}" + ); + error!(log, "{s}"); + status.errors.push(s); + return json!(status); + } + }; + + for volume in soft_deleted_volumes { + let Some(resources_to_clean_up) = &volume.resources_to_clean_up + else { + let s = format!( + "volume {} has no resources to clean up", + volume.id(), + ); + error!(log, "{s}"); + status.errors.push(s); + continue; + }; + + let resources_to_clean_up: CrucibleResources = + match serde_json::from_str(&resources_to_clean_up) { + Ok(v) => v, + Err(e) => { + let s = format!( + "volume {} resources to clean up did not \ + deserialize: {e}", + volume.id(), + ); + error!(log, "{s}"); + status.errors.push(s); + continue; + } + }; + + match self + .clean_up_volume_resources( + log, + &resources_to_clean_up, + &mut status, + ) + .await + { + DeleteResult::Deleted => { + self.conditionally_hard_delete_volume_record( + log, + volume.id(), + &mut status, + ) + .await; + } + + DeleteResult::WaitForNextActivation => { + continue; + } + } + } + + self.delete_freed_crucible_regions(log, &mut status).await; + + json!(status) + } + .boxed() + } +} + +fn is_not_found( + e: &crucible_agent_client::Error, +) -> bool { + match e { + crucible_agent_client::Error::ErrorResponse(rv) => match rv.status() { + http::StatusCode::NOT_FOUND => true, + _ => false, + }, + + _ => false, + } +} + +fn into_external_error( + e: &crucible_agent_client::Error, +) -> Error { + match e { + crucible_agent_client::Error::ErrorResponse(rv) => { + if rv.status().is_client_error() { + Error::invalid_request(&rv.message) + } else { + Error::internal_error(&rv.message) + } + } + + _ => Error::internal_error(format!("unexpected failure: {e}")), + } +} diff --git a/nexus/src/app/disk.rs b/nexus/src/app/disk.rs index 7fe564a6b5b..42bb2246220 100644 --- a/nexus/src/app/disk.rs +++ b/nexus/src/app/disk.rs @@ -371,13 +371,23 @@ impl super::Nexus { let saga_params = sagas::disk_delete::Params { serialized_authn: authn::saga::Serialized::for_opctx(opctx), project_id: project.id(), - disk, + disk: disk.clone(), }; self.sagas .saga_execute::(saga_params) .await?; + match disk { + datastore::Disk::Crucible(_) => { + self.background_tasks.task_volume_delete.activate(); + } + + datastore::Disk::LocalStorage(_) => { + self.background_tasks.task_local_storage_delete.activate(); + } + } + Ok(()) } @@ -400,6 +410,8 @@ impl super::Nexus { datastore::Disk::Crucible(disk) => { self.volume_remove_read_only_parent(&opctx, disk.volume_id()) .await?; + + self.background_tasks.task_volume_delete.activate(); } datastore::Disk::LocalStorage(_) => { diff --git a/nexus/src/app/image.rs b/nexus/src/app/image.rs index 76ab0725dab..16d5b6b394e 100644 --- a/nexus/src/app/image.rs +++ b/nexus/src/app/image.rs @@ -192,6 +192,8 @@ impl super::Nexus { .saga_execute::(saga_params) .await?; + self.background_tasks.task_volume_delete.activate(); + Ok(()) } diff --git a/nexus/src/app/sagas/disk_create.rs b/nexus/src/app/sagas/disk_create.rs index 307c6825d86..349adfa75e5 100644 --- a/nexus/src/app/sagas/disk_create.rs +++ b/nexus/src/app/sagas/disk_create.rs @@ -1073,6 +1073,7 @@ pub(crate) mod test { use nexus_db_queries::context::OpContext; use nexus_db_queries::db; use nexus_db_queries::db::datastore::DataStore; + use nexus_test_utils::background::wait_for_all_volume_deletes; use nexus_test_utils::resource_helpers; use nexus_test_utils::resource_helpers::create_project; use nexus_test_utils_macros::nexus_test; @@ -1608,6 +1609,11 @@ pub(crate) mod test { .await; destroy_disk(&cptestctx).await; + wait_for_all_volume_deletes( + nexus.datastore(), + &cptestctx.lockstep_client, + ) + .await; verify_clean_slate(&cptestctx, &test).await; } diff --git a/nexus/src/app/sagas/disk_delete.rs b/nexus/src/app/sagas/disk_delete.rs index 271852fe0ca..5070522f324 100644 --- a/nexus/src/app/sagas/disk_delete.rs +++ b/nexus/src/app/sagas/disk_delete.rs @@ -5,27 +5,16 @@ use super::ActionRegistry; use super::NexusActionContext; use super::NexusSaga; -use crate::app::InlineErrorChain; -use crate::app::sagas::SagaInitError; use crate::app::sagas::declare_saga_actions; -use crate::app::sagas::sled_out_of_service_gone_check; -use crate::app::sagas::volume_delete; -use crate::app::sagas::zpool_out_of_service_gone_check; use nexus_db_queries::authn; use nexus_db_queries::db; use nexus_db_queries::db::datastore; use nexus_types::saga::saga_action_failed; use omicron_common::api::external::DiskState; use omicron_common::api::external::Error; -use omicron_common::backoff::backon_retry_policy_internal_service; -use progenitor_extras::retry::{ - GoneCheckResult, retry_operation_while_indefinitely, -}; use serde::Deserialize; use serde::Serialize; -use sled_agent_client::types::LocalStorageDatasetDeleteRequest; use steno::ActionError; -use steno::Node; use uuid::Uuid; // disk delete saga: input parameters @@ -49,11 +38,8 @@ declare_saga_actions! { + sdd_account_space - sdd_account_space_undo } - DELETE_LOCAL_STORAGE -> "delete_local_storage" { - + sdd_delete_local_storage - } - DEALLOCATE_LOCAL_STORAGE -> "deallocate_local_storage" { - + sdd_deallocate_local_storage + SOFT_DELETE_CRUCIBLE_VOLUME -> "soft_delete_volume" { + + sdd_soft_delete_volume } } @@ -77,47 +63,13 @@ impl NexusSaga for SagaDiskDelete { builder.append(space_account_action()); match ¶ms.disk { - datastore::Disk::Crucible(disk) => { - let subsaga_params = volume_delete::Params { - serialized_authn: params.serialized_authn.clone(), - volume_id: disk.volume_id(), - }; - - let subsaga_dag = { - let subsaga_builder = - steno::DagBuilder::new(steno::SagaName::new( - volume_delete::SagaVolumeDelete::NAME, - )); - volume_delete::SagaVolumeDelete::make_saga_dag( - &subsaga_params, - subsaga_builder, - )? - }; - - builder.append(Node::constant( - "params_for_volume_delete_subsaga", - serde_json::to_value(&subsaga_params).map_err(|e| { - SagaInitError::SerializeError( - "params_for_volume_delete_subsaga".to_string(), - e, - ) - })?, - )); - - builder.append(Node::subsaga( - "volume_delete_subsaga_no_result", - subsaga_dag, - "params_for_volume_delete_subsaga", - )); + datastore::Disk::Crucible(_) => { + builder.append(soft_delete_crucible_volume_action()); } datastore::Disk::LocalStorage(_) => { - // Attempt deleting the local storage before removing the - // database record. If the delete does not succeed, at least the - // user can re-request the deletion. - - builder.append(delete_local_storage_action()); - builder.append(deallocate_local_storage_action()); + // Local storage clean up is done entirely in the + // local_storage_delete background task } } @@ -208,127 +160,27 @@ async fn sdd_account_space_undo( Ok(()) } -async fn sdd_delete_local_storage( +async fn sdd_soft_delete_volume( sagactx: NexusActionContext, ) -> Result<(), ActionError> { - let osagactx = sagactx.user_data(); let params = sagactx.saga_params::()?; - let opctx = crate::context::op_context_for_saga_action( - &sagactx, - ¶ms.serialized_authn, - ); - - let datastore::Disk::LocalStorage(disk) = params.disk else { - unreachable!( - "check during `make_saga_dag` should have ensured disk type is \ - local storage" - ); - }; - - let Some(allocation) = disk.local_storage_dataset_allocation else { - // Nothing to do! - return Ok(()); - }; - - let sled_id = allocation.sled_id(); - let zpool_id = allocation.pool_id().upcast(); - - let request = LocalStorageDatasetDeleteRequest { - zpool_id: allocation.pool_id(), - dataset_id: allocation.id(), - encrypted_at_rest: allocation.encrypted_at_rest(), - }; - - // Get a sled agent client - - let sled_agent_client = osagactx - .nexus() - .sled_client(&sled_id) - .await - .map_err(saga_action_failed)?; - - // Ensure that the local storage is deleted - - let delete_operation = || async { - sled_agent_client.local_storage_dataset_delete(&request).await - }; - - // Bail out of the retry loop if either the disk or sled is no longer - // in-service. - let gone_check = || async { - match sled_out_of_service_gone_check( - osagactx.datastore(), - &opctx, - sled_id, - ) - .await? - { - GoneCheckResult::StillAvailable => { - // proceed to zpool check - } - - GoneCheckResult::Gone => { - return Ok(GoneCheckResult::Gone); - } - } - - zpool_out_of_service_gone_check(osagactx.datastore(), &opctx, zpool_id) - .await - }; - - let log = osagactx.log().clone(); - let result = retry_operation_while_indefinitely( - backon_retry_policy_internal_service(), - delete_operation, - gone_check, - |notification| { - slog::warn!( - log, - "failed to delete local storage dataset, retrying in {:?}", - notification.delay; - InlineErrorChain::new(¬ification.error), - ); - }, - ) - .await; - - match result { - Ok(_) => Ok(()), - - // In this case, if the particular disk hosting this local storage was - // expunged, or if the sled was expunged, then proceed with the rest of - // the saga. - Err(e) if e.is_gone() => Ok(()), - - Err(e) => Err(saga_action_failed(Error::internal_error(&format!( - "failed to delete local storage: {}", - InlineErrorChain::new(&e) - )))), - } -} - -async fn sdd_deallocate_local_storage( - sagactx: NexusActionContext, -) -> Result<(), ActionError> { let osagactx = sagactx.user_data(); - let params = sagactx.saga_params::()?; - let opctx = crate::context::op_context_for_saga_action( - &sagactx, - ¶ms.serialized_authn, - ); - let datastore::Disk::LocalStorage(disk) = params.disk else { + let datastore::Disk::Crucible(disk) = params.disk else { unreachable!( "check during `make_saga_dag` should have ensured disk type is \ - local storage" + crucible" ); }; - osagactx - .datastore() - .delete_local_storage_dataset_allocations(&opctx, &disk) - .await - .map_err(saga_action_failed)?; + osagactx.datastore().soft_delete_volume(disk.volume_id()).await.map_err( + |e| { + saga_action_failed(Error::internal_error(&format!( + "failed to soft_delete_volume: {:?}", + e, + ))) + }, + )?; Ok(()) } @@ -353,6 +205,8 @@ pub(crate) mod test { use nexus_db_queries::authz; use nexus_db_queries::context::OpContext; use nexus_db_queries::db::datastore::Disk; + use nexus_test_utils::background::wait_for_all_local_storage_deletes; + use nexus_test_utils::background::wait_for_all_volume_deletes; use nexus_test_utils::resource_helpers::DiskTest; use nexus_test_utils::resource_helpers::create_project; use nexus_test_utils_macros::nexus_test; @@ -477,6 +331,12 @@ pub(crate) mod test { ) .await; + wait_for_all_volume_deletes( + nexus.datastore(), + &cptestctx.lockstep_client, + ) + .await; + crate::app::sagas::disk_create::test::verify_clean_slate( &cptestctx, &test, ) @@ -641,6 +501,16 @@ pub(crate) mod test { let nexus = &self.cptestctx.server.server_context().nexus; let datastore = nexus.datastore(); + // Run the local storage delete background task to completion + + wait_for_all_local_storage_deletes( + &datastore, + &self.cptestctx.lockstep_client, + ) + .await; + + // Then check all allocations were deleted + let conn = datastore.pool_connection_for_tests().await.unwrap(); use nexus_db_schema::schema::local_storage_unencrypted_dataset_allocation::dsl; diff --git a/nexus/src/app/sagas/image_delete.rs b/nexus/src/app/sagas/image_delete.rs index 7ace0f5ef88..b6c06074761 100644 --- a/nexus/src/app/sagas/image_delete.rs +++ b/nexus/src/app/sagas/image_delete.rs @@ -3,15 +3,14 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use super::{ActionRegistry, NexusActionContext, NexusSaga}; -use crate::app::sagas; use crate::app::sagas::declare_saga_actions; use nexus_db_queries::{authn, authz, db}; use nexus_types::saga::saga_action_failed; +use omicron_common::api::external::Error; use omicron_uuid_kinds::VolumeUuid; use serde::Deserialize; use serde::Serialize; use steno::ActionError; -use steno::Node; #[derive(Debug, Deserialize, Serialize)] pub(crate) enum ImageParam { @@ -38,6 +37,9 @@ pub(crate) struct Params { declare_saga_actions! { image_delete; + SOFT_DELETE_VOLUME -> "soft_delete_volume" { + + sid_soft_delete_volume + } DELETE_IMAGE_RECORD -> "no_result1" { + sid_delete_image_record } @@ -54,38 +56,11 @@ impl NexusSaga for SagaImageDelete { } fn make_saga_dag( - params: &Self::Params, + _params: &Self::Params, mut builder: steno::DagBuilder, ) -> Result { builder.append(delete_image_record_action()); - - const DELETE_VOLUME_PARAMS: &'static str = "delete_volume_params"; - - let volume_delete_params = sagas::volume_delete::Params { - serialized_authn: params.serialized_authn.clone(), - volume_id: params.image_param.volume_id(), - }; - builder.append(Node::constant( - DELETE_VOLUME_PARAMS, - serde_json::to_value(&volume_delete_params).map_err(|e| { - super::SagaInitError::SerializeError( - String::from("volume_id"), - e, - ) - })?, - )); - - let make_volume_delete_dag = || { - let subsaga_builder = steno::DagBuilder::new(steno::SagaName::new( - sagas::volume_delete::SagaVolumeDelete::NAME, - )); - sagas::volume_delete::create_dag(subsaga_builder) - }; - builder.append(steno::Node::subsaga( - "delete_volume", - make_volume_delete_dag()?, - DELETE_VOLUME_PARAMS, - )); + builder.append(soft_delete_volume_action()); Ok(builder.build()?) } @@ -123,3 +98,23 @@ async fn sid_delete_image_record( Ok(()) } + +async fn sid_soft_delete_volume( + sagactx: NexusActionContext, +) -> Result<(), ActionError> { + let params = sagactx.saga_params::()?; + let osagactx = sagactx.user_data(); + + osagactx + .datastore() + .soft_delete_volume(params.image_param.volume_id()) + .await + .map_err(|e| { + saga_action_failed(Error::internal_error(&format!( + "failed to soft_delete_volume: {:?}", + e, + ))) + })?; + + Ok(()) +} diff --git a/nexus/src/app/sagas/mod.rs b/nexus/src/app/sagas/mod.rs index 6460974475d..0005fb07ff2 100644 --- a/nexus/src/app/sagas/mod.rs +++ b/nexus/src/app/sagas/mod.rs @@ -59,7 +59,6 @@ pub mod snapshot_delete; pub mod subnet_attach; pub mod subnet_detach; pub mod test_saga; -pub mod volume_delete; pub mod volume_remove_rop; pub mod vpc_create; pub mod vpc_subnet_create; @@ -193,7 +192,6 @@ fn make_action_registry() -> ActionRegistry { snapshot_delete::SagaSnapshotDelete, subnet_attach::SagaSubnetAttach, subnet_detach::SagaSubnetDetach, - volume_delete::SagaVolumeDelete, volume_remove_rop::SagaVolumeRemoveROP, vpc_create::SagaVpcCreate, vpc_subnet_create::SagaVpcSubnetCreate, diff --git a/nexus/src/app/sagas/region_replacement_finish.rs b/nexus/src/app/sagas/region_replacement_finish.rs index 73edff9c71c..1dfe55ccc84 100644 --- a/nexus/src/app/sagas/region_replacement_finish.rs +++ b/nexus/src/app/sagas/region_replacement_finish.rs @@ -23,8 +23,7 @@ //! It will set itself as the "operating saga" for a region replacement request, //! change the state to "Completing", and: //! -//! 1. Call the Volume delete saga for the fake Volume that points to the old -//! region. +//! 1. Soft-delete the fake Volume that points to the old region. //! //! 2. Clear the operating saga id from the request record, and change the state //! to Completed. @@ -35,9 +34,9 @@ use super::{ SagaInitError, }; use crate::app::sagas::declare_saga_actions; -use crate::app::sagas::volume_delete; use crate::app::{authn, db}; use nexus_types::saga::saga_action_failed; +use omicron_common::api::external::Error; use omicron_uuid_kinds::VolumeUuid; use serde::Deserialize; use serde::Serialize; @@ -65,6 +64,9 @@ declare_saga_actions! { + srrf_set_saga_id - srrf_set_saga_id_undo } + SOFT_DELETE_VOLUME -> "soft_delete_volume" { + + srrf_soft_delete_volume + } UPDATE_REQUEST_RECORD -> "unused_2" { + srrf_update_request_record } @@ -83,7 +85,7 @@ impl NexusSaga for SagaRegionReplacementFinish { } fn make_saga_dag( - params: &Self::Params, + _params: &Self::Params, mut builder: steno::DagBuilder, ) -> Result { builder.append(Node::action( @@ -93,38 +95,7 @@ impl NexusSaga for SagaRegionReplacementFinish { )); builder.append(set_saga_id_action()); - - let subsaga_params = volume_delete::Params { - serialized_authn: params.serialized_authn.clone(), - volume_id: params.region_volume_id, - }; - - let subsaga_dag = { - let subsaga_builder = steno::DagBuilder::new(steno::SagaName::new( - volume_delete::SagaVolumeDelete::NAME, - )); - volume_delete::SagaVolumeDelete::make_saga_dag( - &subsaga_params, - subsaga_builder, - )? - }; - - builder.append(Node::constant( - "params_for_volume_delete_subsaga", - serde_json::to_value(&subsaga_params).map_err(|e| { - SagaInitError::SerializeError( - "params_for_volume_delete_subsaga".to_string(), - e, - ) - })?, - )); - - builder.append(Node::subsaga( - "volume_delete_subsaga_no_result", - subsaga_dag, - "params_for_volume_delete_subsaga", - )); - + builder.append(soft_delete_volume_action()); builder.append(update_request_record_action()); Ok(builder.build()?) @@ -181,6 +152,26 @@ async fn srrf_set_saga_id_undo( Ok(()) } +async fn srrf_soft_delete_volume( + sagactx: NexusActionContext, +) -> Result<(), ActionError> { + let params = sagactx.saga_params::()?; + let osagactx = sagactx.user_data(); + + osagactx + .datastore() + .soft_delete_volume(params.region_volume_id) + .await + .map_err(|e| { + saga_action_failed(Error::internal_error(&format!( + "failed to soft_delete_volume: {:?}", + e, + ))) + })?; + + Ok(()) +} + async fn srrf_update_request_record( sagactx: NexusActionContext, ) -> Result<(), ActionError> { @@ -218,6 +209,7 @@ pub(crate) mod test { use nexus_db_model::RegionReplacementState; use nexus_db_queries::authn::saga::Serialized; use nexus_db_queries::context::OpContext; + use nexus_test_utils::background::wait_for_all_volume_deletes; use nexus_test_utils_macros::nexus_test; use omicron_uuid_kinds::DatasetUuid; use omicron_uuid_kinds::GenericUuid; @@ -349,7 +341,11 @@ pub(crate) mod test { assert_eq!(result.replacement_state, RegionReplacementState::Complete); assert!(result.operating_saga_id.is_none()); - // Validate the Volume was deleted + // Run the volume delete background task and validate the Volume was + // deleted. + wait_for_all_volume_deletes(datastore, &cptestctx.lockstep_client) + .await; + assert!( datastore.volume_get(old_region_volume_id).await.unwrap().is_none() ); diff --git a/nexus/src/app/sagas/region_snapshot_replacement_finish.rs b/nexus/src/app/sagas/region_snapshot_replacement_finish.rs index d61cf11f25d..4b8bce12bc2 100644 --- a/nexus/src/app/sagas/region_snapshot_replacement_finish.rs +++ b/nexus/src/app/sagas/region_snapshot_replacement_finish.rs @@ -23,10 +23,9 @@ //! ``` //! //! The first thing this saga does is set itself as the "operating saga" for the -//! request, and change the state to "Completing". Then, it performs the volume -//! delete sub-saga for the new region volume. Finally, it updates the region -//! snapshot replacement request by clearing the operating saga id and changing -//! the state to "Complete". +//! request, and change the state to "Completing". Then, it soft-deletes the new +//! region volume. Finally, it updates the region snapshot replacement request +//! by clearing the operating saga id and changing the state to "Complete". //! //! Any unwind will place the state back into Running. @@ -35,9 +34,9 @@ use super::{ SagaInitError, }; use crate::app::sagas::declare_saga_actions; -use crate::app::sagas::volume_delete; use crate::app::{authn, db}; use nexus_types::saga::saga_action_failed; +use omicron_common::api::external::Error; use serde::Deserialize; use serde::Serialize; use steno::ActionError; @@ -60,6 +59,9 @@ declare_saga_actions! { + rsrfs_set_saga_id - rsrfs_set_saga_id_undo } + SOFT_DELETE_VOLUME -> "soft_delete_volume" { + + rsrfs_soft_delete_volume + } UPDATE_REQUEST_RECORD -> "unused_4" { + rsrfs_update_request_record } @@ -78,7 +80,7 @@ impl NexusSaga for SagaRegionSnapshotReplacementFinish { } fn make_saga_dag( - params: &Self::Params, + _params: &Self::Params, mut builder: steno::DagBuilder, ) -> Result { builder.append(Node::action( @@ -88,42 +90,7 @@ impl NexusSaga for SagaRegionSnapshotReplacementFinish { )); builder.append(set_saga_id_action()); - - if let Some(new_region_volume_id) = - params.request.new_region_volume_id() - { - let subsaga_params = volume_delete::Params { - serialized_authn: params.serialized_authn.clone(), - volume_id: new_region_volume_id, - }; - - let subsaga_dag = { - let subsaga_builder = steno::DagBuilder::new( - steno::SagaName::new(volume_delete::SagaVolumeDelete::NAME), - ); - volume_delete::SagaVolumeDelete::make_saga_dag( - &subsaga_params, - subsaga_builder, - )? - }; - - builder.append(Node::constant( - "params_for_volume_delete_subsaga", - serde_json::to_value(&subsaga_params).map_err(|e| { - SagaInitError::SerializeError( - "params_for_volume_delete_subsaga".to_string(), - e, - ) - })?, - )); - - builder.append(Node::subsaga( - "volume_delete_subsaga_no_result", - subsaga_dag, - "params_for_volume_delete_subsaga", - )); - } - + builder.append(soft_delete_volume_action()); builder.append(update_request_record_action()); Ok(builder.build()?) @@ -184,6 +151,28 @@ async fn rsrfs_set_saga_id_undo( Ok(()) } +async fn rsrfs_soft_delete_volume( + sagactx: NexusActionContext, +) -> Result<(), ActionError> { + let params = sagactx.saga_params::()?; + let osagactx = sagactx.user_data(); + + if let Some(new_region_volume_id) = params.request.new_region_volume_id() { + osagactx + .datastore() + .soft_delete_volume(new_region_volume_id) + .await + .map_err(|e| { + saga_action_failed(Error::internal_error(&format!( + "failed to soft_delete_volume: {:?}", + e, + ))) + })?; + } + + Ok(()) +} + async fn rsrfs_update_request_record( sagactx: NexusActionContext, ) -> Result<(), ActionError> { diff --git a/nexus/src/app/sagas/region_snapshot_replacement_garbage_collect.rs b/nexus/src/app/sagas/region_snapshot_replacement_garbage_collect.rs index d1ebdd4cc3e..c01932af6eb 100644 --- a/nexus/src/app/sagas/region_snapshot_replacement_garbage_collect.rs +++ b/nexus/src/app/sagas/region_snapshot_replacement_garbage_collect.rs @@ -2,7 +2,7 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -//! Clean up the volume that stashes the target replaced during the region +//! Soft-delete up the volume that stashes the target replaced during the region //! snapshot replacement start saga. After that's done, change the region //! snapshot replacement state to Running. This saga handles the following //! region snapshot replacement request state transitions: @@ -29,9 +29,9 @@ use super::{ SagaInitError, }; use crate::app::sagas::declare_saga_actions; -use crate::app::sagas::volume_delete; use crate::app::{authn, db}; use nexus_types::saga::saga_action_failed; +use omicron_common::api::external::Error; use omicron_uuid_kinds::VolumeUuid; use serde::Deserialize; use serde::Serialize; @@ -59,6 +59,9 @@ declare_saga_actions! { + rsrgs_set_saga_id - rsrgs_set_saga_id_undo } + SOFT_DELETE_VOLUME -> "soft_delete_volume" { + + rsrgs_soft_delete_volume + } UPDATE_REQUEST_RECORD -> "unused_2" { + rsrgs_update_request_record } @@ -77,7 +80,7 @@ impl NexusSaga for SagaRegionSnapshotReplacementGarbageCollect { } fn make_saga_dag( - params: &Self::Params, + _params: &Self::Params, mut builder: steno::DagBuilder, ) -> Result { builder.append(Node::action( @@ -87,38 +90,7 @@ impl NexusSaga for SagaRegionSnapshotReplacementGarbageCollect { )); builder.append(set_saga_id_action()); - - let subsaga_params = volume_delete::Params { - serialized_authn: params.serialized_authn.clone(), - volume_id: params.old_snapshot_volume_id, - }; - - let subsaga_dag = { - let subsaga_builder = steno::DagBuilder::new(steno::SagaName::new( - volume_delete::SagaVolumeDelete::NAME, - )); - volume_delete::SagaVolumeDelete::make_saga_dag( - &subsaga_params, - subsaga_builder, - )? - }; - - builder.append(Node::constant( - "params_for_volume_delete_subsaga", - serde_json::to_value(&subsaga_params).map_err(|e| { - SagaInitError::SerializeError( - "params_for_volume_delete_subsaga".to_string(), - e, - ) - })?, - )); - - builder.append(Node::subsaga( - "volume_delete_subsaga_no_result", - subsaga_dag, - "params_for_volume_delete_subsaga", - )); - + builder.append(soft_delete_volume_action()); builder.append(update_request_record_action()); Ok(builder.build()?) @@ -180,6 +152,26 @@ async fn rsrgs_set_saga_id_undo( Ok(()) } +async fn rsrgs_soft_delete_volume( + sagactx: NexusActionContext, +) -> Result<(), ActionError> { + let params = sagactx.saga_params::()?; + let osagactx = sagactx.user_data(); + + osagactx + .datastore() + .soft_delete_volume(params.old_snapshot_volume_id) + .await + .map_err(|e| { + saga_action_failed(Error::internal_error(&format!( + "failed to soft_delete_volume: {:?}", + e, + ))) + })?; + + Ok(()) +} + async fn rsrgs_update_request_record( sagactx: NexusActionContext, ) -> Result<(), ActionError> { @@ -218,6 +210,7 @@ pub(crate) mod test { use nexus_db_model::RegionSnapshotReplacementState; use nexus_db_queries::authn::saga::Serialized; use nexus_db_queries::context::OpContext; + use nexus_test_utils::background::wait_for_all_volume_deletes; use nexus_test_utils_macros::nexus_test; use omicron_uuid_kinds::DatasetUuid; use omicron_uuid_kinds::GenericUuid; @@ -319,7 +312,11 @@ pub(crate) mod test { RegionSnapshotReplacementState::Running ); - // Validate the Volume was deleted + // Run the volume delete background task and validate the Volume was + // deleted. + wait_for_all_volume_deletes(datastore, &cptestctx.lockstep_client) + .await; + assert!( datastore .volume_get(old_snapshot_volume_id) diff --git a/nexus/src/app/sagas/region_snapshot_replacement_step_garbage_collect.rs b/nexus/src/app/sagas/region_snapshot_replacement_step_garbage_collect.rs index c508fb01a8e..e97d248b4dc 100644 --- a/nexus/src/app/sagas/region_snapshot_replacement_step_garbage_collect.rs +++ b/nexus/src/app/sagas/region_snapshot_replacement_step_garbage_collect.rs @@ -2,20 +2,19 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -//! Delete the volume that stashes the target replaced during a region snapshot -//! replacement step saga. After that's done, change the region snapshot -//! replacement step's state to "VolumeDeleted". +//! Soft-delete the volume that stashes the target replaced during a region +//! snapshot replacement step saga. After that's done, change the region +//! snapshot replacement step's state to "VolumeDeleted". use super::{ActionRegistry, NexusActionContext, NexusSaga, SagaInitError}; use crate::app::sagas::declare_saga_actions; -use crate::app::sagas::volume_delete; use crate::app::{authn, db}; use nexus_types::saga::saga_action_failed; +use omicron_common::api::external::Error; use omicron_uuid_kinds::VolumeUuid; use serde::Deserialize; use serde::Serialize; use steno::ActionError; -use steno::Node; // region snapshot replacement step garbage collect saga: input parameters @@ -33,6 +32,9 @@ pub(crate) struct Params { declare_saga_actions! { region_snapshot_replacement_step_garbage_collect; + SOFT_DELETE_VOLUME -> "soft_delete_volume" { + + srsgs_soft_delete_volume + } UPDATE_REQUEST_RECORD -> "unused_1" { + srsgs_update_request_record } @@ -54,40 +56,10 @@ impl NexusSaga for SagaRegionSnapshotReplacementStepGarbageCollect { } fn make_saga_dag( - params: &Self::Params, + _params: &Self::Params, mut builder: steno::DagBuilder, ) -> Result { - let subsaga_params = volume_delete::Params { - serialized_authn: params.serialized_authn.clone(), - volume_id: params.old_snapshot_volume_id, - }; - - let subsaga_dag = { - let subsaga_builder = steno::DagBuilder::new(steno::SagaName::new( - volume_delete::SagaVolumeDelete::NAME, - )); - volume_delete::SagaVolumeDelete::make_saga_dag( - &subsaga_params, - subsaga_builder, - )? - }; - - builder.append(Node::constant( - "params_for_volume_delete_subsaga", - serde_json::to_value(&subsaga_params).map_err(|e| { - SagaInitError::SerializeError( - "params_for_volume_delete_subsaga".to_string(), - e, - ) - })?, - )); - - builder.append(Node::subsaga( - "volume_delete_subsaga_no_result", - subsaga_dag, - "params_for_volume_delete_subsaga", - )); - + builder.append(soft_delete_volume_action()); builder.append(update_request_record_action()); Ok(builder.build()?) @@ -96,6 +68,26 @@ impl NexusSaga for SagaRegionSnapshotReplacementStepGarbageCollect { // region snapshot replacement step garbage collect saga: action implementations +async fn srsgs_soft_delete_volume( + sagactx: NexusActionContext, +) -> Result<(), ActionError> { + let params = sagactx.saga_params::()?; + let osagactx = sagactx.user_data(); + + osagactx + .datastore() + .soft_delete_volume(params.old_snapshot_volume_id) + .await + .map_err(|e| { + saga_action_failed(Error::internal_error(&format!( + "failed to soft_delete_volume: {:?}", + e, + ))) + })?; + + Ok(()) +} + async fn srsgs_update_request_record( sagactx: NexusActionContext, ) -> Result<(), ActionError> { @@ -130,6 +122,7 @@ pub(crate) mod test { use nexus_db_queries::authn::saga::Serialized; use nexus_db_queries::context::OpContext; use nexus_db_queries::db::datastore::region_snapshot_replacement; + use nexus_test_utils::background::wait_for_all_volume_deletes; use nexus_test_utils_macros::nexus_test; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::VolumeUuid; @@ -243,7 +236,11 @@ pub(crate) mod test { RegionSnapshotReplacementStepState::VolumeDeleted ); - // Validate the Volume was deleted + // Run the volume delete background task and validate the Volume was + // deleted. + wait_for_all_volume_deletes(datastore, &cptestctx.lockstep_client) + .await; + assert!( datastore .volume_get(old_snapshot_volume_id) diff --git a/nexus/src/app/sagas/snapshot_create.rs b/nexus/src/app/sagas/snapshot_create.rs index 21fa49b903c..f5f1a3fa342 100644 --- a/nexus/src/app/sagas/snapshot_create.rs +++ b/nexus/src/app/sagas/snapshot_create.rs @@ -1784,6 +1784,7 @@ mod test { use nexus_db_queries::db::DataStore; use nexus_db_queries::db::datastore::Disk; use nexus_db_queries::db::datastore::InstanceAndActiveVmm; + use nexus_test_utils::background::wait_for_all_volume_deletes; use nexus_test_utils::resource_helpers::create_default_ip_pools; use nexus_test_utils::resource_helpers::create_disk; use nexus_test_utils::resource_helpers::create_project; @@ -2410,6 +2411,10 @@ mod test { } delete_disk(client, PROJECT_NAME, DISK_NAME).await; + wait_for_all_volume_deletes( + nexus.datastore(), + &cptestctx.lockstep_client, + ).await; verify_clean_slate(cptestctx, &test).await; }) }, diff --git a/nexus/src/app/sagas/snapshot_delete.rs b/nexus/src/app/sagas/snapshot_delete.rs index a9e2f2f42af..605390e1c40 100644 --- a/nexus/src/app/sagas/snapshot_delete.rs +++ b/nexus/src/app/sagas/snapshot_delete.rs @@ -3,14 +3,13 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use super::{ActionRegistry, NexusActionContext, NexusSaga}; -use crate::app::sagas; use crate::app::sagas::declare_saga_actions; use nexus_db_queries::{authn, authz, db}; use nexus_types::saga::saga_action_failed; +use omicron_common::api::external::Error; use serde::Deserialize; use serde::Serialize; use steno::ActionError; -use steno::Node; #[derive(Debug, Deserialize, Serialize)] pub(crate) struct Params { @@ -27,8 +26,11 @@ declare_saga_actions! { SPACE_ACCOUNT -> "no_result2" { + ssd_account_space } - NOOP -> "no_result3" { - + ssd_noop + SOFT_DELETE_VOLUME -> "soft_delete_volume" { + + ssd_soft_delete_volume + } + SOFT_DELETE_DEST_VOLUME -> "soft_delete_dest_volume" { + + ssd_soft_delete_dest_volume } } @@ -43,65 +45,13 @@ impl NexusSaga for SagaSnapshotDelete { } fn make_saga_dag( - params: &Self::Params, + _params: &Self::Params, mut builder: steno::DagBuilder, ) -> Result { builder.append(delete_snapshot_record_action()); builder.append(space_account_action()); - - const DELETE_VOLUME_PARAMS: &'static str = "delete_volume_params"; - const DELETE_VOLUME_DESTINATION_PARAMS: &'static str = - "delete_volume_destination_params"; - - let volume_delete_params = sagas::volume_delete::Params { - serialized_authn: params.serialized_authn.clone(), - volume_id: params.snapshot.volume_id(), - }; - builder.append(Node::constant( - DELETE_VOLUME_PARAMS, - serde_json::to_value(&volume_delete_params).map_err(|e| { - super::SagaInitError::SerializeError( - String::from("volume_id"), - e, - ) - })?, - )); - - let volume_delete_params = sagas::volume_delete::Params { - serialized_authn: params.serialized_authn.clone(), - volume_id: params.snapshot.destination_volume_id(), - }; - builder.append(Node::constant( - DELETE_VOLUME_DESTINATION_PARAMS, - serde_json::to_value(&volume_delete_params).map_err(|e| { - super::SagaInitError::SerializeError( - String::from("destination_volume_id"), - e, - ) - })?, - )); - - let make_volume_delete_dag = || { - let subsaga_builder = steno::DagBuilder::new(steno::SagaName::new( - sagas::volume_delete::SagaVolumeDelete::NAME, - )); - sagas::volume_delete::create_dag(subsaga_builder) - }; - - builder.append_parallel(vec![ - steno::Node::subsaga( - "delete_volume", - make_volume_delete_dag()?, - DELETE_VOLUME_PARAMS, - ), - steno::Node::subsaga( - "delete_destination_volume", - make_volume_delete_dag()?, - DELETE_VOLUME_DESTINATION_PARAMS, - ), - ]); - - builder.append(noop_action()); + builder.append(soft_delete_volume_action()); + builder.append(soft_delete_dest_volume_action()); Ok(builder.build()?) } @@ -158,7 +108,42 @@ async fn ssd_account_space( Ok(()) } -// Sagas must end in one node, not parallel -async fn ssd_noop(_sagactx: NexusActionContext) -> Result<(), ActionError> { +async fn ssd_soft_delete_volume( + sagactx: NexusActionContext, +) -> Result<(), ActionError> { + let params = sagactx.saga_params::()?; + let osagactx = sagactx.user_data(); + + osagactx + .datastore() + .soft_delete_volume(params.snapshot.volume_id()) + .await + .map_err(|e| { + saga_action_failed(Error::internal_error(&format!( + "failed to soft_delete_volume: {:?}", + e, + ))) + })?; + + Ok(()) +} + +async fn ssd_soft_delete_dest_volume( + sagactx: NexusActionContext, +) -> Result<(), ActionError> { + let params = sagactx.saga_params::()?; + let osagactx = sagactx.user_data(); + + osagactx + .datastore() + .soft_delete_volume(params.snapshot.destination_volume_id()) + .await + .map_err(|e| { + saga_action_failed(Error::internal_error(&format!( + "failed to soft_delete_volume: {:?}", + e, + ))) + })?; + Ok(()) } diff --git a/nexus/src/app/sagas/volume_delete.rs b/nexus/src/app/sagas/volume_delete.rs deleted file mode 100644 index b02e7184b8d..00000000000 --- a/nexus/src/app/sagas/volume_delete.rs +++ /dev/null @@ -1,541 +0,0 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at https://mozilla.org/MPL/2.0/. - -//! Nexus is responsible for telling Crucible Agent(s) when to clean up -//! resources - those Agents do not have any idea of what volumes are -//! constructed, currently active, etc. Plus, volumes can (and will) change -//! during their lifetime. Operations like growing a disk, removing a read-only -//! parent after a scrub has completed, or re-encrypting a disk will all change -//! the volume that backs a disk. -//! -//! Nexus has to account for all the Crucible resources it is using, and count -//! how many volumes are using those resources. Only when that count drops to -//! zero is it valid to clean up the appropriate Crucible resource. -//! -//! Complicating things is the fact that ZFS datasets cannot be deleted if there -//! are snapshots of that dataset. Nexus' resource accounting must take this -//! dependency into account. Note that ZFS snapshots can layer, but any snapshot -//! can be deleted without the requirement of (for example) deleting the -//! snapshots in a certain order. -//! -//! One problem to solve is doing this idempotently. Volumes reference Crucible -//! resources, and when they are inserted or deleted the accounting needs to -//! change. Saga nodes must be idempotent in order to work correctly. - -use super::ActionRegistry; -use super::NexusActionContext; -use super::NexusSaga; -use crate::app::sagas::declare_saga_actions; -use nexus_db_queries::authn; -use nexus_db_queries::db::datastore::CrucibleResources; -use nexus_db_queries::db::datastore::FreedCrucibleResources; -use nexus_types::saga::saga_action_failed; -use omicron_common::api::external::Error; -use omicron_uuid_kinds::VolumeUuid; -use serde::Deserialize; -use serde::Serialize; -use steno::ActionError; - -// volume delete saga: input parameters - -#[derive(Debug, Deserialize, Serialize)] -pub(crate) struct Params { - pub serialized_authn: authn::saga::Serialized, - pub volume_id: VolumeUuid, -} - -// volume delete saga: actions - -declare_saga_actions! { - volume_delete; - DECREASE_CRUCIBLE_RESOURCE_COUNT -> "crucible_resources_to_delete" { - + svd_decrease_crucible_resource_count - } - DELETE_CRUCIBLE_REGIONS -> "no_result_1" { - + svd_delete_crucible_regions - } - DELETE_CRUCIBLE_RUNNING_SNAPSHOTS -> "no_result_2" { - + svd_delete_crucible_running_snapshots - } - DELETE_CRUCIBLE_SNAPSHOTS -> "no_result_3" { - + svd_delete_crucible_snapshots - } - DELETE_CRUCIBLE_SNAPSHOT_RECORDS -> "no_result_4" { - + svd_delete_crucible_snapshot_records - } - FIND_FREED_CRUCIBLE_REGIONS -> "freed_crucible_regions" { - + svd_find_freed_crucible_regions - } - DELETE_FREED_CRUCIBLE_REGIONS -> "no_result_5" { - + svd_delete_freed_crucible_regions - } - HARD_DELETE_VOLUME_RECORD -> "volume_hard_deleted" { - + svd_hard_delete_volume_record - } -} - -// volume delete saga: definition - -pub fn create_dag( - mut builder: steno::DagBuilder, -) -> Result { - builder.append(decrease_crucible_resource_count_action()); - builder.append_parallel(vec![ - // clean up top level regions for volume - delete_crucible_regions_action(), - // clean up running snapshots no longer referenced by any volume - delete_crucible_running_snapshots_action(), - ]); - // clean up snapshots no longer referenced by any volume - builder.append(delete_crucible_snapshots_action()); - // remove snapshot db records - builder.append(delete_crucible_snapshot_records_action()); - // clean up regions that were freed by deleting snapshots - builder.append(find_freed_crucible_regions_action()); - builder.append(delete_freed_crucible_regions_action()); - builder.append(hard_delete_volume_record_action()); - - Ok(builder.build()?) -} - -#[derive(Debug)] -pub(crate) struct SagaVolumeDelete; -impl NexusSaga for SagaVolumeDelete { - const NAME: &'static str = "volume-delete"; - type Params = Params; - - fn register_actions(registry: &mut ActionRegistry) { - volume_delete_register_actions(registry); - } - - fn make_saga_dag( - _params: &Self::Params, - builder: steno::DagBuilder, - ) -> Result { - create_dag(builder) - } -} - -// volume delete saga: action implementations - -/// Decrease Crucible resource accounting for this volume, and return Crucible -/// resources to delete. -async fn svd_decrease_crucible_resource_count( - sagactx: NexusActionContext, -) -> Result { - let osagactx = sagactx.user_data(); - let params = sagactx.saga_params::()?; - - let crucible_resources = osagactx - .datastore() - .soft_delete_volume(params.volume_id) - .await - .map_err(|e| { - saga_action_failed(Error::internal_error(&format!( - "failed to soft_delete_volume: {:?}", - e, - ))) - })?; - - Ok(crucible_resources) -} - -/// Clean up regions associated with this volume. -async fn svd_delete_crucible_regions( - sagactx: NexusActionContext, -) -> Result<(), ActionError> { - let log = sagactx.user_data().log(); - let osagactx = sagactx.user_data(); - - let crucible_resources_to_delete = - sagactx.lookup::("crucible_resources_to_delete")?; - - // Send DELETE calls to the corresponding Crucible agents - let datasets_and_regions = osagactx - .datastore() - .regions_to_delete( - &crucible_resources_to_delete, - ) - .await - .map_err(|e| { - saga_action_failed(Error::internal_error(&format!( - "failed to get datasets_and_regions from crucible resources ({:?}): {:?}", - crucible_resources_to_delete, - e, - ))) - })?; - - osagactx - .nexus() - .delete_crucible_regions(log, datasets_and_regions.clone()) - .await - .map_err(|e| { - saga_action_failed(Error::internal_error(&format!( - "failed to delete_crucible_regions: {:?}", - e, - ))) - })?; - - // Remove DB records - let region_ids_to_delete = - datasets_and_regions.iter().map(|(_, r)| r.id()).collect(); - - osagactx - .datastore() - .regions_hard_delete(log, region_ids_to_delete) - .await - .map_err(|e| { - saga_action_failed(Error::internal_error(&format!( - "failed to regions_hard_delete: {:?}", - e, - ))) - })?; - - Ok(()) -} - -/// Clean up running read-only downstairs corresponding to snapshots freed up -/// for deletion by deleting this volume. -/// -/// This Volume may have referenced read-only downstairs (and their snapshots), -/// and deleting it will remove the references - this may free up those -/// resources for deletion, which this Saga node does. -async fn svd_delete_crucible_running_snapshots( - sagactx: NexusActionContext, -) -> Result<(), ActionError> { - let log = sagactx.user_data().log(); - let osagactx = sagactx.user_data(); - - let crucible_resources_to_delete = - sagactx.lookup::("crucible_resources_to_delete")?; - - // Send DELETE calls to the corresponding Crucible agents - let datasets_and_snapshots = osagactx - .datastore() - .snapshots_to_delete( - &crucible_resources_to_delete, - ) - .await - .map_err(|e| { - saga_action_failed(Error::internal_error(&format!( - "failed to get datasets_and_snapshots from crucible resources ({:?}): {:?}", - crucible_resources_to_delete, - e, - ))) - })?; - - osagactx - .nexus() - .delete_crucible_running_snapshots(log, datasets_and_snapshots.clone()) - .await - .map_err(|e| { - saga_action_failed(Error::internal_error(&format!( - "failed to delete_crucible_running_snapshots: {:?}", - e, - ))) - })?; - - Ok(()) -} - -/// Clean up snapshots freed up for deletion by deleting this volume. -/// -/// This Volume may have referenced read-only downstairs (and their snapshots), -/// and deleting it will remove the references - this may free up those -/// resources for deletion, which this Saga node does. -async fn svd_delete_crucible_snapshots( - sagactx: NexusActionContext, -) -> Result<(), ActionError> { - let log = sagactx.user_data().log(); - let osagactx = sagactx.user_data(); - - let crucible_resources_to_delete = - sagactx.lookup::("crucible_resources_to_delete")?; - - // Send DELETE calls to the corresponding Crucible agents - let datasets_and_snapshots = osagactx - .datastore() - .snapshots_to_delete( - &crucible_resources_to_delete, - ) - .await - .map_err(|e| { - saga_action_failed(Error::internal_error(&format!( - "failed to get datasets_and_snapshots from crucible resources ({:?}): {:?}", - crucible_resources_to_delete, - e, - ))) - })?; - - osagactx - .nexus() - .delete_crucible_snapshots(log, datasets_and_snapshots.clone()) - .await - .map_err(|e| { - saga_action_failed(Error::internal_error(&format!( - "failed to delete_crucible_snapshots: {:?}", - e, - ))) - })?; - - Ok(()) -} - -/// Remove records for deleted snapshots -async fn svd_delete_crucible_snapshot_records( - sagactx: NexusActionContext, -) -> Result<(), ActionError> { - let osagactx = sagactx.user_data(); - - let crucible_resources_to_delete = - sagactx.lookup::("crucible_resources_to_delete")?; - - // Remove DB records - let datasets_and_snapshots = osagactx - .datastore() - .snapshots_to_delete( - &crucible_resources_to_delete, - ) - .await - .map_err(|e| { - saga_action_failed(Error::internal_error(&format!( - "failed to get datasets_and_snapshots from crucible resources ({:?}): {:?}", - crucible_resources_to_delete, - e, - ))) - })?; - - for (_, region_snapshot) in datasets_and_snapshots { - osagactx - .datastore() - .region_snapshot_remove( - region_snapshot.dataset_id.into(), - region_snapshot.region_id, - region_snapshot.snapshot_id, - ) - .await - .map_err(|e| { - saga_action_failed(Error::internal_error(&format!( - "failed to region_snapshot_remove {} {} {}: {:?}", - region_snapshot.dataset_id, - region_snapshot.region_id, - region_snapshot.snapshot_id, - e, - ))) - })?; - } - - Ok(()) -} - -/// Deleting region snapshots in a previous saga node may have freed up regions -/// that were deleted in the DB but couldn't be deleted by the Crucible Agent -/// because a snapshot existed. Look for those here. These will be a different -/// volume id (i.e. for a previously deleted disk) than the one in this saga's -/// params struct. -/// -/// It's insufficient to rely on the struct of CrucibleResources to clean up -/// that is returned as part of svd_decrease_crucible_resource_count. Imagine a -/// disk that is composed of three regions (a subset of -/// [`sled_agent_client::VolumeConstructionRequest`] is shown here): -/// -/// ```json -/// { -/// "type": "volume", -/// "id": "6b353c87-afac-4ee2-b71a-6fe35fcf9e46", -/// "sub_volumes": [ -/// { -/// "type": "region", -/// "opts": { -/// "targets": [ -/// "[fd00:1122:3344:101::5]:1000", -/// "[fd00:1122:3344:102::9]:1000", -/// "[fd00:1122:3344:103::2]:1000" -/// ], -/// "read_only": false -/// } -/// } -/// ], -/// "read_only_parent": null, -/// } -/// ``` -/// -/// Taking a snapshot of this will produce the following volume: -/// -/// ```json -/// { -/// "type": "volume", -/// "id": "1ef7282e-a3fb-4222-85a8-b16d3fbfd738", <-- new UUID -/// "sub_volumes": [ -/// { -/// "type": "region", -/// "opts": { -/// "targets": [ -/// "[fd00:1122:3344:101::5]:1001", <-- port changed -/// "[fd00:1122:3344:102::9]:1001", <-- port changed -/// "[fd00:1122:3344:103::2]:1001" <-- port changed -/// ], -/// "read_only": true <-- read_only now true -/// } -/// } -/// ], -/// "read_only_parent": null, -/// } -/// ``` -/// -/// The snapshot targets will use the same IP but different port: snapshots are -/// initially located on the same filesystem as their region. -/// -/// The disk's volume has no read only resources, while the snapshot's volume -/// does. The disk volume's targets are all regions (backed by downstairs that -/// are read/write) while the snapshot volume's targets are all snapshots -/// (backed by downstairs that are read-only). The two volumes are linked in the -/// sense that the snapshots from the second are contained *within* the regions -/// of the first, reflecting the resource nesting from ZFS. This is also -/// reflected in the REST endpoint that the Crucible agent uses: -/// -/// /crucible/0/regions/{id}/snapshots/{name} -/// -/// If the disk is then deleted, the volume delete saga will run for the first -/// volume shown here. The CrucibleResources struct returned as part of -/// [`svd_decrease_crucible_resource_count`] will contain *nothing* to clean up: -/// the regions contain snapshots that are part of other volumes and cannot be -/// deleted, and the disk's volume doesn't reference any read-only resources. -/// -/// This is expected and normal: regions are "leaked" all the time due to -/// snapshots preventing their deletion. This part of the saga detects when -/// those regions can be cleaned up - it must be stored in the output of this -/// saga node as deleting volume records will affect what is returned by -/// `find_deleted_volume_regions`. -/// -/// Note: each delete of a snapshot could trigger another delete of a region, if -/// that region's use has gone to zero. A snapshot delete will never trigger -/// another snapshot delete. -async fn svd_find_freed_crucible_regions( - sagactx: NexusActionContext, -) -> Result { - let osagactx = sagactx.user_data(); - - // Find regions freed up for deletion by a previous saga node deleting the - // region snapshots. - let freed_datasets_regions_and_volumes = - osagactx.datastore().find_deleted_volume_regions().await.map_err( - |e| { - saga_action_failed(Error::internal_error(&format!( - "failed to find_deleted_volume_regions: {:?}", - e, - ))) - }, - )?; - - Ok(freed_datasets_regions_and_volumes) -} - -async fn svd_delete_freed_crucible_regions( - sagactx: NexusActionContext, -) -> Result<(), ActionError> { - let log = sagactx.user_data().log(); - let osagactx = sagactx.user_data(); - - // Find regions freed up for deletion by a previous saga node deleting the - // region snapshots. - let freed_datasets_regions_and_volumes = - sagactx.lookup::("freed_crucible_regions")?; - - for (dataset, region) in - &freed_datasets_regions_and_volumes.datasets_and_regions - { - // Send DELETE calls to the corresponding Crucible agents - osagactx - .nexus() - .delete_crucible_regions( - log, - vec![(dataset.clone(), region.clone())], - ) - .await - .map_err(|e| { - saga_action_failed(Error::internal_error(&format!( - "failed to delete_crucible_regions: {:?}", - e, - ))) - })?; - - // Remove region DB record - osagactx - .datastore() - .regions_hard_delete(log, vec![region.id()]) - .await - .map_err(|e| { - saga_action_failed(Error::internal_error(&format!( - "failed to regions_hard_delete: {:?}", - e, - ))) - })?; - } - - for volume_id in &freed_datasets_regions_and_volumes.volumes { - osagactx.datastore().volume_hard_delete(*volume_id).await.map_err( - |e| { - saga_action_failed(Error::internal_error(&format!( - "failed to volume_hard_delete {}: {:?}", - volume_id, e, - ))) - }, - )?; - } - - Ok(()) -} - -/// Hard delete the volume record -async fn svd_hard_delete_volume_record( - sagactx: NexusActionContext, -) -> Result { - let osagactx = sagactx.user_data(); - let params = sagactx.saga_params::()?; - - // Do not hard delete the volume record if there are lingering regions - // associated with them. This occurs when a region snapshot hasn't been - // deleted, which means we can't delete the region. Later on, deleting the - // region snapshot will free up the region(s) to be deleted (this occurs in - // svd_delete_freed_crucible_regions). - let allocated_regions = osagactx - .datastore() - .get_allocated_regions(params.volume_id) - .await - .map_err(|e| { - saga_action_failed(Error::internal_error(&format!( - "failed to get_allocated_regions for {}: {:?}", - params.volume_id, e, - ))) - })?; - - let log = sagactx.user_data().log(); - - if !allocated_regions.is_empty() { - info!( - &log, - "allocated regions for {} is not-empty, skipping volume_hard_delete", - params.volume_id, - ); - return Ok(false); - } - - info!( - &log, - "allocated regions for {} is empty, calling volume_hard_delete", - params.volume_id, - ); - - osagactx.datastore().volume_hard_delete(params.volume_id).await.map_err( - |e| { - saga_action_failed(Error::internal_error(&format!( - "failed to volume_hard_delete {}: {:?}", - params.volume_id, e, - ))) - }, - )?; - - Ok(true) -} diff --git a/nexus/src/app/sagas/volume_remove_rop.rs b/nexus/src/app/sagas/volume_remove_rop.rs index ed189db5d09..e6456a7f58f 100644 --- a/nexus/src/app/sagas/volume_remove_rop.rs +++ b/nexus/src/app/sagas/volume_remove_rop.rs @@ -3,10 +3,10 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. use super::{ActionRegistry, NexusActionContext, NexusSaga, SagaInitError}; -use crate::app::sagas; use crate::app::sagas::declare_saga_actions; use nexus_db_queries::authn; use nexus_types::saga::saga_action_failed; +use omicron_common::api::external::Error; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::VolumeUuid; use serde::Deserialize; @@ -48,10 +48,13 @@ declare_saga_actions! { + svr_create_temp_volume - svr_create_temp_volume_undo } - // remove the read_only_parent, attach it to the temp volume. + // remove the read_only_parent, attach it to the temp volume. REMOVE_READ_ONLY_PARENT -> "no_result_1" { + svr_remove_read_only_parent } + SOFT_DELETE_VOLUME -> "soft_delete_volume" { + + svr_soft_delete_volume + } } // volume remove read only parent saga: definition @@ -67,25 +70,11 @@ impl NexusSaga for SagaVolumeRemoveROP { } fn make_saga_dag( - params: &Self::Params, + _params: &Self::Params, mut builder: steno::DagBuilder, ) -> Result { // Generate the temp volume ID this saga will use. let temp_volume_id = VolumeUuid::new_v4(); - // Generate the params for the subsaga called at the end. - let subsaga_params = sagas::volume_delete::Params { - serialized_authn: params.serialized_authn.clone(), - volume_id: temp_volume_id, - }; - let subsaga_dag = { - let subsaga_builder = steno::DagBuilder::new(steno::SagaName::new( - sagas::volume_delete::SagaVolumeDelete::NAME, - )); - sagas::volume_delete::SagaVolumeDelete::make_saga_dag( - &subsaga_params, - subsaga_builder, - )? - }; // Add the temp_volume_id to the saga. builder.append(Node::constant( @@ -97,26 +86,12 @@ impl NexusSaga for SagaVolumeRemoveROP { // Create the temporary volume builder.append(create_temp_volume_action()); + // Remove the read only parent, attach to temp volume builder.append(remove_read_only_parent_action()); - // Build the params for the subsaga to delete the temp volume - builder.append(Node::constant( - "params_for_delete_subsaga", - serde_json::to_value(&subsaga_params).map_err(|e| { - SagaInitError::SerializeError( - String::from("params_for_delete_subsaga"), - e, - ) - })?, - )); - - // Call the subsaga to delete the temp volume - builder.append(Node::subsaga( - "final_no_result", - subsaga_dag, - "params_for_delete_subsaga", - )); + // Soft-delete the temp volume + builder.append(soft_delete_volume_action()); Ok(builder.build()?) } @@ -179,3 +154,22 @@ async fn svr_remove_read_only_parent( .map_err(saga_action_failed)?; Ok(()) } + +async fn svr_soft_delete_volume( + sagactx: NexusActionContext, +) -> Result<(), ActionError> { + let osagactx = sagactx.user_data(); + + let temp_volume_id = sagactx.lookup::("temp_volume_id")?; + + osagactx.datastore().soft_delete_volume(temp_volume_id).await.map_err( + |e| { + saga_action_failed(Error::internal_error(&format!( + "failed to soft_delete_volume: {:?}", + e, + ))) + }, + )?; + + Ok(()) +} diff --git a/nexus/src/app/snapshot.rs b/nexus/src/app/snapshot.rs index b17b32b8db0..2c1bdb86bae 100644 --- a/nexus/src/app/snapshot.rs +++ b/nexus/src/app/snapshot.rs @@ -204,6 +204,8 @@ impl super::Nexus { ) .await?; + self.background_tasks.task_volume_delete.activate(); + Ok(()) } } diff --git a/nexus/test-utils/src/background.rs b/nexus/test-utils/src/background.rs index 7580749b19d..ddc91409777 100644 --- a/nexus/test-utils/src/background.rs +++ b/nexus/test-utils/src/background.rs @@ -6,12 +6,17 @@ use crate::http_testing::NexusRequest; use dropshot::test_util::ClientTestContext; +use nexus_db_queries::context::OpContext; +use nexus_db_queries::db::DataStore; use nexus_lockstep_client::types::BackgroundTask; use nexus_lockstep_client::types::CurrentStatus; use nexus_lockstep_client::types::LastResult; +use nexus_types::identity::Asset; use nexus_types::internal_api::background::*; use omicron_test_utils::dev::poll::{CondCheckError, wait_for_condition}; use slog::info; +use slog::o; +use std::sync::Arc; use std::time::Duration; /// Given the name of a background task, wait for it to complete if it's @@ -57,7 +62,7 @@ pub async fn wait_background_task( /// /// The `timeout` parameter controls how long to wait for the task to go idle /// before activating it, and how long to wait for it to complete after -/// activation. Defaults to 10 seconds if not specified. +/// activation. Defaults to 30 seconds if not specified. pub async fn activate_background_task( lockstep_client: &ClientTestContext, task_name: &str, @@ -65,7 +70,7 @@ pub async fn activate_background_task( activate_background_task_with_timeout( lockstep_client, task_name, - Duration::from_secs(10), + Duration::from_secs(30), ) .await } @@ -581,3 +586,166 @@ pub async fn run_blueprint_rendezvous(lockstep_client: &ClientTestContext) { ) .unwrap(); } + +/// Run the volume_delete background task, and assert that there are no reported +/// errors. +pub async fn run_volume_delete(internal_client: &ClientTestContext) { + let status = run_volume_delete_return_status(internal_client).await; + assert!(status.errors.is_empty()); +} + +/// Run the volume_delete background task and return the status. +pub async fn run_volume_delete_return_status( + internal_client: &ClientTestContext, +) -> VolumeDeleteStatus { + let last_background_task = + activate_background_task(&internal_client, "volume_delete").await; + + let LastResult::Completed(last_result_completed) = + last_background_task.last + else { + panic!( + "unexpected {:?} returned from volume_delete task", + last_background_task.last, + ); + }; + + serde_json::from_value::(last_result_completed.details) + .unwrap() +} + +pub async fn wait_for_all_volume_deletes( + datastore: &Arc, + lockstep_client: &ClientTestContext, +) { + wait_for_condition( + || { + let datastore = datastore.clone(); + let opctx = OpContext::for_tests( + lockstep_client.client_log.new(o!()), + datastore.clone(), + ); + + async move { + // Trigger the volume delete background task. Bail out of this + // loop only when there's no more soft-deleted volumes. + // + // Be careful not to check if the background tasks performed any + // actions: the fixed point that we're waiting for is for all + // resources to be cleaned up. + + run_volume_delete(lockstep_client).await; + + let mut soft_deleted_volumes_left = + datastore.get_soft_deleted_volumes(&opctx).await.unwrap(); + + // Filter out volumes that have allocated regions left, these + // will not be deleted. + let soft_deleted_volumes_left = { + let mut result = + Vec::with_capacity(soft_deleted_volumes_left.len()); + + while let Some(volume) = soft_deleted_volumes_left.pop() { + let allocated_regions = datastore + .get_allocated_regions(volume.id()) + .await + .unwrap(); + + if allocated_regions.is_empty() { + result.push(volume); + } + } + + result.len() + }; + + if soft_deleted_volumes_left > 0 { + info!( + &lockstep_client.client_log, + "wait_for_all_volume_deletes: \ + {soft_deleted_volumes_left} soft-deleted volumes left", + ); + + return Err(CondCheckError::<()>::NotYet { status: None }); + } + + Ok(()) + } + }, + &std::time::Duration::from_millis(50), + &std::time::Duration::from_secs(260), + ) + .await + .expect("all deletes finished"); +} + +/// Run the local_storage_delete background task and return the status. +pub async fn run_local_storage_delete(internal_client: &ClientTestContext) { + let last_background_task = + activate_background_task(&internal_client, "local_storage_delete") + .await; + + let LastResult::Completed(last_result_completed) = + last_background_task.last + else { + panic!( + "unexpected {:?} returned from volume_delete task", + last_background_task.last, + ); + }; + + let status = serde_json::from_value::( + last_result_completed.details, + ) + .unwrap(); + + assert!(status.errors.is_empty()); +} + +pub async fn wait_for_all_local_storage_deletes( + datastore: &Arc, + lockstep_client: &ClientTestContext, +) { + wait_for_condition( + || { + let datastore = datastore.clone(); + let opctx = OpContext::for_tests( + lockstep_client.client_log.new(o!()), + datastore.clone(), + ); + + async move { + // Trigger the local storage delete background task. Bail out of + // this loop only when there's no more allocations to clean up. + // + // Be careful not to check if the background tasks performed any + // actions: the fixed point that we're waiting for is for all + // resources to be cleaned up. + + run_local_storage_delete(lockstep_client).await; + + let disks_requiring_work = datastore + .deleted_disks_with_undeleted_local_storage(&opctx) + .await + .unwrap(); + + if !disks_requiring_work.is_empty() { + info!( + &lockstep_client.client_log, + "wait_for_all_local_storage_deletes: {} disks \ + requiring work left", + disks_requiring_work.len(), + ); + + return Err(CondCheckError::<()>::NotYet { status: None }); + } + + Ok(()) + } + }, + &std::time::Duration::from_millis(50), + &std::time::Duration::from_secs(260), + ) + .await + .expect("all deletes finished"); +} diff --git a/nexus/tests/config.test.toml b/nexus/tests/config.test.toml index 567291b21a3..ee564db766a 100644 --- a/nexus/tests/config.test.toml +++ b/nexus/tests/config.test.toml @@ -237,6 +237,8 @@ audit_log_cleanup.period_secs = 600 audit_log_cleanup.retention_days = 90 audit_log_cleanup.max_deleted_per_activation = 10000 populate_switch_ports.period_secs = 30 +volume_delete.period_secs = 10000 +local_storage_delete.period_secs = 10000 [multicast] # Enable multicast functionality for tests (disabled by default in production) diff --git a/nexus/tests/integration_tests/common.rs b/nexus/tests/integration_tests/common.rs new file mode 100644 index 00000000000..daa72618b73 --- /dev/null +++ b/nexus/tests/integration_tests/common.rs @@ -0,0 +1,18 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +use nexus_test_utils::ControlPlaneTestContext; +use nexus_test_utils::background::wait_for_all_volume_deletes; +use nexus_test_utils::resource_helpers::DiskTest; + +pub(crate) async fn assert_all_crucible_resources_deleted<'a>( + cptestctx: &ControlPlaneTestContext, + disk_test: &DiskTest<'a, omicron_nexus::Server>, +) { + let nexus = &cptestctx.server.server_context().nexus; + let datastore = nexus.datastore(); + let lockstep_client = &cptestctx.lockstep_client; + wait_for_all_volume_deletes(&datastore, &lockstep_client).await; + assert!(disk_test.crucible_resources_deleted().await); +} diff --git a/nexus/tests/integration_tests/crucible_replacements.rs b/nexus/tests/integration_tests/crucible_replacements.rs index de4796aaccf..5ef7e6bed13 100644 --- a/nexus/tests/integration_tests/crucible_replacements.rs +++ b/nexus/tests/integration_tests/crucible_replacements.rs @@ -4,6 +4,7 @@ //! Tests related to region and region snapshot replacement +use crate::integration_tests::common::assert_all_crucible_resources_deleted; use async_bb8_diesel::AsyncRunQueryDsl; use diesel::ExpressionMethods; use diesel::QueryDsl; @@ -122,6 +123,13 @@ pub(crate) async fn wait_for_all_replacements( run_all_crucible_replacement_tasks(lockstep_client).await; + // Also, run the volume delete background task. This is + // important because some of the checks for moving replacement + // requests along will check if volumes are soft or hard + // deleted. + + run_volume_delete(lockstep_client).await; + let ro_left_to_do = datastore .find_read_only_regions_on_expunged_physical_disks(&opctx) .await @@ -299,6 +307,7 @@ mod region_replacement { client: ClientTestContext, lockstep_client: ClientTestContext, replacement_request_id: Uuid, + cptestctx: &'a ControlPlaneTestContext, } impl<'a> DeletedVolumeTest<'a> { @@ -372,6 +381,7 @@ mod region_replacement { client: client.clone(), lockstep_client: lockstep_client.clone(), replacement_request_id, + cptestctx, } } @@ -416,8 +426,12 @@ mod region_replacement { RegionReplacementState::Complete, ); - // Assert there are no more Crucible resources - assert!(self.disk_test.crucible_resources_deleted().await); + // Assert there are no undeleted Crucible resources + assert_all_crucible_resources_deleted( + &self.cptestctx, + &self.disk_test, + ) + .await; } async fn wait_for_request_state( @@ -1050,6 +1064,8 @@ async fn test_racing_replacements_for_soft_deleted_disk_volume( let snapshot_id = snapshot.identity.id; async move { + run_volume_delete(&lockstep_client).await; + let region_snapshot = datastore .region_snapshot_get(dataset_id, region_id, snapshot_id) .await @@ -1304,7 +1320,7 @@ async fn test_racing_replacements_for_soft_deleted_disk_volume( // Now, assert that all crucible resources are cleaned up - assert!(disk_test.crucible_resources_deleted().await); + assert_all_crucible_resources_deleted(cptestctx, &disk_test).await; } mod region_snapshot_replacement { @@ -1328,6 +1344,7 @@ mod region_snapshot_replacement { lockstep_client: ClientTestContext, replacement_request_id: Uuid, snapshot_socket_addr: SocketAddr, + cptestctx: &'a ControlPlaneTestContext, } impl<'a> DeletedVolumeTest<'a> { @@ -1468,6 +1485,7 @@ mod region_snapshot_replacement { lockstep_client: lockstep_client.clone(), replacement_request_id, snapshot_socket_addr, + cptestctx, } } @@ -1508,8 +1526,8 @@ mod region_snapshot_replacement { /// completion /// - this harness' region snapshot replacement request has transitioned /// to Complete - /// - there are no more volumes that reference the request's region - /// snapshot + /// - there are no non-deleted volumes that reference the request's + /// region snapshot pub async fn finish_test(&self) { // Make sure that all the background tasks can run to completion. @@ -1532,40 +1550,34 @@ mod region_snapshot_replacement { RegionSnapshotReplacementState::Complete, ); - // Assert no volumes are referencing the snapshot address + // Wait for all volume deletes, then assert no non-deleted volumes + // are referencing the snapshot address - let mut counter = 1; - loop { - let volumes = self - .datastore - .find_volumes_referencing_socket_addr( - &self.opctx(), - self.snapshot_socket_addr, - ) - .await - .unwrap(); + wait_for_all_volume_deletes(&self.datastore, &self.lockstep_client) + .await; - if !volumes.is_empty() { - eprintln!( - "Volume should be gone, try {counter} {:?}", - volumes - ); - tokio::time::sleep(std::time::Duration::from_secs(5)).await; - counter += 1; - if counter > 200 { - panic!( - "Tried 200 times, and still this did not finish" - ); - } - } else { - break; - } - } + let volumes: Vec<_> = self + .datastore + .find_volumes_referencing_socket_addr( + &self.opctx(), + self.snapshot_socket_addr, + ) + .await + .unwrap() + .into_iter() + .filter(|volume| volume.time_deleted.is_none()) + .collect(); + + assert!(volumes.is_empty()); } /// Assert no Crucible resources are leaked pub async fn assert_no_crucible_resources_leaked(&self) { - assert!(self.disk_test.crucible_resources_deleted().await); + assert_all_crucible_resources_deleted( + &self.cptestctx, + &self.disk_test, + ) + .await; } async fn wait_for_request_state( @@ -1738,6 +1750,8 @@ mod region_snapshot_replacement { ); let mut i = 1; loop { + run_volume_delete(&self.lockstep_client).await; + let region_snapshot_replace_request = self .datastore .get_region_snapshot_replacement_request_by_id( @@ -2763,6 +2777,8 @@ async fn test_replacement_sanity_twice_after_snapshot_delete( // Delete the snapshot delete_snapshot(&client, PROJECT_NAME, "snap").await; + wait_for_all_volume_deletes(&datastore, &lockstep_client).await; + // Assert snapshot volume is gone let db_snapshot = datastore .snapshot_get(&opctx, snapshot.identity.id) @@ -2834,4 +2850,6 @@ async fn test_replacement_sanity_twice_after_snapshot_delete( wait_for_all_replacements(&datastore, &lockstep_client).await; } + + wait_for_all_volume_deletes(&datastore, &lockstep_client).await; } diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index dbc6f0ecc66..2f571e7f2c0 100644 --- a/nexus/tests/integration_tests/disks.rs +++ b/nexus/tests/integration_tests/disks.rs @@ -5,6 +5,7 @@ //! Tests basic disk support in the API use super::instances::instance_wait_for_state; +use crate::integration_tests::common::assert_all_crucible_resources_deleted; use dropshot::HttpErrorResponseBody; use dropshot::test_util::ClientTestContext; use http::StatusCode; @@ -19,6 +20,8 @@ use nexus_db_queries::db::datastore::RegionAllocationFor; use nexus_db_queries::db::datastore::RegionAllocationParameters; use nexus_db_queries::db::fixed_data::FLEET_ID; use nexus_test_utils::SLED_AGENT_UUID; +use nexus_test_utils::background::run_volume_delete_return_status; +use nexus_test_utils::background::wait_for_all_volume_deletes; use nexus_test_utils::http_testing::AuthnMode; use nexus_test_utils::http_testing::NexusRequest; use nexus_test_utils::http_testing::RequestBuilder; @@ -59,7 +62,6 @@ use sled_agent_client::VolumeConstructionRequest; use std::collections::HashSet; use std::collections::VecDeque; use std::sync::Arc; -use tokio::sync::oneshot; use uuid::Uuid; type ControlPlaneTestContext = @@ -1321,11 +1323,10 @@ async fn test_disk_virtual_provisioning_collection( ); } +/// Confirm that region deletes can happen async to the higher user-level +/// resources #[nexus_test] -async fn test_disk_virtual_provisioning_collection_failed_delete( - cptestctx: &ControlPlaneTestContext, -) { - // Confirm that there's no panic deleting a project if a disk deletion fails +async fn test_async_region_delete(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; let nexus = &cptestctx.server.server_context().nexus; let datastore = nexus.datastore(); @@ -1387,50 +1388,24 @@ async fn test_disk_virtual_provisioning_collection_failed_delete( .get_crucible_dataset(zpool.id, dataset.id) .set_region_deletion_error(true); - // Delete the disk - expect this to fail + // Delete the disk - this will succeed but the third region won't be + // properly deleted yet NexusRequest::new( RequestBuilder::new(client, Method::DELETE, &disk_url) - .expect_status(Some(StatusCode::INTERNAL_SERVER_ERROR)), + .expect_status(Some(StatusCode::NO_CONTENT)), ) .authn_as(AuthnMode::PrivilegedUser) .execute() .await - .expect("unexpected success deleting 1 GiB disk"); + .expect("unexpected error deleting 1 GiB disk"); - // The virtual provisioning collection numbers haven't changed - let virtual_provisioning_collection = datastore - .virtual_provisioning_collection_get(&opctx, project_id1) - .await - .unwrap(); - assert_eq!( - virtual_provisioning_collection.virtual_disk_bytes_provisioned.0, - disk_size - ); + // Assert running the volume delete task deletes the two other regions but + // errors when trying to delete the third. + let status = + run_volume_delete_return_status(&cptestctx.lockstep_client).await; - // And the disk is now faulted. The name will have changed due to the - // "undelete and fault" function. - let disk_url = format!( - "/v1/disks/deleted-{}?project={}", - disk.identity.id, PROJECT_NAME - ); - let disk = disk_get(&client, &disk_url).await; - assert_eq!(disk.state, DiskState::Faulted); - - // Set the third agent to respond normally - cptestctx - .first_sled_agent() - .get_crucible_dataset(zpool.id, dataset.id) - .set_region_deletion_error(false); - - // Request disk delete again - NexusRequest::new( - RequestBuilder::new(client, Method::DELETE, &disk_url) - .expect_status(Some(StatusCode::NO_CONTENT)), - ) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .expect("unexpected failure deleting 1 GiB disk"); + assert!(!status.errors.is_empty()); + assert_eq!(status.region_results.len(), 2); // Delete the project's default VPC subnet and VPC let subnet_url = @@ -1448,7 +1423,7 @@ async fn test_disk_virtual_provisioning_collection_failed_delete( .await .expect("failed to make request"); - // The project can be deleted now + // Assert the project can be deleted even if the region isn't yet. let url = format!("/v1/projects/{}", PROJECT_NAME); NexusRequest::new( RequestBuilder::new(client, Method::DELETE, &url) @@ -1458,6 +1433,16 @@ async fn test_disk_virtual_provisioning_collection_failed_delete( .execute() .await .expect("unexpected failure deleting project"); + + // Set the third agent to respond normally + cptestctx + .first_sled_agent() + .get_crucible_dataset(zpool.id, dataset.id) + .set_region_deletion_error(false); + + // Wait for all crucible resources to be cleaned up + + assert_all_crucible_resources_deleted(&cptestctx, &disk_test).await; } #[nexus_test] @@ -1591,6 +1576,7 @@ async fn test_disk_size_accounting(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; let nexus = &cptestctx.server.server_context().nexus; let datastore = nexus.datastore(); + let lockstep_client = &cptestctx.lockstep_client; // Create three zpools, each with one dataset. let test = DiskTest::new(&cptestctx).await; @@ -1698,6 +1684,8 @@ async fn test_disk_size_accounting(cptestctx: &ControlPlaneTestContext) { .await .expect("unexpected failure deleting 7 GiB disk"); + wait_for_all_volume_deletes(&datastore, &lockstep_client).await; + // Total occupied size should be 0 for zpool in test.zpools() { let dataset = zpool.crucible_dataset(); @@ -2326,29 +2314,27 @@ async fn test_no_halt_disk_delete_one_region_on_expunged_agent( cptestctx.first_sled_agent().drop_dataset(zpool.id, dataset.id); - // Spawn a task that tries to delete the disk + // Ensure the disk delete does not hang even when a Crucible agent isn't + // responding + let disk_url = get_disk_url(DISK_NAME); - let client = client.clone(); - let (task_started_tx, task_started_rx) = oneshot::channel(); + NexusRequest::object_delete(&client, &disk_url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("failed to delete disk"); - let jh = tokio::spawn(async move { - task_started_tx.send(()).unwrap(); + // Run the volume delete task, and assert two regions are deleted. - NexusRequest::object_delete(&client, &disk_url) - .authn_as(AuthnMode::PrivilegedUser) - .execute() - .await - .expect("failed to delete disk"); - }); + let status = + run_volume_delete_return_status(&cptestctx.lockstep_client).await; - // Wait until the task starts - task_started_rx.await.unwrap(); - - // It won't finish until the dataset is expunged. - assert!(!jh.is_finished()); + assert!(!status.errors.is_empty()); + assert_eq!(status.region_results.len(), 2); // Expunge the physical disk + let (_, db_zpool) = LookupPath::new(&opctx, datastore) .zpool_id(zpool.id) .fetch() @@ -2364,11 +2350,19 @@ async fn test_no_halt_disk_delete_one_region_on_expunged_agent( .await .unwrap(); - // Now, the delete call will finish Ok - jh.await.unwrap(); + // Rerun the volume delete task and assert the last region was deleted. This + // test asserts against 3 regions being deleted because the volume record + // won't be hard-deleted, and the background task will re-attempt each + // delete. + + let status = + run_volume_delete_return_status(&cptestctx.lockstep_client).await; - // Ensure that the disk was properly deleted and all the regions are gone - - // Nexus should hard delete the region records in this case. + assert!(status.errors.is_empty()); + assert_eq!(status.region_results.len(), 3); + + // Assert that all the regions are gone - Nexus should hard delete the + // region records in this case. let datasets_and_regions = datastore.get_allocated_regions(db_disk.volume_id()).await.unwrap(); @@ -2616,6 +2610,12 @@ async fn test_zpool_control_plane_storage_buffer( .await .expect("failed to delete disk"); + // This test is testing the control plane storage buffer, not the background + // volume deletion. Wait for all those to complete, otherwise later on the + // test will see unrelated INSUFFICIENT_STORAGE errors. + + wait_for_all_volume_deletes(&datastore, &cptestctx.lockstep_client).await; + // For any of the zpools, set the control plane storage buffer to 2G. This // should prevent the disk's region allocation from succeeding (as the // reserved sizes of 10G + 5G plus the storage buffer of 2G is 1G over the diff --git a/nexus/tests/integration_tests/mod.rs b/nexus/tests/integration_tests/mod.rs index dc1fc84c2b4..f07bb1bb3c6 100644 --- a/nexus/tests/integration_tests/mod.rs +++ b/nexus/tests/integration_tests/mod.rs @@ -18,6 +18,7 @@ mod bfd; mod certificates; mod cockroach; mod commands; +mod common; mod console_api; mod crucible_replacements; mod data_migrations; diff --git a/nexus/tests/integration_tests/snapshots.rs b/nexus/tests/integration_tests/snapshots.rs index a13eec1a5ec..82ffdf6dff7 100644 --- a/nexus/tests/integration_tests/snapshots.rs +++ b/nexus/tests/integration_tests/snapshots.rs @@ -4,6 +4,7 @@ //! Tests basic snapshot support in the API +use crate::integration_tests::common::assert_all_crucible_resources_deleted; use crate::integration_tests::instances::instance_simulate; use chrono::Utc; use dropshot::test_util::ClientTestContext; @@ -995,7 +996,7 @@ async fn test_snapshot_unwind(cptestctx: &ControlPlaneTestContext) { .expect("failed to delete disk"); // Assert everything was cleaned up - assert!(disk_test.crucible_resources_deleted().await); + assert_all_crucible_resources_deleted(cptestctx, &disk_test).await; } // Test that the code that Saga nodes call is idempotent diff --git a/nexus/tests/integration_tests/volume_management.rs b/nexus/tests/integration_tests/volume_management.rs index 700c0dc3a00..e605dc71426 100644 --- a/nexus/tests/integration_tests/volume_management.rs +++ b/nexus/tests/integration_tests/volume_management.rs @@ -5,6 +5,7 @@ //! Tests that Nexus properly manages and cleans up Crucible resources //! associated with Volumes +use crate::integration_tests::common::assert_all_crucible_resources_deleted; use crate::integration_tests::crucible_replacements::wait_for_all_replacements; use crate::integration_tests::sleds::sleds_list; use async_bb8_diesel::AsyncRunQueryDsl; @@ -37,6 +38,7 @@ use nexus_db_queries::db::datastore::SourceVolume; use nexus_db_queries::db::datastore::VolumeReplaceResult; use nexus_db_queries::db::datastore::VolumeToDelete; use nexus_db_queries::db::datastore::VolumeWithTarget; +use nexus_test_utils::background::wait_for_all_volume_deletes; use nexus_test_utils::http_testing::AuthnMode; use nexus_test_utils::http_testing::NexusRequest; use nexus_test_utils::http_testing::RequestBuilder; @@ -210,7 +212,7 @@ async fn test_snapshot_then_delete_disk(cptestctx: &ControlPlaneTestContext) { .expect("failed to delete snapshot"); // Assert everything was cleaned up - assert!(disk_test.crucible_resources_deleted().await); + assert_all_crucible_resources_deleted(cptestctx, &disk_test).await; } #[nexus_test] @@ -274,7 +276,7 @@ async fn test_delete_snapshot_then_disk(cptestctx: &ControlPlaneTestContext) { .expect("failed to delete disk"); // Assert everything was cleaned up - assert!(disk_test.crucible_resources_deleted().await); + assert_all_crucible_resources_deleted(cptestctx, &disk_test).await; } #[nexus_test] @@ -342,7 +344,7 @@ async fn test_multiple_snapshots(cptestctx: &ControlPlaneTestContext) { } // Assert everything was cleaned up - assert!(disk_test.crucible_resources_deleted().await); + assert_all_crucible_resources_deleted(cptestctx, &disk_test).await; } #[nexus_test] @@ -353,6 +355,11 @@ async fn test_snapshot_prevents_other_disk( // allocation. let client = &cptestctx.external_client; + let lockstep_client = &cptestctx.lockstep_client; + let apictx = &cptestctx.server.server_context(); + let nexus = &apictx.nexus; + let datastore = nexus.datastore(); + let disk_test = DiskTest::new(&cptestctx).await; let disks_url = get_disks_url(); let base_disk_name: Name = "base-disk".parse().unwrap(); @@ -427,6 +434,11 @@ async fn test_snapshot_prevents_other_disk( .await .expect("failed to delete snapshot"); + // Wait for the volume delete background task, else the next disk creation + // may fail with INSUFFICIENT_STORAGE + + wait_for_all_volume_deletes(&datastore, &lockstep_client).await; + // Disk allocation will work now let _next_disk: external::Disk = NexusRequest::new( RequestBuilder::new(client, Method::POST, &disks_url) @@ -451,7 +463,7 @@ async fn test_snapshot_prevents_other_disk( delete_image(client, PROJECT_NAME, "not-alpine").await; // Assert everything was cleaned up - assert!(disk_test.crucible_resources_deleted().await); + assert_all_crucible_resources_deleted(cptestctx, &disk_test).await; } #[nexus_test] @@ -590,7 +602,7 @@ async fn test_multiple_disks_multiple_snapshots_order_1( .expect("failed to delete snapshot"); // Assert everything was cleaned up - assert!(disk_test.crucible_resources_deleted().await); + assert_all_crucible_resources_deleted(cptestctx, &disk_test).await; } #[nexus_test] @@ -729,7 +741,7 @@ async fn test_multiple_disks_multiple_snapshots_order_2( .expect("failed to delete snapshot"); // Assert everything was cleaned up - assert!(disk_test.crucible_resources_deleted().await); + assert_all_crucible_resources_deleted(cptestctx, &disk_test).await; } async fn prepare_for_test_multiple_layers_of_snapshots( @@ -908,7 +920,7 @@ async fn test_multiple_layers_of_snapshots_delete_all_disks_first( } // Assert everything was cleaned up - assert!(disk_test.crucible_resources_deleted().await); + assert_all_crucible_resources_deleted(cptestctx, &disk_test).await; } #[nexus_test] @@ -946,7 +958,7 @@ async fn test_multiple_layers_of_snapshots_delete_all_snapshots_first( } // Assert everything was cleaned up - assert!(disk_test.crucible_resources_deleted().await); + assert_all_crucible_resources_deleted(cptestctx, &disk_test).await; } #[nexus_test] @@ -1010,7 +1022,7 @@ async fn test_multiple_layers_of_snapshots_random_delete_order( } // Assert everything was cleaned up - assert!(disk_test.crucible_resources_deleted().await); + assert_all_crucible_resources_deleted(cptestctx, &disk_test).await; } #[nexus_test] @@ -1160,7 +1172,7 @@ async fn test_create_image_from_snapshot_delete( .expect("failed to delete image"); // Assert everything was cleaned up - assert!(disk_test.crucible_resources_deleted().await); + assert_all_crucible_resources_deleted(cptestctx, &disk_test).await; } enum DeleteImageTestParam { @@ -1287,7 +1299,7 @@ async fn delete_image_test( } // Assert everything was cleaned up - assert!(disk_test.crucible_resources_deleted().await); + assert_all_crucible_resources_deleted(cptestctx, &disk_test).await; } // Make sure that whatever order disks, images, and snapshots are deleted, the @@ -2512,7 +2524,7 @@ async fn test_disk_create_saga_unwinds_correctly( .unwrap(); // Assert everything was cleaned up - assert!(disk_test.crucible_resources_deleted().await); + assert_all_crucible_resources_deleted(cptestctx, &disk_test).await; } #[nexus_test] @@ -2586,7 +2598,7 @@ async fn test_snapshot_create_saga_unwinds_correctly( .expect("failed to delete disk"); // Assert everything was cleaned up - assert!(disk_test.crucible_resources_deleted().await); + assert_all_crucible_resources_deleted(cptestctx, &disk_test).await; } // Test function that creates a VolumeConstructionRequest::Region With gen, @@ -4144,6 +4156,8 @@ async fn test_read_only_region_reference_counting( .await .expect("failed to delete disk"); + wait_for_all_volume_deletes(&datastore, &lockstep_client).await; + let usage = datastore .volume_usage_records_for_resource( VolumeResourceUsage::ReadOnlyRegion { @@ -4185,7 +4199,7 @@ async fn test_read_only_region_reference_counting( assert!(region_destroyed); // Assert everything was cleaned up - assert!(disk_test.crucible_resources_deleted().await); + assert_all_crucible_resources_deleted(cptestctx, &disk_test).await; } /// Assert that a snapshot of a volume with a read-only region is properly @@ -4440,7 +4454,7 @@ async fn test_read_only_region_reference_counting_layers( .expect("failed to delete disk"); // Assert everything was cleaned up - assert!(disk_test.crucible_resources_deleted().await); + assert_all_crucible_resources_deleted(cptestctx, &disk_test).await; } #[nexus_test] @@ -5137,7 +5151,8 @@ async fn test_double_layer_with_read_only_region_delete( .await .expect("failed to delete another-disk-from-snapshot"); - assert!(disk_test.crucible_resources_deleted().await); + // Assert everything was cleaned up + assert_all_crucible_resources_deleted(cptestctx, &disk_test).await; } #[nexus_test(extra_sled_agents = 3)] @@ -5316,8 +5331,7 @@ async fn test_double_layer_snapshot_with_read_only_region_delete_2( .expect("failed to delete disk-from-snapshot"); // Assert everything was cleaned up - - assert!(disk_test.crucible_resources_deleted().await); + assert_all_crucible_resources_deleted(cptestctx, &disk_test).await; } #[nexus_test(extra_sled_agents = 3)] diff --git a/nexus/types/src/internal_api/background.rs b/nexus/types/src/internal_api/background.rs index 2a352a7923a..5982ed09099 100644 --- a/nexus/types/src/internal_api/background.rs +++ b/nexus/types/src/internal_api/background.rs @@ -1365,6 +1365,24 @@ pub struct PhysicalDiskAdoptionStatus { pub errors: Vec, } +/// The status of a `volume_delete` background task activation +#[derive(Serialize, Deserialize, Default, Debug, PartialEq, Eq)] +pub struct VolumeDeleteStatus { + pub region_results: Vec, + pub running_snapshot_results: Vec, + pub snapshot_results: Vec, + pub volumes_deleted: Vec, + pub errors: Vec, +} + +/// The status of a `local_storage_delete` background task activation +#[derive(Serialize, Deserialize, Default, Debug, PartialEq, Eq)] +pub struct LocalStorageDeleteStatus { + pub delete_results: Vec, + pub deallocate_results: Vec, + pub errors: Vec, +} + #[cfg(test)] mod test { use super::TufRepoInfo; diff --git a/smf/nexus/multi-sled/config-partial.toml b/smf/nexus/multi-sled/config-partial.toml index bfe9dbad187..879fe3a147f 100644 --- a/smf/nexus/multi-sled/config-partial.toml +++ b/smf/nexus/multi-sled/config-partial.toml @@ -132,6 +132,8 @@ audit_log_cleanup.period_secs = 600 audit_log_cleanup.retention_days = 90 audit_log_cleanup.max_deleted_per_activation = 10000 populate_switch_ports.period_secs = 30 +volume_delete.period_secs = 30 +local_storage_delete.period_secs = 30 [default_region_allocation_strategy] # by default, allocate across 3 distinct sleds diff --git a/smf/nexus/single-sled/config-partial.toml b/smf/nexus/single-sled/config-partial.toml index 8e36b8bd4ec..ff9b16efc25 100644 --- a/smf/nexus/single-sled/config-partial.toml +++ b/smf/nexus/single-sled/config-partial.toml @@ -132,6 +132,8 @@ audit_log_cleanup.period_secs = 600 audit_log_cleanup.retention_days = 90 audit_log_cleanup.max_deleted_per_activation = 10000 populate_switch_ports.period_secs = 30 +volume_delete.period_secs = 30 +local_storage_delete.period_secs = 30 [default_region_allocation_strategy] # by default, allocate without requirement for distinct sleds. From c50958ca2e0caf68b1476ce6596197ede1d7c771 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Fri, 7 Aug 2026 16:44:38 +0000 Subject: [PATCH 2/2] doctest exposed some stale comments --- .../src/app/background/tasks/volume_delete.rs | 26 ++++++++----------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/nexus/src/app/background/tasks/volume_delete.rs b/nexus/src/app/background/tasks/volume_delete.rs index 71a2b3e10d8..99e27f56e2b 100644 --- a/nexus/src/app/background/tasks/volume_delete.rs +++ b/nexus/src/app/background/tasks/volume_delete.rs @@ -920,16 +920,13 @@ impl VolumeDeleter { status.volumes_deleted.push(volume_id.to_string()); } - /// Deleting region snapshots in a previous saga node may have freed up - /// regions that were deleted in the DB but couldn't be deleted by the - /// Crucible Agent because a snapshot existed. Look for those here. These - /// will be a different volume id (i.e. for a previously deleted disk) than - /// the one in this saga's params struct. + /// Deleting region snapshots may have freed up regions that were deleted in + /// the DB but couldn't previously be deleted by the Crucible Agent because + /// a snapshot existed. Look for those here. /// - /// It's insufficient to rely on the struct of CrucibleResources to clean up - /// that is returned as part of svd_decrease_crucible_resource_count. - /// Imagine a disk that is composed of three regions (a subset of - /// [`sled_agent_client::VolumeConstructionRequest`] is shown here): + /// It's insufficient to rely on the struct of CrucibleResources to clean + /// up: imagine a disk that is composed of three regions (a subset of the + /// VolumeConstructionRequest is shown here): /// /// ```json /// { @@ -989,12 +986,11 @@ impl VolumeDeleter { /// /// /crucible/0/regions/{id}/snapshots/{name} /// - /// If the disk is then deleted, the volume delete saga will run for the - /// first volume shown here. The CrucibleResources struct returned as part - /// of [`svd_decrease_crucible_resource_count`] will contain *nothing* to - /// clean up: the regions contain snapshots that are part of other volumes - /// and cannot be deleted, and the disk's volume doesn't reference any - /// read-only resources. + /// If the disk is then deleted, the CrucibleResources struct returned as + /// from the soft-delete function will contain *nothing* to clean up: the + /// regions contain snapshots that are part of other volumes and cannot be + /// deleted, and the disk's volume doesn't reference any read-only + /// resources. /// /// This is expected and normal: regions are "leaked" all the time due to /// snapshots preventing their deletion. This function detects when those