[#1959] Changes in heartbeat and lease updates

Previously, we'd assume that the communication with the partner is ok
when lease update was sent successfuly. In that case, we'd not send a
heartbeat. Under a heavy load, we could delay heartbeats for a long time.
Sending heartbeats is actually important because the heartbeat response
conveys information about the partner state. We want to have up-to-date
information about partner's state.

diff --git a/src/hooks/dhcp/high_availability/communication_state.cc b/src/hooks/dhcp/high_availability/communication_state.cc
index 4c564c2491d17e574b1da95d6eceecc9641e53a4..242fe7729a0b82254c02d8ab589d0a3b5ffee5ce 100644 (file)
--- a/src/hooks/dhcp/high_availability/communication_state.cc
+++ b/src/hooks/dhcp/high_availability/communication_state.cc
@@ -272,15 +272,13 @@ CommunicationState::pokeInternal() {
 
     if (timer_) {
         // Check the duration since last poke. If it is less than a second, we don't
-        // want to reschedule the timer. The only case when the poke time duration is
-        // lower than 1s is when we're performing lease updates. In order to avoid the
-        // overhead of re-scheduling the timer too frequently we reschedule it only if the
+        // want to reschedule the timer. In order to avoid the overhead of
+        // re-scheduling the timer too frequently we reschedule it only if the
         // duration is 1s or more. This matches the time resolution for heartbeats.
         if (duration_since_poke.total_seconds() > 0) {
             // A poke causes the timer to be re-scheduled to prevent it
             // from triggering a heartbeat shortly after confirming the
-            // connection is ok, based on the lease update or another
-            // command.
+            // connection is ok.
             startHeartbeatInternal();
         }
     }

diff --git a/src/hooks/dhcp/high_availability/communication_state.h b/src/hooks/dhcp/high_availability/communication_state.h
index 0296c75048ee441669b651fe2d8bf30be3601db4..3d2ddc8a6d128d03df63b7d0b448db25491d137f 100644 (file)
--- a/src/hooks/dhcp/high_availability/communication_state.h
+++ b/src/hooks/dhcp/high_availability/communication_state.h
@@ -39,21 +39,17 @@ namespace ha {
 /// the two peers. If the connection is lost it is an indicator that
 /// the partner server may be down and failover actions should be triggered.
 ///
-/// Any command successfully sent over the control channel is an indicator
-/// that the connection is healthy. The most common command sent over the
-/// control channel is a lease update. If the DHCP traffic is heavy, the
-/// number of generated lease updates is sufficient to determine whether
-/// the connection is healthy or not. There is no need to send heartbeat
-/// commands in this case. However, if the DHCP traffic is low there is
-/// a need to send heartbeat commands to the partner at the specified
-/// rate to keep up-to-date information about the state of the connection.
+/// A heartbeat command successfully sent over the control channel is an
+/// indicator that the connection is healthy. A reply to the heartbeat
+/// command includes information about the recipient state, its notion of
+/// time, and other information useful for determining its health and
+/// current activity.
 ///
 /// This class uses an interval timer to run heartbeat commands over the
 /// control channel. The implementation of the heartbeat is external to
 /// this class and is provided via @c CommunicationState::startHeartbeat
 /// method. This implementation is required to run the @c poke method
 /// in case of receiving a successful response to the heartbeat command.
-/// It must also run @c poke when the lease update is successful.
 ///
 /// The @c poke method sets the "last poke time" to current time, thus
 /// indicating that the connection is healthy. The @c getDurationInMillisecs

diff --git a/src/hooks/dhcp/high_availability/ha_service.cc b/src/hooks/dhcp/high_availability/ha_service.cc
index 9d466937c62f8dda4b6cf4f0186b6ee105f9439f..46d7728c360d42a8e0d1ae8e3ef864a066e7ff01 100644 (file)
--- a/src/hooks/dhcp/high_availability/ha_service.cc
+++ b/src/hooks/dhcp/high_availability/ha_service.cc
@@ -1364,16 +1364,10 @@ HAService::asyncSendLeaseUpdate(const QueryPtrType& query,
 
             // We don't care about the result of the lease update to the backup server.
             // It is a best effort update.
-            if (config->getRole() != HAConfig::PeerConfig::BACKUP) {
-                if (lease_update_success) {
-                    // If the lease update was successful and we have sent it to the server
-                    // to which we also send heartbeats (primary, secondary or standby) we
-                    // can assume that the server is online and we can defer next heartbeat.
-                    communication_state_->poke();
-
-                } else {
-                    communication_state_->setPartnerState("unavailable");
-                }
+            if ((config->getRole() != HAConfig::PeerConfig::BACKUP) && !lease_update_success) {
+                // If we were unable to communicate with the partner we set partner's
+                // state as unavailable.
+                communication_state_->setPartnerState("unavailable");
             }
 
             // It is possible to configure the server to not wait for a response from

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 52ae8696eac871c40171d828d400243c0a7f4692..f2fed405ecbbb8a39ba329bf9055778ba11c6359 100644 (file)
--- a/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc
+++ b/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc
@@ -791,14 +791,14 @@ public:
     /// the partner is online or offline.
     ///
     /// @param unpark_handler a function called when packet is unparked.
-    /// @param should_pass indicates if the update is expected to be successful.
+    /// @param should_fail indicates if the update is expected to be unsuccessful.
     /// @param num_updates expected number of servers to which lease updates are
     /// sent.
     /// @param my_state state of the server while lease updates are sent.
     /// @param wait_backup_ack indicates if the server should wait for the acknowledgment
     /// from the backup servers.
     void testSendLeaseUpdates(std::function<void()> unpark_handler,
-                              const bool should_pass,
+                              const bool should_fail,
                               const size_t num_updates,
                               const MyState& my_state = MyState(HA_LOAD_BALANCING_ST),
                               const bool wait_backup_ack = false) {
@@ -861,8 +861,6 @@ public:
         // an acknowledgment.
         EXPECT_EQ(2 * num_updates, service_->getPendingRequest(query));
 
-        EXPECT_FALSE(state->isPoked());
-
         // Let's park the packet and associate it with the callback function which
         // simply records the fact that it has been called. We expect that it wasn't
         // because the parked packet should be dropped as a result of lease updates
@@ -888,10 +886,8 @@ public:
         // The updates should not be sent to this server.
         EXPECT_TRUE(factory_->getResponseCreator()->getReceivedRequests().empty());
 
-        if (should_pass) {
-            EXPECT_TRUE(state->isPoked());
-        } else {
-            EXPECT_FALSE(state->isPoked());
+        if (should_fail) {
+            EXPECT_EQ(HA_UNAVAILABLE_ST, state->getPartnerState());
         }
     }
 
@@ -899,14 +895,14 @@ public:
     /// the partner is online or offline.
     ///
     /// @param unpark_handler a function called when packet is unparked.
-    /// @param should_pass indicates if the update is expected to be successful.
+    /// @param should_fail indicates if the update is expected to be unsuccessful.
     /// @param num_updates expected number of servers to which lease updates are
     /// sent.
     /// @param my_state state of the server while lease updates are sent.
     /// @param wait_backup_ack indicates if the server should wait for the acknowledgment
     /// from the backup servers.
     void testSendLeaseUpdates6(std::function<void()> unpark_handler,
-                               const bool should_pass,
+                               const bool should_fail,
                                const size_t num_updates,
                                const MyState& my_state = MyState(HA_LOAD_BALANCING_ST),
                                const bool wait_backup_ack = false) {
@@ -989,10 +985,8 @@ public:
         // The updates should not be sent to this server.
         EXPECT_TRUE(factory_->getResponseCreator()->getReceivedRequests().empty());
 
-        if (should_pass) {
-            EXPECT_TRUE(state->isPoked());
-        } else {
-            EXPECT_FALSE(state->isPoked());
+        if (should_fail) {
+            EXPECT_EQ(HA_UNAVAILABLE_ST, state->getPartnerState());
         }
     }
 
@@ -1071,7 +1065,7 @@ public:
         // This flag will be set to true if unpark is called.
         bool unpark_called = false;
         testSendLeaseUpdates([&unpark_called] { unpark_called = true; },
-                             true, 1);
+                             false, 1);
 
         // Expecting that the packet was unparked because lease
         // updates are expected to be successful.
@@ -1294,7 +1288,7 @@ public:
         testSendLeaseUpdates([] {
             ADD_FAILURE() << "unpark function called but expected that "
                 "the packet is dropped";
-        }, false, 1);
+        }, true, 1);
 
         // Server 2 should not receive lease4-update.
         auto update_request2 =
@@ -1325,7 +1319,7 @@ public:
         testSendLeaseUpdates([] {
             ADD_FAILURE() << "unpark function called but expected that "
                 "the packet is dropped";
-        }, false, 1);
+        }, true, 1);
 
         // The updates should be sent to server 2 and this server should
         // return error code.
@@ -1375,7 +1369,7 @@ public:
         testSendLeaseUpdates([] {
             ADD_FAILURE() << "unpark function called but expected that "
                 "the packet is dropped";
-        }, false, 1);
+        }, true, 1);
 
         // The updates should be sent to server 2 and this server should
         // return error code.
@@ -1408,7 +1402,7 @@ public:
 
         bool unpark_called = false;
         testSendLeaseUpdates([&unpark_called] { unpark_called = true; },
-                             true, 1);
+                             false, 1);
 
         EXPECT_TRUE(unpark_called);
 
@@ -1452,7 +1446,7 @@ public:
         // This flag will be set to true if unpark is called.
         bool unpark_called = false;
         testSendLeaseUpdates6([&unpark_called] { unpark_called = true; },
-                              true, 1);
+                              false, 1);
 
         // Expecting that the packet was unparked because lease
         // updates are expected to be successful.
@@ -1655,7 +1649,7 @@ public:
         testSendLeaseUpdates6([] {
             ADD_FAILURE() << "unpark function called but expected that "
                 "the packet is dropped";
-        }, false, 1);
+        }, true, 1);
 
         // Server 2 should not receive lease6-bulk-apply.
         auto update_request2 =
@@ -1676,7 +1670,7 @@ public:
 
         bool unpark_called = false;
         testSendLeaseUpdates6([&unpark_called] { unpark_called = true; },
-                              true, 1);
+                              false, 1);
 
         EXPECT_TRUE(unpark_called);
 
@@ -1714,7 +1708,7 @@ public:
         testSendLeaseUpdates6([] {
             ADD_FAILURE() << "unpark function called but expected that "
                 "the packet is dropped";
-        }, false, 1);
+        }, true, 1);
 
         // The updates should be sent to server 2 and this server should return error code.
         EXPECT_EQ(1, factory2_->getResponseCreator()->getReceivedRequests().size());
@@ -1743,7 +1737,7 @@ public:
         testSendLeaseUpdates6([] {
             ADD_FAILURE() << "unpark function called but expected that "
                 "the packet is dropped";
-        }, false, 1);
+        }, true, 1);
 
         // The updates should be sent to server 2 and this server should return error code.
         EXPECT_TRUE(factory2_->getResponseCreator()->getReceivedRequests().empty());
@@ -1806,7 +1800,7 @@ public:
         // This flag will be set to true if unpark is called.
         bool unpark_called = false;
         testSendLeaseUpdates6([&unpark_called] { unpark_called = true; },
-                              true, 1);
+                              false, 1);
 
         // Expecting that the packet was unparked because lease
         // updates are expected to be successful.