[libvirt] [PATCH 4/5] network: set macvtap/hostdev networks active if their state file exists
John Ferlan
jferlan at redhat.com
Wed Apr 16 11:51:27 UTC 2014
On 04/16/2014 06:01 AM, Laine Stump wrote:
> On 04/15/2014 03:31 PM, John Ferlan wrote:
>> On 04/10/2014 09:19 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 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
>>> libirt driver that stores status xml for its objects). The difference
>>> is that /var/run is cleared out when the host reboots, so you cvqan 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 networkFindInactiveConfigs(),
>>> 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
>>> it is restarted, and network with a status file will be marked as
>>> inactive (unless it uses a bridge device and that device for some
>>> reason doesn't exist).
>>> ---
>>> src/conf/network_conf.c | 1 +
>>> src/network/bridge_driver.c | 49 +++++++++++++++++++++++++++++++++------------
>>> 2 files changed, 37 insertions(+), 13 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 */
>>>
>>> cleanup:
>>> VIR_FREE(configFile);
>>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
>>> index 98d10bd..7beab79 100644
>>> --- a/src/network/bridge_driver.c
>>> +++ b/src/network/bridge_driver.c
>>> @@ -328,7 +328,7 @@ networkBridgeDummyNicName(const char *brname)
>>> }
>>>
>>> static void
>>> -networkFindActiveConfigs(virNetworkDriverStatePtr driver)
>>> +networkFindInactiveConfigs(virNetworkDriverStatePtr driver)
>>> {
>>> size_t i;
>>>
>>> @@ -336,29 +336,51 @@ networkFindActiveConfigs(virNetworkDriverStatePtr driver)
>>> virNetworkObjPtr obj = driver->networks.objs[i];
>>>
>>> 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;
>> Goes against comment... double negatives in if condition are throwing
>> me off.
>
>
> Yeah, the original code assumed active == 0, and set active = 1 if the
> bridge existed. I forgot to change the comments.
>
>
>> The switch/case break almost got me too (thinking that the
>> break was to break out of the for loop).
>>
>>> + break;
>>> + }
>>>
>>> /* Try and read dnsmasq/radvd pids if any */
>>> if (obj->def->ips && (obj->def->nips > 0)) {
>>> char *radvdpidbase;
>>>
>>> - ignore_value(virPidFileReadIfAlive(driverState->pidDir, obj->def->name,
>>> + ignore_value(virPidFileReadIfAlive(driverState->pidDir,
>>> + obj->def->name,
>>> &obj->dnsmasqPid,
>>> dnsmasqCapsGetBinaryPath(driver->dnsmasqCaps)));
>>> -
>>> - if (!(radvdpidbase = networkRadvdPidfileBasename(obj->def->name)))
>>> - goto cleanup;
>>> - ignore_value(virPidFileReadIfAlive(driverState->pidDir, radvdpidbase,
>>> + radvdpidbase = networkRadvdPidfileBasename(obj->def->name);
>>> + if (!radvdpidbase)
>>> + break;
>>> + ignore_value(virPidFileReadIfAlive(driverState->pidDir,
>>> + radvdpidbase,
>>> &obj->radvdPid, RADVD));
>>> VIR_FREE(radvdpidbase);
>> I'm confused as to why the code would be doing this as the active value
>> isn't changed as a result of this. Was it just "convenient" to do it
>> here? In the context of the old code, if we're marking active, then
>> this code runs. In the context of the new code, for only specific types
>> of network forward types that we've ostensibly already deemed active
>> somewhere (virNetworkLoadState()?)
Your call on that - although the virNetworkLoadState (Load, Check,
Change, Adjust, Update) seems more appropriate since the code is not
just adjusting active/inactive value. As an aside, seems as though
active could be bool rather than int, but that's a different rabbit hole.
>
> Correct. Previously the basic assumption was that all networks were
> inactive when libvirtd restarted and we would only mark them active if
> they had a bridge and it existed. Since some network types don't use a
> bridge, this is now inadequate, and there's no other external info we
> can use to make this determination, so instead we assume that any
> network which has a state file (that's why the function now runs
> *before* loading persistent configs - so the list only contains those
> with state files) is active, and then we change the polarity of the
> function checking external info to set a network inactive if the check
> fails.
>
>> we'll then run this code...
>
> This is a case of evolution of the code - setting of the active flag and
> refreshing the pids from the pidfiles have been together in the same
> function since "a long time ago", as they both happened during driver
> initialization, and both required cycling through the list of all
> network objects. Perhaps it would be less offensive if the function name
> was changed from networkFindInactiveConfigs() to networkUpdateAllState()
> (or something like that). The idea is that we've just read the state of
> all networks on disk, and this function will update the results with any
> external information from the host, e.g. if a bridge has disappeared
> then the network must be marked down, and the pids (which are not stored
> in the state file as that would be redundant information) are reread
> from the pidfiles.
>
> Also, I thought I had seen you wondering somewhere about the fact that
> the check for pidfiles was previously done for all active networks, but
> now it is only done for networks with forward mode of none, route, or
> nat. While the previous code ended up behaving properly, this was
> accidental - none of the other types of networks use dnsmasq or radvd,
> so they would never have a pidfile.
>
Ah - ok... Some things are less obvious when you're reading/reviewing
code rather than having it firmly ingrained in your brain as a given...
>> and I'm
>> assuming that if we get this far obj->active == 1 - whether that's true
>> or not I'm not 100%, but I assume you'd know. Of course, that could be
>> the problem you're trying to resolve in 5...
>>
>> Furthermore, since the check for inactive is before the load of the
>> config now, are you getting what you expected in the right place here?
>
> Yes. "config" is the persistent config, which is completely
> self-contained in the xml file in /etc/libvirt/qemu/network, but the
> pids are part of the network's state.
>
>
>> Previously it'd be load all configs, check active. Now it's check
>> inactive, load all configs. If the above is loaded during that process,
>> then the above is all for naught.
>
> You mean the pids? Nope, those are unrelated to the config.
>
This was more an "ordering comment" especially as it related to the call
from networkStateInitialize(). I've seen cases where code like this was
changed and other code moved where in the call stack a call such as this
was made and then result was unexpected behaviour.
At the very least as long as the comments are updated - that's fine.
Makes it easier for the next person reading it.
John
>>
>>
>> ACK pending your thoughts on this above hunk. Might need to see a v2...
>>
>> John
>>
>>> }
>>> + break;
>>> +
>>> + case VIR_NETWORK_FORWARD_BRIDGE:
>>> + if (obj->def->bridge) {
>>> + if (virNetDevExists(obj->def->bridge) != 1)
>>> + obj->active = 0;
>>> + break;
>>> + }
>>> + /* intentionally drop through to common case for all
>>> + * macvtap networks (forward='bridge' with no bridge
>>> + * device defined is macvtap using its 'bridge' mode)
>>> + */
>>> + case VIR_NETWORK_FORWARD_PRIVATE:
>>> + case VIR_NETWORK_FORWARD_VEPA:
>>> + case VIR_NETWORK_FORWARD_PASSTHROUGH:
>>> + case VIR_NETWORK_FORWARD_HOSTDEV:
>>> + break;
>>> }
>>>
>>> - cleanup:
>>> virNetworkObjUnlock(obj);
>>> }
>>>
>>> @@ -542,12 +564,13 @@ networkStateInitialize(bool privileged,
>>> driverState->stateDir) < 0)
>>> goto error;
>>>
>>> + networkFindInactiveConfigs(driverState);
>>> +
>>> if (virNetworkLoadAllConfigs(&driverState->networks,
>>> driverState->networkConfigDir,
>>> driverState->networkAutostartDir) < 0)
>>> goto error;
>>>
>>> - networkFindActiveConfigs(driverState);
>>> networkReloadFirewallRules(driverState);
>>> networkRefreshDaemons(driverState);
>>>
>>>
>
More information about the libvir-list
mailing list