From: Marcin Siodelski Date: Wed, 29 Apr 2020 11:38:35 +0000 (+0200) Subject: [#999] wait-backup-ack not enabled in active/active X-Git-Tag: Kea-1.7.8~121 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0df1f43421593923000db30176013165ae704f59;p=thirdparty%2Fkea.git [#999] wait-backup-ack not enabled in active/active It is not allowed to set wait-backup-ack to true when the server operates in the active/active mode. The unit tests were adjusted accordingly. --- diff --git a/src/hooks/dhcp/high_availability/ha_config.cc b/src/hooks/dhcp/high_availability/ha_config.cc index 97ff57fbf1..c340564924 100644 --- a/src/hooks/dhcp/high_availability/ha_config.cc +++ b/src/hooks/dhcp/high_availability/ha_config.cc @@ -294,6 +294,11 @@ HAConfig::validate() const { " balancing configuration"); } + // In the load-balancing mode the wait-backup-ack must be false. + if (wait_backup_ack_) { + isc_throw(HAConfigValidationError, "'wait-backup-ack' must be set to false in the" + " 'load-balancing' mode"); + } } if (ha_mode_ == HOT_STANDBY) { @@ -314,6 +319,12 @@ HAConfig::validate() const { isc_throw(HAConfigValidationError, "primary server required in the hot" " standby configuration"); } + + // In the hot-standby mode the wait-backup-ack must be false. + if (wait_backup_ack_) { + isc_throw(HAConfigValidationError, "'wait-backup-ack' must be set to false in the" + " 'hot-standby' mode"); + } } } diff --git a/src/hooks/dhcp/high_availability/tests/ha_config_unittest.cc b/src/hooks/dhcp/high_availability/tests/ha_config_unittest.cc index ef17e8a2ab..0fcb37c48a 100644 --- a/src/hooks/dhcp/high_availability/tests/ha_config_unittest.cc +++ b/src/hooks/dhcp/high_availability/tests/ha_config_unittest.cc @@ -73,7 +73,7 @@ TEST_F(HAConfigTest, configureLoadBalancing) { " \"max-response-delay\": 11," " \"max-ack-delay\": 5," " \"max-unacked-clients\": 20," - " \"wait-backup-ack\": true," + " \"wait-backup-ack\": false," " \"peers\": [" " {" " \"name\": \"server1\"," @@ -124,7 +124,7 @@ TEST_F(HAConfigTest, configureLoadBalancing) { EXPECT_EQ(11, impl->getConfig()->getMaxResponseDelay()); EXPECT_EQ(5, impl->getConfig()->getMaxAckDelay()); EXPECT_EQ(20, impl->getConfig()->getMaxUnackedClients()); - EXPECT_TRUE(impl->getConfig()->amWaitingBackupAck()); + EXPECT_FALSE(impl->getConfig()->amWaitingBackupAck()); HAConfig::PeerConfigPtr cfg = impl->getConfig()->getThisServerConfig(); ASSERT_TRUE(cfg); @@ -953,6 +953,34 @@ TEST_F(HAConfigTest, duplicatedStates) { "duplicated configuration for the 'waiting' state"); } +// Test that wait-backup-ack must not be enabled in the configuration +// with two active servers. +TEST_F(HAConfigTest, waitBackupAckWithActiveServers) { + testInvalidConfig( + "[" + " {" + " \"this-server-name\": \"server1\"," + " \"mode\": \"hot-standby\"," + " \"wait-backup-ack\": true," + " \"peers\": [" + " {" + " \"name\": \"server1\"," + " \"url\": \"http://127.0.0.1:8080/\"," + " \"role\": \"primary\"," + " \"auto-failover\": false" + " }," + " {" + " \"name\": \"server2\"," + " \"url\": \"http://127.0.0.1:8081/\"," + " \"role\": \"standby\"," + " \"auto-failover\": true" + " }" + " ]" + " }" + "]", + "'wait-backup-ack' must be set to false in the 'hot-standby' mode"); +} + // Test that conversion of the role names works correctly. TEST_F(HAConfigTest, stringToRole) { EXPECT_EQ(HAConfig::PeerConfig::PRIMARY, diff --git a/src/hooks/dhcp/high_availability/tests/ha_impl_unittest.cc b/src/hooks/dhcp/high_availability/tests/ha_impl_unittest.cc index 0c7321d26b..39713c7cef 100644 --- a/src/hooks/dhcp/high_availability/tests/ha_impl_unittest.cc +++ b/src/hooks/dhcp/high_availability/tests/ha_impl_unittest.cc @@ -321,6 +321,11 @@ TEST_F(HAImplTest, leases4Committed) { ASSERT_NO_THROW(ha_impl.startService(io_service_, network_state, HAServerType::DHCPv4)); + // Make sure we wait for the acks from the backup server to be able to + // test the case of sending lease updates even though the service is + // in the state in which the lease updates are normally not sent. + ha_impl.config_->setWaitBackupAck(true); + // Create callout handle to be used for passing arguments to the // callout. CalloutHandlePtr callout_handle = HooksManager::createCalloutHandle(); @@ -388,6 +393,11 @@ TEST_F(HAImplTest, leases6Committed) { ASSERT_NO_THROW(ha_impl.startService(io_service_, network_state, HAServerType::DHCPv6)); + // Make sure we wait for the acks from the backup server to be able to + // test the case of sending lease updates even though the service is + // in the state in which the lease updates are normally not sent. + ha_impl.config_->setWaitBackupAck(true); + // Create callout handle to be used for passing arguments to the // callout. CalloutHandlePtr callout_handle = HooksManager::createCalloutHandle(); 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 964a4dc538..c788a219af 100644 --- a/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc +++ b/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc @@ -666,7 +666,7 @@ public: const bool should_pass, const size_t num_updates, const MyState& my_state = MyState(HA_LOAD_BALANCING_ST), - const bool wait_backup_ack = true) { + const bool wait_backup_ack = false) { // Create HA configuration for 3 servers. This server is // server 1. HAConfigPtr config_storage = createValidConfiguration(); @@ -714,11 +714,16 @@ 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]); + if (num_updates == 0) { + EXPECT_TRUE((service.pending_requests_.count(query) == 0) || + (service.pending_requests_[query] == 0)); + } else { + // 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()); @@ -736,9 +741,13 @@ public: return (service.pending_requests_.empty()); })); - // Try to drop the packet. We expect that the packet has been already - // dropped so this should return false. - EXPECT_FALSE(parking_lot_handle->drop(query)); + // Only if we wait for lease updates to complete it makes senst to test + // that the packet was either dropped or unparked. + if (num_updates > 0) { + // Try to drop the packet. We expect that the packet has been already + // dropped so this should return false. + EXPECT_FALSE(parking_lot_handle->drop(query)); + } // The updates should not be sent to this server. EXPECT_TRUE(factory_->getResponseCreator()->getReceivedRequests().empty()); @@ -764,7 +773,7 @@ public: const bool should_pass, const size_t num_updates, const MyState& my_state = MyState(HA_LOAD_BALANCING_ST), - const bool wait_backup_ack = true) { + const bool wait_backup_ack = false) { // Create HA configuration for 3 servers. This server is // server 1. HAConfigPtr config_storage = createValidConfiguration(); @@ -810,10 +819,15 @@ 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]); + if (num_updates == 0) { + EXPECT_TRUE((service.pending_requests_.count(query) == 0) || + (service.pending_requests_[query] == 0)); + } else { + // 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()); @@ -831,9 +845,13 @@ public: return (service.pending_requests_.empty()); })); - // Try to drop the packet. We expect that the packet has been already - // dropped so this should return false. - EXPECT_FALSE(parking_lot_handle->drop(query)); + // Only if we wait for lease updates to complete it makes senst to test + // that the packet was either dropped or unparked. + if (num_updates > 0) { + // Try to drop the packet. We expect that the packet has been already + // dropped so this should return false. + EXPECT_FALSE(parking_lot_handle->drop(query)); + } // The updates should not be sent to this server. EXPECT_TRUE(factory_->getResponseCreator()->getReceivedRequests().empty()); @@ -1180,7 +1198,7 @@ TEST_F(HAServiceTest, sendSuccessfulUpdates) { bool unpark_called = false; testSendLeaseUpdates([&unpark_called] { unpark_called = true; - }, true, 2); + }, true, 1); // Expecting that the packet was unparked because lease updates are expected // to be successful. @@ -1213,8 +1231,7 @@ TEST_F(HAServiceTest, sendSuccessfulUpdates) { EXPECT_TRUE(delete_request3); } -// Test scenario when lease updates are sent successfully to the backup server -// and not sent to the failover peer when this server is in patrtner-down state. +// Test scenario when lease updates are not sent to the failover peer. TEST_F(HAServiceTest, sendUpdatesPartnerDown) { // Start HTTP servers. ASSERT_NO_THROW({ @@ -1227,11 +1244,12 @@ TEST_F(HAServiceTest, sendUpdatesPartnerDown) { bool unpark_called = false; testSendLeaseUpdates([&unpark_called] { unpark_called = true; - }, false, 1, MyState(HA_PARTNER_DOWN_ST)); + }, false, 0, MyState(HA_PARTNER_DOWN_ST)); - // Expecting that the packet was unparked because lease updates are expected - // to be successful. - EXPECT_TRUE(unpark_called); + // There were no lease updates for which we have been waiting to complete so + // the packet was never unparked. Note that in such situation the packet is + // not parked either. + EXPECT_FALSE(unpark_called); // Server 2 should not receive lease4-update. auto update_request2 = factory2_->getResponseCreator()->findRequest("lease4-update", @@ -1242,19 +1260,6 @@ TEST_F(HAServiceTest, sendUpdatesPartnerDown) { auto delete_request2 = factory2_->getResponseCreator()->findRequest("lease4-del", "192.2.3.4"); EXPECT_FALSE(delete_request2); - - // Lease updates should be successfully sent to server3. - EXPECT_EQ(2, factory3_->getResponseCreator()->getReceivedRequests().size()); - - // Check that the server 3 has received lease4-update command. - auto update_request3 = factory3_->getResponseCreator()->findRequest("lease4-update", - "192.1.2.3"); - EXPECT_TRUE(update_request3); - - // Check that the server 3 has received lease4-del command. - auto delete_request3 = factory3_->getResponseCreator()->findRequest("lease4-del", - "192.2.3.4"); - EXPECT_TRUE(delete_request3); } // Test scenario when one of the servers to which updates are sent is offline. @@ -1268,7 +1273,7 @@ TEST_F(HAServiceTest, sendUpdatesActiveServerOffline) { testSendLeaseUpdates([] { ADD_FAILURE() << "unpark function called but expected that the packet" " is dropped"; - }, false, 2); + }, false, 1); // Server 2 should not receive lease4-update. auto update_request2 = factory2_->getResponseCreator()->findRequest("lease4-update", @@ -1279,19 +1284,6 @@ TEST_F(HAServiceTest, sendUpdatesActiveServerOffline) { auto delete_request2 = factory2_->getResponseCreator()->findRequest("lease4-del", "192.2.3.4"); EXPECT_FALSE(delete_request2); - - // Lease updates should be successfully sent to server3. - EXPECT_EQ(2, factory3_->getResponseCreator()->getReceivedRequests().size()); - - // Check that the server 3 has received lease4-update command. - auto update_request3 = factory3_->getResponseCreator()->findRequest("lease4-update", - "192.1.2.3"); - EXPECT_TRUE(update_request3); - - // Check that the server 3 has received lease4-del command. - auto delete_request3 = factory3_->getResponseCreator()->findRequest("lease4-del", - "192.2.3.4"); - EXPECT_TRUE(delete_request3); } // Test scenario when one of the servers to which updates are sent is offline. @@ -1305,7 +1297,7 @@ TEST_F(HAServiceTest, sendUpdatesBackupServerOffline) { bool unpark_called = false; testSendLeaseUpdates([&unpark_called] { unpark_called = true; - }, true, 1, MyState(HA_LOAD_BALANCING_ST), false); + }, true, 1); EXPECT_TRUE(unpark_called); @@ -1333,47 +1325,6 @@ 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) { @@ -1390,7 +1341,7 @@ TEST_F(HAServiceTest, sendUpdatesControlResultError) { testSendLeaseUpdates([] { ADD_FAILURE() << "unpark function called but expected that the packet" " is dropped"; - }, false, 2); + }, false, 1); // The updates should be sent to server 2 and this server should return error code. EXPECT_EQ(2, factory2_->getResponseCreator()->getReceivedRequests().size()); @@ -1432,7 +1383,7 @@ TEST_F(HAServiceTest, sendSuccessfulUpdates6) { bool unpark_called = false; testSendLeaseUpdates6([&unpark_called] { unpark_called = true; - }, true, 2); + }, true, 1); // Expecting that the packet was unparked because lease updates are expected // to be successful. @@ -1457,8 +1408,7 @@ TEST_F(HAServiceTest, sendSuccessfulUpdates6) { EXPECT_TRUE(update_request3); } -// Test scenario when lease updates are sent successfully to the backup server -// and not sent to the failover peer when this server is in patrtner-down state. +// Test scenario when lease updates are not sent to the failover peer. TEST_F(HAServiceTest, sendUpdatesPartnerDown6) { // Start HTTP servers. ASSERT_NO_THROW({ @@ -1471,26 +1421,18 @@ TEST_F(HAServiceTest, sendUpdatesPartnerDown6) { bool unpark_called = false; testSendLeaseUpdates6([&unpark_called] { unpark_called = true; - }, false, 1, MyState(HA_PARTNER_DOWN_ST)); + }, false, 0, MyState(HA_PARTNER_DOWN_ST)); - // Expecting that the packet was unparked because lease updates are expected - // to be successful. - EXPECT_TRUE(unpark_called); + // There were no lease updates for which we have been waiting to complete so + // the packet was never unparked. Note that in such situation the packet is + // not parked either. + EXPECT_FALSE(unpark_called); // Server 2 should not receive lease6-bulk-apply. auto update_request2 = factory2_->getResponseCreator()->findRequest("lease6-bulk-apply", "2001:db8:1::cafe", "2001:db8:1::efac"); EXPECT_FALSE(update_request2); - - // Lease updates should be successfully sent to server3. - EXPECT_EQ(1, factory3_->getResponseCreator()->getReceivedRequests().size()); - - // Check that the server 3 has received lease6-bulk-apply command. - auto update_request3 = factory3_->getResponseCreator()->findRequest("lease6-bulk-apply", - "2001:db8:1::cafe", - "2001:db8:1::efac"); - EXPECT_TRUE(update_request3); } // Test scenario when one of the servers to which updates are sent is offline. @@ -1504,22 +1446,13 @@ TEST_F(HAServiceTest, sendUpdatesActiveServerOffline6) { testSendLeaseUpdates6([] { ADD_FAILURE() << "unpark function called but expected that the packet" " is dropped"; - }, false, 2); + }, false, 1); // Server 2 should not receive lease6-bulk-apply. auto update_request2 = factory2_->getResponseCreator()->findRequest("lease6-bulk-apply", "2001:db8:1::cafe", "2001:db8:1::efac"); EXPECT_FALSE(update_request2); - - // Lease updates should be successfully sent to server3. - EXPECT_EQ(1, factory3_->getResponseCreator()->getReceivedRequests().size()); - - // Check that the server 3 has received lease6-bulk-apply command. - auto update_request3 = factory3_->getResponseCreator()->findRequest("lease6-bulk-apply", - "2001:db8:1::cafe", - "2001:db8:1::efac"); - EXPECT_TRUE(update_request3); } // Test scenario when one of the servers to which updates are sent is offline. @@ -1533,7 +1466,7 @@ TEST_F(HAServiceTest, sendUpdatesBackupServerOffline6) { bool unpark_called = false; testSendLeaseUpdates6([&unpark_called] { unpark_called = true; - }, true, 1, MyState(HA_LOAD_BALANCING_ST), false); + }, true, 1); EXPECT_TRUE(unpark_called); @@ -1553,39 +1486,6 @@ 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) { @@ -1602,7 +1502,7 @@ TEST_F(HAServiceTest, sendUpdatesControlResultError6) { testSendLeaseUpdates6([] { ADD_FAILURE() << "unpark function called but expected that the packet" " is dropped"; - }, false, 2); + }, false, 1); // The updates should be sent to server 2 and this server should return error code. EXPECT_EQ(1, factory2_->getResponseCreator()->getReceivedRequests().size()); @@ -1612,15 +1512,6 @@ TEST_F(HAServiceTest, sendUpdatesControlResultError6) { "2001:db8:1::cafe", "2001:db8:1::efac"); EXPECT_TRUE(update_request2); - - // Lease updates should be successfully sent to server3. - EXPECT_EQ(1, factory3_->getResponseCreator()->getReceivedRequests().size()); - - // Check that the server 3 has received lease6-bulk-apply command. - auto update_request3 = factory3_->getResponseCreator()->findRequest("lease6-bulk-apply", - "2001:db8:1::cafe", - "2001:db8:1::efac"); - EXPECT_TRUE(update_request3); } // This test verifies that the server accepts the response to the lease6-bulk-apply @@ -1670,7 +1561,7 @@ TEST_F(HAServiceTest, sendUpdatesFailedLeases6) { bool unpark_called = false; testSendLeaseUpdates6([&unpark_called] { unpark_called = true; - }, true, 2); + }, true, 1); // Expecting that the packet was unparked because lease updates are expected // to be successful. @@ -1684,15 +1575,6 @@ TEST_F(HAServiceTest, sendUpdatesFailedLeases6) { "2001:db8:1::cafe", "2001:db8:1::efac"); EXPECT_TRUE(update_request2); - - // Lease updates should be successfully sent to server3. - EXPECT_EQ(1, factory3_->getResponseCreator()->getReceivedRequests().size()); - - // Check that the server 3 has received lease6-bulk-apply command. - auto update_request3 = factory3_->getResponseCreator()->findRequest("lease6-bulk-apply", - "2001:db8:1::cafe", - "2001:db8:1::efac"); - EXPECT_TRUE(update_request3); } // This test verifies that the heartbeat command is processed successfully. diff --git a/src/hooks/dhcp/high_availability/tests/ha_test.cc b/src/hooks/dhcp/high_availability/tests/ha_test.cc index f5d2b62218..a6fee33c25 100644 --- a/src/hooks/dhcp/high_availability/tests/ha_test.cc +++ b/src/hooks/dhcp/high_availability/tests/ha_test.cc @@ -154,7 +154,7 @@ HATest::createValidJsonConfiguration(const HAConfig::HAMode& ha_mode) const { " \"max-response-delay\": 1000," " \"max-ack-delay\": 10000," " \"max-unacked-clients\": 10," - " \"wait-backup-ack\": true," + " \"wait-backup-ack\": false," " \"peers\": [" " {" " \"name\": \"server1\","