Opened 4 years ago

Closed 4 years ago

#4323 closed enhancement (complete)

Enable selection of PostgreSQL backend for host reservations

Reported by: marcin Owned by: marcin
Priority: medium Milestone: Kea1.1
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: 1
Total Hours: 6 Internal?: no

Description

Once the PgSqlHostDataSource is implemented, it should be possible to select this backend via server configuration. This ticket will require a bunch of unit tests to check that the backend can co-existing with reservations specified in the configuration file.

Subtickets

Change History (9)

comment:1 Changed 4 years ago by tmark

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

Implemented by #4277

comment:2 Changed 4 years ago by tmark

  • Owner changed from tmark to Unassigned

The above statement is inaccurate. Unless I'm mistaken none of the tests in generic_host_data_source.cc/h verify configured HRs in conjunction with DB HRs. This means that none of the backends test this.

I am relinquishing ownership, simply because I need to work on ISC_DHCP for two weeks.

comment:3 Changed 4 years ago by marcin

  • Owner changed from Unassigned to marcin
  • Status changed from assigned to accepted

comment:4 Changed 4 years ago by marcin

  • Add Hours to Ticket changed from 0 to 5
  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to reviewing
  • Total Hours changed from 0 to 5

I have implemented additional tests for HostMgr to verify that reservations can also be retrieved from the alternate host data source. As this is user invisible change, I don't provide any ChangeLog proposal.

comment:5 Changed 4 years ago by sar

  • Owner changed from UnAssigned to sar

comment:6 follow-up: Changed 4 years ago by sar

  • Owner changed from sar to marcin

I was able to run the MySQL tests but don't have Postgres installed so I couldn't check them out.

src/lib/dhcpsrv/tests/host_mgr_unittest.cc

In testGetAll the second ASSERT_TRUE (line 212) is potentially confusing, rewriting to be similar to the first ASSERT_TRUE would be a bit clearer.

testGetAll4 (line 252) could use some comments to describe what it is doing.

testGet4 (line 275) testGet6 (line 292)
could use some comments to describe what it is doing
there are multiple functions for get4 and get6 using different arguments - do we test all of them?

testGet6ByPrefix
Shouldn't the IOAddress lines at 326 and 337 be indented?

If it is not tested elsewhere the tests testGetAll, testGetAll4, testGet4 and testGet6 should include adding hosts that are not going to be retrieved in order to verify that only the requested entries are retrieved. For example in testGetAll4 it could add a third host entry with a different ip address.

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

  • Add Hours to Ticket changed from 5 to 1
  • Owner changed from marcin to sar
  • Total Hours changed from 5 to 6

Replying to sar:

I was able to run the MySQL tests but don't have Postgres installed so I couldn't check them out.

I ran PostgreSQL tests in Jenkins: https://jenkins.isc.org/view/Kea_BuildFarm-developers/job/Ubuntu_14.04_32_pgsql_branch/24/ and they passed. I think this should be enough.

src/lib/dhcpsrv/tests/host_mgr_unittest.cc

In testGetAll the second ASSERT_TRUE (line 212) is potentially confusing, rewriting to be similar to the first ASSERT_TRUE would be a bit clearer.

I did that as suggested.

testGetAll4 (line 252) could use some comments to describe what it is doing.

Added comments.

testGet4 (line 275) testGet6 (line 292)
could use some comments to describe what it is doing

Added comments.

there are multiple functions for get4 and get6 using different arguments - do we test all of them?

Variants which take HW address and DUID as arguments are planned to be removed, so I thought it is a waste of effort to write specific tests for them. Also note that these variants use the get4(identifier, identifier_type) function underneath, which is tested. So, I guess we are ok.

testGet6ByPrefix
Shouldn't the IOAddress lines at 326 and 337 be indented?

It is indented now.

If it is not tested elsewhere the tests testGetAll, testGetAll4, testGet4 and testGet6 should include adding hosts that are not going to be retrieved in order to verify that only the requested entries are retrieved. For example in testGetAll4 it could add a third host entry with a different ip address.

The purpose of these tests is to demonstrate that you can use two host data sources simultaneously, i.e. config based reservations and reservations in SQL database. The more complex tests are ran for specific backends and they are implemented in generic_host_data_source_unittest.cc.

comment:8 Changed 4 years ago by sar

  • Owner changed from sar to marcin

I've reviewed the changes.

The code looks fine and is ready for merging.

comment:9 Changed 4 years ago by marcin

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

Merged with commit a966a54a0460f8c6c009c8b0a97b6cf3c9fa9969

Note: See TracTickets for help on using tickets.