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

[Alberto Ruiz]

On Tue, Apr 19, 2016 at 2:34 AM, Laine Stump <laine at laine.org> wrote:

> 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.


Agreed. Will work on a second version of this patch with taking this into
account.


>
>
>
>> 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>
>
>
Sounds fair, and solves another issue I was hoping to discuss which is
per-host leasetime.


> 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!


No worries, my pleasure, will update the patch and get back to you as soon
as I can.


> 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
>>
>>
>


-- 
Alberto Ruiz
Associate Engineering Manager - Desktop Management Tools
Red Hat
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160419/c466304f/attachment-0001.htm>