Replace local storage allocation search with sort / pair algorithm - #11011
Replace local storage allocation search with sort / pair algorithm#11011smklein wants to merge 15 commits into
Conversation
Placing an instance's local storage disks onto distinct zpools does not require searching: a pool fits a disk iff the required dataset size is strictly below the pool's headroom, a single scalar threshold, so pairing the largest requests with the largest-headroom pools succeeds exactly when any valid assignment exists. Add pair_local_storage_requests_to_pools (the pure pairing), choose_local_storage_allocations (the adapter over LocalStorageDisk and ZpoolGetForSledReservationResult), a headroom() accessor so there is a single definition of fit, and LocalStorageDisk::required_dataset_size. Unit tests pin the strict comparison, the spread placement, tie determinism, and (via proptest) equivalence with a brute force matcher. Not yet called from sled_reservation_create; that switch is the next commit.
…hooser sled_reservation_create now computes one assignment per fresh zpool snapshot and lets the insert CTE re-validate it atomically, retrying with a new snapshot (up to LOCAL_STORAGE_ATTEMPTS_PER_SLED times) when the CTE inserts zero rows because the snapshot went stale. If zero rows were inserted because a disk was deleted or detached mid-reservation, fail immediately instead of retrying on every sled: no sled can satisfy that reservation. This deletes CompleteLocalStorageAllocationLists and its exhaustive enumeration of request-to-zpool assignments. When concurrent allocations made the remaining search space infeasible, that iterator had to drain the entire space before reporting failure: roughly one million heap pushes and clones (measured at 3.34 seconds of CPU) for ten disks over nine pools. The greedy chooser proves infeasibility with two sorts instead. The two tests that constructed the iterator directly now drive the chooser output straight into sled_insert_resource_query, proving the CTE (not just app-side bookkeeping) rejects allocations for deleted and detached disks.
Deterministically construct the race the reservation retry loop handles: compute an assignment, let a concurrent reservation consume the chosen pool, and verify that the insert CTE rejects the stale assignment (zero rows), that a fresh reservation lands on the remaining pool, and that a third reservation fails cleanly once both pools are full.
smklein
changed the title
has_room_for_allocation lost its last caller when the exhaustive search was deleted; headroom() is now the single expression of the fit rule. LocalStorageDisk::required_dataset_overhead is only called by required_dataset_size, so drop its pub.
Add a Disk uuid kind for the virtual disk resource (as opposed to the existing PhysicalDisk kind) and use it for LocalStorageRequest and LocalStorageAllocation, converting to an untyped uuid only where the value enters SQL binds and where LocalStorageDisk::id() produces it.
|
Taking a step back from "what this PR is", I do kinda want to also mention "why I wanted to do it": IMO the queue-based approach we used before (which was, in short, "exhaustively explore all possible combinations of zpools / datasets for a local storage allocation") kinda freaked me out, because of how computationally expensive bad cases could be. Also, the act of even "considering a new possibility" involves a fair bit of memory allocation, and (this is subjective!) I found the algorithm confusing and hard-to-reason about. This new approach should also issue fewer queries generally:
|
| sled_target, | ||
| instance_id, | ||
| zpools_for_sled, | ||
| let allocations = match choose_local_storage_allocations( |
There was a problem hiding this comment.
This is the crux of this PR: using this function as a decision point for allocation, rather than iterating through CompleteLocalStorageAllocationLists
State the argument concretely instead of as a compressed proof.
Plain language for why retries exist and why the loop must be bounded.
The old comment said the check was not load-bearing for correctness, which is wrong in one direction: a false positive fails a reservation that could have succeeded. Only a false negative is cushioned by the insert query. Say that instead.
| // pool and dataset policy changes): take a new snapshot and | ||
| // try again, a bounded number of times, before moving to the | ||
| // next sled. | ||
| 'attempts: for _ in 0..LOCAL_STORAGE_ATTEMPTS_PER_SLED { |
There was a problem hiding this comment.
So, this is probably worth discussing.
In the old implementation, when we consider ONE SPECIFIC SLED, and evaluate local storage allocations, we create a big CompleteLocalStorageAllocationLists, and work our way through it.
This involves a lot of CPU-bounded checking, but it basically creates a big queue, finds the next viable placement, and runs the "INSERT" query. In the case where "zero rows are inserted, but nothing else screams about a problem", we assume a concurrent operation landed.
ON MAIN, this meant: looping around local_storage_allocation_search, calling prune_invalidated_allocation_lists (which calls zpool_get_for_sled_reservation and some other database functions). But this prior code had a perspective of "there is a state space of possible disk -> zpool assignments, which we will explore, and which we will trim down".
(Note: this trim-only behavior actually means we would not consider "new space" which is made available if there are concurrent operations which make disk space free)
ON THIS PR, instead, we don't store an auxiliary data structure that needs pruning. We just sort requested disks / observed pools free space by size, and can either do the assignment, or we can't. If we fail, we don't "prune the state space", we just re-load the info for the zpools on that sled, and try again. But in a world where concurrent operations are rapidly "freeing + allocating", and/or our Nexus is slow, we might just see: "INSERT fails -> re-load -> find another viable candidate -> INSERT fails, etc". (Good news here: if this happens, SOMEONE is making progress. Just a different request...)
So this PR does put some bound on this, but it's a bit arbitrary.
smklein
changed the title
smklein
changed the title
| DemoSaga = {}, | ||
| // A virtual disk (the customer-facing `disk` resource), as opposed to | ||
| // PhysicalDisk below. | ||
| Disk = {}, |
There was a problem hiding this comment.
I'm surprised we didn't have this already; probably should propagate to more spots, but I didn't want to continue using untyped UUIDs, so I added it now...
any_request_disk_deleted_or_detached only uses the row count and the attach_instance_id column, so load that column instead of the whole Disk model.
|
I haven't had the time to give this a thorough review yet, but I'm on board with the concept! I presume we are planning on holding off until post-r22 to consider merging this change? |
| impl fmt::Display for LocalStorageUnsatisfiable { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| match self { | ||
| LocalStorageUnsatisfiable::NoAllocationsRequired => { | ||
| write!(f, "no disks require a local storage allocation") | ||
| } | ||
|
|
||
| impl PartialEq for IncompleteAllocationList { | ||
| fn eq(&self, other: &Self) -> bool { | ||
| self.allocations.len() == other.allocations.len() | ||
| LocalStorageUnsatisfiable::NotEnoughPools { requests, pools } => { | ||
| write!( | ||
| f, | ||
| "{requests} disks require a local storage allocation, \ | ||
| but only {pools} pools are usable", | ||
| ) | ||
| } | ||
|
|
||
| LocalStorageUnsatisfiable::RequestDoesNotFit { | ||
| disk_id, | ||
| required_dataset_size, | ||
| pool_id, | ||
| headroom, | ||
| } => { | ||
| write!( | ||
| f, | ||
| "disk {disk_id} requires {required_dataset_size} bytes, \ | ||
| but pool {pool_id} only has {headroom} bytes of headroom", | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
take it or leave it, but: this seems like it could also be derived by thiserror, and we'd get an Error impl for free too?
There was a problem hiding this comment.
Seems like an easy win, sure, updated.
| b.required_dataset_size | ||
| .cmp(&a.required_dataset_size) | ||
| .then(a.disk_id.cmp(&b.disk_id)) | ||
| }); | ||
|
|
||
| instance_id: InstanceUuid, | ||
| pools.sort_by(|a, b| { | ||
| b.headroom.cmp(&a.headroom).then(a.pool_id.cmp(&b.pool_id)) | ||
| }); |
There was a problem hiding this comment.
the use of Ordering::then makes this all quite readable! very nice!
| } | ||
| for (request, pool) in requests.iter().zip(pools.iter()) { | ||
| if request.required_dataset_size >= pool.headroom { | ||
| return Err(LocalStorageUnsatisfiable::RequestDoesNotFit { |
There was a problem hiding this comment.
this is where the sortedness of the list of pools and requests allows us to short-circuit early --- it might be worth a comment calling back to that here, even though it is documented in the higher-level doc comment?
There was a problem hiding this comment.
Sure, I'll add a comment.
| /// Ties are broken by disk id and pool id, so for a given set of requests | ||
| /// and pools the output is deterministic. Only the freshly generated | ||
| /// allocation ids differ between calls. | ||
| fn pair_local_storage_requests_to_pools( |
There was a problem hiding this comment.
goofy high level nitpick, feel free to ignore me: this is completely unrelated to the act of acqually querying the database; it would be nice if it didn't live in db-queries. it might not be easy for it to go elsewhere, though, since instance allocation basically lives in this crate...
There was a problem hiding this comment.
soooo I technically agree with you that it doesn't depend on the database, but like... this doesn't really make sense to go anywhere outside of db-queries either. If it was (a) expensive to compile, or (b) depended on by anyone else, I'd be totally down to make the move. But as of now: It's basically just "logic from db-queries, that happens to be factored out".
TL;DR: I'd prefer to keep it here??
| let requests: Vec<LocalStorageRequest> = local_storage_disks | ||
| .iter() | ||
| .filter(|disk| disk.local_storage_dataset_allocation.is_none()) | ||
| .map(|disk| LocalStorageRequest { | ||
| disk_id: DiskUuid::from_untyped_uuid(disk.id()), | ||
| required_dataset_size: disk.required_dataset_size(), | ||
| }) | ||
| .collect(); | ||
|
|
||
| // Pools that already hold a local storage allocation for this instance | ||
| // (encrypted or unencrypted) are not eligible for further allocations. | ||
| let used_pools: HashSet<ZpoolUuid> = local_storage_disks | ||
| .iter() | ||
| .filter_map(|disk| { | ||
| disk.local_storage_dataset_allocation.as_ref().map(|allocation| { | ||
| ZpoolUuid::from_untyped_uuid( | ||
| allocation.pool_id().into_untyped_uuid(), | ||
| ) | ||
| }) | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
nit, take it or leave it: it would be less functional programming-y, but we could save ourselves the second iteration over local_storage_disks if we did this in one loop that populates both requests and used_pools...
There was a problem hiding this comment.
Yeah, given that we filter on local_storage_dataset_allocation with opposite reactions here, I agree with you. Doing this iteratively might be more clear that "we're handling all cases" here.
…ent the pairing short-circuit, build requests and used_pools in one pass
|
Merging this as-is would cause a regression: with this PR, nexus will bail out of searching for a reservation after a constant number of attempts fail due to other concurrent allocations, meaning a If that constant limit is removed, I think there's the potential for an infinite loop: assuming no database state changes, the sort + pair algorithm will generate the same pairing each time, and if there's a bug that causes zero rows to get inserted, this will infinitely loop because it's not a search through possible combinations, where the search (even though it may take a long time) will eventually stop. |
I tried to identify this in the comment above, here: #11011 (comment) I don't think it's true that this would be a regression. but I appreciate the pushback, because I do think it's like the most visible consequence of going from "maintained data structure" to "oneshot algorithm" in this allocation pathway.
So let me dig into this a bit more, about "why" I don't think this is actually a regression. I 100% agree with you that "removing the constant limit" would be bad, so I actually want to discuss the other angle: can On both main and this PR, we get that result of "zero rows inserted" from the CTE only when concurrent activity happens. We have some read-into-memory state, which we use to make an allocation decision. What do we do on mainIf we get this result, we go into the
If this is happening while concurrent operations are allocating and freeing space, we'll never reconsider the freed space. E.g., Maybe "A" would succeed if we tried again! But we won't. We're continually narrowing a set. What we do in this PRSo, we don't have a data structure we're maintaining across allocations. Instead, we load the full set of zpools (and their usage), and do our sorting thing. This means we actually get the benefit that we can see/use space which is concurrently freed. Compared to the example I cited for Why I don't really think this is a regressionThe choice made by I am flexible about changing the size of |
Yes, that's what I'm pointing out haha
My objection is that both the change introduced in this PR and main may fail if there's a concurrent free operation, but there are situations where this PR will fail and main will not when there are only concurrent reservation operations. That's the regression. |
|
That's why I'm saying "if we increase the constant, this PR can match the number of attempts main is making, and we can bump that number, but I don't think we even need to let it get as large as main's theoretical maximum". Generalizing a bit: if we are trying to allocate Practically, I think the number of attempted INSERTs would actually be lower with pruning, because "zero rows inserted" from the CTE means that some chosen pair of We could compute the value of In the context of a single sled: I believe this fully covers the case you're describing. Any pattern of "concurrent reservations (without frees) that would exhaust this new PR's algorithm" would have caused a failure on |
|
If you want a TL;DR of that block comment: If we change Then I think there are no "concurrent only-reservation" cases, even "adversary-controlled" where |
An attempt is only consumed when concurrent activity claims space between our zpool snapshot and our insert. While space is only being consumed, each lost race permanently kills at least one (disk, zpool) pairing, and there are only disks * zpools pairings, so the sled runs out of pairings (and the chooser bails) before this budget runs out. Concurrent frees can revive pairings, in which case we may give up on a sled early.
3 participants
When an instance using local storage disks is placed on a sled, each local storage disk needs an allocation to its own zpool. Since a zpool which can satisfy a request for a disk can also satisfy a request for any smaller disk (if a zpool can fit a 100 GiB disk, it could also fit 90 GiB), we can find an assignment with the following algorithm:
This replaces
CompleteLocalStorageAllocationLists, which enumerated every injective request-to-zpool assignment through a BinaryHeap. When concurrent allocations made the remaining space infeasible, that iterator had to drain the entire search space before reporting failure.sled_reservation_createnow computes one assignment per fresh zpool snapshot and lets the insert CTE re-validate everything atomically, retrying with a new snapshot (bounded per sled) when the CTE reports the snapshot went stale. If zero rows were inserted because a disk was deleted or detached mid-reservation, the reservation fails immediately instead of retrying on every sled.Placement is deterministic: largest requests onto the pools with the most headroom (spread), ties broken by id.
One follow-up worth noting: the zpool snapshot query does not count encrypted local storage dataset usage, while the CTE's capacity check does. These cannot diverge today (reservation bails out earlier if any disk has an encrypted allocation), but if encrypted allocations ever ship, the snapshot query needs the same usage term added.