From: Thomas Markwalder Date: Fri, 6 Apr 2018 19:16:24 +0000 (-0400) Subject: [5556a] MySQL lease and host backends now support configurable auto-reconnect X-Git-Tag: trac5458a_base~17^2~4 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f09bf17e2e4a39e3afafba5cfc3e91851ce3016b;p=thirdparty%2Fkea.git [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 --- 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 Callback function used in open database testing -bool db_lost_callback(ReconnectCtlPtr /* db_conn_retry */) { - return (callback_called = true); -} - -/// @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(); +/// @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(); + } - 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); + 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(); +} - 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