[libvirt] [PATCH 1/2] qemu: don't kill qemu process on restart if networkNotify fails

[John Ferlan]

On 04/25/2017 12:33 PM, Laine Stump wrote:
> Nothing that could happen during networkNotifyActualDevice() could
> justify unceremoniously killing the qemu process, but that's what we
> were doing.

So that galaxy in far far away land circa commit id '04711a0f' when all
this was added -

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

appeared

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

Nose game, not me ;-)

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

Seems like networkNotifyActualDevice should then return a void too?
And have it's comments/content changed...


IIUC thought, essentially if the network goes away, the domain isn't
necessarily shut down - it gets notified somehow that the network isn't
there any more and soldiers on. So from that aspect I can certain agree
that just because we discover an error when libvirtd restarts doesn't
mean we should now kill the domain.

I think the concept behind the patch is fine - it's the "how much more"
should be done...  (and yes with the eye on keeping backports to a
minimum, yep understood).


John

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