Opened 6 years ago

Closed 5 years ago

#3561 closed enhancement (complete)

Implement HostManager

Reported by: tomek Owned by: marcin
Priority: medium Milestone: Kea0.9.1beta
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

We need a HostManager? that will provide interface for managing host reservations, similar to what LeaseMgr? offers for leases.

Subtickets

Change History (6)

comment:1 Changed 5 years ago by marcin

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

comment:2 Changed 5 years ago by marcin

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

Ticket is ready for a review.

There is a new class in libdhcpsrv, HostMgr. It is an initial version and not everything is implemented. But, the basic functionality which is to retrieve reservations from the server's configuration (specified in the configuration file) is working. I also added a stub implementation for the alternate host data source. This is an optional data source which will retrieve reservations from other sources, e.g. MySQL or PostgreSQL database. They will require implementing dedicated backends which will derive from the BaseHostDataSource, but this is something for the future tickets.

Proposed ChangeLog entry:

XXX.	[func]		marcin
	Implemented Host Manager, which can retrieve host reservations
	specified in the server's configuration. Future tickets will
	extend Host Manager to retrieve reservations from other sources,
	e.g. SQL databases.
	(Trac #3561, git abcd)

comment:3 Changed 5 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:4 follow-up: Changed 5 years ago by tomek

  • Owner changed from tomek to marcin

src/lib/dhcpsrv/host_mgr.h
HostMgr? class description: maintaing => maintaining

create() documentation should specify what parameters are supported
(or explicitly say that none are supported at this time)

instance() documentation seems to be truncated. It is created
using create method with the default c (default what?).

getAll, getAll4 doc should mention how the host search algorithm
is working. If the host is found in the default (config file) host
data source, will the additional one (e.g. MySQL) be queried at all?

getAll() method should mention how it operates when both hwaddr and
duid are specifiec.

I'm wondering whether there would be use cases when more than one
alternate source would be used. One thing that comes to mind is
migration, e.g. you have your hosts in a text file, but the text file
is growing too large, so a decision is made to move to a database.
I admit that it's a rather weak argument. In any case, it is out
of scope for this ticket. If you think this has a merit, maybe
add a todo to the code?

Why is there no getAll6() method?

src/lib/dhcpsrv/host_mgr.cc

HostMgr::getAll4() is broken. Well, sort of. It calls getlAll4() on
the base source and stores the result in hosts. Then gets extra hosts
from alternate source and adds it to the list. Then it's all discarded
and return value of getAll4 is returned. Two observations here.
First, fixing the code will be easy. Extending the unit-tests to cover
alternate source - not so much. I'm sure there's a separate ticket for
alternate source implementation. I'm ok with pushing it to that
ticket, just make sure there's a todo in the code that specifically
mentions that case.

src/lib/dhcpsrv/libdhcpsrv.dox
That's not in the text you added, but can you fix this simple
typo: soluction => solution?

src/lib/dhcpsrv/writable_host_data_source.h
Out of curiosity, what's the use case for having a Host that is
writable? I thought host reservation will be a strictly read-only DB.


I have no further comments. The issues found are sufficiently minor
that I don't need to see this ticket again. Thanks a lot for keeping
the ticket size small.


The code compiles on Ubuntu 14.04 x64 and unit-tests pass.

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

Replying to tomek:

src/lib/dhcpsrv/host_mgr.h
HostMgr? class description: maintaing => maintaining

Fixed.

create() documentation should specify what parameters are supported
(or explicitly say that none are supported at this time)

I explicitly state that at present none parameters are supported.

instance() documentation seems to be truncated. It is created
using create method with the default c (default what?).

Oops. Fixed.

getAll, getAll4 doc should mention how the host search algorithm
is working. If the host is found in the default (config file) host
data source, will the additional one (e.g. MySQL) be queried at all?

I added some description. To answer your question: yes they will be queried and the reservations from all sources will be returned together.

getAll() method should mention how it operates when both hwaddr and
duid are specifiec.

This documentation directs to the BaseHostDataSource class docs where it is extensively described.

I'm wondering whether there would be use cases when more than one
alternate source would be used. One thing that comes to mind is
migration, e.g. you have your hosts in a text file, but the text file
is growing too large, so a decision is made to move to a database.
I admit that it's a rather weak argument. In any case, it is out
of scope for this ticket. If you think this has a merit, maybe
add a todo to the code?

I was considering this but I think it was rejected on a design level. I think, we may want to wait for the specific use cases and add this functionality when they actually appear.

I think it would be better to solve the problem of migration by freezing the updates to the host data source, use a migration tool to move reservations to a different host data source and switch the server configuration to use the latter. Note, that managing the reservations is a separate functionality.

Why is there no getAll6() method?

This is because of the data structures that we modeled for host reservations. The getAll method returns reservations for both v4 and v6. Excluding v4 from the query doesn't make much sense as the v4 reservation is stored with the rest of the information about the host, e.g. duid/hwaddr, subnet id etc.

src/lib/dhcpsrv/host_mgr.cc

HostMgr::getAll4() is broken. Well, sort of. It calls getlAll4() on
the base source and stores the result in hosts. Then gets extra hosts
from alternate source and adds it to the list. Then it's all discarded
and return value of getAll4 is returned. Two observations here.
First, fixing the code will be easy. Extending the unit-tests to cover
alternate source - not so much. I'm sure there's a separate ticket for
alternate source implementation. I'm ok with pushing it to that
ticket, just make sure there's a todo in the code that specifically
mentions that case.

Good catch. This was broken and I corrected it now.

When it comes to the tests. This will be covered when we implement alternate backends. It is planned to be implemented pretty soon, so I don't think there is any need for a todo. There are plenty of things which are not implemented but are part of the design for the host reservations. I don't want to "overtodo" the code with them.

src/lib/dhcpsrv/libdhcpsrv.dox
That's not in the text you added, but can you fix this simple
typo: soluction => solution?

Ok. fixed.

src/lib/dhcpsrv/writable_host_data_source.h
Out of curiosity, what's the use case for having a Host that is
writable? I thought host reservation will be a strictly read-only DB.

Mostly yes. However, the container holding host reservations parsed from the configuration file must be writable, because the configuration parsers write to it. This is probably the only use case for writable data source and it is already implemented with #3628.


I have no further comments. The issues found are sufficiently minor
that I don't need to see this ticket again. Thanks a lot for keeping
the ticket size small.

Thanks for the comments.


The code compiles on Ubuntu 14.04 x64 and unit-tests pass.

Also thanks for testing.

comment:6 Changed 5 years ago by marcin

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

Merged with commit faac5e9746dbf82eb04ffef95658e4b4c7d64a4a.

Note: See TracTickets for help on using tickets.