Opened 6 months ago

Closed 3 months ago

#5469 closed enhancement (complete)

Database fetching: lease6-get-all command

Reported by: tomek Owned by: fdupont
Priority: medium Milestone: Kea1.4
Component: database-all 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

This ticket should cover implementation of lease6-get-all command that retrieves all leases, either from the whole database or optionally from a single subnet. For details, see HADesign.

This is the "server" counter-part to the "client" side covered by #5467.

This is v4 command. For its v6 clone, see #5468.

Subtickets

Change History (13)

comment:1 Changed 5 months ago by fdupont

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

comment:2 follow-up: Changed 5 months ago by fdupont

A point is not clear: do we have 2 commands, a get all and a get all with a subnet id list,
or a get all and a get all with ia type and a subnet id list,
or 3 commands.
Note for the 2 last options (i.e. not a copy of trac5469 with 4 -> 6) what is the hook parameter syntax?

BTW I implemented the first: I am building and likely running tests pretty soon.

Last edited 5 months ago by fdupont (previous) (diff)

comment:3 in reply to: ↑ 2 ; follow-up: Changed 5 months ago by marcin

Replying to fdupont:

A point is not clear: do we have 2 commands, a get all and a get all with a subnet id list,
or a get all and a get all with ia type and a subnet id list,
or 3 commands.
Note for the 2 last options (i.e. not a copy of trac5469 with 4 -> 6) what is the hook parameter syntax?

BTW I implemented the first: I am building and likely running tests pretty soon.

Wih #5468 I created an index that allows for fetching by subnet-id and (subnet-id, ia-type). Thus, it should include commands for retrieving by subnet-id and (subnet-id, ia type).

comment:4 in reply to: ↑ 3 Changed 5 months ago by marcin

Replying to marcin:

Replying to fdupont:

A point is not clear: do we have 2 commands, a get all and a get all with a subnet id list,
or a get all and a get all with ia type and a subnet id list,
or 3 commands.
Note for the 2 last options (i.e. not a copy of trac5469 with 4 -> 6) what is the hook parameter syntax?

BTW I implemented the first: I am building and likely running tests pretty soon.

Wih #5468 I created an index that allows for fetching by subnet-id and (subnet-id, ia-type). Thus, it should include commands for retrieving by subnet-id and (subnet-id, ia type).

I amend this comment. I think we can stick with all leases and all leases for a given subnet for now. So the same as v4

comment:5 Changed 5 months ago by fdupont

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

I changed the code to use all leases or all leases with a subnet ID and lease type pair even the current syntax is all leases or all leases with a subnet ID from a list so all lease types.
Ready for review.

comment:6 Changed 4 months ago by marcin

  • Owner changed from UnAssigned to marcin

comment:7 follow-up: Changed 4 months ago by marcin

  • Owner changed from marcin to fdupont

Reviewed up to the commit e0abc5ac7c7b3caeb4ea3c3a5a65e040be33fc1b

The changes are straight forward, mostly copying the v4 logic. I have made several little edits, so please pull my changes.

This code requires a ChangeLog entry.

src/lib/dhcpsrv/lease_mgr.h
I have been thinking whether or not we should be querying leases by (subnet-id, lease-type) or simply subnet-id. While I think it might be sometimes useful to query by (subnet-id, lease-type), our current use case in the HA context is to query by subnet-id only (or simply all leases). Querying by lease-type multiples the number of queries by 3. So, if you query leases for 3 subnets, it results in 9 queries, as opposed to 3. This is not a problem for a memfile but this is a generic solution for all backends. One of our customer issues recently was that Kea issued to many queries which hammered the server. I think we should simply stick to lease queries by subnet-id for now. I realize that I added an index by lease_type but such composite index can be used to retrieve leases by subnet_id. In the future we may add new calls to retrieve leases by subnet-id and lease-type.

src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc
src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h
I am proposing to remove queries by lease-type, but as it stands now the testGetLeases6SubnetId in fact tests that all leases of specific type and subnet-id are returned. The commentary says "Test method which returns all IPv6 leases for SubnetID".

comment:8 in reply to: ↑ 7 Changed 4 months ago by fdupont

  • Owner changed from fdupont to marcin

Replying to marcin:

I have made several little edits, so please pull my changes.

=> seems fine.

This code requires a ChangeLog entry.

=> I am afraid I'll simply copy the #5468 one changing 4 into 6!

src/lib/dhcpsrv/lease_mgr.h
I have been thinking whether or not we should be querying leases by (subnet-id, lease-type) or simply subnet-id.

=> it was the only choice to make...

While I think it might be sometimes useful to query by (subnet-id, lease-type), our current use case in the HA context is to query by subnet-id only (or simply all leases). Querying by lease-type multiples the number of queries by 3. So, if you query leases for 3 subnets, it results in 9 queries, as opposed to 3. This is not a problem for a memfile but this is a generic solution for all backends. One of our customer issues recently was that Kea issued to many queries which hammered the server. I think we should simply stick to lease queries by subnet-id for now.

=> it is the simplest of course.

I realize that I added an index by lease_type but such composite index can be used to retrieve leases by subnet_id. In the future we may add new calls to retrieve leases by subnet-id and lease-type.

=> the real question is where to merge per lease-type result: in Kea itself or in the database SQL interpreter. So what is your decision?

src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc
src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h
I am proposing to remove queries by lease-type, but as it stands now the testGetLeases6SubnetId in fact tests that all leases of specific type and subnet-id are returned. The commentary says "Test method which returns all IPv6 leases for SubnetID".

=> it would be painful to code something which perhaps will be never useful. For the code itself it is some extra lines, for uni tests it is really to duplicate many things...

So in conclusion what we do know? Merge?

comment:9 Changed 3 months ago by marcin

  • Owner changed from marcin to fdupont

As we agreed on the call today, I am giving it back to you to remove lease searches by lease type and leave searches all leases and all leases for subnet-id.

comment:10 Changed 3 months ago by fdupont

  • Owner changed from fdupont to marcin

I removed the per-type code. As I am in a train I can't easily check the code so either you review it or you wait I got the result...

comment:11 Changed 3 months ago by fdupont

Build done. check of hook done. check of libdhcpsrv done. So for me it is OK.

comment:12 Changed 3 months ago by marcin

  • Owner changed from marcin to fdupont

Your changes are good. I corrected some little errors in the new handlers' descriptions, so please pull before merging. You can merge the ticket. I am fine with using the corresponding v4 changelog entry text for v6.

comment:13 Changed 3 months ago by fdupont

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

Merged. Closing.

Note: See TracTickets for help on using tickets.