#5586 closed defect (complete)

Implement main support for Shared Lease Storage Stats for MySQL

Reported by: tmark Owned by: tmark
Priority: medium Milestone: Kea1.4
Component: database-mysql Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket: Shared Lease Statistics
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Implement the changes described in section 4.1 of the Shared Lease Stats
Design (http://kea.isc.org/wiki/SharedLeaseStorageStats) for MySQL:

4.1.1 New Tables and Triggers
4.1.2 Add new SQL queries to LeaseMgr? derivations
4.1.3 Extend the LeaseStatsQuery? class and derivations
4.1.4 Overload !LeaseMgr::startLeaseStatsQuery<4/6>

(Depends on #5585)

Subtickets

Change History (10)

comment:1 Changed 12 months ago by tmark

  • Milestone changed from Kea-proposed to Kea1.4
  • Owner set to tmark
  • Status changed from new to assigned

comment:2 Changed 11 months ago by tmark

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

MySQL shared lease stats are implemented and ready for review.

These changes are internal, so I'm not certain they require a ChangeLog?.

comment:3 Changed 11 months ago by marcin

  • Owner changed from UnAssigned to marcin

comment:4 follow-up: Changed 11 months ago by marcin

  • Owner changed from marcin to tmark

Reviewed commit a662ab45d9271c5892c0e499fb65385f47bc4639.

I see you have updated both MySQL and PostgreSQL schemas. I suppose, the PgSQL specific changes in the lease manager will be implemented in another ticket?

The code is straight forward and I don't see any major issues with it. However, shouldn't you add some tests for upgrade scripts to verify the presence of new tables and some basic functionality of the triggers?

src/lib/dhcpsrv/memfile_lease_mgr.cc
I corrected some little typos in this file, so please pull.

src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc

You have added the following comment:

// @todo Under MacOS, connecting with an invalid host, causes a TCP/IP socket
// to be orphaned and never closed.  This can interfer with subsequent tests
// which attempt to locate and manipulate MySQL client socket descriptor.
// In the interests of progress, we'll just avoid this test.

I wonder if it would be better to exclude this test on macOS only? Or on OS_BSD? Or, you're concerned that it may behave simiarly on other OSes too, but we don't know yet?

You have added multiple calls to:

    LeaseMgrFactory::destroy();

in the MySqlOpenTest::OpenDatabase?, but the same call is in the try { } statement. Can you remove those calls from the try { } ?

comment:5 in reply to: ↑ 4 Changed 11 months ago by tmark

Replying to marcin:

Reviewed commit a662ab45d9271c5892c0e499fb65385f47bc4639.

I see you have updated both MySQL and PostgreSQL schemas. I suppose, the PgSQL specific changes in the lease manager will be implemented in another ticket?

Yes, PosgreSQL is being done under 5587. I guess there was some bleed over.

The code is straight forward and I don't see any major issues with it. However, shouldn't you add some tests for upgrade scripts to verify the presence of new tables and some basic functionality of the triggers?

Fair point. It also reminded me that the upgrade must account for existing leases in the database. When that's the case the lease stat tables must be populated based on the existing lease content.
I added this to the upgrade script and tests.

Towards that end, I added a 5.1 version of the schema create to the tests directory as the I wanted a schema that contained lease_state so it could be populated with leases and then migrated and then
tested.

src/lib/dhcpsrv/memfile_lease_mgr.cc
I corrected some little typos in this file, so please pull.

Thanks.

src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc

You have added the following comment:

// @todo Under MacOS, connecting with an invalid host, causes a TCP/IP socket
// to be orphaned and never closed.  This can interfer with subsequent tests
// which attempt to locate and manipulate MySQL client socket descriptor.
// In the interests of progress, we'll just avoid this test.

I wonder if it would be better to exclude this test on macOS only? Or on OS_BSD? Or, you're concerned that it may behave simiarly on other OSes too, but we don't know yet?

I could not get this test to fail as it had been under OS-X and it runs fine under Ubuntu 16.04 as well. So I have removed the conditional compile and comment.

You have added multiple calls to:

    LeaseMgrFactory::destroy();

in the MySqlOpenTest::OpenDatabase?, but the same call is in the try { } statement. Can you remove those calls from the try { } ?

Done.

Ready for re-review.

comment:6 Changed 11 months ago by marcin

  • Owner changed from tmark to marcin

comment:7 follow-up: Changed 11 months ago by marcin

  • Owner changed from marcin to tmark

I re-reviewed the branch.

There is a failing unit test on my macOS:

[ RUN      ] MySQLLeaseMgrDbLostCallbackTest.testDbLostCallback
generic_lease_mgr_unittest.cc:2854: Failure
Expected: version = lm.getVersion() throws an exception of type DbOperationError.
  Actual: it throws nothing.
[  FAILED  ] MySQLLeaseMgrDbLostCallbackTest.testDbLostCallback (976 ms)

but it seems unrelated to your current changes. Do you recall where that comes from? Should I create a bug ticket?

Adding a create script for 5.1 seems like moving us backwards to the times when we had a lot of duplicated SQL everywhere. Couldn't we simply run a sequence of existing upgrade scripts to get to 5.1 schema?

Otherwise, the changes look good to me and the new tests pass.

comment:8 in reply to: ↑ 7 Changed 11 months ago by tmark

  • Owner changed from tmark to marcin

Replying to marcin:

I re-reviewed the branch.

There is a failing unit test on my macOS:

[ RUN      ] MySQLLeaseMgrDbLostCallbackTest.testDbLostCallback
generic_lease_mgr_unittest.cc:2854: Failure
Expected: version = lm.getVersion() throws an exception of type DbOperationError.
  Actual: it throws nothing.
[  FAILED  ] MySQLLeaseMgrDbLostCallbackTest.testDbLostCallback (976 ms)

but it seems unrelated to your current changes. Do you recall where that comes from? Should I create a bug ticket?

This was the test that I had #if 0'd out before because it can cause DB connectivity lost tests to fail under OSX. Per our Jabber agreement I have added #ifndef OS_OSX around the test.

Adding a create script for 5.1 seems like moving us backwards to the times when we had a lot of duplicated SQL everywhere. Couldn't we simply run a sequence of existing upgrade scripts to get to 5.1 schema?

Ok, you're right. The 5.1 schema script is gone. I added a function that allows one to upgrade to
a target version number.

Otherwise, the changes look good to me and the new tests pass.

Please re-review.

comment:9 Changed 11 months ago by marcin

  • Owner changed from marcin to tmark

Now it is much better. All tests now pass on my macOS. You're good to go.

comment:10 Changed 11 months ago by tmark

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

Changes merged, minus the PostgreSQL script changes.

Ticket is complete.

Note: See TracTickets for help on using tickets.