Opened 7 months ago

Closed 7 months ago

#5252 closed defect (fixed)

V6 server does not correctly "reuse" a reclaimed expired leases for SARR only clients

Reported by: tmark Owned by: tmark
Priority: medium 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: 5247
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description (last modified by tmark)

V6 server does not correctly "reuse" a reclaimed expired when the returning client starts over with a SOLICIT rather than renew or rebind. It goes through this log message: ALLOC_ENGINE_V6_ALLOC_LEASES_NO_HR and ends up bypassing any logic to change the lease state or adjust stats accordingly.

This ticket came out of work on 5247. That ticket will need to be merged first.

Subtickets

Change History (9)

comment:1 Changed 7 months ago by tmark

  • Description modified (diff)
  • Feature Depending on Ticket set to 5247

comment:2 Changed 7 months ago by tmark

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

comment:3 Changed 7 months ago by tmark

  • Milestone changed from Kea-proposed to Kea1.2-final
  • Owner changed from tmark to Unassigned
  • Status changed from assigned to reviewing

The fix turned out to be pretty simple and is ready for review.

ChangeLog?:

12xx.   [bug]       tmark
    kea-dhcp6 now correctly resets lease state and increments the
    assigned statistic when it reissues an expired-reclaimed lease
    to the lease's original client, in response to a REQUEST from
    said client.
    (Trac #5252, git TBD)

comment:4 Changed 7 months ago by marcin

  • Owner changed from Unassigned to marcin

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

  • Owner changed from marcin to tmark

Reviewed commit a63885a00db21df8411fe31aca2ec49d51945d4b

I removed one extraneous whitespace and pushed, so you have to pull the branch first.

I was wondering if we should maybe fix detailCompareLease to also check that Lease::state_ is equal. That would affect your test though, because you currently use it for comparing the input lease with the lease from the manager. They are not in fact equal because the state is different. I think the test should verify that the state in the lease from the manager is updated to default. The test could update the state in the original lease and continue using the detailCompareLease as it is doing now.

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

Replying to marcin:

Reviewed commit a63885a00db21df8411fe31aca2ec49d51945d4b

I removed one extraneous whitespace and pushed, so you have to pull the branch first.

Got it thanks.

I was wondering if we should maybe fix detailCompareLease to also check that Lease::state_ is equal. That would affect your test though, because you currently use it for comparing the input lease with the lease from the manager. They are not in fact equal because the state is different. I think the test should verify that the state in the lease from the manager is updated to default. The test could update the state in the original lease and continue using the detailCompareLease as it is doing now.

In the interests of getting this merged before the code freeze, I opted to explicitly verify the lease state. Hopefully this will suffice.

Please re-review.

comment:7 Changed 7 months ago by tmark

  • Owner changed from tmark to marcin

comment:8 Changed 7 months ago by marcin

  • Owner changed from marcin to tmark

I reviewed your most recent change and you're good to go.

comment:9 Changed 7 months ago by tmark

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

Changes merged with git 85bde7adbe6a78238bd5e17fecabfa918755f16c
Added Changelog entry 1252.

Ticket is complete.

Note: See TracTickets for help on using tickets.