[libvirt] [PATCH] nwfilter: Handle libvirtd restart if nwfilter binding deleted

[Daniel P. Berrangé]

On Thu, Aug 23, 2018 at 07:59:41AM -0400, John Ferlan wrote:
> 
> 
> On 08/23/2018 07:27 AM, Daniel P. Berrangé wrote:
> > On Wed, Aug 22, 2018 at 05:43:21PM -0400, John Ferlan wrote:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1607202
> >>
> >> It's stated that if the admin wants to shoot themselves in
> >> the foot by removing the nwfilter binding while the domain
> > 
> > So based on your explanation in the other reply, this message
> > is what was misleading me. s/nwfilter binding/nwfilter/
> > 
> 
> Actually perhaps it's more by "first removing the nwfilter binding and
> subsequently undefining the nwfilter that is/was in use for the running
> guest..."
> 
> If just the nwfilter binding was removed, then libvirtd restart would
> have been fine because it would have recreated the nwfilter binding. Of
> course that may not be expected either...
> 
> >> is running we will certainly allow that.  However, in doing
> >> so we also run the risk that a libvirtd restart will cause
> >> the domain to be shutdown, which isn't a good thing.
> >>
> >> So add another boolean to virDomainConfNWFilterInstantiate
> >> which allows us to recover somewhat gracefully in the event
> >> the virNWFilterBindingCreateXML fails when we come from
> >> qemuProcessReconnect and we determine that the filter has
> >> been deleted. It was there at some point (it had to be), but
> >> if it's missing, then we don't want to cause the guest to
> >> stop running, so issue a warning and continue on.
> >>
> >> Signed-off-by: John Ferlan <jferlan at redhat.com>
> >> ---
> >>  src/conf/domain_nwfilter.c | 33 ++++++++++++++++++++++++++++-----
> >>  src/conf/domain_nwfilter.h |  3 ++-
> >>  src/lxc/lxc_process.c      |  3 ++-
> >>  src/qemu/qemu_hotplug.c    |  7 ++++---
> >>  src/qemu/qemu_interface.c  |  6 ++++--
> >>  src/qemu/qemu_process.c    | 10 +++++++---
> >>  src/uml/uml_conf.c         |  3 ++-
> >>  7 files changed, 49 insertions(+), 16 deletions(-)
> > 
> > [snip]
> > 
> >>  static int
> >> -qemuProcessFiltersInstantiate(virDomainDefPtr def, bool ignoreExists)
> >> +qemuProcessFiltersInstantiate(virDomainDefPtr def,
> >> +                              bool ignoreExists,
> >> +                              bool ignoreDeleted)
> >>  {
> >>      size_t i;
> >>  
> >>      for (i = 0; i < def->nnets; i++) {
> >>          virDomainNetDefPtr net = def->nets[i];
> >>          if ((net->filter) && (net->ifname)) {
> >> -            if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net, ignoreExists) < 0)
> >> +            if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net,
> >> +                                                 ignoreExists,
> >> +                                                 ignoreDeleted) < 0)
> >>                  return 1;
> >>          }
> > 
> > Rather than this extra "ignoreDeleted" arg, why can't we just do
> > 
> >            if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net,
> >                                                  ignoreExists) < 0 &&
> > 						 ignoreDeleted)
> >                 return 1;           
> > 
> > This ensures that all things which can cause a nwfilter binding failure
> > on startup will be handled by avoiding tearing down the running guest.
> > 
> 
> Did you mean !ignoreDeleted? There's only one caller to

Heh, yes.

> qemuProcessFiltersInstantiate which does:
> 
>     if (qemuProcessFiltersInstantiate(obj->def, true))
>         goto error;
> 
> Of course what's the purpose of distinguishing between ignoreExists and
> ignoreDeleted then? Essentially what you're indicating is we wouldn't
> care about any error because virDomainConfNWFilterInstantiate wouldn't
> be distinguishing between any error (because there's only one caller to
> qemuProcessFiltersInstantiate).

Oh thats a good point - I forgot ignoreExists was for the same reason.

> 
> I could change the last argument to virDomainConfNWFilterInstantiate to
> be a flag instead of a bool so that if we have future errors we care to
> ignore we don't keep adding bool arguments, but that's just a
> implementation detail.

We've deal with 1 problem scenario (alredy existing binding) and now
adding a second (missing filter). Will someone find a third scenario
and then a fourth. The flags argument ends up effectively being a
bitmask of lines where we ignore errors.

I'm wondering is it *ever* valid to treat failure of this filter call
as a fatal problem and teardown an already running VM ?  My gut says no.

So perhaps we remove the ignoreExists parameter too, and just make that
one caller simply ignore the errors on restarts.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|