From: Razvan Becheriu Date: Thu, 18 Jun 2020 17:04:04 +0000 (+0300) Subject: [#1277] query filter is not kea thread safe X-Git-Tag: Kea-1.7.9~31 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=6aa04ffc81c7558184594776ff8d35ded1cd0f46;p=thirdparty%2Fkea.git [#1277] query filter is not kea thread safe --- diff --git a/src/hooks/dhcp/high_availability/query_filter.cc b/src/hooks/dhcp/high_availability/query_filter.cc index e438b9e1e0..dd9c468963 100644 --- a/src/hooks/dhcp/high_availability/query_filter.cc +++ b/src/hooks/dhcp/high_availability/query_filter.cc @@ -12,12 +12,15 @@ #include #include #include +#include + #include #include #include using namespace isc::dhcp; using namespace isc::log; +using namespace isc::util; namespace { @@ -50,7 +53,8 @@ namespace isc { namespace ha { QueryFilter::QueryFilter(const HAConfigPtr& config) - : config_(config), peers_(), scopes_(), active_servers_(0) { + : config_(config), peers_(), scopes_(), active_servers_(0), + mutex_(new std::mutex) { // Make sure that the configuration is valid. We make certain // assumptions about the availability of the servers' configurations @@ -101,26 +105,56 @@ QueryFilter::QueryFilter(const HAConfigPtr& config) void QueryFilter::serveScope(const std::string& scope_name) { + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(*mutex_); + serveScopeInternal(scope_name); + } else { + serveScopeInternal(scope_name); + } +} + +void +QueryFilter::serveScopeInternal(const std::string& scope_name) { validateScopeName(scope_name); scopes_[scope_name] = true; } void QueryFilter::serveScopeOnly(const std::string& scope_name) { + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(*mutex_); + serveScopeOnlyInternal(scope_name); + } else { + serveScopeOnlyInternal(scope_name); + } +} + +void +QueryFilter::serveScopeOnlyInternal(const std::string& scope_name) { validateScopeName(scope_name); - serveNoScopes(); - serveScope(scope_name); + serveNoScopesInternal(); + serveScopeInternal(scope_name); } void QueryFilter::serveScopes(const std::vector& scopes) { + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(*mutex_); + serveScopesInternal(scopes); + } else { + serveScopesInternal(scopes); + } +} + +void +QueryFilter::serveScopesInternal(const std::vector& scopes) { // Remember currently enabled scopes in case we fail to process // the provided list of scopes. auto current_scopes = scopes_; try { - serveNoScopes(); + serveNoScopesInternal(); for (size_t i = 0; i < scopes.size(); ++i) { - serveScope(scopes[i]); + serveScopeInternal(scopes[i]); } } catch (...) { @@ -133,25 +167,45 @@ QueryFilter::serveScopes(const std::vector& scopes) { void QueryFilter::serveDefaultScopes() { + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(*mutex_); + serveDefaultScopesInternal(); + } else { + serveDefaultScopesInternal(); + } +} + +void +QueryFilter::serveDefaultScopesInternal() { // Get this server instance configuration. HAConfig::PeerConfigPtr my_config = config_->getThisServerConfig(); HAConfig::PeerConfig::Role my_role = my_config->getRole(); // Clear scopes. - serveNoScopes(); + serveNoScopesInternal(); // If I am primary or secondary, then I am only responsible for my own // scope. If I am standby, I am not responsible for any scope. if ((my_role == HAConfig::PeerConfig::PRIMARY) || (my_role == HAConfig::PeerConfig::SECONDARY)) { - serveScope(my_config->getName()); + serveScopeInternal(my_config->getName()); } } void QueryFilter::serveFailoverScopes() { + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(*mutex_); + serveFailoverScopesInternal(); + } else { + serveFailoverScopesInternal(); + } +} + +void +QueryFilter::serveFailoverScopesInternal() { // Clear scopes. - serveNoScopes(); + serveNoScopesInternal(); // Iterate over the roles of all servers to see which scope should // be enabled. @@ -164,13 +218,23 @@ QueryFilter::serveFailoverScopes() { // server. if (((*peer)->getRole() == HAConfig::PeerConfig::PRIMARY) || ((*peer)->getRole() == HAConfig::PeerConfig::SECONDARY)) { - serveScope((*peer)->getName()); + serveScopeInternal((*peer)->getName()); } } } void QueryFilter::serveNoScopes() { + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(*mutex_); + serveNoScopesInternal(); + } else { + serveNoScopesInternal(); + } +} + +void +QueryFilter::serveNoScopesInternal() { scopes_.clear(); // Disable scope for each peer in the configuration. @@ -181,12 +245,32 @@ QueryFilter::serveNoScopes() { bool QueryFilter::amServingScope(const std::string& scope_name) const { + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(*mutex_); + return (amServingScopeInternal(scope_name)); + } else { + return (amServingScopeInternal(scope_name)); + } +} + +bool +QueryFilter::amServingScopeInternal(const std::string& scope_name) const { auto scope = scopes_.find(scope_name); return ((scope == scopes_.end()) || (scope->second)); } std::set QueryFilter::getServedScopes() const { + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(*mutex_); + return (getServedScopesInternal()); + } else { + return (getServedScopesInternal()); + } +} + +std::set +QueryFilter::getServedScopesInternal() const { std::set scope_set; for (auto scope : scopes_) { if (scope.second) { @@ -198,12 +282,22 @@ QueryFilter::getServedScopes() const { bool QueryFilter::inScope(const dhcp::Pkt4Ptr& query4, std::string& scope_class) const { - return (inScopeInternal(query4, scope_class)); + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(*mutex_); + return (inScopeInternal(query4, scope_class)); + } else { + return (inScopeInternal(query4, scope_class)); + } } bool QueryFilter::inScope(const dhcp::Pkt6Ptr& query6, std::string& scope_class) const { - return (inScopeInternal(query6, scope_class)); + if (MultiThreadingMgr::instance().getMode()) { + std::lock_guard lock(*mutex_); + return (inScopeInternal(query6, scope_class)); + } else { + return (inScopeInternal(query6, scope_class)); + } } template @@ -229,7 +323,7 @@ QueryFilter::inScopeInternal(const QueryPtrType& query, auto scope = peers_[candidate_server]->getName(); scope_class = makeScopeClass(scope); - return ((candidate_server >= 0) && amServingScope(scope)); + return ((candidate_server >= 0) && amServingScopeInternal(scope)); } int @@ -315,6 +409,5 @@ QueryFilter::makeScopeClass(const std::string& scope_name) const { return (std::string("HA_") + scope_name); } - } // end of namespace isc::ha } // end of namespace isc diff --git a/src/hooks/dhcp/high_availability/query_filter.h b/src/hooks/dhcp/high_availability/query_filter.h index 541677fab8..c913a24f02 100644 --- a/src/hooks/dhcp/high_availability/query_filter.h +++ b/src/hooks/dhcp/high_availability/query_filter.h @@ -10,11 +10,15 @@ #include #include #include + +#include + #include #include -#include +#include #include #include +#include namespace isc { namespace ha { @@ -165,10 +169,82 @@ public: bool inScope(const dhcp::Pkt6Ptr& query6, std::string& scope_class) const; private: + /// @brief Enable scope. + /// + /// Should be called in a thread safe context. + /// + /// Starts serving queries from the specified scope. It doesn't affect + /// other scopes. + /// + /// @param scope_name name of the scope/server to be enabled. + /// @throw BadValue if scope name doesn't match any of the server names. + void serveScopeInternal(const std::string& scope_name); + + /// @brief Enable scope and disable all other scopes. + /// + /// Should be called in a thread safe context. + /// + /// Starts serving queries from the specified scope. Disable all other + /// scopes. + /// + /// @param scope_name name of the scope/server to be enabled. + /// @throw BadValue if scope name doesn't match any of the server names. + void serveScopeOnlyInternal(const std::string& scope_name); + + /// @brief Enables selected scopes. + /// + /// Should be called in a thread safe context. + /// + /// All non listed scopes are disabled. + /// + /// @param scopes vector of scope names to be enabled. + void serveScopesInternal(const std::vector& scopes); + + /// @brief Serve default scopes for the given HA mode. + /// + /// Should be called in a thread safe context. + /// + /// If this server is primary or secondary (load balancing), the scope + /// of this server is enabled. All other scopes are disabled. + void serveDefaultScopesInternal(); + + /// @brief Enable scopes required in failover case. + /// + /// Should be called in a thread safe context. + /// + /// In the load balancing case, the scopes of the primary and secondary + /// servers are enabled (this server will handle the entire traffic). + /// In the hot standby case, the primary server's scope is enabled + /// (this server will handle the entire traffic normally processed by + /// the primary server). + void serveFailoverScopesInternal(); + + /// @brief Disables all scopes. + /// + /// Should be called in a thread safe context. + void serveNoScopesInternal(); + + /// @brief Checks if this server instance is configured to process traffic + /// belonging to a particular scope. + /// + /// Should be called in a thread safe context. + /// + /// @param scope_name name of the scope/server. + /// @return true if the scope is enabled. + bool amServingScopeInternal(const std::string& scope_name) const; + + /// @brief Returns served scopes. + /// + /// Should be called in a thread safe context. + /// + /// This method is mostly useful for testing purposes. + std::set getServedScopesInternal() const; /// @brief Generic implementation of the @c inScope function for DHCPv4 /// and DHCPv6 queries. /// + /// Should be called in a thread safe context. + /// /// @tparam QueryPtrType type of the query, i.e. DHCPv4 or DHCPv6 query. /// @param query pointer to the DHCP query instance. /// @param [out] scope_class name of the class which corresponds to the @@ -258,6 +334,9 @@ protected: /// @brief Number of the active servers in the given HA mode. int active_servers_; + + /// @brief Mutex to protect the internal state. + boost::scoped_ptr mutex_; }; } // end of namespace isc::ha diff --git a/src/hooks/dhcp/high_availability/tests/query_filter_unittest.cc b/src/hooks/dhcp/high_availability/tests/query_filter_unittest.cc index 36e6203881..8ed6ef33b3 100644 --- a/src/hooks/dhcp/high_availability/tests/query_filter_unittest.cc +++ b/src/hooks/dhcp/high_availability/tests/query_filter_unittest.cc @@ -13,6 +13,8 @@ #include #include #include +#include + #include #include @@ -21,15 +23,86 @@ using namespace isc::data; using namespace isc::dhcp; using namespace isc::ha; using namespace isc::ha::test; +using namespace util; namespace { /// @brief Test fixture class for @c QueryFilter class. -using QueryFilterTest = HATest; +class QueryFilterTest : public HATest { +public: + /// @brief Constructor. + QueryFilterTest() { + MultiThreadingMgr::instance().setMode(false); + } -// This test verifies the case when load balancing is enabled and -// this server is primary. -TEST_F(QueryFilterTest, loadBalancingThisPrimary) { + /// @brief Destructor. + ~QueryFilterTest() { + MultiThreadingMgr::instance().setMode(false); + } + + /// @brief This test verifies that client identifier is used for load + /// balancing. + void loadBalancingClientIdThisPrimary(); + + /// @brief This test verifies the case when load balancing is enabled and + /// this server is primary. + void loadBalancingThisPrimary(); + + /// @brief This test verifies the case when load balancing is enabled and + /// this server is secondary. + void loadBalancingThisSecondary(); + + /// @brief This test verifies the case when load balancing is enabled and + /// this server is backup. + /// @todo Expand these tests once we implement the actual load balancing to + /// verify which packets are in scope. + void loadBalancingThisBackup(); + + /// @brief This test verifies the case when hot-standby is enabled and this + /// server is primary. + void hotStandbyThisPrimary(); + + /// @brief This test verifies the case when hot-standby is enabled and this + /// server is standby. + void hotStandbyThisSecondary(); + + /// @brief This test verifies the case when hot-standby is enabled and this + /// server is backup. + void hotStandbyThisBackup(); + + /// @brief This test verifies the case when load balancing is enabled and + /// this server is primary. + void loadBalancingThisPrimary6(); + + /// @brief This test verifies the case when load balancing is enabled and + /// this server is secondary. + void loadBalancingThisSecondary6(); + + /// @brief This test verifies the case when load balancing is enabled and + /// this server is backup. + /// @todo Expand these tests once we implement the actual load balancing to + /// verify which packets are in scope. + void loadBalancingThisBackup6(); + + /// @brief This test verifies the case when hot-standby is enabled and this + /// server is primary. + void hotStandbyThisPrimary6(); + + /// @brief This test verifies the case when hot-standby is enabled and this + /// server is standby. + void hotStandbyThisSecondary6(); + + /// @brief This test verifies the case when hot-standby is enabled and this + /// server is backup. + void hotStandbyThisBackup6(); + + /// @brief This test verifies that it is possible to explicitly enable and + /// disable certain scopes. + void explicitlyServeScopes(); +}; + +void +QueryFilterTest::loadBalancingThisPrimary() { HAConfigPtr config = createValidConfiguration(); QueryFilter filter(config); @@ -85,8 +158,8 @@ TEST_F(QueryFilterTest, loadBalancingThisPrimary) { EXPECT_FALSE(filter.inScope(query4, scope_class)); } -// This test verifies that client identifier is used for load balancing. -TEST_F(QueryFilterTest, loadBalancingClientIdThisPrimary) { +void +QueryFilterTest::loadBalancingClientIdThisPrimary() { HAConfigPtr config = createValidConfiguration(); QueryFilter filter(config); @@ -140,9 +213,8 @@ TEST_F(QueryFilterTest, loadBalancingClientIdThisPrimary) { } } -// This test verifies the case when load balancing is enabled and -// this server is secondary. -TEST_F(QueryFilterTest, loadBalancingThisSecondary) { +void +QueryFilterTest::loadBalancingThisSecondary() { HAConfigPtr config = createValidConfiguration(); // We're now a secondary server. @@ -196,11 +268,8 @@ TEST_F(QueryFilterTest, loadBalancingThisSecondary) { } } -// This test verifies the case when load balancing is enabled and -// this server is backup. -/// @todo Expand these tests once we implement the actual load balancing to -/// verify which packets are in scope. -TEST_F(QueryFilterTest, loadBalancingThisBackup) { +void +QueryFilterTest::loadBalancingThisBackup() { HAConfigPtr config = createValidConfiguration(); config->setThisServerName("server3"); @@ -241,9 +310,8 @@ TEST_F(QueryFilterTest, loadBalancingThisBackup) { } } -// This test verifies the case when hot-standby is enabled and this -// server is primary. -TEST_F(QueryFilterTest, hotStandbyThisPrimary) { +void +QueryFilterTest::hotStandbyThisPrimary() { HAConfigPtr config = createValidConfiguration(); config->setHAMode("hot-standby"); @@ -278,9 +346,8 @@ TEST_F(QueryFilterTest, hotStandbyThisPrimary) { EXPECT_NE(scope_class, "HA_server2"); } -// This test verifies the case when hot-standby is enabled and this -// server is standby. -TEST_F(QueryFilterTest, hotStandbyThisSecondary) { +void +QueryFilterTest::hotStandbyThisSecondary() { HAConfigPtr config = createValidConfiguration(); config->setHAMode("hot-standby"); @@ -318,9 +385,8 @@ TEST_F(QueryFilterTest, hotStandbyThisSecondary) { EXPECT_NE(scope_class, "HA_server2"); } -// This test verifies the case when hot-standby is enabled and this -// server is backup. -TEST_F(QueryFilterTest, hotStandbyThisBackup) { +void +QueryFilterTest::hotStandbyThisBackup() { HAConfigPtr config = createValidConfiguration(); config->setHAMode("hot-standby"); @@ -354,9 +420,8 @@ TEST_F(QueryFilterTest, hotStandbyThisBackup) { EXPECT_TRUE(filter.inScope(query4, scope_class)); } -// This test verifies the case when load balancing is enabled and -// this DHCPv6 server is primary. -TEST_F(QueryFilterTest, loadBalancingThisPrimary6) { +void +QueryFilterTest::loadBalancingThisPrimary6() { HAConfigPtr config = createValidConfiguration(); QueryFilter filter(config); @@ -411,9 +476,8 @@ TEST_F(QueryFilterTest, loadBalancingThisPrimary6) { EXPECT_FALSE(filter.inScope(query6, scope_class)); } -// This test verifies the case when load balancing is enabled and -// this server is secondary. -TEST_F(QueryFilterTest, loadBalancingThisSecondary6) { +void +QueryFilterTest::loadBalancingThisSecondary6() { HAConfigPtr config = createValidConfiguration(); // We're now a secondary server. @@ -467,11 +531,8 @@ TEST_F(QueryFilterTest, loadBalancingThisSecondary6) { } } -// This test verifies the case when load balancing is enabled and -// this server is backup. -/// @todo Expand these tests once we implement the actual load balancing to -/// verify which packets are in scope. -TEST_F(QueryFilterTest, loadBalancingThisBackup6) { +void +QueryFilterTest::loadBalancingThisBackup6() { HAConfigPtr config = createValidConfiguration(); config->setThisServerName("server3"); @@ -512,9 +573,8 @@ TEST_F(QueryFilterTest, loadBalancingThisBackup6) { } } -// This test verifies the case when hot-standby is enabled and this -// server is primary. -TEST_F(QueryFilterTest, hotStandbyThisPrimary6) { +void +QueryFilterTest::hotStandbyThisPrimary6() { HAConfigPtr config = createValidConfiguration(); config->setHAMode("hot-standby"); @@ -549,9 +609,8 @@ TEST_F(QueryFilterTest, hotStandbyThisPrimary6) { EXPECT_NE(scope_class, "HA_server2"); } -// This test verifies the case when hot-standby is enabled and this -// server is standby. -TEST_F(QueryFilterTest, hotStandbyThisSecondary6) { +void +QueryFilterTest::hotStandbyThisSecondary6() { HAConfigPtr config = createValidConfiguration(); config->setHAMode("hot-standby"); @@ -589,9 +648,43 @@ TEST_F(QueryFilterTest, hotStandbyThisSecondary6) { EXPECT_NE(scope_class, "HA_server2"); } -// This test verifies that it is possible to explicitly enable and -// disable certain scopes. -TEST_F(QueryFilterTest, explicitlyServeScopes) { +void +QueryFilterTest::hotStandbyThisBackup6() { + HAConfigPtr config = createValidConfiguration(); + + config->setHAMode("hot-standby"); + config->getPeerConfig("server2")->setRole("standby"); + config->setThisServerName("server3"); + + QueryFilter filter(config); + + Pkt6Ptr query6 = createQuery6(randomKey(10)); + + // By default the backup server doesn't process any traffic. + EXPECT_FALSE(filter.amServingScope("server1")); + EXPECT_FALSE(filter.amServingScope("server2")); + EXPECT_FALSE(filter.amServingScope("server3")); + + std::string scope_class; + + EXPECT_FALSE(filter.inScope(query6, scope_class)); + + // Simulate failover. Although, backup server never starts handling + // other server's traffic automatically, it can be manually instructed + // to do so. This simulates such scenario. + filter.serveFailoverScopes(); + + // The backup server now handles the entire traffic, i.e. the traffic + // that the primary server handles. + EXPECT_TRUE(filter.amServingScope("server1")); + EXPECT_FALSE(filter.amServingScope("server2")); + EXPECT_FALSE(filter.amServingScope("server3")); + + EXPECT_TRUE(filter.inScope(query6, scope_class)); +} + +void +QueryFilterTest::explicitlyServeScopes() { HAConfigPtr config = createValidConfiguration(); QueryFilter filter(config); @@ -638,4 +731,130 @@ TEST_F(QueryFilterTest, explicitlyServeScopes) { EXPECT_THROW(filter.serveScopes({ "server1", "unsupported" }), BadValue); } +TEST_F(QueryFilterTest, loadBalancingClientIdThisPrimary) { + loadBalancingClientIdThisPrimary(); +} + +TEST_F(QueryFilterTest, loadBalancingClientIdThisPrimaryMultiThreading) { + MultiThreadingMgr::instance().setMode(true); + loadBalancingClientIdThisPrimary(); +} + +TEST_F(QueryFilterTest, loadBalancingThisPrimary) { + loadBalancingThisPrimary(); +} + +TEST_F(QueryFilterTest, loadBalancingThisPrimaryMultiThreading) { + MultiThreadingMgr::instance().setMode(true); + loadBalancingThisPrimary(); +} + +TEST_F(QueryFilterTest, loadBalancingThisSecondary) { + loadBalancingThisSecondary(); +} + +TEST_F(QueryFilterTest, loadBalancingThisSecondaryMultiThreading) { + MultiThreadingMgr::instance().setMode(true); + loadBalancingThisSecondary(); +} + +TEST_F(QueryFilterTest, loadBalancingThisBackup) { + loadBalancingThisBackup(); +} + +TEST_F(QueryFilterTest, loadBalancingThisBackupMultiThreading) { + MultiThreadingMgr::instance().setMode(true); + loadBalancingThisBackup(); +} + +TEST_F(QueryFilterTest, hotStandbyThisPrimary) { + hotStandbyThisPrimary(); +} + +TEST_F(QueryFilterTest, hotStandbyThisPrimaryMultiThreading) { + MultiThreadingMgr::instance().setMode(true); + hotStandbyThisPrimary(); +} + +TEST_F(QueryFilterTest, hotStandbyThisSecondary) { + hotStandbyThisSecondary(); +} + +TEST_F(QueryFilterTest, hotStandbyThisSecondaryMultiThreading) { + MultiThreadingMgr::instance().setMode(true); + hotStandbyThisSecondary(); +} + +TEST_F(QueryFilterTest, hotStandbyThisBackup) { + hotStandbyThisBackup(); +} + +TEST_F(QueryFilterTest, hotStandbyThisBackupMultiThreading) { + MultiThreadingMgr::instance().setMode(true); + hotStandbyThisBackup(); +} + +TEST_F(QueryFilterTest, loadBalancingThisPrimary6) { + loadBalancingThisPrimary6(); +} + +TEST_F(QueryFilterTest, loadBalancingThisPrimary6MultiThreading) { + MultiThreadingMgr::instance().setMode(true); + loadBalancingThisPrimary6(); +} + +TEST_F(QueryFilterTest, loadBalancingThisSecondary6) { + loadBalancingThisSecondary6(); +} + +TEST_F(QueryFilterTest, loadBalancingThisSecondary6MultiThreading) { + MultiThreadingMgr::instance().setMode(true); + loadBalancingThisSecondary6(); +} + +TEST_F(QueryFilterTest, loadBalancingThisBackup6) { + loadBalancingThisBackup6(); +} + +TEST_F(QueryFilterTest, loadBalancingThisBackup6MultiThreading) { + MultiThreadingMgr::instance().setMode(true); + loadBalancingThisBackup6(); +} + +TEST_F(QueryFilterTest, hotStandbyThisPrimary6) { + hotStandbyThisPrimary6(); +} + +TEST_F(QueryFilterTest, hotStandbyThisPrimary6MultiThreading) { + MultiThreadingMgr::instance().setMode(true); + hotStandbyThisPrimary6(); +} + +TEST_F(QueryFilterTest, hotStandbyThisSecondary6) { + hotStandbyThisSecondary6(); +} + +TEST_F(QueryFilterTest, hotStandbyThisSecondary6MultiThreading) { + MultiThreadingMgr::instance().setMode(true); + hotStandbyThisSecondary6(); +} + +TEST_F(QueryFilterTest, hotStandbyThisBackup6) { + hotStandbyThisBackup6(); +} + +TEST_F(QueryFilterTest, hotStandbyThisBackup6MultiThreading) { + MultiThreadingMgr::instance().setMode(true); + hotStandbyThisBackup6(); +} + +TEST_F(QueryFilterTest, explicitlyServeScopes) { + explicitlyServeScopes(); +} + +TEST_F(QueryFilterTest, explicitlyServeScopesMultiThreading) { + MultiThreadingMgr::instance().setMode(true); + explicitlyServeScopes(); +} + }