[#3125] Pkt6 drop statistics not increased

The drop statistics should be maintained by the hook libraries rather than
the server. We had a discrepancy between the DHCPv4 and DHCPv6 server where
the latter increased the drop statistics when the hook library returned
the DROP status for the pkt6-receive or buffer6-receive callout. It
resulted in the increased drop stats for the load balanced packets. It was
not the case for the DHCPv4 server.

diff --git a/src/bin/dhcp4/tests/hooks_unittest.cc b/src/bin/dhcp4/tests/hooks_unittest.cc
index 63ed6a152f3ab224ac68c966801528c9c0ef45d8..41f8661b8256682419721e644c120bcd581776fd 100644 (file)
--- a/src/bin/dhcp4/tests/hooks_unittest.cc
+++ b/src/bin/dhcp4/tests/hooks_unittest.cc
@@ -1324,6 +1324,9 @@ TEST_F(HooksDhcpv4SrvTest, buffer4ReceiveDrop) {
 
     // Check if the callout handle state was reset after the callout.
     checkCalloutHandleReset(discover);
+
+    // Drop statistics should be maintained by the callouts, not by the server.
+    EXPECT_EQ(0, getStatistic("pkt4-receive-drop"));
 }
 
 // Checks if callouts installed on pkt4_receive are indeed called and the
@@ -1496,6 +1499,9 @@ TEST_F(HooksDhcpv4SrvTest, pkt4ReceiveDrop) {
 
     // Check if the callout handle state was reset after the callout.
     checkCalloutHandleReset(discover);
+
+    // Drop statistics should be maintained by the callouts, not by the server.
+    EXPECT_EQ(0, getStatistic("pkt4-receive-drop"));
 }
 
 // Checks if callouts installed on pkt4_send are indeed called and the
@@ -2030,6 +2036,9 @@ TEST_F(HooksDhcpv4SrvTest, subnet4SelectDrop) {
 
     // Check if the callout handle state was reset after the callout.
     checkCalloutHandleReset(discover);
+
+    // Drop statistics should be maintained by the callouts, not by the server.
+    EXPECT_EQ(0, getStatistic("pkt4-receive-drop"));
 }
 
 // This test verifies that the leases4_committed hook point is not triggered

diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc
index 85338837e9904510bf7c599229cb599d65ce64a2..a0facd7923f57768c127f7d7eba9278cd3a5d03e 100644 (file)
--- a/src/bin/dhcp6/dhcp6_srv.cc
+++ b/src/bin/dhcp6/dhcp6_srv.cc
@@ -786,9 +786,11 @@ Dhcpv6Srv::processPacket(Pkt6Ptr query) {
                 .arg(query->getLocalAddr().toText())
                 .arg(query->getIface());
 
-            // Increase the statistic of dropped packets.
-            StatsMgr::instance().addValue("pkt6-receive-drop",
-                                          static_cast<int64_t>(1));
+            // Not increasing the statistics of the dropped packets because it
+            // is the callouts' responsibility to increase it. There are some
+            // cases when the callouts may elect to not increase the statistics.
+            // For example, packets dropped by the load-balancing algorithm must
+            // not increase the statistics.
             return (Pkt6Ptr());
         }
 
@@ -900,9 +902,11 @@ Dhcpv6Srv::processPacket(Pkt6Ptr query) {
             (callout_handle->getStatus() == CalloutHandle::NEXT_STEP_DROP)) {
             LOG_DEBUG(hooks_logger, DBG_DHCP6_HOOKS, DHCP6_HOOK_PACKET_RCVD_SKIP)
                 .arg(query->getLabel());
-            // Increase the statistic of dropped packets.
-            StatsMgr::instance().addValue("pkt6-receive-drop",
-                                          static_cast<int64_t>(1));
+            // Not increasing the statistics of the dropped packets because it
+            // is the callouts' responsibility to increase it. There are some
+            // cases when the callouts may elect to not increase the statistics.
+            // For example, packets dropped by the load-balancing algorithm must
+            // not increase the statistics.
             return (Pkt6Ptr());
         }
 

diff --git a/src/bin/dhcp6/tests/hooks_unittest.cc b/src/bin/dhcp6/tests/hooks_unittest.cc
index 7ee5e1f6b61eda0ea25740f71723dfdf3a9e09cf..c093de6c6a627e8ebc56e2a1ec42065490e3b1fc 100644 (file)
--- a/src/bin/dhcp6/tests/hooks_unittest.cc
+++ b/src/bin/dhcp6/tests/hooks_unittest.cc
@@ -1244,6 +1244,9 @@ TEST_F(HooksDhcpv6SrvTest, buffer6ReceiveDrop) {
 
     // Check if the callout handle state was reset after the callout.
     checkCalloutHandleReset(sol);
+
+    // Drop statistics should be maintained by the callouts, not by the server.
+    EXPECT_EQ(0, getStatistic("pkt6-receive-drop"));
 }
 
 // Checks if callouts installed on pkt6_receive are indeed called and the
@@ -1406,6 +1409,9 @@ TEST_F(HooksDhcpv6SrvTest, pkt6ReceiveDrop) {
 
     // Check if the callout handle state was reset after the callout.
     checkCalloutHandleReset(sol);
+
+    // Drop statistics should be maintained by the callouts, not by the server.
+    EXPECT_EQ(0, getStatistic("pkt6-receive-drop"));
 }
 
 // Checks if callouts installed on pkt6_send are indeed called and the
@@ -1907,6 +1913,9 @@ TEST_F(HooksDhcpv6SrvTest, subnet6SelectDrop) {
 
     // Check if the callout handle state was reset after the callout.
     checkCalloutHandleReset(sol);
+
+    // Drop statistics should be maintained by the callouts, not by the server.
+    EXPECT_EQ(0, getStatistic("pkt6-receive-drop"));
 }
 
 // This test verifies that the leases6_committed hook point is not triggered