Pruning should affect candidate datasets too

[jmpesp]

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]

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

[smklein]

Suggested change

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

[smklein]

Suggested change

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

[smklein]

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.

[smklein]

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)