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
domani startup or NIC hotplug), network is notified and so is
virMacMap module subsequently. The module update functions
networkMacMgrAdd() and networkMacMgrDel() handle the case when
there's no module gracefully. Problem is, the module is created
if and only if network is freshly started, or if daemon restarts
and network previously had the module.

This is not very user friendly - if users want to use the NSS
module they need to destroy their network and bring it up again
(and subsequently all the domains using it).

One disadvantage of this approach implemented here is that one
may get just partial results: any already running network does
not record mac maps, thus only newly plugged domains will be
stored in the module. The network restart scenario is not touched
by this of course. But one can argue that older libvirts had
never recorded the mac maps anyway.

Signed-off-by: Michal Privoznik <mprivozn@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;
 
     case VIR_NETWORK_FORWARD_BRIDGE:
@@ -512,7 +520,6 @@ networkUpdateState(virNetworkObjPtr obj,
     /* Try and read dnsmasq/radvd pids of active networks */
     if (obj->active && obj->def->ips && (obj->def->nips > 0)) {
         char *radvdpidbase;
-        char *macMapFile;
 
         ignore_value(virPidFileReadIfAlive(driver->pidDir,
                                            obj->def->name,
@@ -527,23 +534,13 @@ networkUpdateState(virNetworkObjPtr obj,
                                            radvdpidbase,
                                            &obj->radvdPid, RADVD));
         VIR_FREE(radvdpidbase);
-
-        if (!(macMapFile = networkMacMgrFileName(driver, obj->def->bridge)))
-            goto cleanup;
-
-        if (virFileExists(macMapFile) &&
-            !(obj->macmap = virMacMapNew(macMapFile))) {
-            VIR_FREE(macMapFile);
-            goto cleanup;
-        }
-
-        VIR_FREE(macMapFile);
     }
 
     ret = 0;
  cleanup:
     virObjectUnlock(obj);
     virObjectUnref(dnsmasq_caps);
+    VIR_FREE(macMapFile);
     return ret;
 }
 
-- 
2.11.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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
> domani startup or NIC hotplug), network is notified and so is

s/domani/domain/

> virMacMap module subsequently. The module update functions
> networkMacMgrAdd() and networkMacMgrDel() handle the case when
> there's no module gracefully.

"gracefully handle the case when there's no module"


> Problem is, the module is created

The problem is...

> if and only if network is freshly started, or if daemon restarts

*the* daemon

> and network previously had the module.
> 
> This is not very user friendly - if users want to use the NSS
> module they need to destroy their network and bring it up again
> (and subsequently all the domains using it).

(note that it's no longer quite as bad - as of a few days ago, if you
restart a network, restarting libvirtd will reconnect all guests that
were previously connected to that network. Still not nice of course...)
> 
> One disadvantage of this approach implemented here is that one
> may get just partial results: any already running network does
> not record mac maps, thus only newly plugged domains will be
> stored in the module. The network restart scenario is not touched
> by this of course. But one can argue that older libvirts had
> never recorded the mac maps anyway.

It's unclear above where you're talking about the way the code was, and
the way it is after this patch...

> 
> Signed-off-by: Michal Privoznik <mprivozn@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.

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.


So I'm confused - I must be missing something obvious. Can you explain a
bit more?

>  
>      case VIR_NETWORK_FORWARD_BRIDGE:
> @@ -512,7 +520,6 @@ networkUpdateState(virNetworkObjPtr obj,
>      /* Try and read dnsmasq/radvd pids of active networks */
>      if (obj->active && obj->def->ips && (obj->def->nips > 0)) {
>          char *radvdpidbase;
> -        char *macMapFile;
>  
>          ignore_value(virPidFileReadIfAlive(driver->pidDir,
>                                             obj->def->name,
> @@ -527,23 +534,13 @@ networkUpdateState(virNetworkObjPtr obj,
>                                             radvdpidbase,
>                                             &obj->radvdPid, RADVD));
>          VIR_FREE(radvdpidbase);
> -
> -        if (!(macMapFile = networkMacMgrFileName(driver, obj->def->bridge)))
> -            goto cleanup;
> -
> -        if (virFileExists(macMapFile) &&
> -            !(obj->macmap = virMacMapNew(macMapFile))) {
> -            VIR_FREE(macMapFile);
> -            goto cleanup;
> -        }
> -
> -        VIR_FREE(macMapFile);
>      }
>  
>      ret = 0;
>   cleanup:
>      virObjectUnlock(obj);
>      virObjectUnref(dnsmasq_caps);
> +    VIR_FREE(macMapFile);
>      return ret;
>  }
>  
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 03/28/2017 10:42 PM, Martin Kletzander wrote:
> <snip/>
 >
> 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 :-/

I feel like the consensus is either to have a pointer to an object (in 
which case it's enabled) or not have a pointer at all.

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

Fair enough. Thanks.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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@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?

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. If not, then I still don't understand (and don't have time to try
any further, as I have to leave to catch a plane in a couple hours and
I'm not finished packing :-). If the conditions of my ACK don't hold,
keep in mind that this isn't a NACK - I just haven't understood what is
the gain, but if somebody else understands it, they can certainly ACK it
and I won't complain.


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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list