Re: [libvirt] [PATCH 4/5] network: set macvtap/hostdev networks active if their state file exists

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()?)

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.

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

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