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

[Michal Privoznik]

On 03/28/2017 02:22 AM, Laine Stump wrote:
> On 03/22/2017 10:43 AM, Michal Privoznik wrote:

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

Michal