From: Marcin Siodelski Date: Mon, 5 Nov 2018 17:13:40 +0000 (+0100) Subject: [#78,!85] Addressed review comments. X-Git-Tag: 66-authoritative-flag-in-kea_base~6 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c54ea216463dcc6df693c96c4a5f82c0fbec2ff6;p=thirdparty%2Fkea.git [#78,!85] Addressed review comments. --- diff --git a/doc/guide/hooks-ha.xml b/doc/guide/hooks-ha.xml index 4b36ad598e..6249cb5068 100644 --- a/doc/guide/hooks-ha.xml +++ b/doc/guide/hooks-ha.xml @@ -893,7 +893,7 @@
Lease Information Sharing - The HA enabled server informs its active partner about allocated + An HA enabled server informs its active partner about allocated or renewed leases by sending appropriate control commands. The partner updates the lease information in its own database. When the server starts up for the first time or recovers after a failure it synchronizes its @@ -979,7 +979,7 @@
Controlling Lease Page Size Limit - HA enabled server initiates synchronization of the lease + An HA enabled server initiates synchronization of the lease database after down time or upon receiving ha-sync command. The server uses commands described in to fetch leases from the @@ -1006,8 +1006,8 @@ network, lease database synchronization after the server failure may be a time consuming operation. The synchronizing server needs to gather all leases from the partner which yields a large response - over the RESTful interface. The server receives leases using - paging mechanism as described in . + over the RESTful interface. The server receives leases using the + paging mechanism described in . Before the page of leases is fetched, the synchronizing server sends a dhcp-disable command to disable the DHCP service on the partner server. If the service is already disabled, this diff --git a/src/hooks/dhcp/high_availability/ha_config.h b/src/hooks/dhcp/high_availability/ha_config.h index ef48c8b671..e11283444f 100644 --- a/src/hooks/dhcp/high_availability/ha_config.h +++ b/src/hooks/dhcp/high_availability/ha_config.h @@ -348,10 +348,10 @@ public: sync_timeout_ = sync_timeout; } - /// @brief Returns page size for leases fetched from the partner + /// @brief Returns maximum number of leases per page to be fetched /// during database synchronization. /// - /// @return Page limit. + /// @return Maximum number of leases per page. uint32_t getSyncPageLimit() const { return (sync_page_limit_); } diff --git a/src/hooks/dhcp/high_availability/ha_service.cc b/src/hooks/dhcp/high_availability/ha_service.cc index de9a3470e3..470674eab0 100644 --- a/src/hooks/dhcp/high_availability/ha_service.cc +++ b/src/hooks/dhcp/high_availability/ha_service.cc @@ -1022,10 +1022,10 @@ HAService::startHeartbeat() { } void -HAService::asyncDisable(HttpClient& http_client, - const std::string& server_name, - const unsigned int max_period, - PostRequestCallback post_request_action) { +HAService::asyncDisableDHCPService(HttpClient& http_client, + const std::string& server_name, + const unsigned int max_period, + PostRequestCallback post_request_action) { HAConfig::PeerConfigPtr remote_config = config_->getPeerConfig(server_name); // Create HTTP/1.1 request including our command. @@ -1091,9 +1091,9 @@ HAService::asyncDisable(HttpClient& http_client, } void -HAService::asyncEnable(HttpClient& http_client, - const std::string& server_name, - PostRequestCallback post_request_action) { +HAService::asyncEnableDHCPService(HttpClient& http_client, + const std::string& server_name, + PostRequestCallback post_request_action) { HAConfig::PeerConfigPtr remote_config = config_->getPeerConfig(server_name); // Create HTTP/1.1 request including our command. @@ -1157,12 +1157,12 @@ HAService::asyncEnable(HttpClient& http_client, } void -HAService::localDisable() { +HAService::localDisableDHCPService() { network_state_->disableService(); } void -HAService::localEnable() { +HAService::localEnableDHCPService() { network_state_->enableService(); } @@ -1174,7 +1174,8 @@ HAService::asyncSyncLeases() { unsigned int dhcp_disable_timeout = static_cast(config_->getSyncTimeout() / 1000); if (dhcp_disable_timeout == 0) { - ++dhcp_disable_timeout; + // Ensure that we always use at least 1 second timeout. + dhcp_disable_timeout = 1; } asyncSyncLeases(client_, config_->getFailoverPeerConfig()->getName(), @@ -1193,10 +1194,10 @@ HAService::asyncSyncLeases(http::HttpClient& http_client, // to allocate new leases while we fetch from it. The DHCP service will // be disabled for a certain amount of time and will be automatically // re-enabled if we die during the synchronization. - asyncDisable(http_client, server_name, max_period, - [this, &http_client, server_name, max_period, last_lease, - post_sync_action, dhcp_disabled] - (const bool success, const std::string& error_message) { + asyncDisableDHCPService(http_client, server_name, max_period, + [this, &http_client, server_name, max_period, last_lease, + post_sync_action, dhcp_disabled] + (const bool success, const std::string& error_message) { // If we have successfully disabled the DHCP service on the peer, // we can start fetching the leases. @@ -1350,9 +1351,9 @@ HAService::asyncSyncLeasesInternal(http::HttpClient& http_client, (l + 1 == leases_element.end())) { last_lease = boost::dynamic_pointer_cast(lease); } - } + } catch (const std::exception& ex) { LOG_WARN(ha_logger, HA_LEASE_SYNC_FAILED) .arg((*l)->str()) @@ -1418,9 +1419,9 @@ HAService::synchronize(std::string& status_message, const std::string& server_na // we need to re-enable the DHCP service on the peer if the // DHCP service was disabled in the course of synchronization. if (dhcp_disabled) { - asyncEnable(client, server_name, - [&](const bool success, - const std::string& error_message) { + asyncEnableDHCPService(client, server_name, + [&](const bool success, + const std::string& error_message) { // It is possible that we have already recorded an error // message while synchronizing the lease database. Don't // override the existing error message. diff --git a/src/hooks/dhcp/high_availability/ha_service.h b/src/hooks/dhcp/high_availability/ha_service.h index bacef1fd99..718b331452 100644 --- a/src/hooks/dhcp/high_availability/ha_service.h +++ b/src/hooks/dhcp/high_availability/ha_service.h @@ -515,10 +515,10 @@ protected: /// should be disabled. /// @param post_request_action pointer to the function to be executed when /// the request is completed. - void asyncDisable(http::HttpClient& http_client, - const std::string& server_name, - const unsigned int max_period, - PostRequestCallback post_request_action); + void asyncDisableDHCPService(http::HttpClient& http_client, + const std::string& server_name, + const unsigned int max_period, + PostRequestCallback post_request_action); /// @brief Schedules asynchronous "dhcp-enable" command to the specified /// server. @@ -529,15 +529,15 @@ protected: /// sent. /// @param post_request_action pointer to the function to be executed when /// the request is completed. - void asyncEnable(http::HttpClient& http_client, - const std::string& server_name, - PostRequestCallback post_request_action); + void asyncEnableDHCPService(http::HttpClient& http_client, + const std::string& server_name, + PostRequestCallback post_request_action); /// @brief Disables local DHCP service. - void localDisable(); + void localDisableDHCPService(); /// @brief Enables local DHCP service. - void localEnable(); + void localEnableDHCPService(); /// @brief Asynchronously reads leases from a peer and updates local /// lease database. @@ -574,10 +574,10 @@ protected: /// longer period of time. If the synchronization is progressing the /// timeout must be deferred. /// - /// The @c asyncSyncLeases method calls itself recursively when the - /// previous @c lease4-get-page or @c lease6-get-page command has - /// completed successfully. If the last page of leases was fetched or - /// if any error occurred, the synchronization is terminated and the + /// The @c asyncSyncLeases method calls itself (recurses) when the previous + /// @c lease4-get-page or @c lease6-get-page command has completed + /// successfully. If the last page of leases was fetched or if any + /// error occurred, the synchronization is terminated and the /// @c post_sync_action callback is invoked. /// /// The last parameter passed to the @c post_sync_action callback indicates diff --git a/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc b/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc index b57606c29e..cedc2164b6 100644 --- a/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc +++ b/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc @@ -161,11 +161,11 @@ public: /// should be disabled. /// @param post_request_action pointer to the function to be executed when /// the request is completed. - void asyncDisable(const std::string& server_name, - const unsigned int max_period, - const PostRequestCallback& post_request_action) { - HAService::asyncDisable(client_, server_name, max_period, - post_request_action); + void asyncDisableDHCPService(const std::string& server_name, + const unsigned int max_period, + const PostRequestCallback& post_request_action) { + HAService::asyncDisableDHCPService(client_, server_name, max_period, + post_request_action); } /// @brief Schedules asynchronous "dhcp-enable" command to the specified @@ -177,9 +177,9 @@ public: /// sent. /// @param post_request_action pointer to the function to be executed when /// the request is completed. - void asyncEnable(const std::string& server_name, - const PostRequestCallback& post_request_action) { - HAService::asyncEnable(client_, server_name, post_request_action); + void asyncEnableDHCPService(const std::string& server_name, + const PostRequestCallback& post_request_action) { + HAService::asyncEnableDHCPService(client_, server_name, post_request_action); } using HAService::asyncSendHeartbeat; @@ -2196,7 +2196,7 @@ TEST_F(HAServiceTest, processSynchronize6EnableError) { } // This test verifies that the DHCPv4 service can be disabled on the remote server. -TEST_F(HAServiceTest, asyncDisable4) { +TEST_F(HAServiceTest, asyncDisableDHCPService4) { // Create HA configuration. HAConfigPtr config_storage = createValidConfiguration(); @@ -2211,9 +2211,9 @@ TEST_F(HAServiceTest, asyncDisable4) { // Send dhcp-disable command with max-period of 10 seconds. // When the transaction is finished, the IO service gets stopped. - ASSERT_NO_THROW(service.asyncDisable("server3", 10, - [this](const bool success, - const std::string& error_message) { + ASSERT_NO_THROW(service.asyncDisableDHCPService("server3", 10, + [this](const bool success, + const std::string& error_message) { EXPECT_TRUE(success); EXPECT_TRUE(error_message.empty()); io_service_->stop(); @@ -2231,7 +2231,7 @@ TEST_F(HAServiceTest, asyncDisable4) { // This test verifies that there is no exception thrown as a result of dhcp-disable // command when the server is offline. -TEST_F(HAServiceTest, asyncDisable4ServerOffline) { +TEST_F(HAServiceTest, asyncDisableDHCPService4ServerOffline) { // Create HA configuration. HAConfigPtr config_storage = createValidConfiguration(); @@ -2239,9 +2239,9 @@ TEST_F(HAServiceTest, asyncDisable4ServerOffline) { // Send dhcp-disable command with max-period of 10 seconds. // When the transaction is finished, the IO service gets stopped. - ASSERT_NO_THROW(service.asyncDisable("server2", 10, - [this](const bool success, - const std::string& error_message) { + ASSERT_NO_THROW(service.asyncDisableDHCPService("server2", 10, + [this](const bool success, + const std::string& error_message) { EXPECT_FALSE(success); EXPECT_FALSE(error_message.empty()); io_service_->stop(); @@ -2253,7 +2253,7 @@ TEST_F(HAServiceTest, asyncDisable4ServerOffline) { // This test verifies that an error is returned when the remote server // returns control status error. -TEST_F(HAServiceTest, asyncDisable4ControlResultError) { +TEST_F(HAServiceTest, asyncDisableDHCPService4ControlResultError) { // Create HA configuration. HAConfigPtr config_storage = createValidConfiguration(); @@ -2273,9 +2273,9 @@ TEST_F(HAServiceTest, asyncDisable4ControlResultError) { // Send dhcp-disable command with max-period of 10 seconds. // When the transaction is finished, the IO service gets stopped. - ASSERT_NO_THROW(service.asyncDisable("server3", 10, - [this](const bool success, - const std::string& error_message) { + ASSERT_NO_THROW(service.asyncDisableDHCPService("server3", 10, + [this](const bool success, + const std::string& error_message) { EXPECT_FALSE(success); EXPECT_FALSE(error_message.empty()); io_service_->stop(); @@ -2286,7 +2286,7 @@ TEST_F(HAServiceTest, asyncDisable4ControlResultError) { } // This test verifies that the DHCPv4 service can be enabled on the remote server. -TEST_F(HAServiceTest, asyncEnable4) { +TEST_F(HAServiceTest, asyncEnableDHCPService4) { // Create HA configuration. HAConfigPtr config_storage = createValidConfiguration(); @@ -2301,7 +2301,8 @@ TEST_F(HAServiceTest, asyncEnable4) { // Send dhcp-enable command. When the transaction is finished, // the IO service gets stopped. - ASSERT_NO_THROW(service.asyncEnable("server2", [this](const bool success, + ASSERT_NO_THROW(service.asyncEnableDHCPService("server2", + [this](const bool success, const std::string& error_message) { EXPECT_TRUE(success); EXPECT_TRUE(error_message.empty()); @@ -2319,7 +2320,7 @@ TEST_F(HAServiceTest, asyncEnable4) { // This test verifies that there is no exception thrown as a result of dhcp-enable // command when the server is offline. -TEST_F(HAServiceTest, asyncEnable4ServerOffline) { +TEST_F(HAServiceTest, asyncEnableDHCPService4ServerOffline) { // Create HA configuration. HAConfigPtr config_storage = createValidConfiguration(); @@ -2327,7 +2328,8 @@ TEST_F(HAServiceTest, asyncEnable4ServerOffline) { // Send dhcp-enable command. When the transaction is finished, // the IO service gets stopped. - ASSERT_NO_THROW(service.asyncEnable("server2", [this](const bool success, + ASSERT_NO_THROW(service.asyncEnableDHCPService("server2", + [this](const bool success, const std::string& error_message) { EXPECT_FALSE(success); EXPECT_FALSE(error_message.empty()); @@ -2340,7 +2342,7 @@ TEST_F(HAServiceTest, asyncEnable4ServerOffline) { // This test verifies that an error is returned when the remote server // returns control status error. -TEST_F(HAServiceTest, asyncEnable4ControlResultError) { +TEST_F(HAServiceTest, asyncEnableDHCPService4ControlResultError) { // Create HA configuration. HAConfigPtr config_storage = createValidConfiguration(); @@ -2360,7 +2362,8 @@ TEST_F(HAServiceTest, asyncEnable4ControlResultError) { // Send dhcp-enable command. When the transaction is finished, // the IO service gets stopped. - ASSERT_NO_THROW(service.asyncEnable("server2", [this](const bool success, + ASSERT_NO_THROW(service.asyncEnableDHCPService("server2", + [this](const bool success, const std::string& error_message) { EXPECT_FALSE(success); EXPECT_FALSE(error_message.empty());