Opened 4 years ago

Closed 3 years ago

#4279 closed enhancement (complete)

Extend PostgreSQL host data source with client classes

Reported by: tomek Owned by: tmark
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: 0.5
Total Hours: 12.5 Internal?: no

Description (last modified by tomek)

Tickets #4277 covers implementation of the host reservation in its simple form. In particular, it does not cover:

  • Extend host data source to handle IPv6 reservations (#4278)
  • Extend host data source to handle client classes (this ticket)
  • Extend host data source to handle options (#4280)

This ticket covers implementing IPv4 and IPv6 client classes and storing and retrieving them from PostgreSQL.

Subtickets

Change History (9)

comment:1 Changed 4 years ago by tomek

  • Description modified (diff)

comment:2 Changed 4 years ago by tomek

  • Description modified (diff)

comment:3 Changed 3 years ago by tmark

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

comment:4 Changed 3 years ago by tmark

  • Add Hours to Ticket changed from 0 to 8
  • Owner changed from tmark to Unassigned
  • Status changed from assigned to reviewing
  • Total Hours changed from 0 to 8

The primary effort here was implementing unit tests for both MySQL and PostgreSQL for HR client classes. There was a minor coding error in the MySQl code, but other than that both backends were already capable of storing and retrieving HR client classes.

This ticket also completes #4213.

I suggest the following ChangeLog? entry:

11xx.   [func]      tmark
    Support for assigning client classes to host reservations has been added to
    both the PostgreSQL and MySQL backends.
    (Trac #4277, #4213 git TBD)

comment:5 Changed 3 years ago by marcin

  • Owner changed from Unassigned to marcin

comment:6 follow-up: Changed 3 years ago by marcin

  • Add Hours to Ticket changed from 8 to 2
  • Owner changed from marcin to tmark
  • Total Hours changed from 8 to 10

I reviewed 41e69be383433ab3dc8b1e4219d609f500b5e7f8

src/lib/dhcpsrv/tests/generic_host_data_source_unittest.cc
In all new tests there is a commentary at the beginning "Add host reservation with multiple...." which doesn't correspond to the following statement:

ASSERT_TRUE(hdsptr_);

that merely checks if the pointer to the data source has been initialized for a test case.

testMultipleClientClasses4: Variable "len" is initialized but unused.

All three tests use either "get4" or "get6" methods to retrieve hosts with the classes. These test should also exercise retrieving classes with other methods exposed by the host data source, i.e. getAll, getAll4 and various variants of those methods - using identifier, using addres, using prefix etc. Note that different variants of those functions may use different queries to retrieve the host information. Checking that get4 and get6 works doesn't guarantee that getAll methods work too. It should be really easy to reuse existing tests, just add a parameter that specifies which functions should be executed to retrieve host.

compareClientClasses: This method seems to work/compile fine. But, I recall having issues with using GTEST macros to compare STL containers because GTEST mecros require that overload of the << operator exists. From reading STL classess' documentation I am not sure if such operator must always be defined for STL containers. Admittedly, I checked with both clang and gcc on Centos 5 (which is supposedly picky about various things) that this compiles fine. Perhaps, because the container holds strings, rather than other objects for which this operator may not exist. So, if you're sure that this comparison is portable it is ok to leave it in. Personally, I always use EXPECT_TRUE(std::compare(...,...)) for such comparisons to be on a safe side. The down side, though, is that you don't see the mismatched values in the output from EXPECT_TRUE.

src/lib/dhcpsrv/tests/generic_host_data_source_unittest.h
src/lib/dhcpsrv/tests/mysql_host_data_source_unittest.cc
src/lib/dhcpsrv/tests/pgsql_host_data_source_unittest.cc
Again, typo "IPv5" in description of (test)MultipleClientClassesBoth in all three files.

comment:7 in reply to: ↑ 6 Changed 3 years ago by tmark

  • Owner changed from tmark to marcin
  • Total Hours changed from 10 to 12

Replying to marcin:

I reviewed 41e69be383433ab3dc8b1e4219d609f500b5e7f8

src/lib/dhcpsrv/tests/generic_host_data_source_unittest.cc
In all new tests there is a commentary at the beginning "Add host reservation with multiple...." which doesn't correspond to the following statement:

ASSERT_TRUE(hdsptr_);

that merely checks if the pointer to the data source has been initialized for a test case.

The comment was really describing the test in general but since that is done
in the header I've removed it from the cc files.

testMultipleClientClasses4: Variable "len" is initialized but unused.

Oops. Got it.

All three tests use either "get4" or "get6" methods to retrieve hosts with the classes. These test should also exercise retrieving classes with other methods exposed by the host data source, i.e. getAll, getAll4 and various variants of those methods - using identifier, using addres, using prefix etc. Note that different variants of those functions may use different queries to retrieve the host information. Checking that get4 and get6 works doesn't guarantee that getAll methods work too. It should be really easy to reuse existing tests, just add a parameter that specifies which functions should be executed to retrieve host.

I've added the variants as requested but directly to the existing tests.

compareClientClasses: This method seems to work/compile fine. But, I recall having issues with using GTEST macros to compare STL containers because GTEST mecros require that overload of the << operator exists. From reading STL classess' documentation I am not sure if such operator must always be defined for STL containers. Admittedly, I checked with both clang and gcc on Centos 5 (which is supposedly picky about various things) that this compiles fine. Perhaps, because the container holds strings, rather than other objects for which this operator may not exist. So, if you're sure that this comparison is portable it is ok to leave it in. Personally, I always use EXPECT_TRUE(std::compare(...,...)) for such comparisons to be on a safe side. The down side, though, is that you don't see the mismatched values in the output from EXPECT_TRUE.

After clarifying that you meant "std::equal", I have made the change.

src/lib/dhcpsrv/tests/generic_host_data_source_unittest.h
src/lib/dhcpsrv/tests/mysql_host_data_source_unittest.cc
src/lib/dhcpsrv/tests/pgsql_host_data_source_unittest.cc
Again, typo "IPv5" in description of (test)MultipleClientClassesBoth in all three files.

Fixed.

comment:8 Changed 3 years ago by marcin

  • Add Hours to Ticket changed from 2 to 0.5
  • Owner changed from marcin to tmark
  • Total Hours changed from 12 to 12.5

I reviewed commit 44c9306060af6e705f28dd6b8d5fa0889fd20128, and your changes look ok.

I also pushed a little commit which corrects indentation in two comments, within the tests (trivial change).

You're ready to go with your ticket.

comment:9 Changed 3 years ago by tmark

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

Changes merged with git 6990ab9d542e984c59ce5a11ff926c3c732a75fc
Added ChangeLog? entry 1151.

Ticket is complete.

Note: See TracTickets for help on using tickets.