fix: N+1 node queries in dhcp, api and websocket handlers - #439
Conversation
|
Check where you would like a Mattermost message to be sent to when CI completes and this PR is merged
|
239efcc to
30d680d
Compare
47d1f39 to
202808b
Compare
make_hosts_for_subnets iterated sip.interface_set.order_by("id") for
every allocated IP and then read each interface's node config, node
and parents -- several database queries per host, i.e. O(number of
hosts) round-trips (~18000 for a subnet with 6000 static IPs).
The whole configuration is regenerated on every change and runs in a
database thread, so this scales badly on large subnets.
Move the ordering into a Prefetch and load node_config/node and
parents alongside, so the query count stays constant regardless of
host count.
Signed-off-by: Seyeong Kim <seyeong.kim@canonical.com>
IPAddressesHandler.read serialized each IP's interface_set, whose system_id/resource_uri call Interface.get_node() (node_config.node), so it ran a query per IP against maasserver_nodeconfig and maasserver_node. interface_set is a M2M, so those relations are not covered by a plain prefetch and must be named explicitly. Prefetch interface_set with the node selected in the same query so the node query count stays constant regardless of the number of IPs returned. Signed-off-by: Seyeong Kim <seyeong.kim@canonical.com>
Fabric.delete lists the still-connected interfaces via get_log_string(), which calls get_node() per interface. select_related node_config__node so building that error message does not query the node per interface. Signed-off-by: Seyeong Kim <seyeong.kim@canonical.com>
The vmcluster handler reads vm.machine (system_id/hostname) for each
tracked VM, querying the node per VM. select_related("machine") in
tracked_virtual_machines so the query count does not grow with the
number of VMs.
Signed-off-by: Seyeong Kim <seyeong.kim@canonical.com>
Pod.host calls hints.nodes.first(), which queried the node once per pod in the handler's list. Prefetch hints__nodes ordered by id in the handler queryset so first() is served from the cache. Signed-off-by: Seyeong Kim <seyeong.kim@canonical.com>
202808b to
ab90d7e
Compare
skatsaounis
left a comment
skatsaounis
left a comment
There was a problem hiding this comment.
Hi @xtrusia
Thank you for the PR. Before starting a complete review, I wasn't able to find a linked bug report or tests that are counting queries to prove the improvements. The latter, will also stay in the code base and used as a guard for future changes - no change will be able to introduce more queries than what your changes are achieving in this code path.
Please file a bug report and add it as a footer to your PR description, then proceed with tests that are counting queries. When you are done, let us know to start the review.
This is a link to a past commit that is introducing such tests. You can use it as reference.
The existing tests verify that query counts stay constant as object counts grow, but would still pass if both measurements increased. Assert the counts achieved by the fixes to catch that regression. Resolves LP:2161952 Signed-off-by: Seyeong Kim <seyeong.kim@canonical.com>
Master also queries ReservedIP entries when building DHCP hosts. Include that constant query in the exact regression count. Resolves LP:2161952 Signed-off-by: Seyeong Kim <seyeong.kim@canonical.com>
skatsaounis
left a comment
skatsaounis
left a comment
There was a problem hiding this comment.
Thank you for creating the bug report and introducing query count tests. This is what I was able to detect in your changes. Please take a look, especially at the 2nd comment.
r00ta
left a comment
r00ta
left a comment
There was a problem hiding this comment.
lgtm once @skatsaounis comments are addressed
|
It would be nice if you could grab some numbers with this new implementation so to include them in the release notes. Something like "for a /19 subnet with 6000 allocation X queries were executed; on the same environment with this fix Y queries were executed" |
Fetch only the values needed for the validation error and keep query counts constant for parent-resolved interfaces. Signed-off-by: Seyeong Kim <seyeong.kim@canonical.com>
4 participants
On large subnets, several code paths run one or more node queries per object. With about 6,000 allocated IP addresses on a
/19subnet, one DHCP configuration rebuild runs about 18,000 queries. TheipaddressesAPI has the same N+1 query problem.On one deployment at this scale, slow API responses and deployment timeouts occurred while these DHCP configuration rebuilds were running.
This PR removes the N+1 queries from these paths and adds
count_queries()regression tests.make_hosts_for_subnets()IPAddressesHandler.read()Fabric.delete()Pod.hostlookups through pod hints in the pod WebSocket handlerThe fixes use
select_related(),prefetch_related(), and targeted ORM queries to retrieve the required node data in a fixed number of queries.The regression tests confirm that query counts stay constant for the covered result sets as their object counts increase.
Performance measurements
The DHCP results below come from a source-level reproducer that calls
make_hosts_for_subnets([subnet])directly.10.42.0.0/19subnet.StaticIPAddressto each interface.make_hosts_for_subnets([subnet]).make_hosts_for_subnets([subnet])again.Query counts were collected with
connection.execute_wrapper(). Runtimes are the median of two uninstrumented runs, with the baseline and patched results collected in the same environment.The table shows the result after adding the 6,001st address.
The 3.4 and 3.5 code paths do not load
ReservedIPentries here, so the fixed count is 3. Versions 3.6, 3.7, and master also loadReservedIPentries, so the fixed count is 4.The large-IP paths use
prefetch_related(), which keeps related rows in memory and uses anINclause containing the returned IP IDs. This is the memory trade-off for removing the per-IP queries.Backports are planned for stable branches down to 3.4, where the same code paths run in production.
Resolves LP:2161952