This series is based on latest series from Alberto. It includes a new
entry called <leasetime/> under <dhcp/> scope to add a default lease
time for range and host options for dnsmasq. There is no point to
configure both separately. If they are defined (range and/or host), they
should have the same lease time value.

This series includes some test cases to cover lease time XML syntax
also.

Julio Faracco (2):
  conf: Add <leasetime/> option for <dhcp/> settings
  tests: Add tests for <leasetime/> to cover dnsmasq settings

 docs/schemas/basictypes.rng                   |  9 +++
 docs/schemas/network.rng                      | 11 ++++
 src/conf/network_conf.c                       | 62 ++++++++++++++++++-
 src/conf/network_conf.h                       | 14 +++++
 src/libvirt_private.syms                      |  2 +
 src/network/bridge_driver.c                   | 37 ++++++++++-
 src/util/virdnsmasq.c                         | 40 ++++++------
 src/util/virdnsmasq.h                         |  1 +
 .../networkxml2confdata/leasetime-hours.conf  | 16 +++++
 tests/networkxml2confdata/leasetime-hours.xml | 12 ++++
 .../leasetime-infinite.conf                   | 16 +++++
 .../leasetime-infinite.xml                    | 12 ++++
 .../leasetime-minutes.conf                    | 16 +++++
 .../networkxml2confdata/leasetime-minutes.xml | 12 ++++
 .../leasetime-seconds.conf                    | 16 +++++
 .../networkxml2confdata/leasetime-seconds.xml | 12 ++++
 tests/networkxml2conftest.c                   |  4 ++
 tests/networkxml2xmlin/leasetime-hours.xml    | 12 ++++
 tests/networkxml2xmlin/leasetime-infinite.xml | 12 ++++
 tests/networkxml2xmlin/leasetime-minutes.xml  | 12 ++++
 tests/networkxml2xmlin/leasetime-seconds.xml  | 12 ++++
 tests/networkxml2xmlout/leasetime-hours.xml   | 14 +++++
 .../networkxml2xmlout/leasetime-infinite.xml  | 14 +++++
 tests/networkxml2xmlout/leasetime-minutes.xml | 14 +++++
 tests/networkxml2xmlout/leasetime-seconds.xml | 14 +++++
 tests/networkxml2xmltest.c                    |  4 ++
 26 files changed, 376 insertions(+), 24 deletions(-)
 create mode 100644 tests/networkxml2confdata/leasetime-hours.conf
 create mode 100644 tests/networkxml2confdata/leasetime-hours.xml
 create mode 100644 tests/networkxml2confdata/leasetime-infinite.conf
 create mode 100644 tests/networkxml2confdata/leasetime-infinite.xml
 create mode 100644 tests/networkxml2confdata/leasetime-minutes.conf
 create mode 100644 tests/networkxml2confdata/leasetime-minutes.xml
 create mode 100644 tests/networkxml2confdata/leasetime-seconds.conf
 create mode 100644 tests/networkxml2confdata/leasetime-seconds.xml
 create mode 100644 tests/networkxml2xmlin/leasetime-hours.xml
 create mode 100644 tests/networkxml2xmlin/leasetime-infinite.xml
 create mode 100644 tests/networkxml2xmlin/leasetime-minutes.xml
 create mode 100644 tests/networkxml2xmlin/leasetime-seconds.xml
 create mode 100644 tests/networkxml2xmlout/leasetime-hours.xml
 create mode 100644 tests/networkxml2xmlout/leasetime-infinite.xml
 create mode 100644 tests/networkxml2xmlout/leasetime-minutes.xml
 create mode 100644 tests/networkxml2xmlout/leasetime-seconds.xml

-- 
2.24.1

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

They can be different from each other.
I still think that the idea should be better developed.
I don't like that my example also (it is just an example).
That's why I submitted... To listen opinions from others.

--
Julio Cesar Faracco

Em qua., 15 de abr. de 2020 às 13:19, Julio Faracco
<jcfaracco@gmail.com> escreveu:
>
> This series is based on latest series from Alberto. It includes a new
> entry called <leasetime/> under <dhcp/> scope to add a default lease
> time for range and host options for dnsmasq. There is no point to
> configure both separately. If they are defined (range and/or host), they
> should have the same lease time value.
>
> This series includes some test cases to cover lease time XML syntax
> also.
>
> Julio Faracco (2):
>   conf: Add <leasetime/> option for <dhcp/> settings
>   tests: Add tests for <leasetime/> to cover dnsmasq settings
>
>  docs/schemas/basictypes.rng                   |  9 +++
>  docs/schemas/network.rng                      | 11 ++++
>  src/conf/network_conf.c                       | 62 ++++++++++++++++++-
>  src/conf/network_conf.h                       | 14 +++++
>  src/libvirt_private.syms                      |  2 +
>  src/network/bridge_driver.c                   | 37 ++++++++++-
>  src/util/virdnsmasq.c                         | 40 ++++++------
>  src/util/virdnsmasq.h                         |  1 +
>  .../networkxml2confdata/leasetime-hours.conf  | 16 +++++
>  tests/networkxml2confdata/leasetime-hours.xml | 12 ++++
>  .../leasetime-infinite.conf                   | 16 +++++
>  .../leasetime-infinite.xml                    | 12 ++++
>  .../leasetime-minutes.conf                    | 16 +++++
>  .../networkxml2confdata/leasetime-minutes.xml | 12 ++++
>  .../leasetime-seconds.conf                    | 16 +++++
>  .../networkxml2confdata/leasetime-seconds.xml | 12 ++++
>  tests/networkxml2conftest.c                   |  4 ++
>  tests/networkxml2xmlin/leasetime-hours.xml    | 12 ++++
>  tests/networkxml2xmlin/leasetime-infinite.xml | 12 ++++
>  tests/networkxml2xmlin/leasetime-minutes.xml  | 12 ++++
>  tests/networkxml2xmlin/leasetime-seconds.xml  | 12 ++++
>  tests/networkxml2xmlout/leasetime-hours.xml   | 14 +++++
>  .../networkxml2xmlout/leasetime-infinite.xml  | 14 +++++
>  tests/networkxml2xmlout/leasetime-minutes.xml | 14 +++++
>  tests/networkxml2xmlout/leasetime-seconds.xml | 14 +++++
>  tests/networkxml2xmltest.c                    |  4 ++
>  26 files changed, 376 insertions(+), 24 deletions(-)
>  create mode 100644 tests/networkxml2confdata/leasetime-hours.conf
>  create mode 100644 tests/networkxml2confdata/leasetime-hours.xml
>  create mode 100644 tests/networkxml2confdata/leasetime-infinite.conf
>  create mode 100644 tests/networkxml2confdata/leasetime-infinite.xml
>  create mode 100644 tests/networkxml2confdata/leasetime-minutes.conf
>  create mode 100644 tests/networkxml2confdata/leasetime-minutes.xml
>  create mode 100644 tests/networkxml2confdata/leasetime-seconds.conf
>  create mode 100644 tests/networkxml2confdata/leasetime-seconds.xml
>  create mode 100644 tests/networkxml2xmlin/leasetime-hours.xml
>  create mode 100644 tests/networkxml2xmlin/leasetime-infinite.xml
>  create mode 100644 tests/networkxml2xmlin/leasetime-minutes.xml
>  create mode 100644 tests/networkxml2xmlin/leasetime-seconds.xml
>  create mode 100644 tests/networkxml2xmlout/leasetime-hours.xml
>  create mode 100644 tests/networkxml2xmlout/leasetime-infinite.xml
>  create mode 100644 tests/networkxml2xmlout/leasetime-minutes.xml
>  create mode 100644 tests/networkxml2xmlout/leasetime-seconds.xml
>
> --
> 2.24.1
>

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.

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>

In all of these we can default to "minutes" if no units are given.

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

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.


>
> In all of these we can default to "minutes" if no units are given.
>
> Regards,
> Daniel

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