[libvirt] [PATCH v4] leaseshelper: improvements to support all events

[Peter Krempa]

On 11/20/14 11:24, Daniel P. Berrange wrote:
> On Thu, Nov 20, 2014 at 10:56:53AM +0100, Peter Krempa wrote:
>> On 11/18/14 18:16, Nehal J Wani wrote:
>>> This patch enables the helper program to detect event(s) triggered when there
>>> is a change in lease length or expiry and client-id. This transfers complete
>>> control of leases database to libvirt and obsoletes use of the lease database
>>> file (<network-name>.leases). That file will not be created, read, or written.
>>> This is achieved by adding the option --leasefile-ro to dnsmasq and passing a
>>> custom env var to leaseshelper, which helps us map events related to leases
>>> with their corresponding network bridges, no matter what the event be.
>>>
>>> Also, this requires the addition of a new non-lease entry in our custom lease
>>> database: "server-duid". It is required to identify a DHCPv6 server.
>>>
>>> Now that dnsmasq doesn't maintain its own leases database, it relies on our
>>> helper program to tell it about previous leases and server duid. Thus, this
>>> patch makes our leases program honor an extra action: "init", in which it sends
>>> the known info in a particular format to dnsmasq by printing it to stdout.
>>>
>>> ---
>>>
>>>  This is compatible with libvirt 1.2.6 as only additions have been
>>>  introduced, and the old leases file (*.status) will still be supported.
>>>
>>>  v4: * Removed extra data structures for segregating ipv4 and ipv6 leases.
>>>
>>>  v3: * Add server-duid as an entry in the lease object for every ipv6 lease.
>>>      * Remove unnecessary variables and double copies.
>>>      * Take value from DNSMASQ_OLD_HOSTNAME if hostname is not known.
>>>      http://www.redhat.com/archives/libvir-list/2014-August/msg00028.html
>>>
>>>  v2: http://www.redhat.com/archives/libvir-list/2014-July/msg01109.html
>>>
>>>  v1: https://www.redhat.com/archives/libvir-list/2014-July/msg00568.html
>>>
>>>
>>> ---
>>>  src/network/bridge_driver.c |   3 +
>>>  src/network/leaseshelper.c  | 142 +++++++++++++++++++++++++++++++++++---------
>>>  2 files changed, 117 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>>> index 6cb421c..6ecbc37 100644
>>> --- a/src/network/bridge_driver.c
>>> +++ b/src/network/bridge_driver.c
>>> @@ -1280,7 +1280,10 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network,
>>>  
>>>      cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps));
>>>      virCommandAddArgFormat(cmd, "--conf-file=%s", configfile);
>>> +    /* Libvirt gains full control of leases database */
>>> +    virCommandAddArgFormat(cmd, "--leasefile-ro");
>>
>> One other thing that is worrying is that if you specify this option and
>> the leaseshelper implements the init operation we'd no longer load a
>> possibly pre-existing leases file that was present previous to the
>> upgrade to this handling.
>>
>> This would cause the guests to be re-addressed from the moment of the
>> upgrade. (exactly once).
>>
>> The question here is. Do we care about this slight breakage? I'm not
>> entirely decided yet. I probably wouldn't care.
>>
>> A possible workaround will be to check whether the old leases file
>> exists and refrain from starting with the --leasefile-ro option in that
>> case.
> 
> When libvirtd is upgraded, we don't restart dnsmasq so it will continue
> with its current config. Only once you shutdown the virtual network and
> restart it (eg most likely upon next reboot) would the config change
> loosing the leases.  That probably isn't the end of the world because
> with DHCP there is never a strong guarantee of getting the same IP
> address unless fixed mappings are provided.

Yep, and addresses that have to remain static can be configured explicitly.

> 
> If there's an easy way to notice the old file and convert it to the new
> file format we should do it, but if it is hard, then its not the end of
> the world.

AFAIK the leaseshelper was introduced so that we don't ever have to
parse the dnsmasq lease file so having to parse it ourselves so parsing
it wouldn't be viable. I was thinking of not using the new initial lease
reloading in case an existing lease file is present if we would want to
keep the mapping, but even that increases the complexity.

Thus I'm glad we don't have to strictly keep the leases file and users
will deal with this slight issue just once at upgrade.

> 
> Regards,
> Daniel
> 

Peter


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141120/ec8e1479/attachment-0001.sig>