From: Marcin Siodelski Date: Tue, 28 Apr 2020 19:51:03 +0000 (+0200) Subject: [#999] Added support for wait-backup-ack in HA X-Git-Tag: Kea-1.7.8~124 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ea8d375e3bc1ff184e1b83296ace64b0c8a82916;p=thirdparty%2Fkea.git [#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 6a0950592f..b3a4931ce2 100644 --- 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 c9a0ccaa96..3fb2685d5c 100644 --- 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 e98f2a5305..123531f1dd 100644 --- 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 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 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) {