Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#3978 closed task (complete)

Implement "leases-reclaim" command

Reported by: marcin Owned by: fdupont
Priority: high Milestone: Kea1.0-beta
Component: management API Version: git
Keywords: expiration 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

See the Design for the details.

Subtickets

Change History (8)

comment:1 Changed 4 years ago by fdupont

  • Owner set to fdupont
  • Status changed from new to accepted

comment:2 follow-up: Changed 4 years ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

Almost done. Two comments:

  • I am not satisfied by the unit test I propose because it changes the statistic in a way hard to restore (resetAll() doesn't remove timestamps, perhaps I should call removeAll() in the fixture destructor?)
  • even if the design document suggests no argument IMHO the command should take a boolean remove_lease argument.

Please review it but IMHO it is not finished.

comment:3 Changed 4 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:4 in reply to: ↑ 2 ; follow-up: Changed 4 years ago by marcin

  • Owner changed from marcin to fdupont

Replying to fdupont:

Almost done. Two comments:

  • I am not satisfied by the unit test I propose because it changes the statistic in a way hard to restore (resetAll() doesn't remove timestamps, perhaps I should call removeAll() in the fixture destructor?)

The !CtrlChannelDhcpv4SrvTest destructor calls removeAll() already. The v6 counterpart could call this too.

  • even if the design document suggests no argument IMHO the command should take a boolean remove_lease argument.

Please review it but IMHO it is not finished.

I tend to agree. Please add this parameter.

This change requires a changelog entry.

Now the review of the specific changes.

src/bin/dhcp4/ctrl_dhcp4_srv.cc

Regarding the response message "Leases successfully reclaimed". I think it should rather be "Reclamation of expired leases is complete." There is no guarantee that all leases have been successfully reclaimed because the reclamation routine doesn't return errors. The message could maybe include something along the lines "Consult server log to see if there were any errors".

src/bin/dhcp4/ctrl_dhcp4_srv.h
In the description of the new handler function it should be "Handler for processing..." rather than "handler for processing..."

The description of the handler could be extended that it only supports parameter that controlls whether leases should be removed or their state is to be updated. It should mention that the limits for time and number of processed leases do not apply.

src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc
I suggest that the test uses at least two leases to make sure that it doesn't always reclaim only one expired lease.

src/bin/dhcp6/ctrl_dhcp6_srv.cc
Same comment applies as for ctrl_dhcp4_srv.cc

src/bin/dhcp6/ctrl_dhcp6_srv.h
Same comments apply here as for the v4 counterpart.

src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc
I suggest that the test uses at least two leases to make sure that it doesn't always reclaim only one expired lease.

comment:5 in reply to: ↑ 4 Changed 4 years ago by fdupont

  • Owner changed from fdupont to marcin

Replying to marcin:

Replying to fdupont:

Almost done. Two comments:

  • I am not satisfied by the unit test I propose because it changes the statistic in a way hard to restore (resetAll() doesn't remove timestamps, perhaps I should call removeAll() in the fixture destructor?)

The !CtrlChannelDhcpv4SrvTest destructor calls removeAll() already. The v6 counterpart could call this too.

=> yes, I concluded to the same.

  • even if the design document suggests no argument IMHO the command should take a boolean remove_lease argument.

Please review it but IMHO it is not finished.

I tend to agree. Please add this parameter.

=> done. Perhaps the design document should be updated (the documentation which its own ticket must be).

This change requires a changelog entry.

=> what about Added a new 'leases-reclaim' command which reclaims expired leases immediately.

Now the review of the specific changes.

src/bin/dhcp4/ctrl_dhcp4_srv.cc

Regarding the response message "Leases successfully reclaimed". I think it should rather be "Reclamation of expired leases is complete." There is no guarantee that all leases have been successfully reclaimed because the reclamation routine doesn't return errors. The message could maybe include something along the lines "Consult server log to see if there were any errors".

=> I changed the message.

src/bin/dhcp4/ctrl_dhcp4_srv.h
In the description of the new handler function it should be "Handler for processing..." rather than "handler for processing..."

=> I agree (the @brief is a meta element).

The description of the handler could be extended that it only supports parameter that controlls whether leases should be removed or their state is to be updated. It should mention that the limits for time and number of processed leases do not apply.

=> done.

src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc
I suggest that the test uses at least two leases to make sure that it doesn't always reclaim only one expired lease.

=> done. I added a similar test for the remove=false case.

src/bin/dhcp6/ctrl_dhcp6_srv.cc
Same comment applies as for ctrl_dhcp4_srv.cc

src/bin/dhcp6/ctrl_dhcp6_srv.h
Same comments apply here as for the v4 counterpart.

src/bin/dhcp6/tests/ctrl_dhcp6_srv_unittest.cc
I suggest that the test uses at least two leases to make sure that it doesn't always reclaim only one expired lease.

=> done.

Ready for another round of review.

comment:6 Changed 4 years ago by marcin

  • Owner changed from marcin to fdupont

Reviewed e03cf770b34dfaa962cdb888917d7a46b7001b4f

All ok, including the changelog. Please merge.

As for the design update, you're right, it will need to be updated. I do have a ticket to update the design document post implementation, so it will be addressed.

comment:7 Changed 4 years ago by fdupont

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

Merged. Closing.

comment:8 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.