Nothing that could happen during networkNotifyActualDevice() could justify unceremoniously killing the qemu process, but that's what we were doing.

In particular, new code added in commit 85bcc022 (first appearred in

libvirt-3.2.0) attempts to reattach tap devices to their assigned bridge devices when libvirtd restarts (to make it easier to recover from a restart of a libvirt network). But if the network has been stopped and *not* restarted, the bridge device won't exist and networkNotifyActualDevice() will fail. This patch changes qemuProcessNotifyNets() to return void, so that qemuProcessReconnect() will soldier on regardless of what happens (any errors will still be logged, but we won't terminate the guest). Partially resolves: https://bugzilla.redhat.com/1442700 --- NB: there are many other things that might fail during reconnection of a qemu process that shouldn't lead to killing the qemu process. qemuProcessReconnect() could use a good audit to change what happens when certain items fail.

src/qemu/qemu_process.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 53170d7..fcb5763 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2825,7 +2825,7 @@ int qemuProcessStopCPUs(virQEMUDriverPtr driver, -static int +static void qemuProcessNotifyNets(virDomainDefPtr def) { size_t i; @@ -2840,10 +2840,8 @@ qemuProcessNotifyNets(virDomainDefPtr def) if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT) ignore_value(virNetDevMacVLanReserveName(net->ifname, false)); - if (networkNotifyActualDevice(def, net) < 0) - return -1; + networkNotifyActualDevice(def, net);

} - return 0; } static int @@ -3480,8 +3478,7 @@ qemuProcessReconnect(void *opaque) if (qemuSecurityReserveLabel(driver->securityManager, obj->def, obj->pid) < 0) goto error; - if (qemuProcessNotifyNets(obj->def) < 0) - goto error; + qemuProcessNotifyNets(obj->def); if (qemuProcessFiltersInstantiate(obj->def)) goto error;

On 04/25/2017 12:34 PM, Laine Stump wrote: > If the network isn't active during networkNotifyActualDevice(), we > would log an error message stating that the bridge device didn't > exist. This patch adds a check to see if the network is active, making > the logs more useful in the case that it isn't. > > Partially resolves: https://bugzilla.redhat.com/1442700 > --- > src/network/bridge_driver.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c > index d2d8557..e06f81b 100644 > --- a/src/network/bridge_driver.c > +++ b/src/network/bridge_driver.c > @@ -4676,6 +4676,13 @@ networkNotifyActualDevice(virDomainDefPtr dom, > } > netdef = network->def; > > + if (!virNetworkObjIsActive(network)) { > + virReportError(VIR_ERR_OPERATION_INVALID, > + _("network '%s' is not active"), > + netdef->name); > + goto error; > + } > + /me wonders whether this should just a goto cleanup - IOW: if the network isn't active, so what, why error. Once someone attempts to start it, they'll get errors I assume... Not that goto error or cleanup matters since commit id '4fee4e0' changed the goto cleanup to goto error and added: + +error: + goto cleanup;

I guess I don't have the answer readily available in my head as to how much of the subsequent code would be called at network start time?

John > /* if we're restarting libvirtd after an upgrade from a version > * that didn't save bridge name in actualNetDef for > * actualType==network, we need to copy it in so that it will be >

On 04/26/2017 05:57 PM, Laine Stump wrote: > On 04/26/2017 02:39 PM, John Ferlan wrote: >> >> >> On 04/25/2017 12:34 PM, Laine Stump wrote: >>> If the network isn't active during networkNotifyActualDevice(), we >>> would log an error message stating that the bridge device didn't >>> exist. This patch adds a check to see if the network is active, making >>> the logs more useful in the case that it isn't. >>> >>> Partially resolves: https://bugzilla.redhat.com/1442700 >>> --- >>> src/network/bridge_driver.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c >>> index d2d8557..e06f81b 100644 >>> --- a/src/network/bridge_driver.c >>> +++ b/src/network/bridge_driver.c >>> @@ -4676,6 +4676,13 @@ networkNotifyActualDevice(virDomainDefPtr dom, >>> } >>> netdef = network->def; >>> >>> + if (!virNetworkObjIsActive(network)) { >>> + virReportError(VIR_ERR_OPERATION_INVALID, >>> + _("network '%s' is not active"), >>> + netdef->name); >>> + goto error; >>> + } >>> + >> >> /me wonders whether this should just a goto cleanup - IOW: if the >> network isn't active, so what, why error. Once someone attempts to start >> it, they'll get errors I assume... >> >> Not that goto error or cleanup matters since commit id '4fee4e0' changed >> the goto cleanup to goto error and added: >> >> + >> +error: >> + goto cleanup; > > That's missing the point of that commit. "cleanup:" is there only as a > place for error: to goto the cleanup code that is common to both the > "success:" exit and the "error:" exit (and the three labels are all > there so that that this function is structured similarly to > networkAllocateActualDevice() - I wanted them to be as close to the same > as possible). In the body of the function you either declare the > allocate/notification a success (with "goto success") in which case the > connect count for the network goes up, or you declare it a failure (with > "goto error") in which case the connect count for the network remains > unchanged, but you never directly goto the noncommittal "cleanup:". > OK understood - it's just one of those odd looking constructs to have a goto error and goto cleanup immediately following it, but your reasoning for having the logic this way is understandable. >> >> I guess I don't have the answer readily available in my head as to how >> much of the subsequent code would be called at network start time? > > None of this is called when the network is started - we have no way for > the networkStart() function to learn which interfaces in which domains > are supposed to be connected to a particular network. That needs more > infrastructure change than I wanted to put in right now (we need some > sort of event or callback that will allow the network driver to notify > all active hypervisor drivers whenever a network is started (or > destroyed?), giving the hypervisor driver a chance to call > networkNotifyActualDevice() for any affected domain interface). > > So have I explained myself well enough? > Sure, but for my context... I guess I was putting myself in the mindset of a libvirtd reconnect where "who knows" what was done with each network prior to reconnection and this is the discovery of that. For some paths it's a hard death issue - the physical connection had problems or went away. For other paths it's more of a soft or state issue - someone stopped/destroyed, but didn't undefine a network. I was thinking more that in this latter case, does an error make sense?