Opened 7 months ago

Closed 7 months ago

#5247 closed defect (fixed)

assigned-leases does not increment when expired leases are renewed/reused

Reported by: tmark Owned by: tmark
Priority: high Milestone: Kea1.2-final
Component: Unclassified Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Investigation of a kea-uers email,
https://lists.isc.org/pipermail/kea-users/2017-April/000925.html

Led to the discovery that the v4 server does not increment assigned-leases when expired leases are renewed (same client) or reused (different client), meanwhile we decrement assigned-leases each time we expire a lease, thus rendering assigned-leases stat wrong.

Further testing shows that the v6 server suffers from this same problem for assigned-nas and assigned-pds.

I also discovered that reclaimed-leases is not documented and should be.

Subtickets

Change History (9)

comment:1 Changed 7 months ago by tmark

  • Status changed from new to assigned

comment:2 Changed 7 months ago by tmark

  • Status changed from assigned to accepted

comment:3 Changed 7 months ago by tmark

  • Owner changed from tmark to Unassigned
  • Status changed from accepted to reviewing

The changes to correct this were not that large though a bit painstaking. The bulk of the changes are in unit tests, which may still be lacking some cases. I did also discover secondary issues:

  1. V4 server was not setting the lease state back to default when renewing an expired reclaimed lease for a returning client. The client is issued the lease but it will never expire as its state is stuck at STATE_EXPIRED_RECLAIMED. I was able to fix this.
  1. V6 server has a similar issue but only when the returning client starts over with a SOLICIT rather than renew/rebind. This fix is more complicated and will be done under it's own ticket, #5252.

ChangeLog?:

The assigned lease statistics were not being probably adjusted by either kea-dhcp4 or kea-dhcp6 when reclaimed expired leases were reissued.

Ticket is ready for review.

comment:4 Changed 7 months ago by stephen

  • Owner changed from Unassigned to stephen

comment:5 follow-up: Changed 7 months ago by stephen

  • Owner changed from stephen to tmark

Reviewed commit fe3766aedd24a402ad08e4323b6f9668dfce878c

dhcp4-srv.xml
Suggest some text changes:

reclaimed-leases:
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:
This statistic is the number of expired leases associated with a given subnet ("id" is the the subnet) 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.

(Being pedantic, these should be "counters" not "statistics". According to Wikipedia" "A statistic (singular) or sample statistic is a single measure of some attribute of a sample (e.g., its arithmetic mean value). It is calculated by applying a function (statistical algorithm) to the values of the items of the sample, which are known together as a set of data.". In this case, no function is being applied to the data - it is a raw counter. It's something we can change later.)

dhcp4-srv.xml
Suggest some text changes:

reclaimed-leases:
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:
This statistic is the number of expired leases associated with a given subnet ("id" is the the subnet) 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.

alloc_engine4_unittest.cc
requestReuseDeclinedLease4Stats test: it's not clear why the declined-addresses counter is -1. An addition to the comment preceding the EXPECT_TRUE block would help.

alloc_engine6_unittest.cc
simpleAlloc6 test
fakeAlloc6 test
solictReuseExpiredLease6 test
The assumption is that the counters are initialised to zero before the test. In a set of tests elsewhere in the file, that is explicitly tested: I think it would be more accurate to do so here as well.

solictReuseExpiredLease6 test
requestReuseDeclinedLease6Stats
It is not clear why one of the counters is expected to be -1. (Actually, I can see it, but had to read the test in detail. Even then, there is the implicit assumption that the counters are zero at the start of the test.)

comment:6 in reply to: ↑ 5 Changed 7 months ago by tmark

Replying to stephen:

Reviewed commit fe3766aedd24a402ad08e4323b6f9668dfce878c

dhcp4-srv.xml
Suggest some text changes:

reclaimed-leases:
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:
This statistic is the number of expired leases associated with a given subnet ("id" is the the subnet) 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.

(Being pedantic, these should be "counters" not "statistics". According to Wikipedia" "A statistic (singular) or sample statistic is a single measure of some attribute of a sample (e.g., its arithmetic mean value). It is calculated by applying a function (statistical algorithm) to the values of the items of the sample, which are known together as a set of data.". In this case, no function is being applied to the data - it is a raw counter. It's something we can change later.)

dhcp4-srv.xml
Suggest some text changes:

reclaimed-leases:
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:
This statistic is the number of expired leases associated with a given subnet ("id" is the the subnet) 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.

Wording changed as suggested (mostly). In my defense I was attempting to stay with wording similar to what was around it.

alloc_engine4_unittest.cc
requestReuseDeclinedLease4Stats test: it's not clear why the declined-addresses counter is -1. An addition to the comment preceding the EXPECT_TRUE block would help.

Added.

alloc_engine6_unittest.cc
simpleAlloc6 test
fakeAlloc6 test
solictReuseExpiredLease6 test
The assumption is that the counters are initialised to zero before the test. In a set of tests elsewhere in the file, that is explicitly tested: I think it would be more accurate to do so here as well.

Done.

solictReuseExpiredLease6 test
requestReuseDeclinedLease6Stats
It is not clear why one of the counters is expected to be -1. (Actually, I can see it, but had to read the test in detail. Even then, there is the implicit assumption that the counters are zero at the start of the test.)


Done.

Please re-review.

comment:7 Changed 7 months ago by tmark

  • Owner changed from tmark to stephen

comment:8 Changed 7 months ago by stephen

  • Owner changed from stephen to tmark

Reviewed commit a7941a73f4ce031228f0809c7557ec967d1ed45a

All OK, please merge.

comment:9 Changed 7 months ago by tmark

  • Resolution set to fixed
  • Status changed from reviewing to closed

Changes merged with git 4e5193fb32b14325ccf5824614e58bcceb3e6388
Added ChangeLog? entry 1249.

Ticket is complete.

Note: See TracTickets for help on using tickets.