[libvirt] [PATCH] network: Add support for dhcp-range lease time in the network XML configuration format and dnsmasq
7:18 p.m.
Hello,
I've been asked to send this patch to the list for review which I
originally posted at rhbz#913446 [0].
My main motivation is to be able to set a saner default for dhcp leases in
Boxes for development machines where the instant volatility of the dhcp
lease makes it impossible to reliably work with services after the machines
have to be rebooted. One has to discover the ip again... setup ssh keys...
This patch is a first attempt so feel free to let me know if I'm aiming at
the right direction.
Thanks!
[0]https://bugzilla.redhat.com/show_bug.cgi?id=913446
--
Alberto Ruiz
Associate Engineering Manager - Desktop Management Tools
Red Hat
5:52 p.m.
On 04/15/2016 08:18 PM, Alberto Ruiz wrote:
From 112f61ec5cfdc39f7a157825c4209f7bae34c483 Mon Sep 17 00:00:00
2001
From: Alberto Ruiz <aruiz(a)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.
CC laining for his thoughts
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
8:34 p.m.
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(a)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
9:35 a.m.
On Tue, Apr 19, 2016 at 2:34 AM, Laine Stump <laine(a)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(a)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
9:33 a.m.
On Mon, Apr 18, 2016 at 11:52 PM, Cole Robinson <crobinso(a)redhat.com> wrote:
On 04/15/2016 08:18 PM, Alberto Ruiz wrote:
> From 112f61ec5cfdc39f7a157825c4209f7bae34c483 Mon Sep 17 00:00:00 2001
> From: Alberto Ruiz <aruiz(a)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.
Sounds good to me, though since pointed out in a later email, we might want
to make this a child tag on its own within <dhcp> instead of a per range
property.
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.
Sticking to minutes seems like a loss of granularity, we can't really
predict that a given setup might care about second level granularity for
leases can we? I'm all for the argument of sanity checking the input and
not doing an opaque passthrough, but I think in the end we might want to
support some sort of seconds/minutes/hours/days notation (or stick to
seconds and let people figure the amount out with a calculator).
Thanks a lot for the input.
CC laining for his thoughts
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
3141
days inactive
3144
days old
4 comments
3 participants
participants (3)
-
Alberto Ruiz -
Cole Robinson -
Laine Stump