From f09bf17e2e4a39e3afafba5cfc3e91851ce3016b Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Fri, 6 Apr 2018 15:16:24 -0400 Subject: [PATCH 1/5] [5556a] MySQL lease and host backends now support configurable auto-reconnect src/lib/dhcpsrv/mysql_connection.h MySqlConnection::checkError<>() - modified to invoke db lost callback src/lib/dhcpsrv/dhcpsrv_messages.mes Updated log messages src/lib/dhcpsrv/mysql_lease_mgr.cc MySqlLeaseMgr::getVersion() - updated to use checkError() src/lib/dhcpsrv/pgsql_connection.* PgSqlResult::PgSqlResult(PGresult *result) - now supports construction with null PGresult. This is to accomodate rare cases when PQ* statements can return NULL. src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.* class LeaseMgrDbLostCallbackTest - new test fixture for testing LeaseMgr DBLostCallback behavior src/lib/dhcpsrv/tests/host_mgr_unittest.cc class HostMgrDbLostCallbackTest class MySQLHostMgrDbLostCallbackTest class PostgreSQLHostMgrDbLostCallbackTest - new test fixtures for testing HostMgr DBLostCallback behavior src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc class MySQLLeaseMgrDbLostCallbackTest - new test fixture for testing MySQL LeaseMgr DBLostCallback behavior src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc class PgSqlLeaseMgrDbLostCallbackTest - new test fixture for testing Postgresql LeaseMgr DBLostCallback behavior src/lib/dhcpsrv/tests/test_utils.* int findLastSocketFd() - new function used for finding what should be the fd of the SQL client socket doc/guide/dhcp4-srv.xml doc/guide/dhcp6-srv.xml Updated lease and host database parameter sections --- doc/guide/dhcp4-srv.xml | 41 ++++- doc/guide/dhcp6-srv.xml | 47 +++++- src/bin/dhcp4/dhcp4_messages.mes | 8 +- src/bin/dhcp6/dhcp6_messages.mes | 8 +- src/lib/dhcpsrv/dhcpsrv_messages.mes | 18 ++- src/lib/dhcpsrv/mysql_connection.h | 29 ++-- src/lib/dhcpsrv/mysql_lease_mgr.cc | 16 +- src/lib/dhcpsrv/pgsql_connection.cc | 17 ++- src/lib/dhcpsrv/pgsql_connection.h | 23 +-- src/lib/dhcpsrv/pgsql_lease_mgr.cc | 2 + .../tests/generic_lease_mgr_unittest.cc | 61 ++++++++ .../tests/generic_lease_mgr_unittest.h | 57 +++++++ src/lib/dhcpsrv/tests/host_mgr_unittest.cc | 141 +++++++++++++++++- .../dhcpsrv/tests/mysql_lease_mgr_unittest.cc | 32 ++++ .../dhcpsrv/tests/pgsql_lease_mgr_unittest.cc | 53 +++---- src/lib/dhcpsrv/tests/test_utils.cc | 28 +++- src/lib/dhcpsrv/tests/test_utils.h | 17 ++- 17 files changed, 510 insertions(+), 88 deletions(-) diff --git a/doc/guide/dhcp4-srv.xml b/doc/guide/dhcp4-srv.xml index 395b74b5be..90593ff6d8 100644 --- a/doc/guide/dhcp4-srv.xml +++ b/doc/guide/dhcp4-srv.xml @@ -480,7 +480,26 @@ be followed by a comma and another object definition. The default value of five seconds should be more than adequate for local connections. If a timeout is given though, it should be an integer greater than zero. - + +The maxiumum number of times the server will automatically attempt to reconnect to +the lease database after connectivity has been lost may be specified: + +"Dhcp4": { "lease-database": { "max-reconnect-tries" : number-of-tries, ... }, ... } + +If the server is unable to reconnect to the database after making the maximum number +of attempts the server will exit. A value of zero (the default) disables automatic +recovery and the server will exit immediately upon detecting a loss of connectivity +(MySQL and Postgres only). + + +The number of seconds the server will wait in between attempts to reconnect to the +lease database after connectivity has been lost may also be specified: + +"Dhcp4": { "lease-database": { "reconnect-wait-time" : number-of-seconds, ... }, ... } + +A value of zero (the default) disables automatic recovery and the server will exit +immediately upon detecting a loss of connectivity (MySQL and Postgres only). + Finally, the credentials of the account under which the server will access the database should be set: @@ -627,6 +646,26 @@ Similar parameters can be specified for hosts database. "Dhcp4": { "hosts-database": { "port" : 12345, ... }, ... } + +The maxiumum number of times the server will automatically attempt to reconnect to +the host database after connectivity has been lost may be specified: + +"Dhcp4": { "hosts-database": { "max-reconnect-tries" : number-of-tries, ... }, ... } + +If the server is unable to reconnect to the database after making the maximum number +of attempts the server will exit. A value of zero (the default) disables automatic +recovery and the server will exit immediately upon detecting a loss of connectivity +(MySQL and Postgres only). + + +The number of seconds the server will wait in between attempts to reconnect to the +host database after connectivity has been lost may also be specified: + +"Dhcp4": { "hosts-database": { "reconnect-wait-time" : number-of-seconds, ... }, ... } + +A value of zero (the default) disables automatic recovery and the server will exit +immediately upon detecting a loss of connectivity (MySQL and Postgres only). + Finally, the credentials of the account under which the server will diff --git a/doc/guide/dhcp6-srv.xml b/doc/guide/dhcp6-srv.xml index 9ca2dd62dc..f734d7879d 100644 --- a/doc/guide/dhcp6-srv.xml +++ b/doc/guide/dhcp6-srv.xml @@ -471,7 +471,7 @@ be followed by a comma and another object definition. Should the database use a port different than default, it may be specified as well: -"Dhcp4": { "lease-database": { "port" : 12345, ... }, ... } +"Dhcp6": { "lease-database": { "port" : 12345, ... }, ... } Should the database be located on a different system, you may need to specify a longer interval for the connection timeout: @@ -481,14 +481,33 @@ be followed by a comma and another object definition. The default value of five seconds should be more than adequate for local connections. If a timeout is given though, it should be an integer greater than zero. - + +The maxiumum number of times the server will automatically attempt to reconnect to +the lease database after connectivity has been lost may be specified: + +"Dhcp6": { "lease-database": { "max-reconnect-tries" : number-of-tries, ... }, ... } + +If the server is unable to reconnect to the database after making the maximum number +of attempts the server will exit. A value of zero (the default) disables automatic +recovery and the server will exit immediately upon detecting a loss of connectivity +(MySQL and Postgres only). + + +The number of seconds the server will wait in between attempts to reconnect to the +lease database after connectivity has been lost may also be specified: + +"Dhcp6": { "lease-database": { "reconnect-wait-time" : number-of-seconds, ... }, ... } + +A value of zero (the default) disables automatic recovery and the server will exit +immediately upon detecting a loss of connectivity (MySQL and Postgres only). + Note that host parameter is used by MySQL and PostgreSQL backends. Cassandra has a concept of contact points that could be used to contact the cluster, instead of a single IP or hostname. It takes a list of comma separated IP addresses. This may be specified as: -"Dhcp4": { "lease-database": { "contact-points" : "192.0.2.1,192.0.2.2", ... }, ... } +"Dhcp6": { "lease-database": { "contact-points" : "192.0.2.1,192.0.2.2", ... }, ... } @@ -565,8 +584,28 @@ linkend="cassandra-database-configuration4"/> for details. "Dhcp6": { "hosts-database": { "host" : "", ... }, ... } -"Dhcp4": { "hosts-database": { "port" : 12345, ... }, ... } +"Dhcp6": { "hosts-database": { "port" : 12345, ... }, ... } + + +The maxiumum number of times the server will automatically attempt to reconnect to +the host database after connectivity has been lost may be specified: + +"Dhcp6": { "host-database": { "max-reconnect-tries" : number-of-tries, ... }, ... } + +If the server is unable to reconnect to the database after making the maximum number +of attempts the server will exit. A value of zero (the default) disables automatic +recovery and the server will exit immediately upon detecting a loss of connectivity +(MySQL and Postgres only). + + +The number of seconds the server will wait in between attempts to reconnect to the +host database after connectivity has been lost may also be specified: + +"Dhcp6": { "hosts-database": { "reconnect-wait-time" : number-of-seconds, ... }, ... } + +A value of zero (the default) disables automatic recovery and the server will exit +immediately upon detecting a loss of connectivity (MySQL and Postgres only). Finally, the credentials of the account under which the server will access the database should be set: diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes index 8083a621d9..7846d17648 100644 --- a/src/bin/dhcp4/dhcp4_messages.mes +++ b/src/bin/dhcp4/dhcp4_messages.mes @@ -143,7 +143,7 @@ updated configuration from the Kea configuration system. % DHCP4_DB_RECONNECT_ATTEMPT_SCHEDULE scheduling attempt %1 of %2 in %3 seconds An informational message indicating that the server is scheduling the next attempt to reconnect to its lease and/or host databases. This occurs when the -server has lost databse connectivity and is attempting to reconnect +server has lost databse connectivity and is attempting to reconnect automatically. % DHCP4_DB_RECONNECT_ATTEMPT_FAILED database reconnect failed: %1 @@ -155,15 +155,15 @@ has been lost and an automatic attempt to reconnect has failed. This is an error message indicating a programmatic error that should not occur. It will prohibit the server from attempting to reconnect to its databases if connectivy is lost, and the server will exit. This error -should be reported. +should be reported. -% DHCP4_DB_RECONNECT_DISABLED database reconnect is disabled: max-reconnect-tries %1, reconnect-wait-time %1 +% DHCP4_DB_RECONNECT_DISABLED database reconnect is disabled: max-reconnect-tries %1, reconnect-wait-time %2 This is an informational message indicating that connectivity to either the lease or host database or both and that automatic reconnect is not enabled. % DHCP4_DB_RECONNECT_RETRIES_EXHAUSTED maximum number of database reconnect attempts: %1, has been exhausted without success, server is shutting down! This error indicates that the server is shutting down after failing to reconnect to -the lease and/or host database(s) after making the maximum configured number +the lease and/or host database(s) after making the maximum configured number of reconnect attempts. Loss of connectivity is typically a network or database server issue. diff --git a/src/bin/dhcp6/dhcp6_messages.mes b/src/bin/dhcp6/dhcp6_messages.mes index 204e0ddd02..127022b3e1 100644 --- a/src/bin/dhcp6/dhcp6_messages.mes +++ b/src/bin/dhcp6/dhcp6_messages.mes @@ -103,7 +103,7 @@ lease and other information. % DHCP6_DB_RECONNECT_ATTEMPT_SCHEDULE scheduling attempt %1 of %2 in %3 seconds An informational message indicating that the server is scheduling the next attempt to reconnect to its lease and/or host databases. This occurs when the -server has lost databse connectivity and is attempting to reconnect +server has lost databse connectivity and is attempting to reconnect automatically. % DHCP6_DB_RECONNECT_ATTEMPT_FAILED database reconnect failed: %1 @@ -115,15 +115,15 @@ has been lost and an automatic attempt to reconnect has failed. This is an error message indicating a programmatic error that should not occur. It will prohibit the server from attempting to reconnect to its databases if connectivy is lost, and the server will exit. This error -should be reported. +should be reported. -% DHCP6_DB_RECONNECT_DISABLED database reconnect is disabled: max-reconnect-tries %1, reconnect-wait-time %1 +% DHCP6_DB_RECONNECT_DISABLED database reconnect is disabled: max-reconnect-tries %1, reconnect-wait-time %2 This is an informational message indicating that connectivity to either the lease or host database or both and that automatic reconnect is not enabled. % DHCP6_DB_RECONNECT_RETRIES_EXHAUSTED maximum number of database reconnect attempts: %1, has been exhausted without success, server is shutting down! This error indicates that the server is shutting down after failing to reconnect to -the lease and/or host database(s) after making the maximum configured number +the lease and/or host database(s) after making the maximum configured number of reconnect attempts. Loss of connectivity is typically a network or database server issue. diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.mes b/src/lib/dhcpsrv/dhcpsrv_messages.mes index 245c497f67..132e51d47c 100644 --- a/src/lib/dhcpsrv/dhcpsrv_messages.mes +++ b/src/lib/dhcpsrv/dhcpsrv_messages.mes @@ -704,10 +704,12 @@ leases which have expired longer than a specified period of time. The argument is the amount of time Kea waits after a reclaimed lease expires before considering its removal. -% DHCPSRV_MYSQL_FATAL_ERROR Unrecoverable MySQL error occurred: %1 for <%2>, reason: %3 (error code: %4). Server exiting now! +% DHCPSRV_MYSQL_FATAL_ERROR Unrecoverable MySQL error occurred: %1 for <%2>, reason: %3 (error code: %4). An error message indicating that communication with the MySQL database server -has been lost. When this occurs the server exits immediately with a non-zero -exit code. This is most likely due to a network issue. +has been lost. If automatic recovery has been enabled, then the server will +attempt to recover connectivity. If not the server wil exit with a +non-zero exit code. The cause of such an error is most likely a network issue +or the MySQL server has gone down. % DHCPSRV_MYSQL_GET4 obtaining all IPv4 leases A debug message issued when the server is attempting to obtain all IPv4 @@ -874,10 +876,12 @@ leases which have expired longer than a specified period of time. The argument is the amount of time Kea waits after a reclaimed lease expires before considering its removal. -% DHCPSRV_PGSQL_FATAL_ERROR Unrecoverable PostgreSQL error occurred: Statement: <%1>, reason: %2 (error code: %3). Server exiting now! -An error message indicating that communication with the MySQL database server -has been lost. When this occurs the server exits immediately with a non-zero -exit code. This is most likely due to a network issue. +% DHCPSRV_PGSQL_FATAL_ERROR Unrecoverable PostgreSQL error occurred: Statement: <%1>, reason: %2 (error code: %3). +An error message indicating that communication with the Postgresql database server +has been lost. If automatic recovery has been enabled, then the server will +attempt to recover the connectivity. If not the server wil exit with a +non-zero exit code. The cause of such an error is most likely a network issue +or the Postgresql server has gone down. % DHCPSRV_PGSQL_GET4 obtaining all IPv4 leases A debug message issued when the server is attempting to obtain all IPv4 diff --git a/src/lib/dhcpsrv/mysql_connection.h b/src/lib/dhcpsrv/mysql_connection.h index e1e8d76f27..b1d22d090b 100644 --- a/src/lib/dhcpsrv/mysql_connection.h +++ b/src/lib/dhcpsrv/mysql_connection.h @@ -355,15 +355,16 @@ public: /// in the event of failures, decide whether or not those failures are /// recoverable. /// - /// If the error is recoverable, the method will throw a DbOperationError. - /// In the error is deemed unrecoverable, such as a loss of connectivity - /// with the server, this method will log the error and call exit(-1); + /// If the error is recoverable, the function will throw a DbOperationError. + /// If the error is deemed unrecoverable, such as a loss of connectivity + /// with the server, the function will call invokeDbLostCallback(). If the + /// invocation returns false then either there is no callback registered + /// or the callback has elected not to attempt to reconnect, and exit(-1) + /// is called; /// - /// @todo Calling exit() is viewed as a short term solution for Kea 1.0. - /// Two tickets are likely to alter this behavior, first is #3639, which - /// calls for the ability to attempt to reconnect to the database. The - /// second ticket, #4087 which calls for the implementation of a generic, - /// FatalException class which will propagate outward. + /// If the invocation returns true, this indicates the calling layer will + /// attempt recovery, and the function throws a DbOperationError to allow + /// the caller to error handle the failed db access attempt. /// /// @param status Status code: non-zero implies an error /// @param index Index of statement that caused the error @@ -389,14 +390,22 @@ public: case CR_SERVER_LOST: case CR_OUT_OF_MEMORY: case CR_CONNECTION_ERROR: - // We're exiting on fatal DB_LOG_ERROR(MYSQL_FATAL_ERROR) .arg(what) .arg(text_statements_[static_cast(index)]) .arg(mysql_error(mysql_)) .arg(mysql_errno(mysql_)); - exit (-1); + // If there's no lost db callback or it returns false, + // then we're not attempting to recover so we're done + if (!invokeDbLostCallback()) { + exit (-1); + } + + // We still need to throw so caller can error out of the current + // processing. + isc_throw(DbOperationError, + "fatal database errror or connectivity lost"); default: // Connection is ok, so it must be an SQL error isc_throw(DbOperationError, what << " for <" diff --git a/src/lib/dhcpsrv/mysql_lease_mgr.cc b/src/lib/dhcpsrv/mysql_lease_mgr.cc index b4e489b144..0916427b9e 100644 --- a/src/lib/dhcpsrv/mysql_lease_mgr.cc +++ b/src/lib/dhcpsrv/mysql_lease_mgr.cc @@ -2240,11 +2240,7 @@ MySqlLeaseMgr::getVersion() const { // Execute the prepared statement int status = mysql_stmt_execute(conn_.statements_[stindex]); - if (status != 0) { - isc_throw(DbOperationError, "unable to execute <" - << conn_.text_statements_[stindex] << "> - reason: " << - mysql_error(conn_.mysql_)); - } + checkError(status, stindex, "unable to execute statement"); // Bind the output of the statement to the appropriate variables. MYSQL_BIND bind[2]; @@ -2261,19 +2257,13 @@ MySqlLeaseMgr::getVersion() const { bind[1].buffer_length = sizeof(minor); status = mysql_stmt_bind_result(conn_.statements_[stindex], bind); - if (status != 0) { - isc_throw(DbOperationError, "unable to bind result set: " << - mysql_error(conn_.mysql_)); - } + checkError(status, stindex, "unable to bind result set"); // Fetch the data and set up the "release" object to release associated // resources when this method exits then retrieve the data. MySqlFreeResult fetch_release(conn_.statements_[stindex]); status = mysql_stmt_fetch(conn_.statements_[stindex]); - if (status != 0) { - isc_throw(DbOperationError, "unable to obtain result set: " << - mysql_error(conn_.mysql_)); - } + checkError(status, stindex, "unable to fetch result set"); return (std::make_pair(major, minor)); } diff --git a/src/lib/dhcpsrv/pgsql_connection.cc b/src/lib/dhcpsrv/pgsql_connection.cc index 14a54d3a1b..a64ce7f779 100644 --- a/src/lib/dhcpsrv/pgsql_connection.cc +++ b/src/lib/dhcpsrv/pgsql_connection.cc @@ -40,11 +40,16 @@ const char PgSqlConnection::DUPLICATE_KEY[] = ERRCODE_UNIQUE_VIOLATION; PgSqlResult::PgSqlResult(PGresult *result) : result_(result), rows_(0), cols_(0) { if (!result) { - isc_throw (BadValue, "PgSqlResult result pointer cannot be null"); + // Certain failures, like a loss of connectivity, can return a + // null PGresult and we still need to be able to create a PgSqlResult. + // We'll set row and col counts to -1 to prevent anyone going off the + // rails. + rows_ = -1; + cols_ = -1; + } else { + rows_ = PQntuples(result); + cols_ = PQnfields(result); } - - rows_ = PQntuples(result); - cols_ = PQnfields(result); } void @@ -302,7 +307,9 @@ PgSqlConnection::checkStatementError(const PgSqlResult& r, .arg(statement.name) .arg(PQerrorMessage(conn_)) .arg(sqlstate ? sqlstate : ""); - // If there's no lost db callback, then exit + + // If there's no lost db callback or it returns false, + // then we're not attempting to recover so we're done if (!invokeDbLostCallback()) { exit (-1); } diff --git a/src/lib/dhcpsrv/pgsql_connection.h b/src/lib/dhcpsrv/pgsql_connection.h index 6f307a7153..659c131942 100644 --- a/src/lib/dhcpsrv/pgsql_connection.h +++ b/src/lib/dhcpsrv/pgsql_connection.h @@ -89,6 +89,10 @@ public: /// Store the pointer to the result set to being fetched. Set row /// and column counts for convenience. /// + /// @param result - pointer to the Postgresql client layer result + /// If the value of is NULL, row and col values will be set to -1. + /// This allows PgSqlResult to be passed into statement error + /// checking. PgSqlResult(PGresult *result); /// @brief Destructor @@ -378,22 +382,23 @@ public: /// execution succeeded, and in the event of failures, decide whether or /// not the failures are recoverable. /// - /// If the error is recoverable, the method will throw a DbOperationError. - /// In the error is deemed unrecoverable, such as a loss of connectivity - /// with the server, this method will log the error and call exit(-1); + /// If the error is recoverable, the function will throw a DbOperationError. + /// If the error is deemed unrecoverable, such as a loss of connectivity + /// with the server, the function will call invokeDbLostCallback(). If the + /// invocation returns false then either there is no callback registered + /// or the callback has elected not to attempt to reconnect, and exit(-1) + /// is called; /// - /// @todo Calling exit() is viewed as a short term solution for Kea 1.0. - /// Two tickets are likely to alter this behavior, first is #3639, which - /// calls for the ability to attempt to reconnect to the database. The - /// second ticket, #4087 which calls for the implementation of a generic, - /// FatalException class which will propagate outward. + /// If the invocation returns true, this indicates the calling layer will + /// attempt recovery, and the function throws a DbOperationError to allow + /// the caller to error handle the failed db access attempt. /// /// @param r result of the last PostgreSQL operation /// @param statement - tagged statement that was executed /// /// @throw isc::dhcp::DbOperationError Detailed PostgreSQL failure void checkStatementError(const PgSqlResult& r, - PgSqlTaggedStatement& statement) const; + PgSqlTaggedStatement& statement) const; /// @brief PgSql connection handle /// diff --git a/src/lib/dhcpsrv/pgsql_lease_mgr.cc b/src/lib/dhcpsrv/pgsql_lease_mgr.cc index 34572f73f6..dba13520cf 100644 --- a/src/lib/dhcpsrv/pgsql_lease_mgr.cc +++ b/src/lib/dhcpsrv/pgsql_lease_mgr.cc @@ -139,6 +139,7 @@ PgSqlTaggedStatement tagged_statements[] = { "SELECT address, duid, valid_lifetime, " "extract(epoch from expire)::bigint, subnet_id, pref_lifetime, " "lease_type, iaid, prefix_len, fqdn_fwd, fqdn_rev, hostname, " + "hwaddr, hwtype, hwaddr_source, " "state " "FROM lease6"}, @@ -182,6 +183,7 @@ PgSqlTaggedStatement tagged_statements[] = { "SELECT address, duid, valid_lifetime, " "extract(epoch from expire)::bigint, subnet_id, pref_lifetime, " "lease_type, iaid, prefix_len, fqdn_fwd, fqdn_rev, hostname, " + "hwaddr, hwtype, hwaddr_source, " "state " "FROM lease6 " "WHERE subnet_id = $1"}, diff --git a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc index 242c8152b1..c60fb5e1ba 100644 --- a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -2800,6 +2801,66 @@ GenericLeaseMgrTest::testWipeLeases4() { EXPECT_EQ(0, lmptr_->wipeLeases4(333)); } +void +LeaseMgrDbLostCallbackTest::SetUp() { + destroySchema(); + createSchema(); + isc::dhcp::LeaseMgrFactory::destroy(); +} + +void +LeaseMgrDbLostCallbackTest::TearDown() { + destroySchema(); + isc::dhcp::LeaseMgrFactory::destroy(); +} + +void +LeaseMgrDbLostCallbackTest::testNoCallbackOnOpenFailure() { + DatabaseConnection::db_lost_callback = + boost::bind(&LeaseMgrDbLostCallbackTest::db_lost_callback, this, _1); + + callback_called_ = false; + ASSERT_THROW(LeaseMgrFactory::create(invalidConnectString()), + DbOpenError); + + EXPECT_FALSE(callback_called_); +} + +void +LeaseMgrDbLostCallbackTest::testDbLostCallback() { + // We should not find a socket open + int sql_socket = test::findLastSocketFd(); + ASSERT_EQ(-1, sql_socket); + + DatabaseConnection::db_lost_callback = + boost::bind(&LeaseMgrDbLostCallbackTest::db_lost_callback, this, _1); + + ASSERT_NO_THROW(LeaseMgrFactory::create(validConnectString())); + + // We should find a socket open and it should be for MySQL. + sql_socket = test::findLastSocketFd(); + ASSERT_TRUE(sql_socket > 0); + + callback_called_ = false; + + // Verify we can execute a query. + LeaseMgr& lm = LeaseMgrFactory::instance(); + pair version; + ASSERT_NO_THROW(version = lm.getVersion()); + + // Now close the sql socket out from under backend client + errno = 0; + + close(sql_socket); + ASSERT_EQ(0, errno) << "failed to close socket"; + + // A query should fail with DbOperationError. + ASSERT_THROW(version = lm.getVersion(), DbOperationError); + + // Our lost connectivity callback should have been invoked. + EXPECT_TRUE(callback_called_); +} + }; // namespace test }; // namespace dhcp }; // namespace isc diff --git a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h index aeee7067cd..5bbe6fe8a5 100644 --- a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h +++ b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h @@ -413,6 +413,63 @@ public: LeaseMgr* lmptr_; }; +class LeaseMgrDbLostCallbackTest : public ::testing::Test { +public: + /// @brief Prepares the class for a test. + /// + /// Invoked by gtest prior test entry, we create the + /// appropriate schema and wipe out any residual lease manager + virtual void SetUp(); + + /// @brief Pre-text exit clean up + /// + /// Invoked by gtest upon test exit, we destroy the schema + /// we created and toss our lease manager. + virtual void TearDown(); + + /// @brief Abstract method for destroying the back end specific shcema + virtual void destroySchema() = 0; + + /// @brief Abstract method for creating the back end specific shcema + virtual void createSchema() = 0; + + /// @brief Abstract method which returns the back end specific connection + /// string + virtual std::string validConnectString() = 0; + + /// @brief Abstract method which returns invalid back end specific connection + /// string + virtual std::string invalidConnectString() = 0; + + /// @brief Verifies open failures do NOT invoke db lost callback + /// + /// The db lost callback should only be invoked after succesfully + /// opening the DB and then subsequently losing it. Failing to + /// open should be handled directly by the application layer. + void testNoCallbackOnOpenFailure(); + + /// @brief Verifies the host manager's behavior if DB connection is lost + /// + /// This function creates a lease manager with an back end that + /// supports connectivity lost callback (currently only MySQL and + /// PostgreSQL currently). It verifies connectivity by issuing a known + /// valid query. Next it simulates connectivity lost by identifying and + /// closing the socket connection to the host backend. It then reissues + /// the query and verifies that: + /// -# The Query throws DbOperationError (rather than exiting) + /// -# The registered DbLostCallback was invoked + void testDbLostCallback(); + + /// @brief Callback function registered with the host manager + bool db_lost_callback(ReconnectCtlPtr /* not_used */) { + return (callback_called_ = true); + } + + /// @brief Flag used to detect calls to db_lost_callback function + bool callback_called_; + +}; + }; // namespace test }; // namespace dhcp }; // namespace isc diff --git a/src/lib/dhcpsrv/tests/host_mgr_unittest.cc b/src/lib/dhcpsrv/tests/host_mgr_unittest.cc index 8e0c28e9e8..6f6c95fc0d 100644 --- a/src/lib/dhcpsrv/tests/host_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/host_mgr_unittest.cc @@ -11,6 +11,7 @@ #include #include #include +#include #if defined HAVE_MYSQL #include @@ -326,7 +327,7 @@ HostMgrTest::testGet4Any() { SubnetID(0), IOAddress("192.0.2.5"))); // Abuse of the server's configuration. getCfgHosts()->add(new_host); - + CfgMgr::instance().commit(); // Retrieve the host from the database and expect that the parameters match. @@ -537,6 +538,100 @@ TEST_F(HostMgrTest, addNoDataSource) { EXPECT_THROW(HostMgr::instance().add(host), NoHostDataSourceManager); } +class HostMgrDbLostCallbackTest : public ::testing::Test { +public: + HostMgrDbLostCallbackTest() : callback_called_(false) {}; + + /// @brief Prepares the class for a test. + /// + /// Invoked by gtest prior test entry, we create the + /// appropriate schema and create a basic host manager to + /// wipe out any prior instance + virtual void SetUp() { + destroySchema(); + createSchema(); + // Wipe out any pre-existing mgr + HostMgr::create(); + } + + /// @brief Pre-text exit clean up + /// + /// Invoked by gtest upon test exit, we destroy the schema + /// we created. + virtual void TearDown() { + destroySchema(); + } + + /// @brief Abstract method for destroying the back end specific shcema + virtual void destroySchema() = 0; + + /// @brief Abstract method for creating the back end specific shcema + virtual void createSchema() = 0; + + /// @brief Abstract method which returns the back end specific connection + /// string + virtual std::string validConnectString() = 0; + + /// @brief Verifies the host manager's behavior if DB connection is lost + /// + /// This function creates a host manager with an alternate data source + /// that supports connectivity lost callback (currently only MySQL and + /// PostgreSQL currently). It verifies connectivity by issuing a known + /// valid query. Next it simulates connectivity lost by identifying and + /// closing the socket connection to the host backend. It then reissues + /// the query and verifies that: + /// -# The Query throws DbOperationError (rather than exiting) + /// -# The registered DbLostCallback was invoked + void testDbLostCallback(); + + /// @brief Callback function registered with the host manager + bool db_lost_callback(ReconnectCtlPtr /* not_used */) { + return (callback_called_ = true); + } + + /// @brief Flag used to detect calls to db_lost_callback function + bool callback_called_; +}; + + +void +HostMgrDbLostCallbackTest::testDbLostCallback() { + + // We should not find a socket open + int sql_socket = test::findLastSocketFd(); + ASSERT_EQ(-1, sql_socket); + + HostMgr::create(); + + DatabaseConnection::db_lost_callback = + boost::bind(&HostMgrDbLostCallbackTest::db_lost_callback, this, _1); + + ASSERT_NO_THROW(HostMgr::addBackend(validConnectString())); + + // We should find a socket open and it should be for MySQL. + sql_socket = test::findLastSocketFd(); + ASSERT_TRUE(sql_socket > 0); + + callback_called_ = false; + + // Verify we can execute a query. We don't care about the answer. + ConstHostCollection hosts; + ASSERT_NO_THROW(hosts = HostMgr::instance().getAll4(IOAddress("192.0.2.5"))); + + // Now close the sql socket out from under backend client + errno = 0; + + close(sql_socket); + ASSERT_EQ(0, errno) << "failed to close socket"; + + // A query should fail with DbOperationError. + ASSERT_THROW(hosts = HostMgr::instance().getAll4(IOAddress("192.0.2.5")), + DbOperationError); + + // Our lost connectivity callback should have been invoked. + ASSERT_EQ(true, callback_called_); +} + // The following tests require MySQL enabled. #if defined HAVE_MYSQL @@ -580,6 +675,23 @@ MySQLHostMgrTest::TearDown() { test::destroyMySQLSchema(); } +/// @brief Test fixture class for validating @c HostMgr using +/// MySQL as alternate host data source and MySQL connectivity loss. +class MySQLHostMgrDbLostCallbackTest : public HostMgrDbLostCallbackTest { +public: + virtual void destroySchema() { + test::destroyMySQLSchema(); + } + + virtual void createSchema() { + test::createMySQLSchema(); + } + + virtual std::string validConnectString() { + return (test::validMySQLConnectionString()); + } +}; + // This test verifies that reservations for a particular client can // be retrieved from the configuration file and a database simultaneously. TEST_F(MySQLHostMgrTest, getAll) { @@ -610,6 +722,10 @@ TEST_F(MySQLHostMgrTest, get6ByPrefix) { testGet6ByPrefix(*getCfgHosts(), HostMgr::instance()); } +// Verifies that loss of connectivity to MySQL is handled correctly. +TEST_F(MySQLHostMgrDbLostCallbackTest, testDbLostCallback) { + testDbLostCallback(); +} #endif @@ -656,7 +772,23 @@ PostgreSQLHostMgrTest::TearDown() { test::destroyPgSQLSchema(); } -// This test verifies that reservations for a particular client can +/// @brief Test fixture class for validating @c HostMgr using +/// PostgreSQL as alternate host data source and PostgreSQL connectivity loss. +class PostgreSQLHostMgrDbLostCallbackTest : public HostMgrDbLostCallbackTest { +public: + virtual void destroySchema() { + test::destroyPgSQLSchema(); + } + + virtual void createSchema() { + test::createPgSQLSchema(); + } + + virtual std::string validConnectString() { + return (test::validPgSQLConnectionString()); + } +}; + // be retrieved from the configuration file and a database simultaneously. TEST_F(PostgreSQLHostMgrTest, getAll) { testGetAll(*getCfgHosts(), HostMgr::instance()); @@ -686,9 +818,12 @@ TEST_F(PostgreSQLHostMgrTest, get6ByPrefix) { testGet6ByPrefix(*getCfgHosts(), HostMgr::instance()); } +// Verifies that loss of connectivity to PostgreSQL is handled correctly. +TEST_F(PostgreSQLHostMgrDbLostCallbackTest, testDbLostCallback) { + testDbLostCallback(); +} #endif - // The following tests require Cassandra enabled. #if defined HAVE_CQL diff --git a/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc index c6c12381cc..a92791ed51 100644 --- a/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc @@ -536,4 +536,36 @@ TEST_F(MySqlLeaseMgrTest, DISABLED_wipeLeases6) { testWipeLeases6(); } +/// @brief Test fixture class for validating @c LeaseMgr using +/// MySQL as back end and MySQL connectivity loss. +class MySQLLeaseMgrDbLostCallbackTest : public LeaseMgrDbLostCallbackTest { +public: + virtual void destroySchema() { + test::destroyMySQLSchema(); + } + + virtual void createSchema() { + test::createMySQLSchema(); + } + + virtual std::string validConnectString() { + return (test::validMySQLConnectionString()); + } + + virtual std::string invalidConnectString() { + return (connectionString(MYSQL_VALID_TYPE, VALID_NAME, INVALID_HOST, + VALID_USER, VALID_PASSWORD)); + } +}; + +// Verifies that db lost callback is not invoked on an open failure +TEST_F(MySQLLeaseMgrDbLostCallbackTest, testNoCallbackOnOpenFailure) { + testDbLostCallback(); +} + +// Verifies that loss of connectivity to MySQL is handled correctly. +TEST_F(MySQLLeaseMgrDbLostCallbackTest, testDbLostCallback) { + testDbLostCallback(); +} + } // namespace diff --git a/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc index 49586587bf..d4b1e0df50 100644 --- a/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc @@ -194,35 +194,36 @@ TEST(PgSqlOpenTest, OpenDatabase) { destroyPgSQLSchema(true); } -/// @brief Flag used to detect calls to db_lost_callback function -bool callback_called = false; +/// @brief Test fixture class for validating @c LeaseMgr using +/// PostgreSQL as back end and PostgreSQL connectivity loss. +class PgSqlLeaseMgrDbLostCallbackTest : public LeaseMgrDbLostCallbackTest { +public: + virtual void destroySchema() { + test::destroyPgSQLSchema(); + } -/// @brief Callback function used in open database testing -bool db_lost_callback(ReconnectCtlPtr /* db_conn_retry */) { - return (callback_called = true); + virtual void createSchema() { + test::createPgSQLSchema(); + } + + virtual std::string validConnectString() { + return (test::validPgSQLConnectionString()); + } + + virtual std::string invalidConnectString() { + return (connectionString(PGSQL_VALID_TYPE, VALID_NAME, INVALID_HOST, + VALID_USER, VALID_PASSWORD)); + } +}; + +// Verifies that db lost callback is not invoked on an open failure +TEST_F(PgSqlLeaseMgrDbLostCallbackTest, testNoCallbackOnOpenFailure) { + testDbLostCallback(); } -/// @brief Make sure open failures do NOT invoke db lost callback -/// The db lost callback should only be invoked after succesfully -/// opening the DB and then subsequently losing it. Failing to -/// open should be handled directly by the application layer. -/// There is simply no good way to break the connection in a -/// unit test environment. So testing the callback invocation -/// in a unit test is next to impossible. That has to be done -/// as a system test. -TEST(PgSqlOpenTest, NoCallbackOnOpenFail) { - // Schema needs to be created for the test to work. - destroyPgSQLSchema(); - createPgSQLSchema(); - - callback_called = false; - DatabaseConnection::db_lost_callback = db_lost_callback; - EXPECT_THROW(LeaseMgrFactory::create(connectionString( - PGSQL_VALID_TYPE, VALID_NAME, INVALID_HOST, VALID_USER, VALID_PASSWORD)), - DbOpenError); - EXPECT_FALSE(callback_called); - - destroyPgSQLSchema(); +// Verifies that loss of connectivity to PostgreSQL is handled correctly. +TEST_F(PgSqlLeaseMgrDbLostCallbackTest, testDbLostCallback) { + testDbLostCallback(); } /// @brief Check the getType() method diff --git a/src/lib/dhcpsrv/tests/test_utils.cc b/src/lib/dhcpsrv/tests/test_utils.cc index eec3b29590..04714a05ea 100644 --- a/src/lib/dhcpsrv/tests/test_utils.cc +++ b/src/lib/dhcpsrv/tests/test_utils.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2018 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -9,6 +9,9 @@ #include #include #include +#include +#include +#include using namespace std; using namespace isc::asiolink; @@ -76,6 +79,29 @@ detailCompareLease(const Lease6Ptr& first, const Lease6Ptr& second) { EXPECT_EQ(first->hostname_, second->hostname_); } +int findLastSocketFd() { + int max_fd_number = getdtablesize(); + int last_socket = -1; + struct stat stats; + + // Iterate over the open fds + for (int fd = 0; fd <= max_fd_number; fd++ ) { + errno = 0; + fstat(fd, &stats); + + if (errno == EBADF ) { + break; + } + + // it's a socket, remember it + if (S_ISSOCK(stats.st_mode)) { + last_socket = fd; + } + } + + return (last_socket); +} + }; // namespace test }; // namespace dhcp }; // namespace isc diff --git a/src/lib/dhcpsrv/tests/test_utils.h b/src/lib/dhcpsrv/tests/test_utils.h index a5f46eeabc..febb90cd45 100644 --- a/src/lib/dhcpsrv/tests/test_utils.h +++ b/src/lib/dhcpsrv/tests/test_utils.h @@ -1,4 +1,4 @@ -// Copyright (C) 2012-2015 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2012-2018 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -34,6 +34,21 @@ detailCompareLease(const Lease6Ptr& first, const Lease6Ptr& second); void detailCompareLease(const Lease4Ptr& first, const Lease4Ptr& second); +/// @brief Function that finds the last open socket fd +/// +/// This function is used to attempt lost connectivity +/// with backends, notably MySQL and Postgresql. +/// +/// The theory being, that in a confined test environment +/// the last such fd is the SQL client socket fd. This allows +/// us to the close that fd and simulate a loss of server +/// connectivity. +/// +/// @return the fd of the last open socket or -1 if there +/// are none. +int findLastSocketFd(); + + }; // namespace test }; // namespace dhcp }; // namespace isc From b188a382ee8e9a286a966bcfbc42644b2c820c72 Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Mon, 9 Apr 2018 15:35:52 +0200 Subject: [PATCH 2/5] [5556a] Updated guide copyright --- doc/guide/kea-guide.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/guide/kea-guide.xml b/doc/guide/kea-guide.xml index a8c02aaaf2..b6d77e8ad8 100644 --- a/doc/guide/kea-guide.xml +++ b/doc/guide/kea-guide.xml @@ -31,7 +31,7 @@ This is the reference guide for Kea version &keaversion;. - 2010-2017 + 2010-2018 Internet Systems Consortium, Inc. ("ISC") From 5cf5d9c90838082166f13deaa45acefd5bc79f81 Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Mon, 9 Apr 2018 15:38:11 +0200 Subject: [PATCH 3/5] [5556a] Postgresql -> PostgreSQL --- src/lib/dhcpsrv/dhcpsrv_messages.mes | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.mes b/src/lib/dhcpsrv/dhcpsrv_messages.mes index 132e51d47c..6f51007ef9 100644 --- a/src/lib/dhcpsrv/dhcpsrv_messages.mes +++ b/src/lib/dhcpsrv/dhcpsrv_messages.mes @@ -856,7 +856,7 @@ connection including database name and username needed to access it % DHCPSRV_PGSQL_DEALLOC_ERROR An error occurred deallocating SQL statements while closing the PostgreSQL lease database: %1 This is an error message issued when a DHCP server (either V4 or V6) experienced and error freeing database SQL resources as part of closing its connection to -the Postgresql database. The connection is closed as part of normal server +the PostgreSQL database. The connection is closed as part of normal server shutdown. This error is most likely a programmatic issue that is highly unlikely to occur or negatively impact server operation. @@ -877,11 +877,11 @@ The argument is the amount of time Kea waits after a reclaimed lease expires before considering its removal. % DHCPSRV_PGSQL_FATAL_ERROR Unrecoverable PostgreSQL error occurred: Statement: <%1>, reason: %2 (error code: %3). -An error message indicating that communication with the Postgresql database server +An error message indicating that communication with the PostgreSQL database server has been lost. If automatic recovery has been enabled, then the server will attempt to recover the connectivity. If not the server wil exit with a non-zero exit code. The cause of such an error is most likely a network issue -or the Postgresql server has gone down. +or the PostgreSQL server has gone down. % DHCPSRV_PGSQL_GET4 obtaining all IPv4 leases A debug message issued when the server is attempting to obtain all IPv4 From 76ddd1ffdd5ee858b3dfdad3e58fed8e3e105b26 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Mon, 9 Apr 2018 14:42:42 -0400 Subject: [PATCH 4/5] [5556a] Addressed review comments src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc LeaseMgrDbLostCallbackTest::testNoCallbackOnOpenFailure() { - removed before hand checks for socket descriptor, grab the most recently opend descriptor after connecting to the db and assume its the SQL client's - test return value of close() rather then errno src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h LeaseMgrDbLostCallbackTest - constructor and destructor clear DatabaseConnection::db_lost_callback src/lib/dhcpsrv/tests/host_mgr_unittest.cc HostMgrDbLostCallbackTest - Setup and Teardown methods clear DatabaseConnection::db_lost_callback HostMgrDbLostCallbackTest::testDbLostCallback() - removed before hand checks for socket descriptor, grab the most recently opend descriptor after connecting to the db and assume its the SQL client's - test return value of close() rather then errno src/lib/dhcpsrv/tests/test_utils.cc int findLastSocketFd() - iterate over all descriptors rather than stopping at first invalid --- .../tests/generic_lease_mgr_unittest.cc | 18 +++++------- .../tests/generic_lease_mgr_unittest.h | 8 ++++++ src/lib/dhcpsrv/tests/host_mgr_unittest.cc | 28 ++++++++++--------- src/lib/dhcpsrv/tests/test_utils.cc | 3 +- 4 files changed, 32 insertions(+), 25 deletions(-) diff --git a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc index c60fb5e1ba..f0a9f0b306 100644 --- a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc @@ -2828,19 +2828,18 @@ LeaseMgrDbLostCallbackTest::testNoCallbackOnOpenFailure() { void LeaseMgrDbLostCallbackTest::testDbLostCallback() { - // We should not find a socket open - int sql_socket = test::findLastSocketFd(); - ASSERT_EQ(-1, sql_socket); - + // Set the connectivity lost callback. DatabaseConnection::db_lost_callback = boost::bind(&LeaseMgrDbLostCallbackTest::db_lost_callback, this, _1); + // Connect to the lease backend. ASSERT_NO_THROW(LeaseMgrFactory::create(validConnectString())); - // We should find a socket open and it should be for MySQL. - sql_socket = test::findLastSocketFd(); - ASSERT_TRUE(sql_socket > 0); + // The most recently opened socket should be for our SQL client. + int sql_socket = test::findLastSocketFd(); + ASSERT_TRUE(sql_socket > -1); + // Clear the callback invocation marker. callback_called_ = false; // Verify we can execute a query. @@ -2849,10 +2848,7 @@ LeaseMgrDbLostCallbackTest::testDbLostCallback() { ASSERT_NO_THROW(version = lm.getVersion()); // Now close the sql socket out from under backend client - errno = 0; - - close(sql_socket); - ASSERT_EQ(0, errno) << "failed to close socket"; + ASSERT_EQ(0, close(sql_socket)); // A query should fail with DbOperationError. ASSERT_THROW(version = lm.getVersion(), DbOperationError); diff --git a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h index 5bbe6fe8a5..cd62825eb3 100644 --- a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h +++ b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.h @@ -415,6 +415,14 @@ public: class LeaseMgrDbLostCallbackTest : public ::testing::Test { public: + LeaseMgrDbLostCallbackTest() { + DatabaseConnection::db_lost_callback = 0; + } + + virtual ~LeaseMgrDbLostCallbackTest() { + DatabaseConnection::db_lost_callback = 0; + } + /// @brief Prepares the class for a test. /// /// Invoked by gtest prior test entry, we create the diff --git a/src/lib/dhcpsrv/tests/host_mgr_unittest.cc b/src/lib/dhcpsrv/tests/host_mgr_unittest.cc index 6f6c95fc0d..fccec64651 100644 --- a/src/lib/dhcpsrv/tests/host_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/host_mgr_unittest.cc @@ -548,6 +548,7 @@ public: /// appropriate schema and create a basic host manager to /// wipe out any prior instance virtual void SetUp() { + DatabaseConnection::db_lost_callback = 0; destroySchema(); createSchema(); // Wipe out any pre-existing mgr @@ -559,6 +560,7 @@ public: /// Invoked by gtest upon test exit, we destroy the schema /// we created. virtual void TearDown() { + DatabaseConnection::db_lost_callback = 0; destroySchema(); } @@ -596,22 +598,25 @@ public: void HostMgrDbLostCallbackTest::testDbLostCallback() { - - // We should not find a socket open - int sql_socket = test::findLastSocketFd(); - ASSERT_EQ(-1, sql_socket); - + // Create the HostMgr. HostMgr::create(); + // Set the connectivity lost callback. DatabaseConnection::db_lost_callback = boost::bind(&HostMgrDbLostCallbackTest::db_lost_callback, this, _1); + // Find the most recently opened socket. Our SQL client's socket should + // be the next one. + int last_open_socket = test::findLastSocketFd(); + + // Connect to the host backend. ASSERT_NO_THROW(HostMgr::addBackend(validConnectString())); - // We should find a socket open and it should be for MySQL. - sql_socket = test::findLastSocketFd(); - ASSERT_TRUE(sql_socket > 0); + // Find the SQL client socket. + int sql_socket = test::findLastSocketFd(); + ASSERT_TRUE(sql_socket > last_open_socket); + // Clear the callback invocation marker. callback_called_ = false; // Verify we can execute a query. We don't care about the answer. @@ -619,17 +624,14 @@ HostMgrDbLostCallbackTest::testDbLostCallback() { ASSERT_NO_THROW(hosts = HostMgr::instance().getAll4(IOAddress("192.0.2.5"))); // Now close the sql socket out from under backend client - errno = 0; - - close(sql_socket); - ASSERT_EQ(0, errno) << "failed to close socket"; + ASSERT_FALSE(close(sql_socket)) << "failed to close socket"; // A query should fail with DbOperationError. ASSERT_THROW(hosts = HostMgr::instance().getAll4(IOAddress("192.0.2.5")), DbOperationError); // Our lost connectivity callback should have been invoked. - ASSERT_EQ(true, callback_called_); + EXPECT_TRUE(callback_called_); } // The following tests require MySQL enabled. diff --git a/src/lib/dhcpsrv/tests/test_utils.cc b/src/lib/dhcpsrv/tests/test_utils.cc index 04714a05ea..0d7463280d 100644 --- a/src/lib/dhcpsrv/tests/test_utils.cc +++ b/src/lib/dhcpsrv/tests/test_utils.cc @@ -90,7 +90,8 @@ int findLastSocketFd() { fstat(fd, &stats); if (errno == EBADF ) { - break; + // Skip any that aren't open + continue; } // it's a socket, remember it From 50b0a07068b931ffd863fe550f6a08288f22906c Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Mon, 9 Apr 2018 14:56:22 -0400 Subject: [PATCH 5/5] [5556a] Commentary changes --- src/lib/dhcpsrv/tests/test_utils.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/lib/dhcpsrv/tests/test_utils.h b/src/lib/dhcpsrv/tests/test_utils.h index febb90cd45..913224c0d2 100644 --- a/src/lib/dhcpsrv/tests/test_utils.h +++ b/src/lib/dhcpsrv/tests/test_utils.h @@ -34,17 +34,17 @@ detailCompareLease(const Lease6Ptr& first, const Lease6Ptr& second); void detailCompareLease(const Lease4Ptr& first, const Lease4Ptr& second); -/// @brief Function that finds the last open socket fd +/// @brief Function that finds the last open socket descriptor /// /// This function is used to attempt lost connectivity /// with backends, notably MySQL and Postgresql. /// -/// The theory being, that in a confined test environment -/// the last such fd is the SQL client socket fd. This allows -/// us to the close that fd and simulate a loss of server +/// The theory being, that in a confined test environment the last +/// such descriptor is the SQL client socket descriptor. This allows +/// us to the close that descriptor and simulate a loss of server /// connectivity. /// -/// @return the fd of the last open socket or -1 if there +/// @return the descriptor of the last open socket or -1 if there /// are none. int findLastSocketFd();