[libvirt] [PATCHv2 3/4] network: set macvtap/hostdev networks active if their state file exists

[Laine Stump]

[Did you miss Patch 0.5/4? Or just get to the end of the day first?]

On 04/23/2014 10:00 PM, John Ferlan wrote:
>
> On 04/17/2014 07:43 AM, Laine Stump wrote:
>> libvirt attempts to determine at startup time which networks are
>> already active, and set their active flags. Previously it has done
>> this by assuming that all networks are inactive, then setting the
>> active flag if the network has a bridge device associated with it and
>> that bridge device exists. This is not useful for macvtap and hostdev
>> based networks, since they do not use a bridge device.
>>
>> Of course the reason that such a check had to be done was that the
>> presence of a status file in the network "stateDir" couldn't be
>> trusted as an indicator of whether or not a network was active. This
>> was due to the network driver mistakenly using
>> /var/lib/libvirt/network to store the status files, rather than
>> /var/run/libvirt/network (similar to what is done by every other
>> libvirt driver that stores status xml for its objects). The difference
>> is that /var/run is cleared out when the host reboots, so you can be
>> assured that the state file you are seeing isn't just left over from a
>> previous boot of the host.
>>
>> Now that the network driver has been switched to using
>> /var/run/libvirt/network for status, we can also modify it to assume
>> that any network with an existing status file is by definition active
>> - we do this when reading the status file. To fine tune the results,
>> networkFindActiveConfigs() is changed to networkUpdateAllState(),
>> and only sets active = 0 if the conditions for particular network
>> types are *not* met.
>>
>> The result is that during the first run of libvirtd after the host
>> boots, there are no status files, so no networks are active. Any time
>> libvirtd is restarted, any network with a status file will be marked
>> as active (unless the network uses a bridge device and that device for
>> some reason doesn't exist).
>> ---
>> Changes from V1:
>>
>> * rename networkFindActiveConfigs() to networkUpdateAllState() (rather
>>   than networkFindInactiveConfigs()
>>
>> * undo the change in order of calling the above function
>>   vs. virNetworkReadAllConfigs(), just in case that would cause some
>>   undetected regression.
>>
>> * extricate the reading of pidfiles from the switch statement that
>>   behaves differently for different types of networks - those networks
>>   that don't use dnsmasq/radvd will not have any pidfiles for them
>>   anyway, so it becomes a NOP (and if a new network type that *does*
>>   use one of those processes is created, it will automatically work
>>   correctly here.)
>>
>>
>>  src/conf/network_conf.c     |  1 +
>>  src/network/bridge_driver.c | 69 ++++++++++++++++++++++++++++++++-------------
>>  2 files changed, 51 insertions(+), 19 deletions(-)
>>
>> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
>> index 56c4a09..69ad929 100644
>> --- a/src/conf/network_conf.c
>> +++ b/src/conf/network_conf.c
>> @@ -3060,6 +3060,7 @@ virNetworkLoadState(virNetworkObjListPtr nets,
>>          net->floor_sum = floor_sum_val;
>>  
>>      net->taint = taint;
>> +    net->active = 1; /* any network with a state file is by definition active */
> Typing and thinking - it's a dangerous combination... Since the driver
> initialization now "moves" files from /lib/ to /pid/ without regard to
> whether they were active previously in /lib/ - is it possible that other
> code will erroneously mark something active?
>
> The prior code would look through the state files to mark active... This
> code says, I found a state file thus I must be active.  Of course only
> true for migration case - so hmm.... does the previous patch need a tweak?

Unfortunately there isn't any other place to look to see if the network
is active.

In the case of a network that uses a bridge device, here is the difference:

OLD: assume network is inactive, but mark it active if a bridge device
is found, whether state file exists or not.

                  Bridge Device
                  Yes   No
 State File  Yes   A     I
              No   A     I

NEW: *IFF* state file exists, assume network is active, but mark it
inactive if bridge doesn't exist.

                  Bridge Device
                  Yes   No
 State File  Yes   A     I
              No   I     I

So the one difference is when there is no state file but the bridge
device exists.

However, the previous run of (and version of) libvirt would have never
marked the network Active in the first place if it was unable to
successfully write the state file, so if anything we could encounter
cases where the state file *existed* when the network should actually be
inactive (the opposite of what would cause a problem), and that would
only occur if a "new" libvirt is run immediately after rebooting a
system that last ran the "old" libvirt prior to reboot (which isn't the
way that upgrades normally happen).

In the case of networks that *don't* use a bridge device... well, their
status was already broken, and that's what we're trying to fix -
basically before these patches, those networks were just *always*
considered to be inactive when libvirtd started.

>>    cleanup:
>>      VIR_FREE(configFile);
>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>> index 57dfb2d..0c879b9 100644
>> --- a/src/network/bridge_driver.c
>> +++ b/src/network/bridge_driver.c
>> @@ -328,38 +328,69 @@ networkBridgeDummyNicName(const char *brname)
>>      return nicname;
>>  }
>>  
>> +/* Update the internal status of all allegedly active networks
>> + * according to external conditions on the host (i.e. anything that
>> + * isn't stored directly in each network's state file). */
>>  static void
>> -networkFindActiveConfigs(virNetworkDriverStatePtr driver)
>> +networkUpdateAllState(virNetworkDriverStatePtr driver)
>>  {
>>      size_t i;
>>  
>>      for (i = 0; i < driver->networks.count; i++) {
>>          virNetworkObjPtr obj = driver->networks.objs[i];
>>  
>> +        if (!obj->active)
>> +           continue;
>> +
>>          virNetworkObjLock(obj);
>>  
>> -        /* If bridge exists, then mark it active */
>> -        if (obj->def->bridge &&
>> -            virNetDevExists(obj->def->bridge) == 1) {
>> -            obj->active = 1;
>> +        switch (obj->def->forward.type) {
>> +        case VIR_NETWORK_FORWARD_NONE:
>> +        case VIR_NETWORK_FORWARD_NAT:
>> +        case VIR_NETWORK_FORWARD_ROUTE:
>> +            /* If bridge exists, then mark it active */
>> +            if (!(obj->def->bridge && virNetDevExists(obj->def->bridge) == 1))
>> +                obj->active = 0;
>> +            break;
> Comment is still misleading...

Yep, fixed that finally.


Thanks!

>
> ACK with that change
>
>