Skip to content

Commit 95696ef

Browse files
committed
avoid floating point math
reservation right now is 25%, which means that the requested size of a region can be divided by 4. avoid floating point math where possible. change the reservation percentage stored with the region to an enum, where the only value is 25%. this limits what can be done with manual database edits, and restricts what the Region::reserved_size function has to guard against. it'd be nice if Region::new was a test-only function but the crate doesn't have the same idea of a integration test feature.
1 parent e6b489a commit 95696ef

File tree

19 files changed

+313
-170
lines changed

19 files changed

+313
-170
lines changed

nexus/db-model/src/region.rs

Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
use super::ByteCount;
66
use crate::SqlU16;
7+
use crate::impl_enum_type;
78
use crate::schema::region;
89
use crate::typed_uuid::DbTypedUuid;
910
use db_macros::Asset;
@@ -15,6 +16,19 @@ use omicron_uuid_kinds::VolumeUuid;
1516
use serde::{Deserialize, Serialize};
1617
use uuid::Uuid;
1718

19+
impl_enum_type!(
20+
#[derive(SqlType, Debug, QueryId)]
21+
#[diesel(postgres_type(name = "region_reservation_percent", schema = "public"))]
22+
pub struct RegionReservationPercentEnum;
23+
24+
#[derive(Copy, Clone, Debug, AsExpression, FromSqlRow, Serialize, Deserialize, PartialEq)]
25+
#[diesel(sql_type = RegionReservationPercentEnum)]
26+
pub enum RegionReservationPercent;
27+
28+
// Enum values
29+
TwentyFive => b"25"
30+
);
31+
1832
/// Database representation of a Region.
1933
///
2034
/// A region represents a portion of a Crucible Downstairs dataset
@@ -58,13 +72,13 @@ pub struct Region {
5872

5973
// The Agent will reserve space for Downstairs overhead when creating the
6074
// corresponding ZFS dataset. Nexus has to account for that: store that
61-
// reservation factor here as it may change in the future, and it can be
62-
// used during Crucible related accounting.
63-
reservation_factor: f64,
75+
// reservation percent here as it may change in the future, and it can be
76+
// used during Crucible related accounting. This is stored as an enum to
77+
// restrict the values to what the Crucible Agent uses.
78+
reservation_percent: RegionReservationPercent,
6479
}
6580

6681
impl Region {
67-
#[allow(clippy::too_many_arguments)]
6882
pub fn new(
6983
dataset_id: DatasetUuid,
7084
volume_id: VolumeUuid,
@@ -73,7 +87,6 @@ impl Region {
7387
extent_count: u64,
7488
port: u16,
7589
read_only: bool,
76-
reservation_factor: f64,
7790
) -> Self {
7891
Self {
7992
identity: RegionIdentity::new(Uuid::new_v4()),
@@ -85,7 +98,10 @@ impl Region {
8598
port: Some(port.into()),
8699
read_only,
87100
deleting: false,
88-
reservation_factor,
101+
// When the Crucible agent's reservation percentage changes, this
102+
// function should accept that as argument. Until then, it can only
103+
// ever be 25%.
104+
reservation_percent: RegionReservationPercent::TwentyFive,
89105
}
90106
}
91107

@@ -122,7 +138,9 @@ impl Region {
122138
self.deleting
123139
}
124140

125-
/// The size of the Region without accounting for any overhead
141+
/// The size of the Region without accounting for any overhead. The
142+
/// `allocation_query` function should have validated that this won't
143+
/// overflow.
126144
pub fn requested_size(&self) -> u64 {
127145
self.block_size().to_bytes()
128146
* self.blocks_per_extent()
@@ -133,6 +151,10 @@ impl Region {
133151
/// which is some factor higher than the requested region size to account
134152
/// for on-disk overhead.
135153
pub fn reserved_size(&self) -> u64 {
136-
(self.requested_size() as f64 * self.reservation_factor).round() as u64
154+
let overhead = match &self.reservation_percent {
155+
RegionReservationPercent::TwentyFive => self.requested_size() / 4,
156+
};
157+
158+
self.requested_size() + overhead
137159
}
138160
}

nexus/db-model/src/schema.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1126,7 +1126,7 @@ table! {
11261126

11271127
deleting -> Bool,
11281128

1129-
reservation_factor -> Float8,
1129+
reservation_percent -> crate::RegionReservationPercentEnum,
11301130
}
11311131
}
11321132

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -269,7 +269,7 @@ impl DataStore {
269269
},
270270
allocation_strategy,
271271
num_regions_required,
272-
);
272+
)?;
273273

274274
let conn = self.pool_connection_authorized(&opctx).await?;
275275

@@ -317,7 +317,7 @@ impl DataStore {
317317
// Update datasets to which the regions belonged.
318318
// XXX put this file somewhere else
319319
sql_query(include_str!(
320-
"../../../../../schema/crdb/crucible-agent-reservation-overhead/up03.sql"
320+
"../../../../../schema/crdb/crucible-agent-reservation-overhead/up04.sql"
321321
))
322322
.execute_async(&conn)
323323
.await?;

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4715,7 +4715,6 @@ mod tests {
47154715
region.extent_count(),
47164716
111,
47174717
false, // read-write
4718-
1.0,
47194718
);
47204719

47214720
use nexus_db_model::schema::region::dsl;
@@ -4953,7 +4952,6 @@ mod tests {
49534952
region.extent_count(),
49544953
111,
49554954
true, // read-only
4956-
1.0,
49574955
);
49584956

49594957
use nexus_db_model::schema::region::dsl;

0 commit comments

Comments
 (0)