[#3110] Addressed review comments

Reworked asyncSendLeaseUpdate() and made minor corrections

diff --git a/src/bin/dhcp4/tests/hooks_unittest.cc b/src/bin/dhcp4/tests/hooks_unittest.cc
index dbf9d9e7a068f6804935f7a3be006b27523af25c..5ab9a9a295f78d5d6c5b4bfab0559261b2e2da76 100644 (file)
--- a/src/bin/dhcp4/tests/hooks_unittest.cc
+++ b/src/bin/dhcp4/tests/hooks_unittest.cc
@@ -841,6 +841,15 @@ public:
         return (0);
     }
 
+    /// @brief Test callback that marks address in use and asks the server to
+    /// park the packet.
+    ///
+    /// The server's unpark lambda uses the callout argument, "offer_adddress_in_use",
+    /// to deteremine if it should decline the lease or send the offer to the
+    /// client.  This function sets it to true.
+    ///
+    /// @param callout_handle handle passed by the hooks framework
+    /// @return always 0
     static int
     lease4_offer_park_in_use_callout(CalloutHandle& callout_handle) {
         callback_name_ = string("lease4_offer");
@@ -873,7 +882,6 @@ public:
         return (0);
     }
 
-
     /// @brief Test callback that stores callout name and passed parameters.
     ///
     /// @param callout_handle handle passed by the hooks framework
@@ -3988,7 +3996,7 @@ TEST_F(HooksDhcpv4SrvTest, lease4OfferDiscoverDecline) {
     EXPECT_EQ(expected_address, callback_lease4_->addr_);
 
     // Since the callout set offer_address_in_use flag to true the offer should
-    // have been discarded. Make sure that we did not receive a repsonse.
+    // have been discarded. Make sure that we did not receive a response.
     ASSERT_FALSE(client.getContext().response_);
 
     // Clear static buffers

diff --git a/src/hooks/dhcp/high_availability/ha_messages.mes b/src/hooks/dhcp/high_availability/ha_messages.mes
index 17994cb64fcb1c58b462729d97ed5c5fa4a50119..d1f69d1c67ce94311499b26d2a20feb3d4bc133f 100644 (file)
--- a/src/hooks/dhcp/high_availability/ha_messages.mes
+++ b/src/hooks/dhcp/high_availability/ha_messages.mes
@@ -633,7 +633,7 @@ state. The partner must be restarted before the server can synchronize the
 database and start normal operation.
 
 % HA_LEASE4_SERVER_DECLINE_FAILED lease4_server_decline callout failed: %1
-This error message is issued when the callout for the lease4_server_declinehook
+This error message is issued when the callout for the lease4_server_decline hook
 point failed. This includes unexpected errors like wrong arguments provided to
 the callout by the DHCP server (unlikely internal server error).
 The argument contains a reason for the error.

diff --git a/src/hooks/dhcp/high_availability/ha_service.cc b/src/hooks/dhcp/high_availability/ha_service.cc
index a94eac19fd697372b083795300eba07a85b90a1f..142109b8002756a0e76864f079af67c5e75514d4 100644 (file)
--- a/src/hooks/dhcp/high_availability/ha_service.cc
+++ b/src/hooks/dhcp/high_availability/ha_service.cc
@@ -1253,43 +1253,11 @@ HAService::asyncSendSingleLeaseUpdate(const dhcp::Pkt4Ptr& query,
 
     size_t sent_num = 0;
 
-    // Schedule sending lease updates to each peer.
-    for (auto p = peers_configs.begin(); p != peers_configs.end(); ++p) {
-        HAConfig::PeerConfigPtr conf = p->second;
-
-        // Check if the lease updates should be queued. This is the case when the
-        // server is in the communication-recovery state. Queued lease updates may
-        // be sent when the communication is re-established.
-        if (shouldQueueLeaseUpdates(conf)) {
-            lease_update_backlog_.push(LeaseUpdateBacklog::ADD, lease);
-            continue;
-        }
-
-        // Check if the lease update should be sent to the server. If we're in
-        // the partner-down state we don't send lease updates to the partner.
-        if (!shouldSendLeaseUpdates(conf)) {
-            // If we decide to not send the lease updates to an active partner, we
-            // should make a record of it in the communication state. The partner
-            // can check if there were any unsent lease updates when he determines
-            // whether it should synchronize its database or not when it recovers
-            // from the partner-down state.
-            if (conf->getRole() != HAConfig::PeerConfig::BACKUP) {
-                communication_state_->increaseUnsentUpdateCount();
-            }
-            continue;
-        }
+    Lease4CollectionPtr leases(new Lease4Collection());
+    leases->push_back(lease);
+    Lease4CollectionPtr deleted_leases(new Lease4Collection());
 
-        asyncSendLeaseUpdate(query, conf, CommandCreator::createLease4Update(*lease), parking_lot);
-
-        // 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;
-        }
-    }
-
-    return (sent_num);
+    return (asyncSendLeaseUpdates(query, leases, deleted_leases, parking_lot));
 }
 
 size_t

diff --git a/src/hooks/dhcp/high_availability/ha_service.h b/src/hooks/dhcp/high_availability/ha_service.h
index d073e8bb5d2eda7a7ddfd0c57d720de0e1d19ed2..860ce2fc9e45f5c50fe8e80f21338efe40337525 100644 (file)
--- a/src/hooks/dhcp/high_availability/ha_service.h
+++ b/src/hooks/dhcp/high_availability/ha_service.h
@@ -582,22 +582,9 @@ public:
 
     /// @brief Schedules an asynchronous IPv4 lease update.
     ///
-    /// This method schedules an asynchronous lease update for a single lease.
-    /// It is currently only used for "lease4_server_decline" callout.
-    /// The lease update is transmitted over HTTP to the peers specified in
-    /// the configuration (except self).
-    /// If the server is in the partner-down state the lease update is not
-    /// sent to the partner but is sent to all backup servers.
-    /// In other states in which the server responds to DHCP queries, the
-    /// lease update is sent to all servers. The scheduled lease update
-    /// is performed after the callouts return. The server may or may not
-    /// parks the processed DHCP packet and runs IO service shared between
-    /// the server and the hook library.
-    ////
-    /// If the lease update to the partner (primary, secondary or standby)
-    /// fails, the packet, if parked, is dropped. If the lease update to
-    /// any of the backup server fails, an error message is logged but the DHCP
-    /// packet is not dropped.
+    /// This method is a convenience wrapper around asyncSendLeaseUpdates() for
+    /// propagating updates for a single lease. It is currently only used by
+    /// the "lease4_server_decline" callout.
     ///
     /// @param query Pointer to the processed DHCP client message.
     /// @param lease Pointer to the updated lease

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 df6ed1f688001aeed852bae231ef23923d492f9e..32626bf79665336caa3c00a7d3ac6255833f887e 100644 (file)
--- a/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc
+++ b/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc
@@ -2231,7 +2231,7 @@ public:
     }
 
     /// @brief Tests successful scenarios when a single lease update is done using
-    /// sendLeaseUpdate().
+    /// asyncSendLeaseUpdate().
     ///
     /// @param with_parking True if packet parking should be used, false is not.
     void testSuccessSendSingleLeaseUpdate(bool with_parking) {
@@ -8197,7 +8197,7 @@ TEST_F(HAServiceTest, successfulSendSingleLeaseUpdateWithoutParking) {
     testSuccessSendSingleLeaseUpdate(false);
 }
 
-// Test scenario when a single lease4 update is sent successfully, parkin is
+// Test scenario when a single lease4 update is sent successfully, parking is
 // employed.
 TEST_F(HAServiceTest, successfulSendSingleLeaseUpdateWithParking) {
     testSuccessSendSingleLeaseUpdate(true);