[#2408] HA interprets conflict status code

src/hooks/dhcp/high_availability/communication_state.cc

@@ -701,6 +701,26 @@ CommunicationState4::reportRejectedLeaseUpdate(const boost::shared_ptr<dhcp::Pkt
     return (false);
 }
 
+bool
+CommunicationState4::reportSuccessfulLeaseUpdate(const boost::shared_ptr<dhcp::Pkt>& message) {
+    Pkt4Ptr msg = boost::dynamic_pointer_cast<Pkt4>(message);
+    if (!msg) {
+        isc_throw(BadValue, "DHCP message for which the lease update was successful is not a DHCPv4 message");
+    }
+    std::vector<uint8_t> client_id;
+    OptionPtr opt_client_id = msg->getOption(DHO_DHCP_CLIENT_IDENTIFIER);
+    if (opt_client_id) {
+        client_id = opt_client_id->getData();
+    }
+    auto& idx = rejected_clients_.get<0>();
+    auto existing_client = idx.find(boost::make_tuple(msg->getHWAddr()->hwaddr_, client_id));
+    if (existing_client != idx.end()) {
+        idx.erase(existing_client);
+        return (true);
+    }
+    return (false);
+}
+
 void
 CommunicationState4::clearRejectedLeaseUpdates() {
     rejected_clients_.clear();
@@ -858,6 +878,25 @@ CommunicationState6::reportRejectedLeaseUpdate(const boost::shared_ptr<dhcp::Pkt
     return (false);
 }
 
+bool
+CommunicationState6::reportSuccessfulLeaseUpdate(const boost::shared_ptr<dhcp::Pkt>& message) {
+    Pkt6Ptr msg = boost::dynamic_pointer_cast<Pkt6>(message);
+    if (!msg) {
+        isc_throw(BadValue, "DHCP message for which the lease update was successful is not a DHCPv6 message");
+    }
+    OptionPtr duid = msg->getOption(D6O_CLIENTID);
+    if (!duid) {
+        return (false);
+    }
+    auto& idx = rejected_clients_.get<0>();
+    auto existing_client = idx.find(duid->getData());
+    if (existing_client != idx.end()) {
+        idx.erase(existing_client);
+        return (true);
+    }
+    return (false);
+}
+
 void
 CommunicationState6::clearRejectedLeaseUpdates() {
     rejected_clients_.clear();

src/hooks/dhcp/high_availability/communication_state.h

@@ -306,6 +306,17 @@ public:
     /// otherwise.
     virtual bool reportRejectedLeaseUpdate(const boost::shared_ptr<dhcp::Pkt>& message) = 0;
 
+    /// @brief Marks the lease update successful.
+    ///
+    /// If the lease update was previously marked "in conflict", it is
+    /// now cleared, effectively lowering the number of conflicted leases.
+    ///
+    /// @param message DHCP message for which the lease update was
+    /// successful.
+    /// @return true when the lease was marked "in conflict" and it is
+    /// now cleared.
+    virtual bool reportSuccessfulLeaseUpdate(const boost::shared_ptr<dhcp::Pkt>& message) = 0;
+
     /// @brief Clears rejected client leases.
     virtual void clearRejectedLeaseUpdates() = 0;
 
@@ -690,6 +701,17 @@ public:
     /// otherwise.
     virtual bool reportRejectedLeaseUpdate(const boost::shared_ptr<dhcp::Pkt>& message);
 
+    /// @brief Marks the lease update successful.
+    ///
+    /// If the lease update was previously marked "in conflict", it is
+    /// now cleared, effectively lowering the number of conflicted leases.
+    ///
+    /// @param message DHCP message for which the lease update was
+    /// successful.
+    /// @return true when the lease was marked "in conflict" and it is
+    /// now cleared.
+    virtual bool reportSuccessfulLeaseUpdate(const boost::shared_ptr<dhcp::Pkt>& message);
+
     /// @brief Clears rejected client leases.
     virtual void clearRejectedLeaseUpdates();
 
@@ -869,6 +891,17 @@ public:
     /// otherwise.
     virtual bool reportRejectedLeaseUpdate(const boost::shared_ptr<dhcp::Pkt>& message);
 
+    /// @brief Marks the lease update successful.
+    ///
+    /// If the lease update was previously marked "in conflict", it is
+    /// now cleared, effectively lowering the number of conflicted leases.
+    ///
+    /// @param message DHCP message for which the lease update was
+    /// successful.
+    /// @return true when the lease was marked "in conflict" and it is
+    /// now cleared.
+    virtual bool reportSuccessfulLeaseUpdate(const boost::shared_ptr<dhcp::Pkt>& message);
+
     /// @brief Clears rejected client leases.
     virtual void clearRejectedLeaseUpdates();
 

src/hooks/dhcp/high_availability/ha_service.cc

@@ -1436,10 +1436,22 @@ 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) && !lease_update_conflict && !lease_update_success) {
+            if (config->getRole() != HAConfig::PeerConfig::BACKUP) {
-                // If we were unable to communicate with the partner we set partner's
+                // If the lease update was unsuccessful we may need to set the partner
                 // state as unavailable.
-                communication_state_->setPartnerState("unavailable");
+                if (!lease_update_success) {
+                    // Do not set it as unavailable if it was a conflict because the
+                    // partner actually responded.
+                    if (!lease_update_conflict) {
+                        // If we were unable to communicate with the partner we set partner's
+                        // state as unavailable.
+                        communication_state_->setPartnerState("unavailable");
+                    }
+                } else if (communication_state_->getRejectedLeaseUpdatesCount() > 0) {
+                    // Lease update successful and we may need to clear some previously
+                    // rejected lease updates.
+                    communication_state_->reportSuccessfulLeaseUpdate(query);
+                }
             }
 
             // It is possible to configure the server to not wait for a response from

src/hooks/dhcp/high_availability/tests/communication_state_unittest.cc

@@ -114,6 +114,10 @@ public:
     /// @brief Test that gathering rejected leases works fine in DHCPv4 case.
     void reportRejectedLeasesV4Test();
 
+    /// @brief Test that rejected leases are cleared after reporting respective
+    /// successful leases.
+    void reportSuccessfulLeasesV4Test();
+
     /// @brief Test checking that invalid values not accepted when reporting
     /// rejected leases.
     void reportRejectedLeasesV4InvalidValuesTest();
@@ -121,6 +125,10 @@ public:
     /// @brief Test that gathering rejected leases works fine in DHCPv4 case.
     void reportRejectedLeasesV6Test();
 
+    /// @brief Test that rejected leases are cleared after reporting respective
+    /// successful leases.
+    void reportSuccessfulLeasesV6Test();
+
     /// @brief Test checking that invalid values not accepted when reporting
     /// rejected leases.
     void reportRejectedLeasesV6InvalidValuesTest();
@@ -711,20 +719,43 @@ CommunicationStateTest::reportRejectedLeasesV4Test() {
     state_.reportRejectedLeaseUpdate(msg);
     EXPECT_EQ(2, state_.getRejectedLeaseUpdatesCount());
 
-    msg = createMessage4(DHCPREQUEST, 2, 0, 0);
+    msg = createMessage4(DHCPREQUEST, 2, 1, 0);
     state_.reportRejectedLeaseUpdate(msg);
-    EXPECT_EQ(2, state_.getRejectedLeaseUpdatesCount());
+    EXPECT_EQ(3, state_.getRejectedLeaseUpdatesCount());
 
     state_.clearRejectedLeaseUpdates();
     EXPECT_EQ(0, state_.getRejectedLeaseUpdatesCount());
 }
 
+void
+CommunicationStateTest::reportSuccessfulLeasesV4Test() {
+    EXPECT_EQ(0, state_.getRejectedLeaseUpdatesCount());
+    auto msg0 = createMessage4(DHCPREQUEST, 1, 0, 0);
+    state_.reportRejectedLeaseUpdate(msg0);
+    EXPECT_EQ(1, state_.getRejectedLeaseUpdatesCount());
+
+    auto msg1 = createMessage4(DHCPREQUEST, 2, 0, 0);
+    state_.reportRejectedLeaseUpdate(msg1);
+    EXPECT_EQ(2, state_.getRejectedLeaseUpdatesCount());
+
+    EXPECT_TRUE(state_.reportSuccessfulLeaseUpdate(msg0));
+    EXPECT_EQ(1, state_.getRejectedLeaseUpdatesCount());
+
+    auto msg2 = createMessage4(DHCPREQUEST, 1, 1, 0);
+    EXPECT_FALSE(state_.reportSuccessfulLeaseUpdate(msg2));
+    EXPECT_EQ(1, state_.getRejectedLeaseUpdatesCount());
+
+    EXPECT_TRUE(state_.reportSuccessfulLeaseUpdate(msg1));
+    EXPECT_EQ(0, state_.getRejectedLeaseUpdatesCount());
+}
+
 void
 CommunicationStateTest::reportRejectedLeasesV4InvalidValuesTest() {
     // Using DHCPv6 message in the DHCPv4 context is a programming
     // error and deserves an exception.
     auto msg = createMessage6(DHCPV6_REQUEST, 1, 0);
     EXPECT_THROW(state_.reportRejectedLeaseUpdate(msg), BadValue);
+    EXPECT_THROW(state_.reportSuccessfulLeaseUpdate(msg), BadValue);
 }
 
 void
@@ -746,16 +777,40 @@ CommunicationStateTest::reportRejectedLeasesV6Test() {
     EXPECT_EQ(0, state6_.getRejectedLeaseUpdatesCount());
 }
 
+void
+CommunicationStateTest::reportSuccessfulLeasesV6Test() {
+    EXPECT_EQ(0, state6_.getRejectedLeaseUpdatesCount());
+    auto msg0 = createMessage6(DHCPV6_SOLICIT, 1, 0);
+    EXPECT_TRUE(state6_.reportRejectedLeaseUpdate(msg0));
+    EXPECT_EQ(1, state6_.getRejectedLeaseUpdatesCount());
+
+    auto msg1 = createMessage6(DHCPV6_SOLICIT, 2, 0);
+    EXPECT_TRUE(state6_.reportRejectedLeaseUpdate(msg1));
+    EXPECT_EQ(2, state6_.getRejectedLeaseUpdatesCount());
+
+    EXPECT_TRUE(state6_.reportSuccessfulLeaseUpdate(msg0));
+    EXPECT_EQ(1, state6_.getRejectedLeaseUpdatesCount());
+
+    auto msg2 = createMessage6(DHCPV6_SOLICIT, 3, 0);
+    EXPECT_FALSE(state6_.reportSuccessfulLeaseUpdate(msg2));
+    EXPECT_EQ(1, state6_.getRejectedLeaseUpdatesCount());
+
+    EXPECT_TRUE(state6_.reportSuccessfulLeaseUpdate(msg1));
+    EXPECT_EQ(0, state6_.getRejectedLeaseUpdatesCount());
+}
+
 void
 CommunicationStateTest::reportRejectedLeasesV6InvalidValuesTest() {
     // Using DHCPv4 message in the DHCPv6 context is a programming
     // error and deserves an exception.
     auto msg0 = createMessage4(DHCPREQUEST, 1, 1, 0);
     EXPECT_THROW(state6_.reportRejectedLeaseUpdate(msg0), BadValue);
+    EXPECT_THROW(state6_.reportSuccessfulLeaseUpdate(msg0), BadValue);
 
     auto msg1 = createMessage6(DHCPV6_SOLICIT, 1, 0);
     msg1->delOption(D6O_CLIENTID);
     EXPECT_FALSE(state6_.reportRejectedLeaseUpdate(msg1));
+    EXPECT_FALSE(state6_.reportSuccessfulLeaseUpdate(msg1));
 }
 
 TEST_F(CommunicationStateTest, partnerStateTest) {
@@ -911,6 +966,15 @@ TEST_F(CommunicationStateTest, reportRejectedLeasesV4TestMultiThreading) {
     reportRejectedLeasesV4Test();
 }
 
+TEST_F(CommunicationStateTest, reportSuccessfulLeasesV4Test) {
+    reportSuccessfulLeasesV4Test();
+}
+
+TEST_F(CommunicationStateTest, reportSuccessfulLeasesV4TestMultiThreading) {
+    MultiThreadingMgr::instance().setMode(true);
+    reportSuccessfulLeasesV4Test();
+}
+
 TEST_F(CommunicationStateTest, reportRejectedLeasesV4InvalidValuesTest) {
     reportRejectedLeasesV4InvalidValuesTest();
 }
@@ -929,6 +993,15 @@ TEST_F(CommunicationStateTest, reportRejectedLeasesV6TestMultiThreading) {
     reportRejectedLeasesV6Test();
 }
 
+TEST_F(CommunicationStateTest, reportSuccessfulLeasesV6Test) {
+    reportSuccessfulLeasesV6Test();
+}
+
+TEST_F(CommunicationStateTest, reportSuccessfulLeasesV6TestMultiThreading) {
+    MultiThreadingMgr::instance().setMode(true);
+    reportSuccessfulLeasesV6Test();
+}
+
 TEST_F(CommunicationStateTest, reportRejectedLeasesV6InvalidValuesTest) {
     reportRejectedLeasesV6InvalidValuesTest();
 }

src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc

@@ -802,22 +802,14 @@ public:
     /// @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.
+    /// @param create_service a boolean flag indicating whether the test should
+    /// re-create HA service and communication state.
     void testSendLeaseUpdates(std::function<void()> unpark_handler,
                               const bool should_fail,
                               const size_t num_updates,
                               const MyState& my_state = MyState(HA_LOAD_BALANCING_ST),
-                              const bool wait_backup_ack = false) {
+                              const bool wait_backup_ack = false,
-        // Create HA configuration for 3 servers. This server is
+                              const bool create_service = true) {
-        // server 1.
-        HAConfigPtr config_storage = createValidConfiguration();
-        config_storage->setWaitBackupAck(wait_backup_ack);
-        // Let's override the default value. The lower value makes it easier
-        // for some unit tests to simulate the server's overflow. Simulating it
-        // requires appending leases to the backlog. It is easier to add 10
-        // than 100.
-        config_storage->setDelayedUpdatesLimit(10);
-        setBasicAuth(config_storage);
-
         // Create parking lot where query is going to be parked and unparked.
         ParkingLotPtr parking_lot(new ParkingLot());
         ParkingLotHandlePtr parking_lot_handle(new ParkingLotHandle(parking_lot));
@@ -840,22 +832,36 @@ public:
                                             60, 0, 1));
         deleted_leases4->push_back(deleted_lease4);
 
-        // The communication state is the member of the HAServce object. We have to
+        if (create_service) {
-        // replace this object with our own implementation to have an ability to
+            // Create HA configuration for 3 servers. This server is
-        // modify its poke time.
+            // server 1.
-        NakedCommunicationState4Ptr state(new NakedCommunicationState4(io_service_,
+            HAConfigPtr config_storage = createValidConfiguration();
-                                                                       config_storage));
+            config_storage->setWaitBackupAck(wait_backup_ack);
-        // Set poke time 30s in the past. If the state is poked it will be reset
+            // Let's override the default value. The lower value makes it easier
-        // to the current time. This allows for testing whether the object has been
+            // for some unit tests to simulate the server's overflow. Simulating it
-        // poked by the HA service.
+            // requires appending leases to the backlog. It is easier to add 10
-        state->modifyPokeTime(-30);
+            // than 100.
+            config_storage->setDelayedUpdatesLimit(10);
+            setBasicAuth(config_storage);
 
-        // Create HA service and schedule lease updates.
+            // The communication state is the member of the HAServce object. We have to
-        createSTService(network_state_, config_storage);
+            // replace this object with our own implementation to have an ability to
-        service_->communication_state_ = state;
+            // modify its poke time.
+            NakedCommunicationState4Ptr state(new NakedCommunicationState4(io_service_,
+                                                                       config_storage));
+            // Set poke time 30s in the past. If the state is poked it will be reset
+            // to the current time. This allows for testing whether the object has been
+            // poked by the HA service.
+            state->modifyPokeTime(-30);
+
+            // Create HA service.
+            createSTService(network_state_, config_storage);
+            service_->communication_state_ = state;
+        }
 
         service_->transition(my_state.state_, HAService::NOP_EVT);
 
+        // Schedule lease updates.
         EXPECT_EQ(num_updates,
                   service_->asyncSendLeaseUpdates(query, leases4, deleted_leases4,
                                                   parking_lot_handle));
@@ -892,9 +898,9 @@ public:
         EXPECT_TRUE(factory_->getResponseCreator()->getReceivedRequests().empty());
 
         if (should_fail) {
-            EXPECT_EQ(HA_UNAVAILABLE_ST, state->getPartnerState());
+            EXPECT_EQ(HA_UNAVAILABLE_ST, service_->communication_state_->getPartnerState());
         } else {
-            EXPECT_NE(state->getPartnerState(), HA_UNAVAILABLE_ST);
+            EXPECT_NE(service_->communication_state_->getPartnerState(), HA_UNAVAILABLE_ST);
         }
     }
 
@@ -908,17 +914,14 @@ public:
     /// @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.
+    /// @param create_service a boolean flag indicating whether the test should
+    /// re-create HA service and communication state.
     void testSendLeaseUpdates6(std::function<void()> unpark_handler,
                                const bool should_fail,
                                const size_t num_updates,
                                const MyState& my_state = MyState(HA_LOAD_BALANCING_ST),
-                               const bool wait_backup_ack = false) {
+                               const bool wait_backup_ack = false,
-        // Create HA configuration for 3 servers. This server is
+                               const bool create_service = true) {
-        // server 1.
-        HAConfigPtr config_storage = createValidConfiguration();
-        config_storage->setDelayedUpdatesLimit(10);
-        config_storage->setWaitBackupAck(wait_backup_ack);
-        setBasicAuth(config_storage);
 
         // Create parking lot where query is going to be parked and unparked.
         ParkingLotPtr parking_lot(new ParkingLot());
@@ -940,22 +943,32 @@ public:
                                             duid, 1234, 50, 60, 1));
         deleted_leases6->push_back(deleted_lease6);
 
-        // The communication state is the member of the HAServce object. We have to
+        if (create_service) {
-        // replace this object with our own implementation to have an ability to
+            // Create HA configuration for 3 servers. This server is
-        // modify its poke time.
+            // server 1.
-        NakedCommunicationState6Ptr state(new NakedCommunicationState6(io_service_,
+            HAConfigPtr config_storage = createValidConfiguration();
-                                                                       config_storage));
+            config_storage->setDelayedUpdatesLimit(10);
-        // Set poke time 30s in the past. If the state is poked it will be reset
+            config_storage->setWaitBackupAck(wait_backup_ack);
-        // to the current time. This allows for testing whether the object has been
+            setBasicAuth(config_storage);
-        // poked by the HA service.
-        state->modifyPokeTime(-30);
 
-        // Create HA service and schedule lease updates.
+            // The communication state is the member of the HAServce object. We have to
-        createSTService(network_state_, config_storage, HAServerType::DHCPv6);
+            // replace this object with our own implementation to have an ability to
-        service_->communication_state_ = state;
+            // modify its poke time.
+            NakedCommunicationState6Ptr state(new NakedCommunicationState6(io_service_,
+                                                                       config_storage));
+            // Set poke time 30s in the past. If the state is poked it will be reset
+            // to the current time. This allows for testing whether the object has been
+            // poked by the HA service.
+            state->modifyPokeTime(-30);
+
+            // Create HA service.
+            createSTService(network_state_, config_storage, HAServerType::DHCPv6);
+            service_->communication_state_ = state;
+        }
 
         service_->transition(my_state.state_, HAService::NOP_EVT);
 
+        // Schedule lease updates.
         EXPECT_EQ(num_updates,
                   service_->asyncSendLeaseUpdates(query, leases6, deleted_leases6,
                                                   parking_lot_handle));
@@ -965,7 +978,8 @@ public:
         // and the deletions in a single bulk update command.
         EXPECT_EQ(num_updates, service_->getPendingRequest(query));
 
-        EXPECT_FALSE(state->isPoked());
+        EXPECT_FALSE(boost::dynamic_pointer_cast<NakedCommunicationState6>
+                     (service_->communication_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
@@ -993,7 +1007,7 @@ public:
         EXPECT_TRUE(factory_->getResponseCreator()->getReceivedRequests().empty());
 
         if (should_fail) {
-            EXPECT_EQ(HA_UNAVAILABLE_ST, state->getPartnerState());
+            EXPECT_EQ(HA_UNAVAILABLE_ST, service_->communication_state_->getPartnerState());
         }
     }
 
@@ -1451,8 +1465,8 @@ public:
         EXPECT_FALSE(delete_request3);
     }
 
-    /// @brief Tests scenarios when one of the servers to which a
+    /// @brief Test the scenario when the servers receiving a lease update
-    /// lease update is sent returns an error.
+    /// return the conflict status code.
     void testSendUpdatesControlResultConflict() {
         // Instruct the server 2 to return an error as a result of receiving a command.
         factory2_->getResponseCreator()->setControlResult(CONTROL_RESULT_CONFLICT);
@@ -1473,6 +1487,16 @@ public:
         // Ensure that the server has recorded a lease update conflict. The conflict
         // reported by the backup server should not count.
         EXPECT_EQ(1, service_->communication_state_->getRejectedLeaseUpdatesCount());
+
+        factory2_->getResponseCreator()->setControlResult(CONTROL_RESULT_SUCCESS);
+        factory3_->getResponseCreator()->setControlResult(CONTROL_RESULT_CONFLICT);
+
+        bool unpark_called = false;
+        testSendLeaseUpdates([&unpark_called] {
+            unpark_called = true;
+        }, false, 1, MyState(HA_LOAD_BALANCING_ST), true, false);
+        EXPECT_TRUE(unpark_called);
+        EXPECT_EQ(0, service_->communication_state_->getRejectedLeaseUpdatesCount());
     }
 
     /// @brief Tests scenarios when all lease updates are sent successfully.