[libvirt] [PATCH] network: Add support for dhcp-range lease time in the network XML configuration format and dnsmasq

[Laine Stump]

On 04/18/2016 06:52 PM, Cole Robinson wrote:
> On 04/15/2016 08:18 PM, Alberto Ruiz wrote:
>>  From 112f61ec5cfdc39f7a157825c4209f7bae34c483 Mon Sep 17 00:00:00 2001
>> From: Alberto Ruiz <aruiz at gnome.org>
>> Date: Wed, 13 Apr 2016 17:00:45 +0100
>> Subject: [PATCH] network: Add support for dhcp-range lease time in the network
>>   XML configuration format and dnsmasq
>>
> Also mention the bug in the commit message, just link it like
>
> https://bugzilla.redhat.com/show_bug.cgi?id=913446
>
> Needs documentation but that will be dependent on what the final patch looks
> like, so fine to skip for now.
>
> The main questions are:
>
> 1) is the XML format fine? <range ... lease='XXX'/>. lease sounds kinda
> non-specific to me, maybe leasetime or leaseTime.
>
> 2) what to use for the input format? right now it's just string passthrough to
> dnsmasq, which takes a format like XX[s|m|h|d|w], or 'infinite'. Accepting
> that format kind of sticks us with that for all time, which probably isn't a
> good precedent. the easy way would probably be to just say the value needs to
> be in minutes, and maybe -1 == infinite. But that will take a bit more code to
> adapt that value to the dnsmasq format.

Yeah, you should never just read an opaque string and pass it directly 
through to dnsmasq. Instead, parse an integer (and whatever scaling info 
- hours / minutes / seconds - I know we do that for bytes vs kbytes vs 
KiB etc, and if we don't already have the same thing for times 
somewhere, we should), scale it, check the range for some amount of 
sanity, and convert that scaled integer into whatever dnsmasq wants when 
building the commandline.

>
> CC laining for his thoughts

Aside from the missing documentation that you pointed out (and that is 
just a pain to put in until the exact placement in the XML is figured 
out anyway), my main sticking point is having the lease time put as an 
attribute to each range. That just seems.... odd. I know that dnsmasq 
allows for specifying a lease time per range, but is that the case for 
other dhcp server implementations? (yeah, I know we don't support any 
other now, but someday we might :-). And even if dnsmasq *allows* it, 
unless you're using their tagging to put certain clients into certain IP 
ranges, there's no practical value in having a different lease time for 
each range. Maybe it should be an attribute of the <dhcp> element (or, 
to allow for different scaling, a subelement); every range on the 
dnsmasq commandline would just get the same lease time. Something like this:

    <dhcp>
      <leaseTime units='seconds'>3600</leasetime>
      <range blah blah blah/>
      ....
    </dhcp>

If the need for per-range leasetime arose later, that could be added as 
a sub-element to <range> that would override the leasetime directly 
under <dhcp>.

(It's been at least 15 years since I used ISC's dhcpd, but I glanced at 
the config file manpage just now and it looks like it's possible to have 
a single "max-lease" that applies to all "pools" (their name for ranges) 
or to specify a separate max-lease for each pool. I admit I skipped 98% 
of the contents though :-)).

In practice, I doubt there will be much difference between what you 
proposed and what I've suggested - probably 100% of the libvirt virtual 
networks in existence have only a single range anyway. I *think* 
splitting it out from the range could prevent us from being painted into 
a corner though.

Aside from all that, thanks for taking the time to code this up!


>
> And one tiny comment below:
>
>> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
>> index 4fb2e2a..449c9ed 100644
>> --- a/src/conf/network_conf.c
>> +++ b/src/conf/network_conf.c
>> @@ -313,6 +313,10 @@ static void
>>   virNetworkIpDefClear(virNetworkIpDefPtr def)
>>   {
>>       VIR_FREE(def->family);
>> +
>> +    while (def->nranges)
>> +        VIR_FREE(def->ranges[--def->nranges].lease);
>> +
>>       VIR_FREE(def->ranges);
>>   
>>       while (def->nhosts)
>> @@ -855,7 +859,6 @@ int virNetworkIpDefNetmask(const virNetworkIpDef *def,
>>                                           VIR_SOCKET_ADDR_FAMILY(&def->address));
>>   }
>>   
>> -
> stray whitespace change here
>
> - Cole
>