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

[Michal Privoznik]

On 03/31/2017 03:41 PM, Laine Stump wrote:
> On 03/31/2017 07:20 AM, Michal Privoznik wrote:
>> 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
>
> Oh wait, now I see the difference - previously it would only create a
> new macMap if macMapFile existed, and now it does it even if macMapFile
> doesn't exist. Is that the difference you're talking about?

Exactly! That's the difference I am talking about.

>
> If that is what you say makes it "more frequently" (I haven't had the
> time or spare brain cells to verify that, but I'll take your word) then
> ACK.

Thank you, I will push this after the release - I don't feel like this 
is that critical bug that it should go in during the freeze.

Michal