Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#3987 closed enhancement (complete)

Implement lease6_decline 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 (8)

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

Code is now ready for review. Branched from trac3499.

Proposed ChangeLog:

10XX.	[func]		tomek
	A new hook point lease4_decline has been added. It is called when
	the DHCPv4 server is about to decline a lease as a result of
	processing incoming DECLINE message.
	(Trac #3986, git tbd)

comment:3 Changed 4 years ago by fdupont

  • Owner changed from UnAssigned to fdupont

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

  • Owner changed from fdupont to tomek

There are some DHCPv4 changes in this ticket. As #3987 and #3986 should be merged together I have no concern.

dhcp6_hooks.dox lease6_decline subsection: request -> DECLINE message

dhcp6_messages.mes DHCP6_HOOK_DECLINE_SKIP: set status to DROP -> set status to SKIP

In Dhcpv6Srv::declineLease (suggestion): upper layers -> callers

New HooksDhcpv6SrvTest tests don't follow the file style for test names but as they follow the general style I have no concern. Note a low priority ticket should be created to fix already existing names.

ChangeLog? should be updated (4->6, #3986 -> #3987) or simply (so better) merged with the #3986 one.

Jenkins finished (with success) #3986 check, I've launched it for this ticket. Here only the message concern is not about comments, so there is still no reason to not merge.

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

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

Replying to fdupont:

Argh: Jenkins raised 3 errors related to this ticket, cf https://jenkins.isc.org/view/Kea_BuildFarm-developers/job/Fedora22_32_branch/8/parsed_console/

=> I found the bug and push the fix into:
c0d4109..b6849af trac3987 -> trac3987

So please review my fix...

comment:7 in reply to: ↑ 4 Changed 4 years ago by tomek

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

Replying to fdupont:

There are some DHCPv4 changes in this ticket. As #3987 and #3986 should be merged together I have no concern.

dhcp6_hooks.dox lease6_decline subsection: request -> DECLINE message

Updated.

dhcp6_messages.mes DHCP6_HOOK_DECLINE_SKIP: set status to DROP -> set status to SKIP

Updated.

In Dhcpv6Srv::declineLease (suggestion): upper layers -> callers

Renamed as suggested.

New HooksDhcpv6SrvTest tests don't follow the file style for test names but as they follow the general style I have no concern. Note a low priority ticket should be created to fix already existing names.

trac is so slow these days that I decided to rename the tests. It was faster than dealing with another ticket.

ChangeLog? should be updated (4->6, #3986 -> #3987) or simply (so better) merged with the #3986 one.

Oops. Updated.

Jenkins finished (with success) #3986 check, I've launched it for this ticket. Here only the message concern is not about comments, so there is still no reason to not merge.

I reviewed your fix. Looks good. Thanks.

There was also one other fix required in the DHCPv4 tests (lease manager was destroyed in Dhcpv4Srv configuration). I used the same approach as in v6: Dhcp4Client object is now passed to the acquireAndDecline method.

Code is now merged to master. Thanks for your review. Closing ticket.

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.