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