#5468 closed enhancement (complete)

Database fetching: lease4-get-all command

Reported by: tomek Owned by: marcin
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 lease4-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 #5466.

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

Subtickets

Change History (15)

comment:1 Changed 22 months ago by marcin

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

comment:2 Changed 22 months ago by marcin

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

This ticket is now ready for review. I have added new methods to the lease manager API and implemented them for Memfile, MySQL and Postgres backend. I didn't implement them for CQL for which I will create a new ticket.

Proposed ChangeLog entry:

13XX.	[func]		marcin
	Implemented lease4-get-all command in lease_cmds hooks library.
	(Trac #5468, git cafe)

comment:3 Changed 22 months ago by fdupont

  • Owner changed from UnAssigned to fdupont

comment:4 Changed 22 months ago by fdupont

Read the code: it seems OK by I have to build and test with SQL libs.

Fixed a few thing, the only not cosmetics is a double paste in CQL header. So please pull.

I have a concern with big databases: it is not reasonable to just dump it as it will take too much memory and time. It is possible to paginate the select with limit/offset but for a first implementation I can live with current code.

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

  • Owner changed from fdupont to marcin

Ran build and ran make check with success on my macOS with MySQL and PostgreSQL.

Ready for merge. I give up on pagination and Cassandra but please do IPv6 too (aka #5469).

comment:6 follow-up: Changed 22 months ago by tmark

I guess Francis beat me to it, so I will add these:

General:

We should warn people that leaseX-get-all commands can result in extremely
large responses, since we impose no restriction on the answer set size.
I suppose given a large enough response set, one could negatively impact
the server's ability to respond as well.

This is where running commands in an alternate thread would prove useful.
Something we might consider at some point, is having a way for command handlers
to know or be designated as "threadable" and spin off a thread?

Some commands clearly need to run synchronously, but others like queries of
pretty much any kind could run in parallel. I realize this is beyond the
scope of this ticket.


src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc

checkLease4() - Won't this crash at line 371, if l is a list that doesn't
contain a lease with a matching ip_address?


I would like to see tests for no leases (with and and without subnets).


comment:7 in reply to: ↑ 5 Changed 22 months ago by marcin

Replying to fdupont:

Ran build and ran make check with success on my macOS with MySQL and PostgreSQL.

Ready for merge. I give up on pagination and Cassandra but please do IPv6 too (aka #5469).

#5469 is planned for 1.4 but will be implemented later, because v4 is a priority.

comment:8 in reply to: ↑ 6 ; follow-up: Changed 22 months ago by marcin

  • Owner changed from marcin to tmark

Replying to tmark:

I guess Francis beat me to it, so I will add these:

General:

We should warn people that leaseX-get-all commands can result in extremely
large responses, since we impose no restriction on the answer set size.
I suppose given a large enough response set, one could negatively impact
the server's ability to respond as well.

I added a note in the user's guide.

This is where running commands in an alternate thread would prove useful.
Something we might consider at some point, is having a way for command handlers
to know or be designated as "threadable" and spin off a thread?

I agree.

Some commands clearly need to run synchronously, but others like queries of
pretty much any kind could run in parallel. I realize this is beyond the
scope of this ticket.

Indeed. But, I agree.


src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc

checkLease4() - Won't this crash at line 371, if l is a list that doesn't
contain a lease with a matching ip_address?

I added an assert.


I would like to see tests for no leases (with and and without subnets).

Added tests as requested.


comment:9 in reply to: ↑ 8 ; follow-up: Changed 22 months ago by tmark

  • Owner changed from tmark to marcin

Replying to marcin:

Replying to tmark:

Changes look good to here.


I would like to see tests for no leases (with and and without subnets).

Added tests as requested.


The new tests look good but they highlight an issue I didn't' spot before. Finding no leases should be return CONTROL_RESULT_EMPTY. As far as I know everyplace we treat no results found this way. We need to be consistent.

comment:10 in reply to: ↑ 9 Changed 22 months ago by marcin

  • Owner changed from marcin to tmark

Replying to tmark:

Replying to marcin:

Replying to tmark:

Changes look good to here.


I would like to see tests for no leases (with and and without subnets).

Added tests as requested.


The new tests look good but they highlight an issue I didn't' spot before. Finding no leases should be return CONTROL_RESULT_EMPTY. As far as I know everyplace we treat no results found this way. We need to be consistent.

Good catch. I have to admit I was unaware that we even have such status code. I now modified the status code.

comment:11 Changed 22 months ago by tmark

  • Owner changed from tmark to marcin

Changes are good, merge on!

comment:12 Changed 22 months ago by fdupont

  • Owner changed from marcin to fdupont

comment:13 follow-up: Changed 22 months ago by fdupont

  • Owner changed from fdupont to marcin

Put it on hold to get a chance to merge it with #5469 as it shares some code...
So I have 2 questions:

  • do you prefer to merge #5468 now and postpone #5469?
  • #5469 is ready only if what we want for it is exactly the same than #5468, cf the question in the ticket about ia type.

comment:14 in reply to: ↑ 13 Changed 22 months ago by marcin

Replying to fdupont:

Put it on hold to get a chance to merge it with #5469 as it shares some code...
So I have 2 questions:

  • do you prefer to merge #5468 now and postpone #5469?
  • #5469 is ready only if what we want for it is exactly the same than #5468, cf the question in the ticket about ia type.

The HA for v6 is planned for later. Our top priority is HA for v4 and thus I want to merge #5468 now and not wait for #5469.

comment:15 Changed 22 months ago by marcin

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

Merged with commit a378ec28489e98df64830d1f26c3bebd20e256b2

Note: See TracTickets for help on using tickets.