[#999] Added support for wait-backup-ack in HA

If the server is configured to not wait for the response from the backup
servers to the lease updates we don't wait for such response.

diff --git a/src/hooks/dhcp/high_availability/ha_service.cc b/src/hooks/dhcp/high_availability/ha_service.cc
index 6a0950592f0fe63155512fe2bc79889c7141a9a1..b3a4931ce2f7662650255f1014edd4a520a1aafa 100644 (file)
--- a/src/hooks/dhcp/high_availability/ha_service.cc
+++ b/src/hooks/dhcp/high_availability/ha_service.cc
@@ -809,8 +809,12 @@ HAService::asyncSendLeaseUpdates(const dhcp::Pkt4Ptr& query,
             continue;
         }
 
-        // Count contacted servers.
-        ++sent_num;
+        // If we're contacting a backup server from which we don't expect a
+        // response prior to responding to the DHCP client we don't count
+        // it.
+        if (config_->amWaitingBackupAck() || (conf->getRole() != HAConfig::PeerConfig::BACKUP)) {
+            ++sent_num;
+        }
 
         // Lease updates for deleted leases.
         for (auto l = deleted_leases->begin(); l != deleted_leases->end(); ++l) {
@@ -849,8 +853,12 @@ HAService::asyncSendLeaseUpdates(const dhcp::Pkt6Ptr& query,
             continue;
         }
 
-        // Count contacted servers.
-        ++sent_num;
+        // If we're contacting a backup server from which we don't expect a
+        // response prior to responding to the DHCP client we don't count
+        // it.
+        if (config_->amWaitingBackupAck() || (conf->getRole() != HAConfig::PeerConfig::BACKUP)) {
+            ++sent_num;
+        }
 
         // Send new/updated leases and deleted leases in one command.
         asyncSendLeaseUpdate(query, conf, CommandCreator::createLease6BulkApply(leases, deleted_leases),
@@ -947,12 +955,26 @@ HAService::asyncSendLeaseUpdate(const QueryPtrType& query,
                     communication_state_->poke();
 
                 } else {
-                    // Lease update was unsuccessful, so drop the parked DHCP packet.
-                    parking_lot->drop(query);
                     communication_state_->setPartnerState("unavailable");
                 }
             }
 
+            // It is possible to configure the server to not wait for a response from
+            // the backup server before we unpark the packet and respond to the client.
+            // Here we check if we're dealing with such situation.
+            if (config_->amWaitingBackupAck() || (config->getRole() != HAConfig::PeerConfig::BACKUP)) {
+                // We're expecting a response from the backup server or it is not
+                // a backup server and the lease update was unsuccessful. In such
+                // case the DHCP exchange fails.
+                if (!lease_update_success) {
+                    parking_lot->drop(query);
+                }
+            } else {
+                // This was a response from the backup server and we're configured to
+                // not wait for their ackowledgments, so there is nothing more to do.
+                return;
+            }
+
             auto it = pending_requests_.find(query);
 
             // If there are no more pending requests for this query, let's unpark
@@ -979,12 +1001,18 @@ HAService::asyncSendLeaseUpdate(const QueryPtrType& query,
         boost::bind(&HAService::clientCloseHandler, this, _1)
     );
 
-    // Request scheduled, so update the request counters for the query.
-    if (pending_requests_.count(query) == 0) {
-        pending_requests_[query] = 1;
+    // The number of pending requests is the number of requests for which we
+    // expect an acknowledgement prior to responding to the DHCP clients. If
+    // we're configured to wait for the acks from the backups or it is not
+    // a backup increase the number of pending requests.
+    if (config_->amWaitingBackupAck() || (config->getRole() != HAConfig::PeerConfig::BACKUP)) {
+        // Request scheduled, so update the request counters for the query.
+        if (pending_requests_.count(query) == 0) {
+            pending_requests_[query] = 1;
 
-    } else {
-        ++pending_requests_[query];
+        } else {
+            ++pending_requests_[query];
+        }
     }
 }
 

diff --git a/src/hooks/dhcp/high_availability/ha_service.h b/src/hooks/dhcp/high_availability/ha_service.h
index c9a0ccaa96dc25aa8a1eae6f5e1305f284a73f81..3fb2685d5cbc8a8bfe30c063528c60592fd804db 100644 (file)
--- a/src/hooks/dhcp/high_availability/ha_service.h
+++ b/src/hooks/dhcp/high_availability/ha_service.h
@@ -455,8 +455,8 @@ public:
     /// the asynchronous updates are completed.
     ///
     /// @return Number of peers to whom lease updates have been scheduled
-    /// to be sent. This value is used to determine if the DHCP query
-    /// should be parked while waiting for the lease update to complete.
+    /// to be sent and from which we expect a response prior to unparking
+    /// the packet and sending a response to the DHCP client.
     size_t asyncSendLeaseUpdates(const dhcp::Pkt4Ptr& query,
                                  const dhcp::Lease4CollectionPtr& leases,
                                  const dhcp::Lease4CollectionPtr& deleted_leases,
@@ -479,8 +479,8 @@ public:
     /// the asynchronous updates are completed.
     ///
     /// @return Number of peers to whom lease updates have been scheduled
-    /// to be sent. This value is used to determine if the DHCP query
-    /// should be parked while waiting for the lease update to complete.
+    /// to be sent and from which we expect a response prior to unparking
+    /// the packet and sending a response to the DHCP client.
     size_t asyncSendLeaseUpdates(const dhcp::Pkt6Ptr& query,
                                  const dhcp::Lease6CollectionPtr& leases,
                                  const dhcp::Lease6CollectionPtr& deleted_leases,

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 e98f2a530502149e24fee9c368aba1514af2a9b8..123531f1dd424edc60d149273def620b85d1d1e6 100644 (file)
--- a/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc
+++ b/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc
@@ -660,13 +660,17 @@ public:
     /// @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 size_t num_updates,
-                              const MyState& my_state = MyState(HA_LOAD_BALANCING_ST)) {
+                              const MyState& my_state = MyState(HA_LOAD_BALANCING_ST),
+                              const bool wait_backup_ack = true) {
         // Create HA configuration for 3 servers. This server is
         // server 1.
         HAConfigPtr config_storage = createValidConfiguration();
+        config_storage->setWaitBackupAck(wait_backup_ack);
 
         // Create parking lot where query is going to be parked and unparked.
         ParkingLotPtr parking_lot(new ParkingLot());
@@ -710,6 +714,12 @@ public:
                   service.asyncSendLeaseUpdates(query, leases4, deleted_leases4,
                                                 parking_lot_handle));
 
+        // The number of pending requests should be 2 times the number of
+        // contacted servers because we send one lease update and one
+        // lease deletion to each contacted server from which we expect
+        // an acknowledgment.
+        EXPECT_EQ(2*num_updates, service.pending_requests_[query]);
+
         EXPECT_FALSE(state->isPoked());
 
         ASSERT_NO_THROW(parking_lot->reference(query));
@@ -748,13 +758,17 @@ public:
     /// @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 size_t num_updates,
-                               const MyState& my_state = MyState(HA_LOAD_BALANCING_ST)) {
+                               const MyState& my_state = MyState(HA_LOAD_BALANCING_ST),
+                               const bool wait_backup_ack = true) {
         // Create HA configuration for 3 servers. This server is
         // server 1.
         HAConfigPtr config_storage = createValidConfiguration();
+        config_storage->setWaitBackupAck(wait_backup_ack);
 
         // Create parking lot where query is going to be parked and unparked.
         ParkingLotPtr parking_lot(new ParkingLot());
@@ -796,6 +810,11 @@ public:
                   service.asyncSendLeaseUpdates(query, leases6, deleted_leases6,
                                                 parking_lot_handle));
 
+        // The number of requests we send is equal to the number of servers
+        // from which we expect an acknowledgement. We send both lease updates
+        // and the deletions in a single bulk update command.
+        EXPECT_EQ(num_updates, service.pending_requests_[query]);
+
         EXPECT_FALSE(state->isPoked());
 
         ASSERT_NO_THROW(parking_lot->reference(query));
@@ -1286,7 +1305,7 @@ TEST_F(HAServiceTest, sendUpdatesBackupServerOffline) {
     bool unpark_called = false;
     testSendLeaseUpdates([&unpark_called] {
         unpark_called = true;
-    }, true, 2);
+    }, true, 1, MyState(HA_LOAD_BALANCING_ST), false);
 
     EXPECT_TRUE(unpark_called);
 
@@ -1314,6 +1333,47 @@ TEST_F(HAServiceTest, sendUpdatesBackupServerOffline) {
     EXPECT_FALSE(delete_request3);
 }
 
+// Test scenario when the backup server is offline but we do expect an
+// ack from it prior to responding to the DHCP client.
+TEST_F(HAServiceTest, sendUpdatesBackupServerOfflineAckExpected) {
+    // Start only two servers out of three. The server 2 is not running.
+    ASSERT_NO_THROW({
+            listener_->start();
+            listener2_->start();
+    });
+
+    bool unpark_called = false;
+    testSendLeaseUpdates([&unpark_called] {
+        unpark_called = true;
+    }, true, 2, MyState(HA_LOAD_BALANCING_ST), true);
+
+    // The packet shouldn't be unparked. It should be dropped.
+    EXPECT_FALSE(unpark_called);
+
+    // The server 2 should have received two commands.
+    EXPECT_EQ(2, factory2_->getResponseCreator()->getReceivedRequests().size());
+
+    // Check that the server 2 has received lease4-update command.
+    auto update_request2 = factory2_->getResponseCreator()->findRequest("lease4-update",
+                                                                        "192.1.2.3");
+    EXPECT_TRUE(update_request2);
+
+    // Check that the server 2 has received lease4-del command.
+    auto delete_request2 = factory2_->getResponseCreator()->findRequest("lease4-del",
+                                                                        "192.2.3.4");
+    EXPECT_TRUE(delete_request2);
+
+    // Server 3 should not receive lease4-update.
+    auto update_request3 = factory3_->getResponseCreator()->findRequest("lease4-update",
+                                                                        "192.1.2.3");
+    EXPECT_FALSE(update_request3);
+
+    // Server 3 should not receive lease4-del.
+    auto delete_request3 = factory3_->getResponseCreator()->findRequest("lease4-del",
+                                                                        "192.2.3.4");
+    EXPECT_FALSE(delete_request3);
+}
+
 // Test scenario when one of the servers to which a lease update is sent
 // returns an error.
 TEST_F(HAServiceTest, sendUpdatesControlResultError) {
@@ -1473,7 +1533,7 @@ TEST_F(HAServiceTest, sendUpdatesBackupServerOffline6) {
     bool unpark_called = false;
     testSendLeaseUpdates6([&unpark_called] {
         unpark_called = true;
-    }, true, 2);
+    }, true, 1, MyState(HA_LOAD_BALANCING_ST), false);
 
     EXPECT_TRUE(unpark_called);
 
@@ -1493,6 +1553,39 @@ TEST_F(HAServiceTest, sendUpdatesBackupServerOffline6) {
     EXPECT_FALSE(update_request3);
 }
 
+// Test scenario when the backup server is offline but we do expect an
+// ack from it prior to responding to the DHCP client.
+TEST_F(HAServiceTest, sendUpdatesBackupServerOfflineAckExpected6) {
+    // Start only two servers out of three. The server 2 is not running.
+    ASSERT_NO_THROW({
+            listener_->start();
+            listener2_->start();
+    });
+
+    bool unpark_called = false;
+    testSendLeaseUpdates6([&unpark_called] {
+        unpark_called = true;
+    }, true, 2, MyState(HA_LOAD_BALANCING_ST), true);
+
+    // The packet shouldn't be unparked. It should be dropped.
+    EXPECT_FALSE(unpark_called);
+
+    // The server 2 should have received one command.
+    EXPECT_EQ(1, factory2_->getResponseCreator()->getReceivedRequests().size());
+
+    // Check that the server 2 has received lease6-bulk-apply command.
+    auto update_request2 = factory2_->getResponseCreator()->findRequest("lease6-bulk-apply",
+                                                                        "2001:db8:1::cafe",
+                                                                        "2001:db8:1::efac");
+    EXPECT_TRUE(update_request2);
+
+    // Server 3 should not receive lease6-bulk-apply.
+    auto update_request3 = factory3_->getResponseCreator()->findRequest("lease6-bulk-apply",
+                                                                        "2001:db8:1::cafe",
+                                                                        "2001:db8:1::efac");
+    EXPECT_FALSE(update_request3);
+}
+
 // Test scenario when one of the servers to which a lease update is sent
 // returns an error.
 TEST_F(HAServiceTest, sendUpdatesControlResultError6) {