From: Thomas Markwalder Date: Wed, 5 Aug 2020 20:32:01 +0000 (-0400) Subject: [#1396] Implement CB connection recovery X-Git-Tag: Kea-1.8.0~99 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c0f0e34bc59c2fe8ba0626055a4de88fe72fc962;p=thirdparty%2Fkea.git [#1396] Implement CB connection recovery src/bin/dhcp4/ctrl_dhcp4_srv.cc ControlledDhcpv4Srv::dbReconnect() - added logic to reopen CB backends (if any configured) src/lib/database/database_connection.* Added DbConnectionUnusable exception DatabaseConnection - added: unusable_ marUnusable() checkUnusable() src/lib/mysql/mysql_connection.* MySqlConnection::checkError() - calls markUnusable() Added invocations of DatabaseConnection::checkUnusable() to functions using the connection src/lib/pgsql/pgsql_connection.cc PgSqlConnection::checkStatementError() - calls markUnusable() --- diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc index b1c20aa577..16e48d8705 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc +++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc @@ -1136,10 +1136,17 @@ void ControlledDhcpv4Srv::dbReconnect(ReconnectCtlPtr db_reconnect_ctl) { bool reopened = false; - // Re-open lease and host database with new parameters. + // We lost at least one of them, Reopen all of them (lease, host, and CB databases) try { CfgDbAccessPtr cfg_db = CfgMgr::instance().getCurrentCfg()->getCfgDbAccess(); cfg_db->createManagers(); + + auto ctl_info = CfgMgr::instance().getCurrentCfg()->getConfigControlInfo(); + if (ctl_info) { + auto srv_cfg = CfgMgr::instance().getCurrentCfg(); + server_->getCBControl()->databaseConfigConnect(srv_cfg); + } + reopened = true; } catch (const std::exception& ex) { LOG_ERROR(dhcp4_logger, DHCP4_DB_RECONNECT_ATTEMPT_FAILED).arg(ex.what()); @@ -1221,11 +1228,9 @@ ControlledDhcpv4Srv::cbFetchUpdates(const SrvConfigPtr& srv_cfg, server_->getCBControl()->databaseConfigFetch(srv_cfg, CBControlDHCPv4::FetchMode::FETCH_UPDATE); (*failure_count) = 0; - } catch (const std::exception& ex) { LOG_ERROR(dhcp4_logger, DHCP4_CB_PERIODIC_FETCH_UPDATES_FAIL) .arg(ex.what()); - // We allow at most 10 consecutive failures after which we stop // making further attempts to fetch the configuration updates. // Let's return without re-scheduling the timer. diff --git a/src/lib/database/database_connection.h b/src/lib/database/database_connection.h index a4164f3341..773554d716 100644 --- a/src/lib/database/database_connection.h +++ b/src/lib/database/database_connection.h @@ -47,6 +47,15 @@ public: isc::Exception(file, line, what) {} }; +/// @brief Exception thrown when a specific connection has been rendered unusable +/// either through loss of connectivity or API lib error +class DbConnectionUnusable: public Exception { +public: + DbConnectionUnusable(const char* file, size_t line, const char* what) : + isc::Exception(file, line, what) {} +}; + + /// @brief Invalid type exception /// /// Thrown when the factory doesn't recognize the type of the backend. @@ -164,7 +173,7 @@ public: /// @param parameters A data structure relating keywords and values /// concerned with the database. DatabaseConnection(const ParameterMap& parameters) - :parameters_(parameters) { + :parameters_(parameters), unusable_(false) { } /// @brief Destructor @@ -240,6 +249,18 @@ public: /// open connection subsequently fails static DbLostCallback db_lost_callback; + /// @brief Throws an exception if the connection is not usable. + void checkUnusable() { + if (unusable_) { + isc_throw (DbConnectionUnusable, "Attempt to use an invalid connection"); + } + } + +protected: + /// @brief Sets the usable flag to the given value. + /// @param usable new value for the flag. + void markUnusable() { unusable_ = true; } + private: /// @brief List of parameters passed in dbconfig @@ -249,6 +270,13 @@ private: /// intended to keep any DHCP-related parameters. ParameterMap parameters_; + /// @brief Indicates if the connection can no longer be used for normal + /// operations. Typically a connection is marked unusable after an unrecoverable + /// DB error. There may be a time during which the connection exists after + /// such an every during which it cannot be used for anything beyond checking + /// parameters and error information. This flag can be used as a guard in + /// code to prevent inadvertant use of a broken connection. + bool unusable_; }; } // namespace db diff --git a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc index f8ce430e44..d5228e07b3 100644 --- a/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/generic_lease_mgr_unittest.cc @@ -3319,9 +3319,9 @@ LeaseMgrDbLostCallbackTest::testDbLostCallback() { // Now close the sql socket out from under backend client ASSERT_EQ(0, close(sql_socket)); - // A query should fail with DbOperationError. + // A query should fail with DbConnectionUnusable. ASSERT_THROW(lease = lm.getLease4(IOAddress("192.0.1.0")), - DbOperationError); + DbConnectionUnusable); // Our lost connectivity callback should have been invoked. EXPECT_TRUE(callback_called_); diff --git a/src/lib/dhcpsrv/tests/host_mgr_unittest.cc b/src/lib/dhcpsrv/tests/host_mgr_unittest.cc index c14de8a73d..6798af604f 100644 --- a/src/lib/dhcpsrv/tests/host_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/host_mgr_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2014-2019 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2014-2020 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 @@ -1109,7 +1109,7 @@ public: /// 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 Query throws DbConnectionUnusable (rather than exiting) /// -# The registered DbLostCallback was invoked void testDbLostCallback(); #endif @@ -1154,9 +1154,9 @@ HostMgrDbLostCallbackTest::testDbLostCallback() { // Now close the sql socket out from under backend client ASSERT_FALSE(close(sql_socket)) << "failed to close socket"; - // A query should fail with DbOperationError. + // A query should fail with DbConnectionUnusable. ASSERT_THROW(hosts = HostMgr::instance().getAll4(IOAddress("192.0.2.5")), - DbOperationError); + DbConnectionUnusable); // Our lost connectivity callback should have been invoked. EXPECT_TRUE(callback_called_); diff --git a/src/lib/mysql/mysql_connection.cc b/src/lib/mysql/mysql_connection.cc index 5e559e36ba..b8577c97da 100644 --- a/src/lib/mysql/mysql_connection.cc +++ b/src/lib/mysql/mysql_connection.cc @@ -299,6 +299,7 @@ MySqlConnection::getVersion(const ParameterMap& parameters) { void MySqlConnection::prepareStatement(uint32_t index, const char* text) { + checkUnusable(); // Validate that there is space for the statement in the statements array // and that nothing has been placed there before. if ((index >= statements_.size()) || (statements_[index] != NULL)) { @@ -325,6 +326,7 @@ MySqlConnection::prepareStatement(uint32_t index, const char* text) { void MySqlConnection::prepareStatements(const TaggedStatement* start_statement, const TaggedStatement* end_statement) { + checkUnusable(); // Created the MySQL prepared statements for each DML statement. for (const TaggedStatement* tagged_statement = start_statement; tagged_statement != end_statement; ++tagged_statement) { @@ -390,6 +392,7 @@ MySqlConnection::convertFromDatabaseTime(const MYSQL_TIME& expire, void MySqlConnection::startTransaction() { DB_LOG_DEBUG(DB_DBG_TRACE_DETAIL, MYSQL_START_TRANSACTION); + checkUnusable(); // We create prepared statements for all other queries, but MySQL // don't support prepared statements for START TRANSACTION. int status = mysql_query(mysql_, "START TRANSACTION"); @@ -402,6 +405,7 @@ MySqlConnection::startTransaction() { void MySqlConnection::commit() { DB_LOG_DEBUG(DB_DBG_TRACE_DETAIL, MYSQL_COMMIT); + checkUnusable(); if (mysql_commit(mysql_) != 0) { isc_throw(DbOperationError, "commit failed: " << mysql_error(mysql_)); @@ -411,6 +415,7 @@ MySqlConnection::commit() { void MySqlConnection::rollback() { DB_LOG_DEBUG(DB_DBG_TRACE_DETAIL, MYSQL_ROLLBACK); + checkUnusable(); if (mysql_rollback(mysql_) != 0) { isc_throw(DbOperationError, "rollback failed: " << mysql_error(mysql_)); diff --git a/src/lib/mysql/mysql_connection.h b/src/lib/mysql/mysql_connection.h index 6fbfabef7e..c9808b803d 100644 --- a/src/lib/mysql/mysql_connection.h +++ b/src/lib/mysql/mysql_connection.h @@ -399,6 +399,7 @@ public: const MySqlBindingCollection& in_bindings, MySqlBindingCollection& out_bindings, ConsumeResultFun process_result) { + checkUnusable(); // Extract native input bindings. std::vector in_bind_vec; for (MySqlBindingPtr in_binding : in_bindings) { @@ -476,6 +477,7 @@ public: template void insertQuery(const StatementIndex& index, const MySqlBindingCollection& in_bindings) { + checkUnusable(); std::vector in_bind_vec; for (MySqlBindingPtr in_binding : in_bindings) { in_bind_vec.push_back(in_binding->getMySqlBinding()); @@ -519,6 +521,7 @@ public: template uint64_t updateDeleteQuery(const StatementIndex& index, const MySqlBindingCollection& in_bindings) { + checkUnusable(); std::vector in_bind_vec; for (MySqlBindingPtr in_binding : in_bindings) { in_bind_vec.push_back(in_binding->getMySqlBinding()); @@ -603,7 +606,7 @@ public: /// failed. template void checkError(const int status, const StatementIndex& index, - const char* what) const { + const char* what) { if (status != 0) { switch(mysql_errno(mysql_)) { // These are the ones we consider fatal. Remember this method is @@ -622,6 +625,9 @@ public: .arg(mysql_error(mysql_)) .arg(mysql_errno(mysql_)); + // Mark the connection as no longer usuable. + markUnusable(); + // If there's no lost db callback or it returns false, // then we're not attempting to recover so we're done. if (!invokeDbLostCallback()) { @@ -631,7 +637,7 @@ public: // We still need to throw so caller can error out of the current // processing. - isc_throw(db::DbOperationError, + isc_throw(db::DbConnectionUnusable, "fatal database errror or connectivity lost"); default: // Connection is ok, so it must be an SQL error diff --git a/src/lib/pgsql/pgsql_connection.cc b/src/lib/pgsql/pgsql_connection.cc index 0f22511db1..7af6be2313 100644 --- a/src/lib/pgsql/pgsql_connection.cc +++ b/src/lib/pgsql/pgsql_connection.cc @@ -335,6 +335,9 @@ PgSqlConnection::checkStatementError(const PgSqlResult& r, .arg(PQerrorMessage(conn_)) .arg(sqlstate ? sqlstate : ""); + // Mark this connection as no longer usable. + markUnusable(); + // If there's no lost db callback or it returns false, // then we're not attempting to recover so we're done. if (!invokeDbLostCallback()) { @@ -344,7 +347,7 @@ PgSqlConnection::checkStatementError(const PgSqlResult& r, // We still need to throw so caller can error out of the current // processing. - isc_throw(DbOperationError, + isc_throw(DbConnectionUnusable, "fatal database error or connectivity lost"); }