From: Thomas Markwalder Date: Mon, 5 Apr 2021 11:25:49 +0000 (-0400) Subject: [#1733] kea-dhcp6 now proactively parks packets X-Git-Tag: Kea-1.9.7~80 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2bf7d39ee18e2df006f2bafa216a00dd0ab58430;p=thirdparty%2Fkea.git [#1733] kea-dhcp6 now proactively parks packets src/bin/dhcp6/dhcp6_srv.cc Dhcpv6Srv::processDhcp6Query() - modified to proactively park packets. src/hooks/dhcp/high_availability/ha_impl.cc HAImpl::leases4Committed() HAImpl::leases6Committed() - revised to reference before the call to asynSendLeases() and dereference if it returns 0. src/hooks/dhcp/high_availability/libloadtests/close_unittests.cc src/hooks/dhcp/high_availability/tests/ha_impl_unittest.cc src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc Revised tests to park first as needed. src/lib/hooks/parking_lots.h Fixed a typo in throw messages. --- diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 1173b6cb4d..3b758bd9c2 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -965,12 +965,10 @@ Dhcpv6Srv::processDhcp6Query(Pkt6Ptr& query, Pkt6Ptr& rsp) { rsp->setIndex(query->getIndex()); rsp->setIface(query->getIface()); - bool packet_park = false; - + CalloutHandlePtr callout_handle = getCalloutHandle(query); if (!ctx.fake_allocation_ && (ctx.query_->getType() != DHCPV6_CONFIRM) && (ctx.query_->getType() != DHCPV6_INFORMATION_REQUEST) && HooksManager::calloutsPresent(Hooks.hook_index_leases6_committed_)) { - CalloutHandlePtr callout_handle = getCalloutHandle(query); // Use the RAII wrapper to make sure that the callout handle state is // reset when this object goes out of scope. All hook points must do @@ -1020,34 +1018,10 @@ Dhcpv6Srv::processDhcp6Query(Pkt6Ptr& query, Pkt6Ptr& rsp) { } callout_handle->setArgument("deleted_leases6", deleted_leases); - // Call all installed callouts - HooksManager::callCallouts(Hooks.hook_index_leases6_committed_, - *callout_handle); - - if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP) { - LOG_DEBUG(hooks_logger, DBG_DHCP6_HOOKS, - DHCP6_HOOK_LEASES6_COMMITTED_DROP) - .arg(query->getLabel()); - rsp.reset(); - - } else if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_PARK) { - packet_park = true; - } - } - - if (!rsp) { - return; - } - - // PARKING SPOT after leases6_committed hook point. - CalloutHandlePtr callout_handle = getCalloutHandle(query); - if (packet_park) { - LOG_DEBUG(hooks_logger, DBG_DHCP6_HOOKS, - DHCP6_HOOK_LEASES6_COMMITTED_PARK) - .arg(query->getLabel()); - - // Park the packet. The function we bind here will be executed when the hook - // library unparks the packet. + // We proactively park the packet. We'll unpark it without invoking + // the callback (i.e. drop) unless the callout status is set to + // NEXT_STEP_PARK. Otherwise the callback we bind here will be + // executed when the hook library unparks the packet. HooksManager::park("leases6_committed", query, [this, callout_handle, query, rsp]() mutable { if (MultiThreadingMgr::instance().getMode()) { @@ -1062,12 +1036,37 @@ Dhcpv6Srv::processDhcp6Query(Pkt6Ptr& query, Pkt6Ptr& rsp) { } }); - // If we have parked the packet, let's reset the pointer to the - // response to indicate to the caller that it should return, as - // the packet processing will continue via the callback. - rsp.reset(); + try { + // Call all installed callouts + HooksManager::callCallouts(Hooks.hook_index_leases6_committed_, + *callout_handle); + } catch(...) { + // Make sure we don't orphan a parked packet. + HooksManager::drop("leases4_committed", query); + throw; + } - } else { + if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_PARK) { + LOG_DEBUG(hooks_logger, DBG_DHCP6_HOOKS, DHCP6_HOOK_LEASES6_COMMITTED_PARK) + .arg(query->getLabel()); + // Since the hook library(ies) are going to do the unparking, then + // reset the pointer to the response to indicate to the caller that + // it should return, as the packet processing will continue via + // the callback. + rsp.reset(); + } else { + // Drop the park job on the packet, it isn't needed. + HooksManager::drop("leases6_committed", query); + if (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP) { + LOG_DEBUG(hooks_logger, DBG_DHCP6_HOOKS, DHCP6_HOOK_LEASES6_COMMITTED_DROP) + .arg(query->getLabel()); + rsp.reset(); + } + } + } + + // If we have a response prep it for shipment. + if (rsp) { processPacketPktSend(callout_handle, query, rsp); } } diff --git a/src/hooks/dhcp/high_availability/ha_impl.cc b/src/hooks/dhcp/high_availability/ha_impl.cc index 6ed0568109..06182202d1 100644 --- a/src/hooks/dhcp/high_availability/ha_impl.cc +++ b/src/hooks/dhcp/high_availability/ha_impl.cc @@ -148,7 +148,7 @@ HAImpl::leases4Committed(CalloutHandle& callout_handle) { return; } - // The callout returns this status code to indicate to the server that it + // The callout returns this status code to indicate to the server that it // should leave the packet parked. It will be unparked until each hook // library with a reference, unparks the packet. callout_handle.setStatus(CalloutHandle::NEXT_STEP_PARK); @@ -242,22 +242,23 @@ HAImpl::leases6Committed(CalloutHandle& callout_handle) { // pointer until we unpark the packet. ParkingLotHandlePtr parking_lot = callout_handle.getParkingLotHandlePtr(); + // Create a reference to the parked packet. This signals that we have a + // stake in unparking it. + parking_lot->reference(query6); + // Asynchronously send lease updates. In some cases no updates will be sent, // e.g. when this server is in the partner-down state and there are no backup // servers. In those cases we simply return without parking the DHCP query. // The response will be sent to the client immediately. if (service_->asyncSendLeaseUpdates(query6, leases6, deleted_leases6, parking_lot) == 0) { + // Dereference the parked packet. This releases our stake in it. + parking_lot->dereference(query6); return; } - // This is required step every time we ask the server to park the packet. - // The reference counting is required to keep the packet parked until - // all callouts call unpark. Then, the packet gets unparked and the - // associated callback is triggered. The callback resumes packet processing. - parking_lot->reference(query6); - // The callout returns this status code to indicate to the server that it - // should park the query packet. + // should leave the packet parked. It will be unparked until each hook + // library with a reference, unparks the packet. callout_handle.setStatus(CalloutHandle::NEXT_STEP_PARK); } diff --git a/src/hooks/dhcp/high_availability/libloadtests/close_unittests.cc b/src/hooks/dhcp/high_availability/libloadtests/close_unittests.cc index e82811a2d4..67e27b1009 100644 --- a/src/hooks/dhcp/high_availability/libloadtests/close_unittests.cc +++ b/src/hooks/dhcp/high_availability/libloadtests/close_unittests.cc @@ -400,6 +400,11 @@ TEST_F(CloseHATest, close4) { CalloutHandlePtr handle = query->getCalloutHandle(); ScopedCalloutHandleState handle_state(handle); handle->setArgument("query4", query); + + // Park the packtet proactively as the server normally would. + HooksManager::park("leases4_committed", query, []{}); + + // Invoke the callouts. HooksManager::callCallouts(testHooks.hook_index_buffer4_receive_, *handle); EXPECT_EQ(CalloutHandle::NEXT_STEP_SKIP, handle->getStatus()); @@ -633,6 +638,11 @@ TEST_F(CloseHATest, close6) { CalloutHandlePtr handle = query->getCalloutHandle(); ScopedCalloutHandleState handle_state(handle); handle->setArgument("query6", query); + + // Park the packtet proactively as the server normally would. + HooksManager::park("leases6_committed", query, []{}); + + // Invoke the callouts. HooksManager::callCallouts(testHooks.hook_index_buffer6_receive_, *handle); EXPECT_EQ(CalloutHandle::NEXT_STEP_SKIP, handle->getStatus()); 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 f7e75a870a..fa09162dea 100644 --- a/src/hooks/dhcp/high_availability/tests/ha_impl_unittest.cc +++ b/src/hooks/dhcp/high_availability/tests/ha_impl_unittest.cc @@ -17,6 +17,7 @@ #include #include #include +#include #include #include #include @@ -31,6 +32,29 @@ using namespace isc::hooks; namespace { +/// @brief Structure that holds registered hook indexes. +/// +/// This allows us to park packets. +struct TestHooks { + /// @brief Index of leases4_committed callout. + int hook_index_leases4_committed_; + + /// @brief Index of leases6_committed callout. + int hook_index_leases6_committed_; + + /// @brief Constructor + /// + /// The constructor registers hook points for callout tests. + TestHooks() { + hook_index_leases4_committed_ = + HooksManager::registerHook("leases4_committed"); + hook_index_leases6_committed_ = + HooksManager::registerHook("leases6_committed"); + } +}; + +TestHooks test_hooks; + /// @brief Derivation of the @c HAImpl which provides access to protected /// methods and members. class TestHAImpl : public HAImpl { @@ -331,6 +355,9 @@ TEST_F(HAImplTest, leases4Committed) { CalloutHandlePtr callout_handle = HooksManager::createCalloutHandle(); ASSERT_TRUE(callout_handle); + // Set the hook index so we can park packets. + callout_handle->setCurrentHook(test_hooks.hook_index_leases4_committed_); + // query4 Pkt4Ptr query4 = createMessage4(DHCPREQUEST, 1, 0, 0); callout_handle->setArgument("query4", query4); @@ -346,12 +373,15 @@ TEST_F(HAImplTest, leases4Committed) { // Set initial status. callout_handle->setStatus(CalloutHandle::NEXT_STEP_CONTINUE); + // Park the packet. + HooksManager::park("leases4_committed", query4, []{}); + // There are no leases so the callout should return. ASSERT_NO_THROW(ha_impl.leases4Committed(*callout_handle)); // No updates are generated so the default status should not be modified. EXPECT_EQ(CalloutHandle::NEXT_STEP_CONTINUE, callout_handle->getStatus()); - EXPECT_FALSE(callout_handle->getParkingLotHandlePtr()->drop(query4)); + EXPECT_TRUE(callout_handle->getParkingLotHandlePtr()->drop(query4)); // Create a lease and pass it to the callout, but temporarily disable lease // updates. @@ -364,17 +394,25 @@ TEST_F(HAImplTest, leases4Committed) { ha_impl.config_->setSendLeaseUpdates(false); + // Park the packet. + HooksManager::park("leases4_committed", query4, []{}); + // Run the callout again. ASSERT_NO_THROW(ha_impl.leases4Committed(*callout_handle)); // No updates are generated so the default status should not be modified. EXPECT_EQ(CalloutHandle::NEXT_STEP_CONTINUE, callout_handle->getStatus()); - EXPECT_FALSE(callout_handle->getParkingLotHandlePtr()->drop(query4)); + EXPECT_TRUE(callout_handle->getParkingLotHandlePtr()->drop(query4)); // Enable updates and retry. ha_impl.config_->setSendLeaseUpdates(true); callout_handle->setArgument("leases4", leases4); - ASSERT_NO_THROW(ha_impl.leases4Committed(*callout_handle)); + + // Park the packet. + HooksManager::park("leases4_committed", query4, []{}); + + // Run the callout again. + ASSERT_NO_THROW_LOG(ha_impl.leases4Committed(*callout_handle)); // This time the lease update should be generated and the status should // be set to "park". @@ -403,6 +441,9 @@ TEST_F(HAImplTest, leases6Committed) { CalloutHandlePtr callout_handle = HooksManager::createCalloutHandle(); ASSERT_TRUE(callout_handle); + // Set the hook index so we can park packets. + callout_handle->setCurrentHook(test_hooks.hook_index_leases6_committed_); + // query6 Pkt6Ptr query6 = createMessage6(DHCPV6_REQUEST, 1, 0); callout_handle->setArgument("query6", query6); @@ -418,12 +459,15 @@ TEST_F(HAImplTest, leases6Committed) { // Set initial status. callout_handle->setStatus(CalloutHandle::NEXT_STEP_CONTINUE); + // Park the packet. + HooksManager::park("leases6_committed", query6, []{}); + // There are no leases so the callout should return. ASSERT_NO_THROW(ha_impl.leases6Committed(*callout_handle)); // No updates are generated so the default status should not be modified. EXPECT_EQ(CalloutHandle::NEXT_STEP_CONTINUE, callout_handle->getStatus()); - EXPECT_FALSE(callout_handle->getParkingLotHandlePtr()->drop(query6)); + EXPECT_TRUE(callout_handle->getParkingLotHandlePtr()->drop(query6)); // Create a lease and pass it to the callout, but temporarily disable lease // updates. @@ -435,16 +479,23 @@ TEST_F(HAImplTest, leases6Committed) { ha_impl.config_->setSendLeaseUpdates(false); + // Park the packet. + HooksManager::park("leases6_committed", query6, []{}); + // Run the callout again. ASSERT_NO_THROW(ha_impl.leases6Committed(*callout_handle)); // No updates are generated so the default status should not be modified. EXPECT_EQ(CalloutHandle::NEXT_STEP_CONTINUE, callout_handle->getStatus()); - EXPECT_FALSE(callout_handle->getParkingLotHandlePtr()->drop(query6)); + EXPECT_TRUE(callout_handle->getParkingLotHandlePtr()->drop(query6)); // Enable updates and retry. ha_impl.config_->setSendLeaseUpdates(true); callout_handle->setArgument("leases6", leases6); + + // Park the packet. + HooksManager::park("leases6_committed", query6, []{}); + ASSERT_NO_THROW(ha_impl.leases6Committed(*callout_handle)); // This time the lease update should be generated and the status should 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 94002c91f5..995ea3b047 100644 --- a/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc +++ b/src/hooks/dhcp/high_availability/tests/ha_service_unittest.cc @@ -837,14 +837,14 @@ public: EXPECT_FALSE(state->isPoked()); - ASSERT_NO_THROW(parking_lot->reference(query)); - // 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 // because the parked packet should be dropped as a result of lease updates // failures. ASSERT_NO_THROW(parking_lot->park(query, unpark_handler)); + ASSERT_NO_THROW(parking_lot->reference(query)); + // Actually perform the lease updates. ASSERT_NO_THROW(runIOService(TEST_TIMEOUT, [this]() { // Finish running IO service when there are no more pending requests. @@ -939,14 +939,14 @@ public: EXPECT_FALSE(state->isPoked()); - ASSERT_NO_THROW(parking_lot->reference(query)); - // 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 // because the parked packet should be dropped as a result of lease updates // failures. ASSERT_NO_THROW(parking_lot->park(query, unpark_handler)); + ASSERT_NO_THROW(parking_lot->reference(query)); + // Actually perform the lease updates. ASSERT_NO_THROW(runIOService(TEST_TIMEOUT, [this]() { // Finish running IO service when there are no more pending requests. diff --git a/src/lib/hooks/parking_lots.h b/src/lib/hooks/parking_lots.h index 96a58ffa91..a16593ad10 100644 --- a/src/lib/hooks/parking_lots.h +++ b/src/lib/hooks/parking_lots.h @@ -108,7 +108,7 @@ public: auto it = find(parked_object); if (it == parking_.end()) { isc_throw(InvalidOperation, "cannot reference an object" - "that has not been parked."); + " that has not been parked."); } // Bump and return the reference count @@ -129,7 +129,7 @@ public: auto it = find(parked_object); if (it == parking_.end()) { isc_throw(InvalidOperation, "cannot dereference an object" - "that has not been parked."); + " that has not been parked."); } // Decrement and return the reference count.