Changes between Initial Version and Version 2 of Ticket #3491


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

Comments regarding context-based DHCP server.

First of all, I consider contexts a very powerful concept. It can be useful in many use cases that you mentioned. Another use cases that was not mentioned may be possible deployment scenarios for draft-ietf-dhc-dynamic-shared-v4allocation-01, which assumes that one IPv4 lease is not a whole IPv4 address, but rather a fraction of it (each client is allowed to use only specified port-set or port range). It's not exactly the same as other context use cases, but it is somewhat similar in the sense that there will be multiple leases for the same address, so address uniqueness is no longer guaranteed.

Implementing contexts is a difficult task, so please don't be discouraged if this part of your patch will take extra time and work is merged.

One fundamental thing that is currently not addressed is the ability to store more than one lease with the same address. ( when each lease belongs to a different context). It will be mandatory to develop such capability for existing databases (at least one of them at the beginning, but ultimately we'll have to extend them all), so we could run tests and debug the context capability.

Before we go into the details of my review, I'd like to discuss the data model. Let's assume that we have 3 contexts.

Context A with vlan=10, MAC unspecified and 192.168.1.0/24 subnet. Context B with vlan=20, MAC 00:01:02:03:04:05 and 192.168.1.0/24 subnet. Context C with vlan=30, MAC 00:aa:bb:cc:dd:ee and 192.168.1.0/24 subnet.

Normally, such a configuration would be meaningless, as the subnets are the same. In fact, we have plans to implement checks that subnets do not overlap. This is a useful sanity check in normal (context-less) environment, but would prevent contexts from operating. This is easy to solve. We need a global configuration parameter that is disabled by default and, when specified, enables context mode. In that mode some sanity checks could be disabled.

Now, this configuration will be translated to 3 Subnet4 objects, each with its own subnet_id (1,2 and 3, respectively). It may be useful to read my comments for subnet.cc|h below, when I explain in detail why Subnet4Context is not needed. Each Subnet4 structure will represent a subnet, but also a context. If you have 10 contexts, you'll have 10 Subnet4 structures. Each of those Subnet4 object may have the same subnet and/or pool.

The aforementioned layout can be also used to conveniently access vlan, MAC and any other parameter that will appear in the future. To differentiate between contexts, you can use a single subnet_id Instead of using (vlan, mac) tuples.

Let's talks about how would the leases be organized in contexts. When client connects to a network that is covered in context A, he'll get a lease that looks like this: Lease4(addr=192.168.1.5, subnet_id=1)

Client in context B will get lease that looks similar to this: Lease4(addr=192.168.1.8, subnet_id=2)

And so on. It is even possible that the client in context C will get the same address as in other contexts: LEase4(addr=192.168.1.5, subnet_id=3)

You proposed to keep context parameters in each lease. In my opinion this is not the right way to go. The context parameters are a property of the subnet, not the lease. Think about a case where you have a million clients in one context. All of them will have the same vlan and the same relay's MAC address. So it doesn't make sense to keep it million times. Also, this data organization is self-consistent: whatever is set in Subnet4 applies to all leases within it. For example, if you want to change vlan, you just do it once (in Subnet4 object) and it applies automatically to all leases with this context.

If we were to go with the data model you proposed, the data could become inconsistent, e.g. your Subnet4 would have vlan=123, but the lease within this context could have vlan=456.

We may need to implement an extra method, something like CfgMgr::getSubnet4(subnet_id), but it is trivial to do. If you need to access vlan and relay MAC parameters, you can obtain them from Subnet4 structure. Subnet4 objects are uniquely identified by the subnet_id, so if you ever need to get parameters for any given lease, you can do so using CfgMgr::getSubnet4(lease4->subnet_id_)->vlan.

With that layout, many of the things you need are greatly simplified. You can use getLease4(hwaddr, subnet_id) to obtain both regular and context leases, so getLease4Context() is not needed at all. The new equality operators, the new Lease4Context class with all its methods, are no longer needed.

Having said that, there are couple things that you probably haven't encountered yet (or perhaps you did, but haven't dealt with in your patch):

Issue 1: the call getLease4(addr) will no longer work if contexts are enabled, because the address used to be unique, but it is not anymore. I think all the places that use that call need to be modified:

OLD:

  lease = LeaseMgrFactory::instance().getLease4(release->getCiaddr());

NEW:

if (!CfgMgr()::instance().contextMode()) {
   lease = LeaseMgrFactory::instance().getLease4(release->getCiaddr());
} else {
   lease = LeaseMgrFactory::instance().getLease4(release->getCiaddr(),
                                                 subnet->subnet_id_);
}

It is important to keep the original call when context-mode is disabled, because in many cases such a query is faster (database is indexed by addresses).

Issue 2: MySQL and Postgres databases assume that the address field is unique and they use it as a primary key. In current configuration, they will not allow inserting leases with duplicate addresses, even though they belong to separate subnets (contexts).

Now, on to the specific file modifications:

src/lib/dhcpsrv/subnet.cc|h I think the concept of having a class derived from Subnet is ok, but there's a better way to handle context subnets. What if you added setContextField(), getContextField() and getNumberOfContextFields() in the base Subnet class? This way it would be automatically usable in IPv4 and IPv6. We'd also have 2 less classes to maintain. With this approach, Subnet4 structure would represent a subnet, but also a context. So if you have 10 contexts, there would be 10 Subnet4 structures.

My second comment is about vlan and relay mac address. There's already a structure for those parameters. Please move them to Subnet::RelayInfo. As was mentioned in the PDF you provided, there may be additional parameters in the future (second vlan in Q-in-Q mode, GRE, IP-IP, etc.), so this will not be just merely vlan+mac and will grow over time. Therefore it's important to design the API in a way that wouldn't require extending the list of parameters every time we add new parameter and just pass one structure. This point applies to other classes, not just Subnet.

src/lib/asiolink/io_address.cc|h IOAddress object is for IPv4 and IPv6 addresses. The HWAddr conversion does not belong here. The convertHWAddrToString and onvertStringToHWAddr are not needed. They are already implemented in HWAddr class as toText() and fromText(). If the code there doesn't do exactly what you need, please tweak it rather than add new functions that do almost the same.

src/lib/dhcpsrv/alloc_engine.cc|h Once you apply changes I proposed in subnet.cc|h, you'll need to tweak your alloc_engine code as well. Fortunately, it will become simpler as well.

src/lib/dhcpsrv/lease_mgr.cc|h If we follow the data model I described above, most (if not all) of your modifications are not needed. That would make the code simpler and easier to maintain. Even if we decide, that such calls are needed, they should be modified to not contain so many parameters. This is not the problem of having too many parameters per se, but rather the problem lies in the fact that the number of parameters will grow. Your code now supports vlan and relay MAC address. But once you start supporting Q-in-Q, there may be 2 vlan IDs, there may be IP address if you go with IP-IP tunnel, or some extra identifiers for GRE. It would be better to pass one structure that describes all parameters. Subnet::RelayInfo? looks like a good structure here. It needs to be extended with additional fields, but that's easy to do.

So instead of:

Lease4Ptr getLease4Context(const isc::dhcp::HWAddr& hwaddr, SubnetID subnet_id,
                           HWAddrPtr & relay_mac, uint16_t vlan);

It will looks more or less like:

Lease4Ptr getLease4Context(const isc::dhcp::HWAddr& hwaddr, SubnetID subnet_id, 
                           const RelayInfo& relay_details);

But I still think that if we implement the data model I proposed, the relay_details information will not be needed at all, because the the subnet_id uniquely identifies the context/subnet, so the relay_details are not needed at all. Hence you can use getLease4(hwaddr, subnet_id), a call that is already there. Hence no new extra calls required.


There are several trailing whitespaces in the patch. Please try to avoid those. If you happen to use emacs, there's covenient macro to remove them whitespace-cleanup. Other editors provide similar capability.

Please try to limit lines width to no more than 100 characters. The coding guidelines (see contributor's guide) say that it should be no more 80 characaters, but that's just an old value that is no longer appropriate for C++ and its long names. We'll update the coding guidelines in a near future.

Also, when generating a patch, make sure you specify the base revision it was based upon. Right now the patch applies with only minor conflicts, but this may change in the future.

This is somewhat related to context, so I thought I should mention it here:

In the comment for subnet4_preselect you mentioned that the selected subnet could potentially be stored in some other place, not in CfgMgr. I have mixed feelings about this. But let's assume for now that it will be possible. The only limitation is that subnet_id_ for all subnets must be unique, regardless of their storage.

(The reason why I don't like the concept of having Subnets stored somewhere else than in CfgMgr is that there are many, many use cases where the server engine selects a subnet. For any given circumstances (the same relay, vlan, etc.), the code should always get the same subnet.

Legend:

Unmodified
Added
Removed
Modified
  • Ticket #3491

    • Property Summary changed from Context-based DHCP service (submitted patch) to Context-based DHCP service (contributed patch)
    • Property Milestone changed from Kea-proposed to Kea1.0
  • Ticket #3491 – Description

    initial v2  
    1 This ticket will cover parts of the patch submitted by Shawn Lewis that are related to context-based DHCP service.
     1This ticket will cover parts of the patch submitted by Shawn Lewis and David Gong that are related to context-based DHCP service.