From: Thomas Markwalder Date: Fri, 21 Apr 2017 19:28:35 +0000 (-0400) Subject: [5247] Addressed review comments X-Git-Tag: trac5243x_base~7^2 X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=a7941a73f4ce031228f0809c7557ec967d1ed45a;p=thirdparty%2Fkea.git [5247] Addressed review comments --- diff --git a/doc/guide/dhcp4-srv.xml b/doc/guide/dhcp4-srv.xml index c7b2e19195..ff3873ac9a 100644 --- a/doc/guide/dhcp4-srv.xml +++ b/doc/guide/dhcp4-srv.xml @@ -3617,20 +3617,20 @@ src/lib/dhcpsrv/cfg_host_operations.cc --> reclaimed-leases integer - This statistic shows the number of expired leases that have - have been reclaimed since server startup. It increases every time an - expired lease undergoes expiration reclamation. This statistic is reset - during reconfiguration event. + This statistic is the number of expired leases that have + been reclaimed since server startup. It is incremented each time + an expired lease is reclaimed and is reset when the server is + reconfigured. subnet[id].reclaimed-leases integer - This statistic shows the number of expired leases that have - and been reclaimed since server startup. It increases every time an expired - lease undergoes expiration reclamation. The id is - the subnet-id of the subnet. This statistic is exposed for each subnet - separately. This statistic is reset during reconfiguration event. + This statistic is the number of expired leases associated + with a given subnet (id is the subnet-id) + that have been reclaimed since server startup. It is incremented + each time an expired lease is reclaimed and is reset when the + server is reconfigured. diff --git a/doc/guide/dhcp6-srv.xml b/doc/guide/dhcp6-srv.xml index a61ba959b2..b5a815c8ed 100644 --- a/doc/guide/dhcp6-srv.xml +++ b/doc/guide/dhcp6-srv.xml @@ -3859,23 +3859,21 @@ If not specified, the default value is: reclaimed-leases integer - This statistic shows the number of expired leases that have - and been reclaimed since server startup. It increases every time an - expired lease undergoes expiration reclamation. This statistic is reset - during reconfiguration event. Note it counts both NA and PD reclamations. - This statistic is reset during reconfiguration event. + This statistic is the number of expired leases that have been + reclaimed since server startup. It is incremented each time an expired + lease is reclaimed (it counts both NA and PD reclamations) and is reset + when the server is reconfigured. subnet[id].reclaimed-leases integer - This statistic shows the number of expired leases that have - and been reclaimed since server startup. It increases every time an expired - lease undergoes expiration reclamation. The id is - the subnet-id of the subnet. This statistic is exposed for each subnet - separately. Note it counts both NA and PD reclamations. - This statistic is reset during reconfiguration event. + This statistic is the number of expired leases associated with + a given subnet ("id" is the subnet-id) that have + been reclaimed since server startup. It is incremented each time an expired + lease is reclaimed (it counts both NA and PD reclamations) and is reset + when the server is reconfigured. diff --git a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc index 00ae10ea95..44c68638ea 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc @@ -744,7 +744,8 @@ TEST_F(AllocEngine4Test, requestReuseDeclinedLease4Stats) { // Check that the stats are correct. Note that assigned-addresses does // not get decremented when a lease is declined, ergo not incremented - // when it is reused. + // when it is reused. Declined address stats will be -1 since + // lease was created as declined which does not increment the stat. EXPECT_TRUE(testStatistics("assigned-addresses", 0, subnet_->getID())); EXPECT_TRUE(testStatistics("declined-addresses", -1)); EXPECT_TRUE(testStatistics("reclaimed-declined-addresses", 1)); diff --git a/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc index 47b268db51..dc88f05305 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine6_unittest.cc @@ -70,52 +70,48 @@ TEST_F(AllocEngine6Test, constructor) { // This test checks if the simple allocation (REQUEST) can succeed // and the stats counter is properly bumped by 1 TEST_F(AllocEngine6Test, simpleAlloc6) { - + // Assigned count should be zero. + EXPECT_TRUE(testStatistics("assigned-nas", 0, subnet_->getID())); simpleAlloc6Test(pool_, IOAddress("::"), false); - // We should have bumped the address counter by 1 - string name = StatsMgr::generateName("subnet", subnet_->getID(), "assigned-nas"); - ObservationPtr stat = StatsMgr::instance().getObservation(name); - ASSERT_TRUE(stat); - EXPECT_EQ(1, stat->getInteger().first); + // We should have bumped the assigned counter by 1 + EXPECT_TRUE(testStatistics("assigned-nas", 1, subnet_->getID())); } // This test checks if the simple PD allocation (REQUEST) can succeed // and the stats counter is properly bumped by 1 TEST_F(AllocEngine6Test, pdSimpleAlloc6) { + // Assigned count should be zero. + EXPECT_TRUE(testStatistics("assigned-pds", 0, subnet_->getID())); simpleAlloc6Test(pd_pool_, IOAddress("::"), false); - // We should have bumped the address counter by 1 - string name = StatsMgr::generateName("subnet", subnet_->getID(), "assigned-pds"); - ObservationPtr stat = StatsMgr::instance().getObservation(name); - ASSERT_TRUE(stat); - EXPECT_EQ(1, stat->getInteger().first); + // We should have bumped the assigned counter by 1 + EXPECT_TRUE(testStatistics("assigned-pds", 1, subnet_->getID())); } // This test checks if the fake allocation (for SOLICIT) can succeed // and the stats counter isn't bumped TEST_F(AllocEngine6Test, fakeAlloc6) { + // Assigned count should be zero. + EXPECT_TRUE(testStatistics("assigned-nas", 0, subnet_->getID())); simpleAlloc6Test(pool_, IOAddress("::"), true); - // We should not have bumped the address counter - string name = StatsMgr::generateName("subnet", subnet_->getID(), "assigned-nas"); - ObservationPtr stat = StatsMgr::instance().getObservation(name); - ASSERT_TRUE(stat); - EXPECT_EQ(0, stat->getInteger().first); + // We should not have bumped the assigned counter. + EXPECT_TRUE(testStatistics("assigned-nas", 0, subnet_->getID())); } // This test checks if the fake PD allocation (for SOLICIT) can succeed // and the stats counter isn't bumped TEST_F(AllocEngine6Test, pdFakeAlloc6) { + // Assigned count should be zero. + EXPECT_TRUE(testStatistics("assigned-pds", 0, subnet_->getID())); + simpleAlloc6Test(pd_pool_, IOAddress("::"), true); - // We should not have bumped the address counter - string name = StatsMgr::generateName("subnet", subnet_->getID(), "assigned-pds"); - ObservationPtr stat = StatsMgr::instance().getObservation(name); - ASSERT_TRUE(stat); - EXPECT_EQ(0, stat->getInteger().first); + // We should not have bumped the assigned counter + EXPECT_TRUE(testStatistics("assigned-pds", 0, subnet_->getID())); }; // This test checks if the allocation with a hint that is valid (in range, @@ -508,6 +504,11 @@ TEST_F(AllocEngine6Test, solicitReuseExpiredLease6) { // Initialize FQDN data for the lease. initFqdn("myhost.example.com", true, true); + // Verify the none of relelvant stats are zero. + EXPECT_TRUE(testStatistics("assigned-nas", 0, subnet_->getID())); + EXPECT_TRUE(testStatistics("reclaimed-leases", 0)); + EXPECT_TRUE(testStatistics("reclaimed-leases", 0, subnet_->getID())); + // Just a different duid DuidPtr other_duid = DuidPtr(new DUID(vector(12, 0xff))); const uint32_t other_iaid = 3568; @@ -1839,6 +1840,14 @@ TEST_F(AllocEngine6Test, solicitReuseDeclinedLease6Stats) { IOAddress addr(addr_txt); initSubnet(IOAddress("2001:db8:1::"), addr, addr); + // Stats should be zero. + EXPECT_TRUE(testStatistics("assigned-nas", 0, subnet_->getID())); + EXPECT_TRUE(testStatistics("declined-addresses", 0)); + EXPECT_TRUE(testStatistics("reclaimed-declined-addresses", 0)); + EXPECT_TRUE(testStatistics("declined-addresses", 0, subnet_->getID())); + EXPECT_TRUE(testStatistics("reclaimed-declined-addresses", 0, subnet_->getID())); + + // Now create a declined lease, decline it and rewind its cltt, so it // is expired. Lease6Ptr declined = generateDeclinedLease(addr_txt, 100, -10); @@ -1856,7 +1865,7 @@ TEST_F(AllocEngine6Test, solicitReuseDeclinedLease6Stats) { EXPECT_TRUE(testStatistics("reclaimed-declined-addresses", 0, subnet_->getID())); } -// This test checks if statistics are not updated when expired declined lease +// This test checks if statistics are updated when expired declined lease // is reused when responding to REQUEST (actual allocation) TEST_F(AllocEngine6Test, requestReuseDeclinedLease6Stats) { @@ -1870,6 +1879,13 @@ TEST_F(AllocEngine6Test, requestReuseDeclinedLease6Stats) { IOAddress addr(addr_txt); initSubnet(IOAddress("2001:db8::"), addr, addr); + // Stats should be zero. + EXPECT_TRUE(testStatistics("assigned-nas", 0, subnet_->getID())); + EXPECT_TRUE(testStatistics("declined-addresses", 0)); + EXPECT_TRUE(testStatistics("reclaimed-declined-addresses", 0)); + EXPECT_TRUE(testStatistics("declined-addresses", 0, subnet_->getID())); + EXPECT_TRUE(testStatistics("reclaimed-declined-addresses", 0, subnet_->getID())); + // Now create a declined lease, decline it and rewind its cltt, so it // is expired. Lease6Ptr declined = generateDeclinedLease(addr_txt, 100, -10); @@ -1880,6 +1896,11 @@ TEST_F(AllocEngine6Test, requestReuseDeclinedLease6Stats) { testReuseLease6(engine, declined, "::", false, SHOULD_PASS, assigned); // Check that the stats were modified as expected. + // assigned-nas should NOT get incremented. Currently we do not adjust assigned + // counts when we declines + // declined-addresses will -1, as the artificial creation of declined lease + // doens't increment it from zero. reclaimed-declined-addresses will be 1 + // becuase the leases are implicitly reclaimed before they can be assigned. EXPECT_TRUE(testStatistics("assigned-nas", 0, subnet_->getID())); EXPECT_TRUE(testStatistics("declined-addresses", -1)); EXPECT_TRUE(testStatistics("reclaimed-declined-addresses", 1));