Sprint 48 ns 3437 dhcp memcache

[flan]

For the most part, this looks really good and very interesting, but a few changes will need to be made before I can accept the pull and merge it into 2.0.x. I can create a new branch for you to target (or I think you can specify that in the pull-request), or you can effect the changes on your side first and send an updated PR.

First, you've included your config file and a symlink to the extension. These will need to be excluded from the commit.

Since you've added a dependency on netaddr, this should be mentioned prominently in the header of httpdb.py. Alternatively, since it's only used to do a CIDR check, it could be replaced by a simple function; I can implement that if an intermediate branch is used, probably as a class method on IPv4 until there's need to create an IPv4Network type for something bigger. Related: the subnet mask should always be an integer, so use %i, not %s, because early-trapped exceptions are good.

In httpdb.py, you have _logger.debug('Handling %s' % str((packet, packet_type, mac, ip, giaddr, pxe_options))). I'm strongly inclined to want to reduce that to 'Unknown MAC %(mac)s (ip=%(ip)s; giaddr=%(giaddr)s)'.

The new config options that are added need to be documented in doc/customisation/configuration.rst. I'd create a new section above Database called Caching and refactor the CACHE_ON_DISK and PERSISTENT_CACHE text from the database blocks (all of which should be copy-paste) into that section.

The MemcachedCache class's constructor currently takes "address:port" as a string assembled by its caller. In case the memcache client API ever changes to want a tuple or something else, and to make things easier for subclasses that might need to do unexpected things, these two elements should be passed separately (e.g., memcache_server_address, memcache_server_port) and get combined at the time that memcache.Client is initialised.

Lastly, since this is a really cool feature to have for HA alongside dynamism and because config-variables are incredibly hard to kill off, I'd really want to change MEMCACHED_CACHE to CACHING_MODEL and default it to "in-process", but add "memcache" as a second option, calling a new function in CachingDatabase, like _buildMemoryCache(self, name, chained_cache=None), which makes the decision between boring dictionary and real-problem-that-I-have-been-asked-about-solving network storage. On that note, though, it might be best to make MemcachedCache's reinitialise() a no-op. If you let any client flush it, you're probably losing whatever benefit would have been afforded by using a network store.

[zwerheim]

Thanks for that feedback! It will be really helpful as we continue to build up this change. @lathrop-navisite Contacted you earlier about supporting the same MAC across multiple subnets and this is the beginning of our effort to implement that. We have a forked version of your repo that we are working. I accidentally created this pull request against your repo as I have both checked out.

We will be sure to address the issues you highlighted as we continue. When we have completed our regular development we will open a full updated pull request with you to start working on integrating it according to your specifications.

[flan]

Ah, okay, I'm looking forward to it!

In the meantime, though I should probably stage my changes in branches to make it easier for you to merge (still adjusting to github), I've implemented subnet-membership-test functionality in ipv4.py: aae042e90054f21c95942e0f08023dab866f5f5a

>>> import ipv4
>>> ipv4.IPv4.parseSubnet("192.168.0.0/24")
('192.168.0.0', 24)
>>> ipv4.IPv4.parseSubnet("192.168.0.0/255.255.255.0")
('192.168.0.0', '255.255.255.0')
>>> x = ipv4.IPv4("192.168.7.100")
>>> x.isSubnetMember(*x.parseSubnet("192.168.0.0/24"))
False
>>> x.isSubnetMember(*x.parseSubnet("192.168.7.0/24"))
True
>>> x.isSubnetMember(*x.parseSubnet("192.168.7.200/24"))
True
>>> x.isSubnetMember(*x.parseSubnet("192.168.8.200/24"))
False
>>> x.isSubnetMember(*x.parseSubnet("192.168.7.0/255.255.255.0"))
True