[PATCH 0/2] Include lease time option into DHCP settings

Mon Apr 20 09:16:57 UTC 2020

On Sun, Apr 19, 2020 at 12:16:39AM -0400, Laine Stump wrote:
> On 4/17/20 12:00 PM, Daniel P. Berrangé wrote:
> > On Wed, Apr 15, 2020 at 01:25:37PM -0300, Julio Faracco wrote:
> > > I resubmitted this series because our team needs to hack dnsmasq
> > > settings to change lease time. This feature would be so important to
> > > us to avoid workarounds.
> > > 
> > > It is based on Alberto's patch from 2017. But personally I don't like
> > > this approach.
> > > IMHO, it would be nice to have specific attributes to configure lease time.
> > > For example:
> > > <range ... leasetime="10m"/>
> > > <host ... leasetime="20m"/>
> > It is generally considered bad practice to have an XML attribute
> > value which then has to be parsed again to extract information.
> > 
> > For this reason, libvirt will use two attrbutes, one of the
> > value and one for the units (with some sensible default
> > units if not specified), even though this is admittedly
> > more verbose.
> > 
> > I agree it would be useful to have lease time per-host, as well
> > as globally though, and IIRC one of the original versions of
> > this patch did support that.
> 
> 
> The name of the original contributor that you (Julio) referenced in your
> cover letter sounded familiar, and I tried to find the original BZ for this
> when I saw your patches on the list, but I failed (bugzilla kept timing out
> on a *very* basic search term - this seems to happen to 80% of my queries
> these days...) But then it passed by in mail when Dan was cleaning up all
> the upstream libvirt BZes and moving them to gitlab :-), so just so everyone
> has all the background info:
> 
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=913446
> 
> 
> I also found the other similar patch from Nehal J Wani from around the same
> time:
> 
> 
> https://www.redhat.com/archives/libvir-list/2016-September/msg01063.html
> 
> 
> Without talking about the specifics of either patch, my recollection of the
> discussion from that time was that both contributors were trying to use a
> leasetime setting to solve a problem that it wouldn't have solved - their
> issue was that if a guest was turned off during the time when its lease
> expired, then when the guest was subsequently restarted it would end up with
> a different IP address. Of course setting a longer lease expiry would only
> delay the problem, not eliminate it. Further discussion revealed that if
> libvirt would just set the "dhcp-authoritative" option in the dnsmasq
> config, then dnsmasq would attempt to reissue the same IP to a guest even if
> its lease had already expired - Martin Wilck made this change to libvirt in
> commit 4ac20b3ae4, which seemed to satisfy the people who had thought they
> needed to modify the dhcp lease time, and so neither of the lease time
> patches was pushed upstream.
> 
> 
> The reason I bring up this old history is just as a cautionary tale that
> sometimes what you think you need isn't really what you actually need :-)
> 
> 
> (Recently Cole added the dnsmasq private namespace to the <network> XML, but
> that is only useful to add an entire option line, and can't do what you
> need, which is adding an extra option to every dhcp-host and dhcp-range
> line; there unfortunately is no standalone dnsmasq option to specify a
> global lease time)
> 
> 
> > 
> > We could do one of
> > 
> >    <ip address="192.168.122.1" netmask="255.255.255.0">
> >      <dhcp>
> >        <range start="192.168.122.2" end="192.168.122.254" lease="12" units="hours"/>
> >        <host id="0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66" ip="2001:db8:ca2:2:3::2" lease="30" units="mins"/>
> >      </dhcp>
> >    </ip>
> > 
> > or
> > 
> >    <ip address="192.168.122.1" netmask="255.255.255.0">
> >      <dhcp>
> >        <range start="192.168.122.2" end="192.168.122.254" leasetime="12" leaseunits="hours"/>
> >        <host id="0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66" ip="2001:db8:ca2:2:3::2" leasetime="30" leaseunits="mins"/>
> >      </dhcp>
> >    </ip>
> > 
> > or
> > 
> >    <ip address="192.168.122.1" netmask="255.255.255.0">
> >      <dhcp>
> >        <range start="192.168.122.2" end="192.168.122.254">
> >           <lease expiry="12" units="hours"/>
> >        </range>
> >        <host id="0:1:0:1:18:aa:62:fe:0:16:3e:44:55:66" ip="2001:db8:ca2:2:3::2">
> >           <lease expiry="30" units="mins"/>
> >        </host>
> >      </dhcp>
> >    </ip>
> 
> 
> Nehal's patch had used this syntax:
> 
> 
>  <leasetime units='hours'>12</leasetime>
> 
> 
> based on (I guess) the syntax of libvirt's <memory> element:
> 
> 
>  <memory unit='KiB'>524288</memory>
> 
> 
> I don't have a preference for any of them, just thought I would point out
> existing usage in libvirt that has a value/units combination.

Annoyingly it seems we used "units" and "unit", though "unit" wins by
a large margin over "units"

I guess I have a slight preference for the last option, with the use of
the child-element, as it is the cleanest XML design, even if it is slightly
more verbose.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|