Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#3569 closed enhancement (complete)

Allow configuration of host sources: config or database

Reported by: tomek Owned by: tmark
Priority: low Milestone: Kea1.0-beta
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 (last modified by tomek)

Once database backends are capable of storing hosts info, we need to allow configuration to point out to that storage.

Subtickets

Change History (13)

comment:1 Changed 5 years ago by tomek

  • Priority changed from medium to low

Moving to low, as agreed on one of the recent Kea calls.

comment:2 Changed 5 years ago by tomek

  • Milestone changed from Kea0.9.1beta to Kea0.9.1

Moving from 0.9.1beta to 0.9.1.

comment:3 Changed 5 years ago by marcin

  • Milestone changed from Kea0.9.1 to Kea1.1

comment:4 Changed 4 years ago by tomek

  • Milestone changed from Kea1.1 to DHCP Outstanding Tasks

comment:5 Changed 4 years ago by tomek

  • Milestone changed from DHCP Outstanding Tasks to Kea1.1

comment:6 Changed 4 years ago by tomek

  • Description modified (diff)
  • Owner set to tomek
  • Priority changed from low to medium
  • Status changed from new to assigned

comment:7 Changed 4 years ago by tomek

  • Owner changed from tomek to Unassigned
  • Status changed from assigned to reviewing

The code is now ready for review. Proposed ChangeLog?:

10XX.	[func]		tomek
	Configuration parameters for setting up external hosts storage
	are now implemented. The change is not usable on its own,
	as code change from #3682 is necessary.
	(Trac #3569, git tbd)

comment:8 Changed 4 years ago by tomek

  • Milestone changed from Kea1.1 to Kea1.0
  • Priority changed from medium to low

The updated code is now ready for review. See branch trac3569_rebase. The lines count may look somewhat large, but a lot of them is simply moving createSchema/destroySchema from MySQLLeaseMgr and MySQLHostDataSource unit-tests into a single file.

comment:9 Changed 4 years ago by tmark

  • Owner changed from Unassigned to tmark

comment:10 follow-up: Changed 4 years ago by tmark

  • Owner changed from tmark to tomek

src/lib/dhcpsrv/host_mgr.h

Instance member HostMgr::alternate_source should end with an underscore.


src/lib/dhcpsrv/parsers/dbaccess_parser.cc

DbAccessParser::commit()

Commentary block in the HOSTS_DB case is incorrect.
HostDataSourceFactory::create is called by HostMgr::create() not
HostMgr?'s constructor. It would also be good if this block pointed
out that the HostMgr::create() will destroy a pre-existing instance.


src/lib/dhcpsrv/host_mgr.cc

HostMgr::create(const std::string& access)

HostMgr::alternate_source is always set whether access is empty or not.
That seems strange. If I go from an external source (i.e. access not
empty) to an internal source (i.e. access empty) won't I get the previous
value for data source? HostDataSourceFactory::getHostDataSourcePtr()
returns a local static pointer which won't get reset by anything.


src/bin/dhcp6/tests/dhcp6_srv_unittest.cc

TEST_F(Dhcpv6SrvTest, hostDataSourceMySQL)

3682 is done and merged, this should active now right?

+ / @todo: Uncomment this once #3682 is merged.
+ / EXPECT_TRUE(hds);

Same thing in dhcp4.


doc/guide/dhcp6-srv.xml

Right you only have the comment itself commented out not the whole
section on Hosts Storage:

+<!-- Uncomment this once 4212 is merged. While the host storage for
+     MySQL is there, it is able to store hosts, but not IPv6
+     reservations. So technically we have host reservations for v6 in
+     MySQL, but in practice it's kinda useless right now as it can't
+     store addresses or prefixes. Let's keep it commented out until
+     we implement that functionality -->

I don't think this is what you intended to do. Did you intend to comment
out the Hosts Storage section itself?


comment:11 in reply to: ↑ 10 Changed 4 years ago by tomek

  • Owner changed from tomek to tmark

Replying to tmark:


src/lib/dhcpsrv/host_mgr.h

Instance member HostMgr::alternate_source should end with an underscore.

Fixed.


src/lib/dhcpsrv/parsers/dbaccess_parser.cc

DbAccessParser::commit()

Commentary block in the HOSTS_DB case is incorrect.
HostDataSourceFactory::create is called by HostMgr::create() not
HostMgr?'s constructor. It would also be good if this block pointed
out that the HostMgr::create() will destroy a pre-existing instance.

Updated. Hopefully is more correct and more informative now.


src/lib/dhcpsrv/host_mgr.cc

HostMgr::create(const std::string& access)

HostMgr::alternate_source is always set whether access is empty or not.
That seems strange. If I go from an external source (i.e. access not
empty) to an internal source (i.e. access empty) won't I get the previous
value for data source? HostDataSourceFactory::getHostDataSourcePtr()
returns a local static pointer which won't get reset by anything.

Excellent catch. Added explicit call to destroy() if the access string is
empty and added a bit of a comment.


src/bin/dhcp6/tests/dhcp6_srv_unittest.cc

TEST_F(Dhcpv6SrvTest, hostDataSourceMySQL)

3682 is done and merged, this should active now right?

+ / @todo: Uncomment this once #3682 is merged.
+ / EXPECT_TRUE(hds);

Same thing in dhcp4.

After much discussion on Jabber, Thomas and I decided to remove this test. Currently we
do not test MySQL or Postgres from the server perspective. It's doable, but requires
non-trivial reorganization in the code, which is far too big for this ticket.
Therefore created #4214 to address the bigger picture.


doc/guide/dhcp6-srv.xml

Right you only have the comment itself commented out not the whole
section on Hosts Storage:

+<!-- Uncomment this once 4212 is merged. While the host storage for
+     MySQL is there, it is able to store hosts, but not IPv6
+     reservations. So technically we have host reservations for v6 in
+     MySQL, but in practice it's kinda useless right now as it can't
+     store addresses or prefixes. Let's keep it commented out until
+     we implement that functionality -->

I don't think this is what you intended to do. Did you intend to comment
out the Hosts Storage section itself?

Uh oh, yes. Corrected.

comment:12 Changed 4 years ago by tomek

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

As signed off on jabber, this ticket is ready to go.

Code merged.

Thank you for the review.

Closing ticket.

comment:13 Changed 4 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.