CP-52880: Avoid O(N^2) behaviour in Xapi_vdi.update_allowed_operations

[edwintorok]

Activate old xapi_vdi.update_allowed_operations optimization: get_internal_records_where does a linear scan currently, so operating on N VDIs is O(N^2).

Look at the VBD records directly, like before this 2013 commit which regressed it: 5097475

(We are going to optimize get_record separately so it doesn't go through serialization)

This still requires validation and measurements (e.g. using @contificate 's benchmark)

[psafont]

So the change here is avoiding the None value? Why can't the function's signature be change to remove the optional codepath to explicitly remove the n^2 behaviour?

[robhoes]

I agree with Pau: the get_internal_records_where branch is still there, but effectively made dead code?

Also, the trade-off here, which triggered the original change 11 years ago, is that if you are not no the coordinator, the get_internal_records_where call is a single round-trip over the network, where now you have a round-trip for each VBD. The network overhead may outweigh the DB handling speedup in that case. It depends whether this function may run on any host or just on the coordinator.

[Vincent-lau]

So the O(N^2) behaviour comes from the fact that we retrieve a vbd ref but never uses it?

[edwintorok]

I agree with Pau: the get_internal_records_where branch is still there, but effectively made dead code?

It is still used by assert_operation_valid, which operates on a single VDI (so the old code is the best we can do for now).

I should probably try to optimize the db filter code to actually use the "index" when available, and avoid all those linear scans (that might improve other parts of the codebase too).

s that if you are not no the coordinator, the get_internal_records_where call is a single round-trip over the network, where now you have a round-trip for each VBD.

if a VM has many disks in a pool, and this gets called on a non-member host, then yes. It is not very clear where this function is called from: I see callers from message_forwarding, so that should be OK, but there are calls from elsewhere too.

Perhaps the code could check in which situation it is: if we are pool master then fetch all the VBDs individually as here, and if not fall back to the other call (and later we can optimize the other call).

[edwintorok]

Perhaps the code could check in which situation it is: if we are pool master then fetch all the VBDs individually as here, and if not fall back to the other call (and later we can optimize the other call).

I've implemented this change

[edwintorok]

The python failure is unrelated (coverage), I rebased on latest master to get the CI fixes.