Account for Crucible Agent reservation overhead #7885

jmpesp · 2025-03-27T20:51:39Z

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.

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.

smklein · 2025-03-28T00:49:46Z

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

+    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;


Do we want to ceil() here, and elsewhere we call round()?

Hrm, actually, reading this and the code below, it weirds me out a little bit that we're calculating this information twice, as far as I can tell? See below, in proposed_dataset_changes, seems like we're doing this same calculation again...

maybe this is fine, but it just raised a flag to me -- if we're doing floating point math on accounting, it kinda seems like we should do it "exactly once".

95696ef removes the floating point stuff, and also uses the size_delta variable in the spot you identified. nice catch :)

smklein

Couple small questions, but the structure here seems reasonable to me. Thanks for adding the data migration test.

smklein · 2025-03-28T01:00:31Z

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

-                            )
-                            .execute_async(&conn).await?;
-                    }
+                    // XXX put this file somewhere else


As you say, we should probably fix this before merging

yeah, I thought about this, and ended up embedding it as a string: 4834bd9. it's going to have to change independent of that update CTE anyway in the future.

smklein · 2025-03-28T01:06:04Z

schema/crdb/dbinit.sql

-    deleting BOOL NOT NULL
+    deleting BOOL NOT NULL,
+
+    reservation_factor FLOAT NOT NULL


This definitely could benefit from docs, and it would probably be worth updating the "size_used" field in this file too.

13f2a07 adds some notes here, lmk what you think.

bnaecker

Drive-by review, but just a few suggestions on the floating-point math, which almost always sucks.

bnaecker · 2025-03-28T01:12:36Z

schema/crdb/dbinit.sql

-    deleting BOOL NOT NULL
+    deleting BOOL NOT NULL,
+
+    reservation_factor FLOAT NOT NULL


It seems like we might want a CHECK reservation_factor >= 1.0 here.

this changed to an enum, the value is no longer unrestricted

bnaecker · 2025-03-28T01:14:16Z

nexus/db-model/src/region.rs

+    /// 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


It seems like ceil() might be safer than round()ing. I'm also not sure how to handle it, but checking for overflow in the conversion back to a u64 seems helpful too. One way might be adding checks in the Region::new() constructor, say that the factor is >= 1.0 and that the reserved size won't overflow a u64.

95696ef removes the floating point math, and restricts the factor via an enum.

I like it, much simpler.

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.

it will have to change independent of the schema update anyway

jmpesp · 2025-03-28T20:56:16Z

@smklein @bnaecker thanks for the reviews - I've pushed updates so that there's no more floating point math in the db queries or the related rust code, let me know what you think :)

leftwo

I get a 500 if I'm out of space:

18:10:26.013Z INFO 4b3e083b-f9b8-41d9-a48d-a22683a7853e (dropshot_external): request completed
    error_message_external = Internal Server Error
    error_message_internal = saga ACTION error at node "regions_ensure": Failed to create region, unexpected state: Failed
    file = /home/alan/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/dropshot-0.16.0/src/server.rs:855
    latency_us = 939684
    local_addr = 172.30.2.5:80
    method = POST
    remote_addr = 192.168.1.199:57148
    req_id = 1e314f60-df29-4c7e-8ae3-130ccc33883c
    response_code = 500
    uri = /v1/disks?project=alan

leftwo · 2025-03-31T17:27:14Z

nexus/db-model/src/region.rs

+    #[diesel(sql_type = RegionReservationPercentEnum)]
+    pub enum RegionReservationPercent;
+
+    // Enum values
+    TwentyFive => b"25"


This is really 1.25%, or it could be called RegionAdditionalReservationPercent (which name I hate, too long)

nexus/db-model/src/region.rs

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

leftwo

A few more nit-picks for you

nexus/test-utils/src/resource_helpers.rs

schema/crdb/dbinit.sql

jmpesp · 2025-04-02T15:41:52Z

@leftwo I think the 500 you're seeing is a result of #7902 - the follow-up PR that creates a storage buffer should prevent this 500 from happening, and instead return the much more appropriate 507.

smklein · 2025-04-03T16:56:22Z

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

+type AllocationQuery =
+    TypedSqlQuery<(SelectableSql<CrucibleDataset>, SelectableSql<Region>)>;
+
+impl std::fmt::Debug for AllocationQuery {


How load-bearing is this Debug impl?

not needed at all, removed in dd6ae81

smklein · 2025-04-03T17:31:16Z

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

+    }
+
+    // After the above check, unconditionally cast from u64 to i64. The value is
+    // low enough that this shouldn't truncate.


FWIW I don't think it would truncate either way, I think it would fail with an error?

after discussing it a bit, went with returning an Err instead of unwrapping in 7d9fed6

smklein · 2025-04-03T17:35:56Z

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

+    // The Crucible Agent's current reservation factor is 25%, so add that here.
+    // Check first that the requested region size is divisible by this. This
+    // should basically never fail because all block sizes are divisible by 4.
+    if requested_size % 4 != 0 {


This is contingent on the value of reservation_percent, and the enum RegionReservationPercent only having a single value?

If we add another value to that enum, this code will happily still compile (incorrectly), right?

I know this is pedantic, but could we:

Move up the

let reservation_percent = crate::db::model::RegionReservationPercent::TwentyFive;

from below

Run these "mod by four, set factor = 4" checks in a match arm, based on reservation_percent?

Then, if we add another option for RegionReservationPercent, the code will complain about a missing match arm, which would help identify this spot.

I think this pattern is always a good idea, done in 1e47888

smklein · 2025-04-03T17:37:36Z

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

        logctx.cleanup_successful();
    }
+
+    #[tokio::test]


Nice tests!

As a nitpick, neither of these tests needs to be async (they could just be #[test]) if you want 'em to be marginally cheaper

👍 be6599b

smklein · 2025-04-03T17:38:45Z

schema/crdb/dbinit.sql

     * otherwise ignores this field. It's updated by Nexus as region allocations
     * and deletions are performed using this dataset.
+     *
+     * Note that the value in this column is _not_ the sum of requested region


Thank you for adding this!!!

jmpesp requested a review from smklein March 27, 2025 20:51

jmpesp mentioned this pull request Mar 27, 2025

keep the system from running out of disk space #7875

Open

18 tasks

smklein reviewed Mar 28, 2025

View reviewed changes

bnaecker reviewed Mar 28, 2025

View reviewed changes

morlandi7 added this to the 14 milestone Mar 28, 2025

morlandi7 assigned jmpesp Mar 28, 2025

jmpesp added 4 commits March 28, 2025 20:34

embed size_used update CTE

4834bd9

it will have to change independent of the schema update anyway

fmt

f031305

more docs is always better

13f2a07

jmpesp added 3 commits March 31, 2025 17:34

Merge branch 'main' into crucible_agent_region_overhead

d60e501

Merge branch 'main' into crucible_agent_region_overhead

5fd77aa

fix after merge

f7dbdf8

leftwo reviewed Apr 1, 2025

View reviewed changes

leftwo reviewed Apr 2, 2025

View reviewed changes

nexus/test-utils/src/resource_helpers.rs Show resolved Hide resolved

schema/crdb/dbinit.sql Outdated Show resolved Hide resolved

better comment for size_used

84c9d7e

smklein approved these changes Apr 3, 2025

View reviewed changes

jmpesp added 5 commits April 3, 2025 18:31

impl not necessary

dd6ae81

use match to make adding variants a compile error

1e47888

async not required

be6599b

fmt

f8b9fa8

no unwrap, return err instead

7d9fed6

jmpesp enabled auto-merge (squash) April 3, 2025 19:04

smklein mentioned this pull request Apr 3, 2025

zpool reports a different total size than zfs #7902

Open

jmpesp merged commit e6b3fea into oxidecomputer:main Apr 3, 2025
16 checks passed

jmpesp deleted the crucible_agent_region_overhead branch April 3, 2025 20:41

jmpesp mentioned this pull request Apr 9, 2025

crucible zone running out of space earlier than the control plane "thinks" #4234

Closed

Account for Crucible Agent reservation overhead #7885

Account for Crucible Agent reservation overhead #7885

Uh oh!

Conversation

jmpesp commented Mar 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smklein left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnaecker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmpesp commented Mar 28, 2025

Uh oh!

leftwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

leftwo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jmpesp commented Apr 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!