Commit c54ea216 authored by Marcin Siodelski's avatar Marcin Siodelski

[#78,!85] Addressed review comments.

parent f6243bae
......@@ -893,7 +893,7 @@
<section xml:id="ha-sharing-lease-info">
<title>Lease Information Sharing</title>
<para>The HA enabled server informs its active partner about allocated
<para>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 @@
<section xml:id="ha-syncing-page-limit">
<title>Controlling Lease Page Size Limit</title>
<para>HA enabled server initiates synchronization of the lease
<para>An HA enabled server initiates synchronization of the lease
database after down time or upon receiving <command>ha-sync</command>
command. The server uses commands described in
<xref linkend="lease-get-page-cmds"/> 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 <xref linkend="ha-syncing-page-limit"/>.
over the RESTful interface. The server receives leases using the
paging mechanism described in <xref linkend="ha-syncing-page-limit"/>.
Before the page of leases is fetched, the synchronizing server
sends a <command>dhcp-disable</command> command to disable the DHCP
service on the partner server. If the service is already disabled, this
......
......@@ -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_);
}
......
......@@ -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<unsigned int>(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>(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.
......
......@@ -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
......
......@@ -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());
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment