Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#3988 closed enhancement (complete)

Implement lease4_decline_recover hook

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Kea1.0-beta
Component: hooks 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

For details, see DeclineDesign.

Subtickets

Change History (10)

comment:1 Changed 4 years ago by tomek

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

comment:2 Changed 4 years ago by tomek

  • Owner changed from tomek to UnAssigned
  • Status changed from assigned to reviewing

The code is now ready for review. Changes cover both v4 (#3988) and v6 (#3989) as they are very similar and it's easier to implement and review them together.

Note: I simplified the hook name. It's lease4_recover rather than planned lead4_decline_recover.

Proposed ChangeLog? entry:

10XX.	[func]		tomek
	Two new hook points lease4_recover and lease6_recover have been
	implemented. They are called when a declined IPv4 or IPv6 lease
	concludes its probation period and is being recovered into
	unsable state.
	(Trac #3988, 3989, git tbd)

Switching both tickets to review state.

comment:3 Changed 4 years ago by fdupont

  • Owner changed from UnAssigned to fdupont

dhcp6_hooks.dox: dhcpv6HooksLease4Recover -> dhcpv6HooksLease6Recover and in its description IPv4 -> IPv6

comment:4 Changed 4 years ago by fdupont

  • Owner changed from fdupont to tomek

AllocEngine::reclaimDeclined(const Lease4Ptr& lease): lease6_recover (in comments) -> 4

The Let's clear any garbage... at the end of ExpirationAllocEngine6Test is not in ExpirationAllocEngine4Test.

ExpirationAllocEngine4Test reclaimDeclinedHook1 and 2: lease6_recover -> 4

I have to run the code but from reading there were only some 6/4 mismatches in comments.

comment:5 Changed 4 years ago by fdupont

A problem to solve:

[ RUN      ] ExpirationAllocEngine6Test.reclaimDeclined2
alloc_engine_expiration_unittest.cc:987: Failure
Value of: testLeases(&leaseDoesntExist, &evenLeaseIndex)
  Actual: false
Expected: true
[  FAILED  ] ExpirationAllocEngine6Test.reclaimDeclined2 (2176 ms)

on OS X 10.11 Xcode 7.0.1.

comment:6 Changed 4 years ago by fdupont

Found the issue:

reclaimExpiredLeases6 has:

                bool remove_tmp = remove_lease;
                if (lease->state_ == Lease::STATE_DECLINED) {
                    // There's no point in keeping declined lease after its
                    // reclaimation. Declined lease doesn't have any client
                    // identifying information anymore.
                    remove_tmp = true;

                    // Do extra steps required for declined lease reclaimation:
                    // - bump decline-related stats
                    // - log separate message
                    // - call lease6_recover hooks
                    // (hooks can override the removal decision and keep the lease)
                    remove_tmp = reclaimDeclined(lease) && remove_lease;
                }

and reclaimExpiredLeases4 has

                bool remove_tmp = remove_lease;
                if (lease->state_ == Lease::STATE_DECLINED) {
                    // There's no point in keeping declined lease after its
                    // reclaimation. Declined lease doesn't have any client
                    // identifying information anymore.
                    remove_tmp = true;

                    // Do extra steps required for declined lease reclaimation:
                    // - bump decline-related stats
                    // - log separate message
                    remove_tmp = reclaimDeclined(lease) && remove_tmp;
                }

Only the second (with && remove_tmp is correct but I have a concern for both about the style: I suggest to simply assign remove_tmp inside the if to the return of reclaimDeclined. And merge comments.

comment:7 Changed 4 years ago by tomek

  • Owner changed from tomek to fdupont

Implemented all changes you proposed. Can you confirm that the unit-tests are now passing?

comment:8 Changed 4 years ago by fdupont

  • Owner changed from fdupont to tomek

Tests are good. Ready for merge.

comment:9 Changed 4 years ago by tomek

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

Thanks for your review. Code merged. Closing #3988 and #3989.

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