Pruning should affect candidate datasets too by jmpesp · Pull Request #10877 · oxidecomputer/omicron

jmpesp · 2026-07-21T15:58:40Z

A few weeks ago we were debugging an exhaustive search through all local storage allocations, and determined it was caused by the invalid PgNumeric conversion to ByteCount. The next step was to cause that search to bail out in order to unwind the saga: this was supposed to be achieved by adjusting size_used column of the rendezvous local storage dataset to a number that would not be affected by the invalid conversion, and we did this, but observed that the search continued anyway!

Consider the following incomplete allocation list:

    0: allocation0 -> zpool0
    1: allocation1 -> zpool1
    2: allocation2 -> zpool2
    3: allocation3 -> zpool3
    4: allocation4 -> zpool4
    5: allocation5 -> zpool5
    6: allocation6 -> zpool6
    7: allocation7 -> zpool7

Note the allocations and zpools here are ordered to make the example easier but will actually be random depending on the order of rows returned.

The remaining allocations to perform are 8 and 9, and they have a list of zpools to try to allocate against. The pathologically bad case is if any zpool could be a candidate:

    allocation8: [zpool0, zpool1, ..., zpool6, ..., zpool8, zpool9]
    allocation9: [zpool0, zpool1, ..., zpool6, ..., zpool8, zpool9]

Previous to this commit, the pruning step only affected the list of incomplete allocations. This doesn't cause the search to bail out! If zpool6 runs out of space due to concurrent allocations, the pruning step will not retain incomplete allocations that use zpool6:

    0: allocation0 -> zpool0
    1: allocation1 -> zpool1
    2: allocation2 -> zpool2
    3: allocation3 -> zpool3
    4: allocation4 -> zpool4
    5: allocation5 -> zpool5
    6: allocation6 -> zpool6 <- any search that made it this far
    7: allocation7 -> zpool7    will be pruned

but the iterator will continue to descend down the search space using zpool6 because it's still in the candidate list:

    0: allocation0 -> zpool0
    1: allocation1 -> zpool1
    2: allocation2 -> zpool2
    3: allocation3 -> zpool3
    4: allocation4 -> zpool4
    5: allocation5 -> zpool5

    allocation6: [zpool0, zpool1, ..., zpool6, ..., zpool8, zpool9]
                                         ^- still in the list!

This commit adds a pruning step that removes from an allocation's candidate list based on the updated ZpoolGetForSledReservationResult information.

A few weeks ago we were debugging an exhaustive search through all local storage allocations, and determined it was caused by the invalid PgNumeric conversion to ByteCount. The next step was to cause that search to bail out in order to unwind the saga: this was _supposed_ to be achieved by adjusting `size_used` column of the rendezvous local storage dataset to a number that would not be affected by the invalid conversion, and we did this, but observed that the search continued anyway! Consider the following incomplete allocation list: 0: allocation0 -> zpool0 1: allocation1 -> zpool1 2: allocation2 -> zpool2 3: allocation3 -> zpool3 4: allocation4 -> zpool4 5: allocation5 -> zpool5 6: allocation6 -> zpool6 7: allocation7 -> zpool7 Note the allocations and zpools here are ordered to make the example easier but will actually be random depending on the order of rows returned. The remaining allocations to perform are 8 and 9, and they have a list of zpools to try to allocate against. The pathologically bad case is if any zpool could be a candidate: allocation8: [zpool0, zpool1, ..., zpool6, ..., zpool8, zpool9] allocation9: [zpool0, zpool1, ..., zpool6, ..., zpool8, zpool9] Previous to this commit, the pruning step only affected the list of incomplete allocations. This doesn't cause the search to bail out! If zpool6 runs out of space due to concurrent allocations, the pruning step will not retain incomplete allocations that use zpool6: 0: allocation0 -> zpool0 1: allocation1 -> zpool1 2: allocation2 -> zpool2 3: allocation3 -> zpool3 4: allocation4 -> zpool4 5: allocation5 -> zpool5 6: allocation6 -> zpool6 <- any search that made it this far 7: allocation7 -> zpool7 will be pruned but the iterator will continue to descend down the search space using zpool6 because it's still in the candidate list: 0: allocation0 -> zpool0 1: allocation1 -> zpool1 2: allocation2 -> zpool2 3: allocation3 -> zpool3 4: allocation4 -> zpool4 5: allocation5 -> zpool5 allocation6: [zpool0, zpool1, ..., zpool6, ..., zpool8, zpool9] ^- still in the list! This commit adds a pruning step that removes from an allocation's candidate list based on the updated ZpoolGetForSledReservationResult information.

smklein · 2026-07-21T19:48:17Z

+            info!(self.log, "removed {removed} from queue");
+        }
+
+        removed = 0;


nit: Can we use a different variable name here? I'd rather these be distinct, rather than trying to rely on "resetting" a single value with two different meanings

+1, done in 0dc86bb

smklein · 2026-07-21T19:50:29Z

        });
+
+        if removed > 0 {
+            info!(self.log, "removed {removed} from queue");


Suggested change

info!(self.log, "removed {removed} from queue");

info!(self.log, "removed allocation lists from consideration during instance provisioning"; "count" => removed);

changed to be less vague in 0dc86bb, but I didn't separate out the count, I'd rather that be part of the message

smklein · 2026-07-21T19:51:36Z

+        }
+
+        if removed > 0 {
+            info!(self.log, "removed {removed} from candidate datasets");


Suggested change

info!(self.log, "removed {removed} from candidate datasets");

info!(self.log, "removed candidate datasets from consideration during instance allocation"; "count" => removed);

also 0dc86bb

smklein · 2026-07-21T21:17:16Z

+        if removed > 0 {
+            info!(self.log, "removed {removed} from candidate datasets");
+        }
    }


What do you think about re-assessing a more holistic check of "can this set fit" here?

There are a couple checks we perform when initially assembling the CompleteLocalStorageAllocationsList:

"Are there enough pools to satisfy this request"
https://github.com/jmpesp/omicron/blob/5886e0028363594e435a8fdaa1fdc01ba787cfd2/nexus/db-queries/src/db/datastore/sled.rs#L465-L477

"Are there any remaining candidate datasets"
https://github.com/jmpesp/omicron/blob/5886e0028363594e435a8fdaa1fdc01ba787cfd2/nexus/db-queries/src/db/datastore/sled.rs#L505-L516

We could presumably re-check these constraints here, with something like:

let distinct_pools: HashSet<ZpoolUuid> = self .allocations_to_perform .iter() .flat_map(|r| r.candidate_datasets.iter().map(|c| c.pool_id)) .collect(); let infeasible = self .allocations_to_perform .iter() .any(|r| r.candidate_datasets.is_empty()) || distinct_pools.len() < self.allocations_to_perform.len(); if infeasible { self.queue.clear(); }

Without doing this, I think these cases of "you asked for N disks, but we only have M < N zpools" will be doing a significant amount of work in the invocation of next() within CompleteLocalStorageAllocationLists. Internally, the self.queue.pop() in that next implementation will need to get exhausted before returning, which seems expensive.

For example:

Suppose we're in that while let Some(incomplete_allocation_list) = self.queue.pop() loop, and we've made k projected allocations thus far

For the next request k, we loop over candidate_datasets, keeping the ones that are still in candidates_left (which, in this case, would be M - k pools). For each of those M-k pools, we push new possible IncompleteAllocationLists.

Because we have a "loop, pushing children (and doing clones) for each candidate_dataset", I think this is a factorial space exploration before we exhaust the set of candidate allocations.

As an aside: We might want to re-evaluate this whole approach here, and consider a more efficient bipartite matching algorithm to avoid having a worst-case-factorial search here in the first place? I think my proposal would help in cases where we have an instance requesting N homogenously-sized disks, but I'm not sure it works if the local storage disks are differently-sized.

I think you're right that the iterator logic will spin if someone asked for N disks but we only have M < N zpools, but there's already a Not enough zpools to satisfy the number of allocations required check for that in the iterator's new. I added a test that exercises this in 511cd30

smklein · 2026-07-21T21:59:20Z

+                    affinity: None,
+                    ncpus: 2,
+                    memory: external::ByteCount::from_gibibytes_u32(16),
+                    disks: (0..10)


I think the pessimistic case I described above could be triggered by changing this line to:

Suggested change

disks: (0..10)

disks: (0..1)

Basically:

Have a request to provision many disks

Concurrently, eliminate one disk's viability

Observe that walking the remaining search space shouldn't take a terribly long time.

(FWIW, I think this could be a companion test to this existing one, if you want to still test both behaviors: one where the concurrent request consumes all U.2s, and one where it only consumes one of them)

we could probably even be proptesting across all possible local disk counts, although i don't know if that would really get us anything more useful than just having 0..1 and 0..10 test cases

yeah I think the trickiness here is that we actually do handle both cases, but I think the performance profile is the thing that varies significantly between them

Added this test in 4b2c631, it finished in about 10 seconds on my workstation

hawkw · 2026-07-28T17:34:39Z

+        self.size().to_bytes() as i64
+            + self.required_dataset_overhead().to_bytes() as i64


Since external::ByteCount is represented as a u64 whose max value is capped at i64::MAX, this addition can overflow. I think we are probably checking someplace that a local disk is smaller than that (perhaps at creation-time) but I'm not entirely sure where. It might be nice to have a comment here about that. Not a big deal, because again, I'm pretty sure this is fine in practice because something else enforces a limit, but it felt worth mentioning.

this is a good thing to check against: I added overflow checks and a check against the latest inventory's zpool sizes for local storage backed disks in e1c449c

hawkw · 2026-07-28T17:39:26Z

+
+        removed = 0;
+
+        for allocation in &mut self.allocations_to_perform {


nit: it might be nice if this loop had a slightly bigger comment explaining what we are doing here, mostly to explain how it differs from the previous pruning step --- some of the PR description could maybe fit in well here.

added in 6b8c203

hawkw · 2026-07-28T17:42:08Z

+                match zpools_for_sled.get(&candidate_dataset.pool_id) {
+                    Some(zpool_for_sled) => {
+                        // Zpool still exists, does it still have room?
+
+                        if !zpool_for_sled.has_room_for_allocation(
+                            allocation.request.required_dataset_size(),
+                        ) {
+                            // Do not retain this as a candidate dataset, it no
+                            // longer has room.
+                            removed += 1;
+                            return false;
+                        }
+                    }
+
+                    None => {
+                        // Zpool doesn't exist anymore!
+                        removed += 1;
+                        return false;
+                    }
+                }


style nit, take it or leave it: we could maybe reduce "rightward drift" by one level of indentation here by writing:

Suggested change

match zpools_for_sled.get(&candidate_dataset.pool_id) {

Some(zpool_for_sled) => {

// Zpool still exists, does it still have room?

if !zpool_for_sled.has_room_for_allocation(

allocation.request.required_dataset_size(),

) {

// Do not retain this as a candidate dataset, it no

// longer has room.

removed += 1;

return false;

}

}

None => {

// Zpool doesn't exist anymore!

removed += 1;

return false;

}

}

let Some(zpool_for_sled) = zpools_for_sled.get(&candidate_dataset.pool_id)

else {

// Zpool doesn't exist anymore!

removed += 1;

return false;

};

// Zpool still exists, does it still have room?

if !zpool_for_sled.has_room_for_allocation(

allocation.request.required_dataset_size(),

) {

// Do not retain this as a candidate dataset, it no

// longer has room.

removed += 1;

return false;

}

IMO that reads a little nicer because the logic flows top-to-bottom instead of kind of middle-out?

personally I like the way it reads with the match :)

hawkw · 2026-07-28T17:44:17Z

+                    affinity: None,
+                    ncpus: 2,
+                    memory: external::ByteCount::from_gibibytes_u32(16),
+                    disks: (0..10)


we could probably even be proptesting across all possible local disk counts, although i don't know if that would really get us anything more useful than just having 0..1 and 0..10 test cases

…e zpools

jmpesp requested review from hawkw and smklein July 21, 2026 15:58

smklein reviewed Jul 21, 2026

View reviewed changes

hawkw reviewed Jul 28, 2026

View reviewed changes

jmpesp added 6 commits July 29, 2026 16:24

another test of pruning

4b2c631

guard against overflow when computing required dataset size

e1c449c

a comment about the new pruning

6b8c203

nits: different variable names, log messages

0dc86bb

added test for what happens when you ask for more disks than there ar…

511cd30

…e zpools

Merge branch 'main' into prune_more

bcdce33

	info!(self.log, "removed {removed} from queue");
	info!(self.log, "removed allocation lists from consideration during instance provisioning"; "count" => removed);

	info!(self.log, "removed {removed} from candidate datasets");
	info!(self.log, "removed candidate datasets from consideration during instance allocation"; "count" => removed);

		self.size().to_bytes() as i64
		+ self.required_dataset_overhead().to_bytes() as i64


		removed = 0;

		for allocation in &mut self.allocations_to_perform {

Uh oh!

Conversation

jmpesp commented Jul 21, 2026

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants