Fix broken disk list (#9553)

For shame!

---------

Co-authored-by: Eliza Weisman <eliza@elizas.website>

Author: jmpesp

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<DbConnection>,
-        instance_id: Uuid,
-        pagparams: &PaginatedBy<'_>,
+        results: Vec<(
+            model::Disk,
+            Option<DiskTypeCrucible>,
+            Option<DiskTypeLocalStorage>,
+        )>,
     ) -> ListResultVec<Disk> {
-        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::<DiskTypeCrucible>::as_select(),
-            Option::<DiskTypeLocalStorage>::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<DbConnection>,
+        instance_id: Uuid,
+        pagparams: &PaginatedBy<'_>,
+    ) -> ListResultVec<Disk> {
+        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(Name::ref_cast),
+            ),
+        }
+        .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::<DiskTypeCrucible>::as_select(),
+            Option::<DiskTypeLocalStorage>::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<DbConnection>,
         err: OptionalError<Error>,
@@ -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::<DiskTypeCrucible>::as_select(),
+            Option::<DiskTypeLocalStorage>::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:

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)