From 3ff8c5b9816c42529331d90dff8c58be6111ee98 Mon Sep 17 00:00:00 2001 From: James MacMahon Date: Fri, 19 Dec 2025 21:47:37 +0000 Subject: [PATCH] Fix broken disk list For shame! --- nexus/db-queries/src/db/datastore/disk.rs | 121 +++++++++++----------- nexus/tests/integration_tests/disks.rs | 75 ++++++++++++++ 2 files changed, 136 insertions(+), 60 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/disk.rs b/nexus/db-queries/src/db/datastore/disk.rs index a0365aa76e7..29f0126abf9 100644 --- a/nexus/db-queries/src/db/datastore/disk.rs +++ b/nexus/db-queries/src/db/datastore/disk.rs @@ -445,46 +445,16 @@ impl DataStore { .await } - /// List disks associated with a given instance by name. - pub async fn instance_list_disks_on_conn( - &self, + /// Consume a query result listing all parts of the higher level Disk type, + /// and assemble it. + async fn process_tuples_to_disk_list( conn: &async_bb8_diesel::Connection, - instance_id: Uuid, - pagparams: &PaginatedBy<'_>, + results: Vec<( + model::Disk, + Option, + Option, + )>, ) -> ListResultVec { - use nexus_db_schema::schema::disk::dsl; - use nexus_db_schema::schema::disk_type_crucible::dsl as disk_type_crucible_dsl; - use nexus_db_schema::schema::disk_type_local_storage::dsl as disk_type_local_storage_dsl; - - let results = match pagparams { - PaginatedBy::Id(pagparams) => { - paginated(dsl::disk, dsl::id, &pagparams) - } - PaginatedBy::Name(pagparams) => paginated( - dsl::disk, - dsl::name, - &pagparams.map_name(|n| Name::ref_cast(n)), - ), - } - .left_join( - disk_type_crucible_dsl::disk_type_crucible - .on(dsl::id.eq(disk_type_crucible_dsl::disk_id)), - ) - .left_join( - disk_type_local_storage_dsl::disk_type_local_storage - .on(dsl::id.eq(disk_type_local_storage_dsl::disk_id)), - ) - .filter(dsl::time_deleted.is_null()) - .filter(dsl::attach_instance_id.eq(instance_id)) - .select(( - model::Disk::as_select(), - Option::::as_select(), - Option::::as_select(), - )) - .get_results_async(conn) - .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; - let mut list = Vec::with_capacity(results.len()); for result in results { @@ -555,6 +525,49 @@ impl DataStore { Ok(list) } + /// List disks associated with a given instance by name. + pub async fn instance_list_disks_on_conn( + &self, + conn: &async_bb8_diesel::Connection, + instance_id: Uuid, + pagparams: &PaginatedBy<'_>, + ) -> ListResultVec { + use nexus_db_schema::schema::disk::dsl; + use nexus_db_schema::schema::disk_type_crucible::dsl as disk_type_crucible_dsl; + use nexus_db_schema::schema::disk_type_local_storage::dsl as disk_type_local_storage_dsl; + + let results = match pagparams { + PaginatedBy::Id(pagparams) => { + paginated(dsl::disk, dsl::id, &pagparams) + } + PaginatedBy::Name(pagparams) => paginated( + dsl::disk, + dsl::name, + &pagparams.map_name(|n| Name::ref_cast(n)), + ), + } + .left_join( + disk_type_crucible_dsl::disk_type_crucible + .on(dsl::id.eq(disk_type_crucible_dsl::disk_id)), + ) + .left_join( + disk_type_local_storage_dsl::disk_type_local_storage + .on(dsl::id.eq(disk_type_local_storage_dsl::disk_id)), + ) + .filter(dsl::time_deleted.is_null()) + .filter(dsl::attach_instance_id.eq(instance_id)) + .select(( + model::Disk::as_select(), + Option::::as_select(), + Option::::as_select(), + )) + .get_results_async(conn) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + + Self::process_tuples_to_disk_list(conn, results).await + } + pub(super) async fn project_create_disk_in_txn( conn: &async_bb8_diesel::Connection, err: OptionalError, @@ -702,6 +715,9 @@ impl DataStore { use nexus_db_schema::schema::disk::dsl; use nexus_db_schema::schema::disk_type_crucible::dsl as disk_type_crucible_dsl; + use nexus_db_schema::schema::disk_type_local_storage::dsl as disk_type_local_storage_dsl; + + let conn = self.pool_connection_authorized(opctx).await?; let results = match pagparams { PaginatedBy::Id(pagparams) => { @@ -717,37 +733,22 @@ impl DataStore { disk_type_crucible_dsl::disk_type_crucible .on(dsl::id.eq(disk_type_crucible_dsl::disk_id)), ) + .left_join( + disk_type_local_storage_dsl::disk_type_local_storage + .on(dsl::id.eq(disk_type_local_storage_dsl::disk_id)), + ) .filter(dsl::time_deleted.is_null()) .filter(dsl::project_id.eq(authz_project.id())) .select(( model::Disk::as_select(), Option::::as_select(), + Option::::as_select(), )) - .get_results_async(&*self.pool_connection_authorized(opctx).await?) + .get_results_async(&*conn) .await .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; - let mut list = Vec::with_capacity(results.len()); - - for result in results { - match result { - (disk, Some(disk_type_crucible)) => { - list.push(Disk::Crucible(CrucibleDisk { - disk, - disk_type_crucible, - })); - } - - (disk, None) => { - return Err(Error::internal_error(&format!( - "disk {} is invalid!", - disk.id(), - ))); - } - } - } - - Ok(list) + Self::process_tuples_to_disk_list(&conn, results).await } /// Attaches a disk to an instance, if both objects: diff --git a/nexus/tests/integration_tests/disks.rs b/nexus/tests/integration_tests/disks.rs index 66adb606918..47114387a93 100644 --- a/nexus/tests/integration_tests/disks.rs +++ b/nexus/tests/integration_tests/disks.rs @@ -2658,6 +2658,81 @@ async fn test_zpool_control_plane_storage_buffer( .unwrap(); } +#[nexus_test] +async fn test_list_all_types_of_disk(cptestctx: &ControlPlaneTestContext) { + // Create three zpools, each with one dataset + DiskTestBuilder::new(&cptestctx) + .on_all_sleds() + .with_zpool_count(3) + .build() + .await; + + // Assert default is still 16 GiB + assert_eq!(16, DiskTest::DEFAULT_ZPOOL_SIZE_GIB); + + let client = &cptestctx.external_client; + create_project_and_pool(client).await; + + let disks_url = get_disks_url(); + + // Distributed disk + let new_disk = params::DiskCreate { + identity: IdentityMetadataCreateParams { + name: "disk1".parse().unwrap(), + description: String::from("sells rainsticks"), + }, + disk_backend: params::DiskBackend::Distributed { + disk_source: params::DiskSource::Blank { + block_size: params::BlockSize::try_from(512).unwrap(), + }, + }, + size: ByteCount::from_gibibytes_u32(1), + }; + + NexusRequest::new( + RequestBuilder::new(client, Method::POST, &disks_url) + .body(Some(&new_disk)) + .expect_status(Some(StatusCode::CREATED)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap(); + + // Local disk + + let new_disk = params::DiskCreate { + identity: IdentityMetadataCreateParams { + name: "disk2".parse().unwrap(), + description: String::from("sells rainsticks"), + }, + disk_backend: params::DiskBackend::Local {}, + size: ByteCount::from_gibibytes_u32(1), + }; + + NexusRequest::new( + RequestBuilder::new(client, Method::POST, &disks_url) + .body(Some(&new_disk)) + .expect_status(Some(StatusCode::CREATED)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap(); + + // List them all + + NexusRequest::new( + RequestBuilder::new(client, Method::GET, &disks_url) + .body(Some(&new_disk)) + .expect_status(Some(StatusCode::OK)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap(); +} + async fn disk_get(client: &ClientTestContext, disk_url: &str) -> Disk { NexusRequest::object_get(client, disk_url) .authn_as(AuthnMode::PrivilegedUser)