[libvirt] [PATCH 1/2] networkUpdateState: Create virMacMap module more frequently

[Martin Kletzander]

On Tue, Mar 28, 2017 at 11:08:01AM +0200, Michal Privoznik wrote:
>On 03/28/2017 02:22 AM, Laine Stump wrote:
>> On 03/22/2017 10:43 AM, Michal Privoznik wrote:
>>> The virMacMap module is there for dumping [domain, <list of is
>>> MACs>] pairs into a file so that libvirt_guest NSS module can use
>>> it. Whenever a interface is allocated from network (e.g. on

s/a interface/an interface/

>>>
>>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>>> ---
>>>  src/network/bridge_driver.c | 21 +++++++++------------
>>>  1 file changed, 9 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>>> index a753cd78b..0ea8e2348 100644
>>> --- a/src/network/bridge_driver.c
>>> +++ b/src/network/bridge_driver.c
>>> @@ -470,6 +470,7 @@ networkUpdateState(virNetworkObjPtr obj,
>>>  {
>>>      virNetworkDriverStatePtr driver = opaque;
>>>      dnsmasqCapsPtr dnsmasq_caps = networkGetDnsmasqCaps(driver);
>>> +    char *macMapFile = NULL;
>>>      int ret = -1;
>>>
>>>      virObjectLock(obj);
>>> @@ -486,6 +487,13 @@ networkUpdateState(virNetworkObjPtr obj,
>>>          /* If bridge doesn't exist, then mark it inactive */
>>>          if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) == 1))
>>>              obj->active = 0;
>>> +
>>> +        if (!(macMapFile = networkMacMgrFileName(driver, obj->def->bridge)))
>>> +            goto cleanup;
>>> +
>>> +        if (!(obj->macmap = virMacMapNew(macMapFile)))
>>> +            goto cleanup;
>>> +
>>>          break;
>>
>>
>> ... but from what I can understand, the only differences are:
>>
>> 1) the macMapFile used to be regenerated right after reading the
>> radvdpidfile (which in most cases doesn't exist because I think most of
>> the time that same duty is handled by dnsmasq instead), and now it is
>> regenerated *just before* reading radvdpidfile.
>
>I don't think this is any problem. It's just an ordering issue. Those
>two features are orthogonal.
>
>>
>> 2) it used to be regenerated for any network with a def->bridge (so it
>> would also happen for "unmanaged" bridge networks, where libvirt just
>> points to an already-existing bridge), and it is now regenerated only
>> for networks where libvirt creates and destroys the bridge (and might
>> have a dnsmasq instance running.
>
>Ah. Is that right? I agree that the code is a bit unclear, but the
>following should be true:
>
>1) when a network is being freshly started up, the virMacMap module is
>created and stored in the network object if and only if dnsmasq is
>spawned (because only then it's us who assigns IP addresses).
>
>2) when an interface is allocated/freed from a network that has the
>module, the $br.macs file is updated accordingly. The file is created on
>the first interface allocation, and unlinked on network undef.
>
>3) when the daemon restarts, networkUpdateState() is called for every
>running network. And if the $br.macs file exists for given network, the
>module is created (we are reconstructing the network objects after all)
>and the file is parsed to restore the previous state. Note, that the
>dnsmasq is not spawned again - it kept running while libvirtd was
>restarting.
>
>Now, there are two problems that I see:
>
>a) if there is no interface added in the 2nd step and the libvirt daemon
>is restarted, in the 3rd step the file does not exist and thus the code
>thinks no virMacMap module was used for the network so it does not
>create one. This is obviously a bug.
>
>b) if you want to have the module for your network, you need to shut it
>down (and thus cut off your domains from the connectivity) and start
>over again. It's very annoying. When implementing this, my reasoning was
>that it's better to give no results than partial results, it's better to
>not have no $br.macs file than have a file containing just newly
>allocated interfaces. Well, I was wrong. I think people don't want to
>destroy their network unless really necessary. More so if the network
>continues running even when the daemon is killed.
>
>Therefore, what I am suggesting here is to rework step 3) so that the
>module is created more frequently. It would solve both problems I'm
>describing.
>

Makes sense to me, however you might want to look at an idea in the
second patch.  Maybe it would help a bit.  Maybe it wouldn't :-/

Anyway, even when I'm not against this, I'll leave ACKing this to Laine.

>Michal
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170328/95111b0b/attachment-0001.sig>