Changes between Initial Version and Version 2 of Ticket #3496


Ignore:
Timestamp:
Jul 20, 2014, 12:21:40 PM (5 years ago)
Author:
tomek
Comment:

Here are my comments regarding new hooks implementation patch.

The subnet4_preselect hook may be useful, but I think we need to reach a mutual understanding why it is needed. You said: "In the subnet4_select hook, a subnet has already been selected for the DHCP message, which leaves the library developers little space to custom the subnet.". This is not true. The hook is really like the server asking "this is the subnet I plan to use. Is it ok? If not, tell me which subnet to use". Note that the callout can change server's proposal and return different value in subnet4 (it is both in *and* out parameter). If the callout changes the value (i.e. the callout answers "no, use this other subnet instead"), the server will honour it and will continue with whatever subnet the callout returned.

Having said that, I think it is still useful to have subnet4_preselect call. The reason for its existence would be performance. If you want to use some complex criteria for selecting a subnet (like vlan must be in range 1000 to 1024, relay's MAC address must match the one specified in Subnet and client must not belong to cable modem class), then you may develop some custom subnet selection routines in you callout that would be faster than what Kea engine offers.

So if your only reason for implementing subnet4_preselect is to pick a different subnet, it doesn't make sense. If you want to use custom subnet selection algorithm and you are concerned about performance, then it makes sense.

As for the lease4_prerelease, why can't you just use lease4_release? It seems as if you want to provide an alternative storage for leases. That's not the way to go. All leases should be stored in one location. That's ok if you want to use Regis or other mem-only database for storing leases, just keep all leases (context and "regular") in one place. There are many, many places in the code that check if certain lease is present or not and acts specifically, depending on that knowledge.

If we go with the data model I proposed in comments for 3491, then we could just use lease4_release (it would fetch the lease, regardless if it's regular or context).

src/bin/dhcp4/dhcp4_srv.cc|h The changes in patch benu7.16.patch include only changes for subnet_preselect. The changes for lease4_prerelease are missing. You did include snippets from it in the PDF.


David or Shawn asked what would be the best way for callouts to signal that a packet should be dropped. I created #3499 with a proposed solution.

Legend:

Unmodified
Added
Removed
Modified
  • Ticket #3496

    • Property Status changed from new to reviewing
    • Property Milestone changed from Kea-proposed to Kea1.0
  • Ticket #3496 – Description

    initial v2  
    1 This is the sixth ticket created for processing patch by Shawn Lewis.
     1This is the sixth ticket created for processing patch by Shawn Lewis and David Gong.
    22
    33Shawn: