Opened 6 years ago

Closed 5 years ago

#3556 closed enhancement (complete)

Extend Lease6 storage to cover MAC address (MySQL)

Reported by: tomek Owned by: tomek
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

Once #3555 is done, we'll need to extend MySQL to store MAC addresses.

Subtickets

Change History (7)

comment:1 Changed 5 years ago by tomek

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

comment:2 Changed 5 years ago by tomek

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

The code is ready for review. I did update the unit-tests, so it matches the changes done in #3599. If we finish this review earlier, it will have to wait for #3599.

If you want to test it, beyond running unit-tests, kea-admin from #3599 is needed. Please use the following:
kea-admin upgrade mysql (if upgrading existing db)
or
kea-admin init mysql (if starting from scratch)

Proposed changelog:

8XX.	[func]		tomek
	MySQL backend is now able to store information about hardware
	addresses and associated information in DHCPv6.
	(Trac #3556, git tbd)

comment:3 Changed 5 years ago by marcin

  • Owner changed from UnAssigned to marcin

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

  • Owner changed from marcin to tomek

Reviewed commit 1ea3e6c973ae6694deb2282ef00cbed817c101e3

src/lib/dhcpsrv/mysql_lease_mgr.cc
Not a big deal, but the binding preparation for the hwaddr, hwtype and hw source (between line 838 and 897) could be enclosed in a single if-else statement:

if (hwaddr) {
...
} else {
...
}

But, if you prefer to split them for the clarity of the code, that's fine. I am not sure it has any performance penalty. Perhaps negligible.

src/lib/dhcpsrv/tests/schema_mysql_copy.h
Instead of this:

UPDATE schema_version SET version="2", minor="0";

could you simply update the statement in line 94:

INSERT INTO schema_version VALUES (2, 0)

??

I also wonder whether the lease6_hwaddr_source table is needed at all? It slows down the test execution and it is unclear for me how this will be used. If this is for logging purposes, perhaps it is better to use the constants defined in Pkt class instead? The libdhcp could have a function which maps the values gathered from the database to the names of the hw address sources. This could be used for all backends, whereas when it is defined in the lease database, the lease database needs to be updated every time there is new source defined.

Besides that, if you created the new table for unit tests, shouldn't you have created it in the actual schema used by the running server?

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

  • Owner changed from tomek to marcin

Replying to marcin:

Reviewed commit 1ea3e6c973ae6694deb2282ef00cbed817c101e3

src/lib/dhcpsrv/mysql_lease_mgr.cc
Not a big deal, but the binding preparation for the hwaddr, hwtype and hw source (between line 838 and 897) could be enclosed in a single if-else statement:

if (hwaddr) {
...
} else {
...
}

But, if you prefer to split them for the clarity of the code, that's fine. I am not sure it has any performance penalty. Perhaps negligible.

Yes, that was my intention. Once we add more fields to the table, the logic will be more and more convoluted, but it's good to have a clear code that intiates each field, one by one.

src/lib/dhcpsrv/tests/schema_mysql_copy.h
Instead of this:

UPDATE schema_version SET version="2", minor="0";

could you simply update the statement in line 94:

INSERT INTO schema_version VALUES (2, 0)

??

I wanted to keep each phase of the schema upgrade separated. Added extra comments to make it more obvious.

I also wonder whether the lease6_hwaddr_source table is needed at all?

I originally added it to keep parity with the actual schema, but on reflection I think it is useless. The table is now not created. I left its initialization code, but it is commented out. There's also extra comment that explains the reasons.

comment:6 Changed 5 years ago by marcin

  • Owner changed from marcin to tomek

I reviewed the commit c96df51663810d0d8c3237b3a5914188d1176cc5

I updated copyright headers in 3 files, so please pull my change before you merge it!

You left the "lease6_hwaddr_source" in the schema_mysql_copy.h commented out. That's fine if we really want this table to exist in the production database. I am kind of opposed to this idea for the following reasons:

  • This table is unused in the Kea code and there are no known usage scenarios for this table outside of Kea.
  • Every update to the list of sources of hw addresses rises a need to update relevant tables in the supported databases, i.e. MySQL and Postgres. This implies the bump in the schema version. There is a question if this is acceptable burden relative to the actual usefulness.

If you still think this table is needed, I am fine with keeping it. But, it may be worth to rename it to: "lease_hwaddr_source" (removed "6") because the collection of sources already seems to be common for both DHCPv4 and DHCPv6, as it is defined in the !Pkt class which is a base for !Pkt4 and !Pkt6. In fact, some of the sources (e.g. raw socket) are specific to DHCPv4, and there is no relevant implementation for DHCPv6.

Whatever you decide I don't need to see this ticket again, because in the worst case, the changes will affect the code that is commented out anyway.

comment:7 Changed 5 years ago by tomek

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

lease6_hwaddr_source renamed to reflect similar changes in #3559. I decided to keep the table commented out. We have similar table for lease6 types that is also not used in kea code.

Code merged, closing ticket.

Thanks for your review.

Note: See TracTickets for help on using tickets.