Opened 4 years ago

Closed 4 years ago

#3913 closed defect (fixed)

Kea6 sends existing lease without renewing (was: Kea4 saves one lease multiple times)

Reported by: wlodekwencel Owned by: marcin
Priority: medium Milestone: Kea0.9.2
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

Every time client requests an address Kea4 saves one lease, if client request address 100 times we have 100 leases with identical data. One thing that changes is expire time.
Kea was configured with simple config file (attached) with perfdhcp I got a more than 1500 same leases saved to .csv file:

192.168.50.1,00:0c:01:02:03:04,,4000,1434623764,1,0,0,
192.168.50.1,00:0c:01:02:03:04,,4000,1434623764,1,0,0,
192.168.50.1,00:0c:01:02:03:04,,4000,1434623764,1,0,0,
192.168.50.1,00:0c:01:02:03:04,,4000,1434623764,1,0,0,
192.168.50.1,00:0c:01:02:03:04,,4000,1434623764,1,0,0,
192.168.50.1,00:0c:01:02:03:04,,4000,1434623764,1,0,0,
192.168.50.1,00:0c:01:02:03:04,,4000,1434623764,1,0,0,
192.168.50.1,00:0c:01:02:03:04,,4000,1434623764,1,0,0,
192.168.50.1,00:0c:01:02:03:04,,4000,1434623764,1,0,0,
192.168.50.1,00:0c:01:02:03:04,,4000,1434623764,1,0,0,
192.168.50.1,00:0c:01:02:03:04,,4000,1434623765,1,0,0,
192.168.50.1,00:0c:01:02:03:04,,4000,1434623765,1,0,0,
192.168.50.1,00:0c:01:02:03:04,,4000,1434623765,1,0,0,
192.168.50.1,00:0c:01:02:03:04,,4000,1434623765,1,0,0,
192.168.50.1,00:0c:01:02:03:04,,4000,1434623765,1,0,0,
192.168.50.1,00:0c:01:02:03:04,,4000,1434623765,1,0,0,
192.168.50.1,00:0c:01:02:03:04,,4000,1434623765,1,0,0,
192.168.50.1,00:0c:01:02:03:04,,4000,1434623765,1,0,0,
192.168.50.1,00:0c:01:02:03:04,,4000,1434623765,1,0,0,
192.168.50.1,00:0c:01:02:03:04,,4000,1434623765,1,0,0,
192.168.50.1,00:0c:01:02:03:04,,4000,1434623765,1,0,0,
192.168.50.1,00:0c:01:02:03:04,,4000,1434623765,1,0,0,
...

capture, config files and leases file can be downloaded:
http://kea.isc.org/~wlodek/tests/multiple_leases.tar.gz

Subtickets

Change History (12)

comment:1 Changed 4 years ago by tomek

  • Summary changed from Kea4 saves one lease multiple times to Kea6 sends existing lease without renewing (was: Kea4 saves one lease multiple times)

This behavior is sub-optimal, but correct for v4, it's invalid for v6.

Consider this scenario: Lease lifetime is 1h. Client gets a lease. After 59min50s it reboots and sends a request. The lease is valid for only 10 more seconds. We can't send it back to client and claim "here's your 1 hour lease", because the client will go away and won't return until 30 minutes later. By that time the lease will be long expired and possibly recycled by the server for other clients.

Unfortunately, that's what v6 code does. The lease has to be changed.

On a related note, we should implement cache-threshold feature from isc-dhcp in Kea. It's a parameter expressed in percentage. If the lease is attempted to be renewed in the initial cache-treshold percent of the lease lifetime, the lease is not renewed, just existing lease is sent back (with lifetime being equal to the remaing lifetime rather than full, renewed lifetime). That way no database update is needed.

comment:2 Changed 4 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea0.9.2
  • Priority changed from high to medium

comment:3 Changed 4 years ago by tomek

  • Milestone changed from Kea0.9.2 to Kea0.9.2-final

comment:4 Changed 4 years ago by marcin

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

comment:5 Changed 4 years ago by marcin

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

The DHCPv6 server now always extends the lifetime. I haven't implemented the threshold mechanism because it is a new functionality and we should rather avoid adding new functionality during the Beta testing.

Proposed ChangeLog entry:

XXX.	[bug]		marcin
	DHCPv6 server extends the lifetime of the client's lease
	in the database when the client sends the Request message.
	This prevents premature lease expiration before the client
	renews the lease, according to the timers and lifetimes
	returned by the server.
	(Trac #3913, git abcd)

comment:6 follow-up: Changed 4 years ago by tomek

  • Owner changed from UnAssigned to marcin

That was a short review. I have only 3 comments:

ChangeLog?
I would shorten this entry. Just the first sentence is enough IMHO.
A reader who doesn't know what's the issue is, will be unlikely to
figure it out from "according to the times and lifetimes returned
by the server" alone. But I don't insist on it, if you want to
keep it.

alloc_engine.cc
conditionalExntendLifetime() should be a member of the
AllocEngine? class. Also, lease parameter is both in and
out, so doxygen's comment should reflect that.

Please file two tickets for implementing cache-threshold
(that's how this feature is called in ISC DHCP). It looks like
1.1 feature, but we'll do the usual triaging next Wednesday.


Code compiled and unit-test passed on Ubuntu 14.04 x64.

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

  • Owner changed from marcin to tomek

Replying to tomek:

That was a short review. I have only 3 comments:

ChangeLog?
I would shorten this entry. Just the first sentence is enough IMHO.
A reader who doesn't know what's the issue is, will be unlikely to
figure it out from "according to the times and lifetimes returned
by the server" alone. But I don't insist on it, if you want to
keep it.

I can probably shorten it a bit.

alloc_engine.cc
conditionalExntendLifetime() should be a member of the
AllocEngine? class. Also, lease parameter is both in and
out, so doxygen's comment should reflect that.

Why should I do it? This is just a convenience function which doesn't depend on the lease AllocEngine? class state or anything, so putting it in the anonymous namespace gives me the flexibility to only modify it in the .cc file if required, rather than having to modify the header file to add/remove parameters etc. I'd insist to leave it as is.

Please file two tickets for implementing cache-threshold
(that's how this feature is called in ISC DHCP). It looks like
1.1 feature, but we'll do the usual triaging next Wednesday.


Code compiled and unit-test passed on Ubuntu 14.04 x64.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 4 years ago by tomek

  • Owner changed from tomek to marcin

Replying to marcin:

alloc_engine.cc
conditionalExntendLifetime() should be a member of the
AllocEngine? class. Also, lease parameter is both in and
out, so doxygen's comment should reflect that.

Why should I do it? This is just a convenience function which doesn't depend on the lease AllocEngine? class state or anything, so putting it in the anonymous namespace gives me the flexibility to only modify it in the .cc file if required, rather than having to modify the header file to add/remove parameters etc. I'd insist to leave it as is.

This is a method that logically belongs to AllocEngine?, a class that's responsible for allocating and extending leases. It's simple convenience function for now, but it will be much more complicated in the future. I don't like dangling methods that are not associated with anything. That's what classes and private methods are for. I haven't looked at Doxygen documentation, but I imagine this method won't be listed along with AllocEngine? documentation, but with unrelated methods in anonymous namespace (or even not listed at all).

In my opinion it should be part of the class. Its parameters are unlikely to change (there will be additional calls to cfgmgr, which doesn't require interface change), but even if they will, so what? Method signatures change. It shouldn't affect anyone if it's a private method.

comment:9 in reply to: ↑ 8 Changed 4 years ago by marcin

Replying to tomek:

Replying to marcin:

alloc_engine.cc
conditionalExntendLifetime() should be a member of the
AllocEngine? class. Also, lease parameter is both in and
out, so doxygen's comment should reflect that.

Why should I do it? This is just a convenience function which doesn't depend on the lease AllocEngine? class state or anything, so putting it in the anonymous namespace gives me the flexibility to only modify it in the .cc file if required, rather than having to modify the header file to add/remove parameters etc. I'd insist to leave it as is.

This is a method that logically belongs to AllocEngine?, a class that's responsible for allocating and extending leases. It's simple convenience function for now, but it will be much more complicated in the future. I don't like dangling methods that are not associated with anything. That's what classes and private methods are for. I haven't looked at Doxygen documentation, but I imagine this method won't be listed along with AllocEngine? documentation, but with unrelated methods in anonymous namespace (or even not listed at all).

In my opinion it should be part of the class. Its parameters are unlikely to change (there will be additional calls to cfgmgr, which doesn't require interface change), but even if they will, so what? Method signatures change. It shouldn't affect anyone if it's a private method.

This method will never depend on the AllocEngine state and thus there is no need for it to belong to the class. Once the idea of using the threshold evolves the function may need some updates, in which case just changing the .cc file (and not .h file) doesn't require dependent modules to recompile because of the header file change.

At the same time, the fact that it is going to be a private method of the AllocEngine class doesn't bring any benefit for the documentation because doxygen doesn't create documentation for the private methods.

But, we had a discussion on Jabber and Thomas was in favor of your proposal, therefore I'll do the change you're requesting, even though I disagree it is necessary or improves anything.

comment:10 follow-up: Changed 4 years ago by marcin

  • Owner changed from marcin to tomek

I made a suggested change. Please review.

I also submitted the http://kea.isc.org/ticket/3926 for the threshold stuff.

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

  • Owner changed from tomek to marcin

Replying to marcin:

I made a suggested change. Please review.

All OK. Please merge.

comment:12 Changed 4 years ago by marcin

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

Merged with commit 1d64829a3f1a8288dc833ed388d9ffc9fe4cf491

Note: See TracTickets for help on using tickets.