Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#3984 closed enhancement (complete)

Automated declined v4 lease recovery

Reported by: tomek Owned by: tomek
Priority: high Milestone: Kea1.0-beta
Component: dhcp4 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 (last modified by tomek)

Declined v4 lease should be recovered after probation time elapses. For details see DeclineDesign.
This code will be used from #3976.

Subtickets

Change History (7)

comment:1 Changed 5 years ago by tomek

  • Summary changed from Declined v4 lease recovery to Automated declined v4 lease recovery

comment:2 Changed 5 years ago by tomek

  • Owner set to tomek
  • Status changed from new to assigned

comment:3 Changed 5 years ago by tomek

  • Description modified (diff)
  • Owner changed from tomek to UnAssigned
  • Status changed from assigned to reviewing

The code is now ready for review. This also covers #3976.

Proposed ChangeLog?:

10XX.	[func]		tomek
	Expired declined leases can now be reclaimed (returned to the
	available pool) after probation	period elapses.
	(Trac #3984, #3976, git tbd)

comment:4 Changed 5 years ago by fdupont

  • Owner changed from UnAssigned to fdupont

comment:5 follow-up: Changed 5 years ago by fdupont

  • Owner changed from fdupont to tomek

alloc_engine.h: misplaced comment (fixed, please pull)

alloc_engine.cc:

  • reclaimExpiredLeases4() IMHO a comment should be added before the new declined block.
  • reclaimDeclined(): I don't know if it is already in guidelines but IMHO .arg(xxx) in logging code should be stacked, i.e., one per line (easier to ready and BTW easier to count too).

alloc_engine_messages.mes: should get an English language expert opinion but IMHO the second "now" in

This time now has elapsed and the address is now returned to available pool.

should be removed.

alloc_engine4_unittest.cc: resued -> reused (fixed, please pull)

alloc_engine_expiration_unittest.cc:
for (int i; -> for (unsigned int i; (twice)

alloc_engine_utils.h:

  • testReuseLease4(): missing doxygen param for fake_allocation
  • generateDeclinedLease(): IMHO had expired -> expired (and I suggest become -> stay)

alloc_engine_utils.{h,cc}: testReuseLease4() ambiguity (perhaps from a rename) between old_lease and expired_lease in comments in both files.

alloc_engine_utils.cc:

  • for my culture why using a temporary variable for hardware address but not for client ID?
  • IMHO now should replace the second call to time(NULL) or a comment explain why current time should be taken again.

generic_lease_mgr_unittest.cc: testGetDeclinedLeases4():

  • please revisit the Mark every other lease as expired. comment as it doesn't reflect the code.
  • IMHO decline_state and default_state counters should not be signed.
  • please add an ASSERT_XXX before [2 * index] to check we are still in the range. (3 times)

Note all these points are minor and Jenkins happy so review comments and merge.

comment:6 in reply to: ↑ 5 Changed 5 years ago by tomek

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

Replying to fdupont:

alloc_engine.h: misplaced comment (fixed, please pull)

Thanks.

alloc_engine.cc:

  • reclaimExpiredLeases4() IMHO a comment should be added before the new declined block.

Comment added.

  • reclaimDeclined(): I don't know if it is already in guidelines but IMHO .arg(xxx) in logging code should be stacked, i.e., one per line (easier to ready and BTW easier to count too).

I don't think it's in the guidelines, but updated the code as suggested.

alloc_engine_messages.mes: should get an English language expert opinion but IMHO the second "now" in

This time now has elapsed and the address is now returned to available pool.

should be removed.

Removed.

alloc_engine4_unittest.cc: resued -> reused (fixed, please pull)

Thanks.

alloc_engine_expiration_unittest.cc:
for (int i; -> for (unsigned int i; (twice)

All counters are now unsigned.

alloc_engine_utils.h:

  • testReuseLease4(): missing doxygen param for fake_allocation

Added.

  • generateDeclinedLease(): IMHO had expired -> expired (and I suggest become -> stay)

Updated.

alloc_engine_utils.{h,cc}: testReuseLease4() ambiguity (perhaps from a rename) between old_lease and expired_lease in comments in both files.

Corrected.

alloc_engine_utils.cc:

  • for my culture why using a temporary variable for hardware address but not for client ID?

The difference is that client-id is set to NULL, while the hardware address is set to an object that represents empty hardware address. The distinction is there because there's an unknown, but large number of places in the code that assume that hardware address is always there.

  • IMHO now should replace the second call to time(NULL) or a comment explain why current time should be taken again.

Relaced as suggested.

generic_lease_mgr_unittest.cc: testGetDeclinedLeases4():

  • please revisit the Mark every other lease as expired. comment as it doesn't reflect the code.

Updated the comment.

  • IMHO decline_state and default_state counters should not be signed.

They are unsigned now.

  • please add an ASSERT_XXX before [2 * index] to check we are still in the range. (3 times)

Added.

Note all these points are minor and Jenkins happy so review comments and merge.

Great. Thanks a lot for your review. Code merged. Closing this ticket and #3976.

comment:7 Changed 4 years ago by tomek

  • Milestone changed from Kea1.0 to Kea1.0-beta

Milestone renamed

Note: See TracTickets for help on using tickets.