#5437 closed enhancement (fixed)

Send one query for host reservations instead of multiple

Reported by: marcin Owned by: marcin
Priority: high Milestone: Kea1.4
Component: host-reservations Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

When using shared networks, the Kea server will send multiple queries to the database to retrieve host reservations for all subnets belonging to a shared network. We should rather send one query as sending multiple has negative impact on the server performance.

Also see the following thread: https://lists.isc.org/pipermail/kea-users/2017-November/001440.html

Subtickets

Change History (8)

comment:1 Changed 19 months ago by tomek

  • Milestone changed from Kea-proposed to Kea1.4
  • Priority changed from medium to low

This is a follow-up to a 1.3 feature. We'll try to improve the situation, but features planned for 1.4 take precedence, thus low priority.

comment:2 Changed 16 months ago by tomek

  • Priority changed from low to high

comment:3 Changed 16 months ago by marcin

  • Owner set to marcin
  • Status changed from new to accepted

comment:4 Changed 16 months ago by fdupont

IMHO in AllocEngine::findReservationInternal:

  • build the list of subnets.
  • if there is one just use the current code (i.e. the body of the loop).
  • if there is more than one (BTW there is at least one at the start) use HostMgr...->getAll(identifier_{type, begin,end) which is already defined and tested.

comment:5 Changed 16 months ago by marcin

  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to reviewing

The change described in the ticket was just a starting point to more thorough optimizations in lease database queries that Kea makes for both host reservations and leases. The changes are mostly focused on minimizing the number of queries by making more general queries and eliminating non interesting results. This shows pretty good performance improvements measured with perfdhcp.

There is one major change in subnet selection within a shared network. The code now remembers when the subnets have been last used and they are preferred over other subnets. This minimizes likelihood of selecting a subnet with exhausted address pools and an overhead of iterating over the entire subnet to find this out.

Proposed ChangeLog entry:

13XX.	[bug]		marcin
	Multiple critical performance optimizations in the allocation
	engine for shared networks.
	(Trac #5437, git cafe)

comment:6 Changed 16 months ago by tomek

  • Owner changed from UnAssigned to tomek

comment:7 Changed 16 months ago by tomek

  • Owner changed from tomek to marcin

Thanks for writing this code and fixing those issues. Obviously, the
code now being proven in battle, there's no point in radically
changing it. Having said that, there are minor tweaks to be done.

dora_unittest.c

I've update name of the test. It's not really hijacking anything (the
name suggests some bad behavior). The test is now called
reservationIgnoredInDisabledMode.

PgSQL schema update.

Why did you bump it to 4.0 version? Index changes are technically
backwards compatible, right? If you really want to, you could run this
on your old datase. This should be a minor, not major bump (3.3).

The same is for MySQL.

shared_network.cc
The getPreferredSubnet is suboptimal in the sense of also needing
classes that may not be relevant. Imaging this situation:

Packet belongs to foo and there are following subnets:

  • subnet 1: class foo
  • subnet 2: class foo, bar
  • subnet 3: class foo, baz
  • subnet 4: class foo, xyzzy

No matter what your starting subnet its, you'll never get any other
subnet than the one you started with. It's not a big issue, though.
Just the optimization sometimes won't work. The logic should have
packet passed here (or at least a list of classes we care about,
i.e. those the packet belongs to). But that's definitely a future
imporvement here.

This is mostly theoretical at this moment, though, because while
the data structures allow multiple classes, the parser allows only
one, thus the scenario can't happen in production.

If you agree with my comment, please add a @todo there.

alloc_engine.cc
I've simplified some of the code to use C++11 syntax.

Initially I was a bit concerned with the changes in addressReserved
method (if the subnet was not selected, we'd automatically assume
everything is not reserved), but then I was not able to come up with
any scenario that would fail to reserve a subnet and check for address
reservation. So we should be ok here.

After this change we will need to investatigate whether there are
query methods that are not used in production. That's a low
priority thing, though.


I have pushed several minor improvements. Please review them. If
you're ok with them, please merge. If you disagree, please merge
anyway :)

The code builds and unit-tests pass on Ubuntu 17.04 x64.

comment:8 Changed 16 months ago by marcin

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged with commit 9d8bcd39802795d48c737a05ef3de3634a28ca4e

Note: See TracTickets for help on using tickets.