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

[Michal Privoznik]

On 03/30/2017 06:00 PM, Laine Stump wrote:
> On 03/28/2017 05:08 AM, Michal Privoznik wrote:
>> 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.
>
>
> I was wrong about this part. Previously the call to create macMapFile
> we're talking about happened during network driver initialization for
> any *active* network that had IP addresses defined on the network (and
> it just happens that a libvirt network can only have IP addresses
> defined if we are managing the bridge and running dnsmasq).
>
> With this patch, it is created during network driver initialization for
> any network that has forward mode = nat/route/open/none (i.e. any
> network that libvirt is running dnsmasq for). It's no longer inside a
> check for networking being active/not, but anyway networkUpdateState()
> returns immediately when it's called if the network isn't already active
> anyway.
>
> So essentially the code is doing the same thing it previously did.
>
>>
>> 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.
>
> Yeah, I agree with that.
>
>>
>> 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.
>
> But I don't think you are creating it any more frequently than before.
> You're doing it at exactly the same times as previously.
>
> In the end, I don't have anything *against* this patch, I just don't
> think it's doing anything useful.

How come? It's creating the macMap module more frequently - exactly in 
the cases described above. As a result, the $brname.macs file is created 
more frequently and thus users can use libvirt_guests on freshly started 
domains without need for restarting the network. Or am I missing 
something - is there a bug in my code that prevents this from happening 
and I'm just describing what I think the code does instead of what it 
actually does?

Michal