Account for Crucible Agent reservation overhead

When creating a region's dataset, the Crucible Agent will include a
reservation 25% larger than the region's size to account for on-disk
overhead (storing encryption contexts and other metadata). Nexus does
not take this overhead into account when computing `size_used` for
crucible_dataset rows, or when allocating regions. This leads to the
scenario where Nexus thinks there's enough room for a region but the
Agent will fail to create the dataset due to not having enough space for
the reservation to succeed.

Fix this: add a reservation factor column to the Region model, and
account for this when performing region allocation and when computing
the `size_used` column for crucible datasets.

This commit also adds an upgrader that will set all currently allocated
Region's reservation factor to 1.25, and recompute all the `size_used`
values for all non-deleted crucible datasets. This may lead to
`size_used` being greater than the pool's total_size - a follow up
commit will add an omdb command to identify these cases, and identify
candidate regions to request replacement for in order to remedy this.

The `regions_hard_delete` function now uses this upgrader's CTE to set
`size_used` for all crucible datasets at once, instead of in a for loop
during an interactive transaction.

Author: jmpesp

nexus/db-model/src/region.rs

@@ -55,9 +55,16 @@ pub struct Region {
     // Shared read-only regions require a "deleting" flag to avoid a
     // use-after-free scenario
     deleting: bool,
+
+    // The Agent will reserve space for Downstairs overhead when creating the
+    // corresponding ZFS dataset. Nexus has to account for that: store that
+    // reservation factor here as it may change in the future, and it can be
+    // used during Crucible related accounting.
+    reservation_factor: f64,
 }
 
 impl Region {
+    #[allow(clippy::too_many_arguments)]
     pub fn new(
         dataset_id: DatasetUuid,
         volume_id: VolumeUuid,
@@ -66,6 +73,7 @@ impl Region {
         extent_count: u64,
         port: u16,
         read_only: bool,
+        reservation_factor: f64,
     ) -> Self {
         Self {
             identity: RegionIdentity::new(Uuid::new_v4()),
@@ -77,6 +85,7 @@ impl Region {
             port: Some(port.into()),
             read_only,
             deleting: false,
+            reservation_factor,
         }
     }
 
@@ -112,4 +121,18 @@ impl Region {
     pub fn deleting(&self) -> bool {
         self.deleting
     }
+
+    /// The size of the Region without accounting for any overhead
+    pub fn requested_size(&self) -> u64 {
+        self.block_size().to_bytes()
+            * self.blocks_per_extent()
+            * self.extent_count()
+    }
+
+    /// The size the Crucible agent would have reserved during ZFS creation,
+    /// which is some factor higher than the requested region size to account
+    /// for on-disk overhead.
+    pub fn reserved_size(&self) -> u64 {
+        (self.requested_size() as f64 * self.reservation_factor).round() as u64
+    }
 }

nexus/db-model/src/schema.rs

@@ -1125,6 +1125,8 @@ table! {
         read_only -> Bool,
 
         deleting -> Bool,
+
+        reservation_factor -> Float8,
     }
 }
 

nexus/db-model/src/schema_versions.rs

@@ -16,7 +16,7 @@ use std::{collections::BTreeMap, sync::LazyLock};
 ///
 /// This must be updated when you change the database schema.  Refer to
 /// schema/crdb/README.adoc in the root of this repository for details.
-pub const SCHEMA_VERSION: Version = Version::new(132, 0, 0);
+pub const SCHEMA_VERSION: Version = Version::new(133, 0, 0);
 
 /// List of all past database schema versions, in *reverse* order
 ///
@@ -28,6 +28,7 @@ static KNOWN_VERSIONS: LazyLock<Vec<KnownVersion>> = LazyLock::new(|| {
         // |  leaving the first copy as an example for the next person.
         // v
         // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"),
+        KnownVersion::new(133, "crucible-agent-reservation-overhead"),
         KnownVersion::new(132, "bp-omicron-zone-filesystem-pool-not-null"),
         KnownVersion::new(131, "tuf-generation"),
         KnownVersion::new(130, "bp-sled-agent-generation"),

nexus/db-queries/src/db/datastore/region.rs

@@ -23,8 +23,8 @@ use crate::db::pagination::paginated;
 use crate::db::queries::region_allocation::RegionParameters;
 use crate::db::update_and_check::UpdateAndCheck;
 use crate::db::update_and_check::UpdateStatus;
-use crate::transaction_retry::OptionalError;
 use async_bb8_diesel::AsyncRunQueryDsl;
+use diesel::dsl::sql_query;
 use diesel::prelude::*;
 use nexus_config::RegionAllocationStrategy;
 use nexus_types::external_api::params;
@@ -301,68 +301,26 @@ impl DataStore {
             return Ok(());
         }
 
-        #[derive(Debug, thiserror::Error)]
-        enum RegionDeleteError {
-            #[error("Numeric error: {0}")]
-            NumericError(String),
-        }
-        let err = OptionalError::new();
         let conn = self.pool_connection_unauthorized().await?;
         self.transaction_retry_wrapper("regions_hard_delete")
             .transaction(&conn, |conn| {
-                let err = err.clone();
                 let region_ids = region_ids.clone();
                 async move {
-                    use db::schema::crucible_dataset::dsl as dataset_dsl;
-                    use db::schema::region::dsl as region_dsl;
+                    use db::schema::region::dsl as dsl;
 
-                    // Remove the regions, collecting datasets they're from.
-                    let datasets = diesel::delete(region_dsl::region)
-                        .filter(region_dsl::id.eq_any(region_ids))
-                        .returning(region_dsl::dataset_id)
-                        .get_results_async::<Uuid>(&conn).await?;
+                    // Remove the regions
+                    diesel::delete(dsl::region)
+                        .filter(dsl::id.eq_any(region_ids))
+                        .execute_async(&conn)
+                        .await?;
 
                     // Update datasets to which the regions belonged.
-                    for dataset in datasets {
-                        let dataset_total_occupied_size: Option<
-                            diesel::pg::data_types::PgNumeric,
-                        > = region_dsl::region
-                            .filter(region_dsl::dataset_id.eq(dataset))
-                            .select(diesel::dsl::sum(
-                                region_dsl::block_size
-                                    * region_dsl::blocks_per_extent
-                                    * region_dsl::extent_count,
-                            ))
-                            .nullable()
-                            .get_result_async(&conn).await?;
-
-                        let dataset_total_occupied_size: i64 = if let Some(
-                            dataset_total_occupied_size,
-                        ) =
-                            dataset_total_occupied_size
-                        {
-                            let dataset_total_occupied_size: db::model::ByteCount =
-                                dataset_total_occupied_size.try_into().map_err(
-                                    |e: anyhow::Error| {
-                                        err.bail(RegionDeleteError::NumericError(
-                                            e.to_string(),
-                                        ))
-                                    },
-                                )?;
-
-                            dataset_total_occupied_size.into()
-                        } else {
-                            0
-                        };
-
-                        diesel::update(dataset_dsl::crucible_dataset)
-                            .filter(dataset_dsl::id.eq(dataset))
-                            .set(
-                                dataset_dsl::size_used
-                                    .eq(dataset_total_occupied_size),
-                            )
-                            .execute_async(&conn).await?;
-                    }
+                    // XXX put this file somewhere else
+                    sql_query(include_str!(
+                        "../../../../../schema/crdb/crucible-agent-reservation-overhead/up03.sql"
+                    ))
+                    .execute_async(&conn)
+                    .await?;
 
                     // Whenever a region is hard-deleted, validate invariants
                     // for all volumes
@@ -373,52 +331,25 @@ impl DataStore {
                 }
             })
             .await
-            .map_err(|e| {
-                if let Some(err) = err.take() {
-                    match err {
-                        RegionDeleteError::NumericError(err) => {
-                            return Error::internal_error(
-                                &format!("Transaction error: {}", err)
-                            );
-                        }
-                    }
-                }
-                public_error_from_diesel(e, ErrorHandler::Server)
-            })
+            .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))
     }
 
-    /// Return the total occupied size for a dataset
-    pub async fn regions_total_occupied_size(
+    /// Return the total reserved size for all the regions allocated to a
+    /// dataset
+    pub async fn regions_total_reserved_size(
         &self,
         dataset_id: DatasetUuid,
     ) -> Result<u64, Error> {
-        use db::schema::region::dsl as region_dsl;
-
-        let total_occupied_size: Option<diesel::pg::data_types::PgNumeric> =
-            region_dsl::region
-                .filter(region_dsl::dataset_id.eq(to_db_typed_uuid(dataset_id)))
-                .select(diesel::dsl::sum(
-                    region_dsl::block_size
-                        * region_dsl::blocks_per_extent
-                        * region_dsl::extent_count,
-                ))
-                .nullable()
-                .get_result_async(&*self.pool_connection_unauthorized().await?)
-                .await
-                .map_err(|e| {
-                    public_error_from_diesel(e, ErrorHandler::Server)
-                })?;
+        use db::schema::region::dsl;
 
-        if let Some(total_occupied_size) = total_occupied_size {
-            let total_occupied_size: db::model::ByteCount =
-                total_occupied_size.try_into().map_err(
-                    |e: anyhow::Error| Error::internal_error(&e.to_string()),
-                )?;
+        let dataset_regions: Vec<Region> = dsl::region
+            .filter(dsl::dataset_id.eq(to_db_typed_uuid(dataset_id)))
+            .select(Region::as_select())
+            .load_async(&*self.pool_connection_unauthorized().await?)
+            .await
+            .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?;
 
-            Ok(total_occupied_size.to_bytes())
-        } else {
-            Ok(0)
-        }
+        Ok(dataset_regions.iter().map(|r| r.reserved_size()).sum())
     }
 
     /// Find read/write regions on expunged disks

nexus/db-queries/src/db/datastore/volume.rs

@@ -4715,6 +4715,7 @@ mod tests {
                 region.extent_count(),
                 111,
                 false, // read-write
+                1.0,
             );
 
             use nexus_db_model::schema::region::dsl;
@@ -4952,6 +4953,7 @@ mod tests {
                 region.extent_count(),
                 111,
                 true, // read-only
+                1.0,
             );
 
             use nexus_db_model::schema::region::dsl;

nexus/db-queries/src/db/queries/region_allocation.rs

@@ -117,8 +117,14 @@ pub fn allocation_query(
 
     let seed = seed.to_le_bytes().to_vec();
 
-    let size_delta =
+    // The Crucible Agent's current reservation factor
+    const RESERVATION_FACTOR: f64 = 1.25;
+
+    let requested_size: u64 =
         params.block_size * params.blocks_per_extent * params.extent_count;
+    let size_delta: u64 =
+        (requested_size as f64 * RESERVATION_FACTOR).round() as u64;
+
     let redundancy: i64 = i64::try_from(redundancy).unwrap();
 
     let mut builder = QueryBuilder::new();
@@ -272,7 +278,8 @@ pub fn allocation_query(
       ").param().sql(" AS extent_count,
       NULL AS port,
       ").param().sql(" AS read_only,
-      FALSE as deleting
+      FALSE as deleting,
+      ").param().sql(" AS reservation_factor
     FROM shuffled_candidate_datasets")
   // Only select the *additional* number of candidate regions for the required
   // redundancy level
@@ -286,6 +293,7 @@ pub fn allocation_query(
     .bind::<sql_types::BigInt, _>(params.blocks_per_extent as i64)
     .bind::<sql_types::BigInt, _>(params.extent_count as i64)
     .bind::<sql_types::Bool, _>(params.read_only)
+    .bind::<sql_types::Float8, _>(RESERVATION_FACTOR)
     .bind::<sql_types::BigInt, _>(redundancy)
 
     // A subquery which summarizes the changes we intend to make, showing:
@@ -298,7 +306,17 @@ pub fn allocation_query(
     SELECT
       candidate_regions.dataset_id AS id,
       crucible_dataset.pool_id AS pool_id,
-      ((candidate_regions.block_size * candidate_regions.blocks_per_extent) * candidate_regions.extent_count) AS size_used_delta
+      CAST(
+       CAST(
+        candidate_regions.block_size *
+        candidate_regions.blocks_per_extent *
+        candidate_regions.extent_count
+        AS FLOAT
+       )
+       * candidate_regions.reservation_factor
+       AS INT
+      )
+    AS size_used_delta
     FROM (candidate_regions INNER JOIN crucible_dataset ON (crucible_dataset.id = candidate_regions.dataset_id))
   ),")
 
@@ -385,7 +403,7 @@ pub fn allocation_query(
     .sql("
   inserted_regions AS (
     INSERT INTO region
-      (id, time_created, time_modified, dataset_id, volume_id, block_size, blocks_per_extent, extent_count, port, read_only, deleting)
+      (id, time_created, time_modified, dataset_id, volume_id, block_size, blocks_per_extent, extent_count, port, read_only, deleting, reservation_factor)
     SELECT ").sql(AllColumnsOfRegion::with_prefix("candidate_regions")).sql("
     FROM candidate_regions
     WHERE