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

John Ferlan posted 1 patch 5 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180822214321.29819-1-jferlan@redhat.com
Test syntax-check passed
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(-)
[libvirt] [PATCH] nwfilter: Handle libvirtd restart if nwfilter binding deleted
Posted by John Ferlan 5 years, 7 months ago
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
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@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(-)

diff --git a/src/conf/domain_nwfilter.c b/src/conf/domain_nwfilter.c
index f39c8a1f9b..3e6e462def 100644
--- a/src/conf/domain_nwfilter.c
+++ b/src/conf/domain_nwfilter.c
@@ -85,16 +85,19 @@ int
 virDomainConfNWFilterInstantiate(const char *vmname,
                                  const unsigned char *vmuuid,
                                  virDomainNetDefPtr net,
-                                 bool ignoreExists)
+                                 bool ignoreExists,
+                                 bool ignoreDeleted)
 {
     virConnectPtr conn = virGetConnectNWFilter();
     virNWFilterBindingDefPtr def = NULL;
     virNWFilterBindingPtr binding = NULL;
+    virNWFilterPtr nwfilter = NULL;
     char *xml = NULL;
     int ret = -1;
 
-    VIR_DEBUG("vmname=%s portdev=%s filter=%s ignoreExists=%d",
-              vmname, NULLSTR(net->ifname), NULLSTR(net->filter), ignoreExists);
+    VIR_DEBUG("vmname=%s portdev=%s filter=%s ignoreExists=%d ignoreDeleted=%d",
+              vmname, NULLSTR(net->ifname), NULLSTR(net->filter),
+              ignoreExists, ignoreDeleted);
 
     if (!conn)
         goto cleanup;
@@ -113,14 +116,34 @@ virDomainConfNWFilterInstantiate(const char *vmname,
     if (!(xml = virNWFilterBindingDefFormat(def)))
         goto cleanup;
 
-    if (!(binding = virNWFilterBindingCreateXML(conn, xml, 0)))
-        goto cleanup;
+    if (!(binding = virNWFilterBindingCreateXML(conn, xml, 0))) {
+        virErrorPtr orig_err;
+
+        if (!ignoreDeleted)
+            goto cleanup;
+
+        /* Let's determine if the error was because the filter was deleted.
+         * Save the orig_err just in case it's not a failure to find the
+         * filter by name. */
+        orig_err = virSaveLastError();
+        nwfilter = virNWFilterLookupByName(conn, def->filter);
+        virSetError(orig_err);
+        virFreeError(orig_err);
+        if (nwfilter)
+            goto cleanup;
+
+        VIR_WARN("filter '%s' for binding '%s' has been deleted while the "
+                 "guest was running, ignoring for restart processing",
+                 def->filter, def->portdevname);
+        virResetLastError();
+    }
 
     ret = 0;
 
  cleanup:
     VIR_FREE(xml);
     virNWFilterBindingDefFree(def);
+    virObjectUnref(nwfilter);
     virObjectUnref(binding);
     virObjectUnref(conn);
     return ret;
diff --git a/src/conf/domain_nwfilter.h b/src/conf/domain_nwfilter.h
index 6bda228fc8..e3a2f7a7f2 100644
--- a/src/conf/domain_nwfilter.h
+++ b/src/conf/domain_nwfilter.h
@@ -26,7 +26,8 @@
 int virDomainConfNWFilterInstantiate(const char *vmname,
                                      const unsigned char *vmuuid,
                                      virDomainNetDefPtr net,
-                                     bool ignoreExists);
+                                     bool ignoreExists,
+                                     bool ignoreDeleted);
 void virDomainConfNWFilterTeardown(virDomainNetDefPtr net);
 void virDomainConfVMNWFilterTeardown(virDomainObjPtr vm);
 
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c
index 33c806630b..b8b014ca72 100644
--- a/src/lxc/lxc_process.c
+++ b/src/lxc/lxc_process.c
@@ -303,7 +303,8 @@ virLXCProcessSetupInterfaceTap(virDomainDefPtr vm,
     }
 
     if (net->filter &&
-        virDomainConfNWFilterInstantiate(vm->name, vm->uuid, net, false) < 0)
+        virDomainConfNWFilterInstantiate(vm->name, vm->uuid, net,
+                                         false, false) < 0)
         goto cleanup;
 
     ret = containerVeth;
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index 0b84a503bb..11b10cbe14 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3434,8 +3434,8 @@ qemuDomainChangeNetFilter(virDomainObjPtr vm,
     virDomainConfNWFilterTeardown(olddev);
 
     if (newdev->filter &&
-        virDomainConfNWFilterInstantiate(vm->def->name,
-                                         vm->def->uuid, newdev, false) < 0) {
+        virDomainConfNWFilterInstantiate(vm->def->name, vm->def->uuid, newdev,
+                                        false, false) < 0) {
         virErrorPtr errobj;
 
         virReportError(VIR_ERR_OPERATION_FAILED,
@@ -3444,7 +3444,8 @@ qemuDomainChangeNetFilter(virDomainObjPtr vm,
                        olddev->ifname);
         virErrorPreserveLast(&errobj);
         ignore_value(virDomainConfNWFilterInstantiate(vm->def->name,
-                                                      vm->def->uuid, olddev, false));
+                                                      vm->def->uuid, olddev,
+                                                      false, false));
         virErrorRestore(&errobj);
         return -1;
     }
diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
index a3f13093f5..fc5f39b76d 100644
--- a/src/qemu/qemu_interface.c
+++ b/src/qemu/qemu_interface.c
@@ -467,7 +467,8 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def,
         goto cleanup;
 
     if (net->filter &&
-        virDomainConfNWFilterInstantiate(def->name, def->uuid, net, false) < 0) {
+        virDomainConfNWFilterInstantiate(def->name, def->uuid, net,
+                                         false, false) < 0) {
         goto cleanup;
     }
 
@@ -586,7 +587,8 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def,
         goto cleanup;
 
     if (net->filter &&
-        virDomainConfNWFilterInstantiate(def->name, def->uuid, net, false) < 0) {
+        virDomainConfNWFilterInstantiate(def->name, def->uuid, net,
+                                         false, false) < 0) {
         goto cleanup;
     }
 
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index ab749389ee..4d8b3017b4 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3161,14 +3161,18 @@ qemuProcessNotifyNets(virDomainDefPtr def)
 }
 
 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;
         }
     }
@@ -7892,7 +7896,7 @@ qemuProcessReconnect(void *opaque)
 
     qemuProcessNotifyNets(obj->def);
 
-    if (qemuProcessFiltersInstantiate(obj->def, true))
+    if (qemuProcessFiltersInstantiate(obj->def, true, true))
         goto error;
 
     if (qemuProcessRefreshDisks(driver, obj, QEMU_ASYNC_JOB_NONE) < 0)
diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
index f116e619ef..29d26848f3 100644
--- a/src/uml/uml_conf.c
+++ b/src/uml/uml_conf.c
@@ -137,7 +137,8 @@ umlConnectTapDevice(virDomainDefPtr vm,
     }
 
     if (net->filter) {
-        if (virDomainConfNWFilterInstantiate(vm->name, vm->uuid, net, false) < 0) {
+        if (virDomainConfNWFilterInstantiate(vm->name, vm->uuid, net,
+                                             false, false) < 0) {
             if (template_ifname)
                 VIR_FREE(net->ifname);
             goto error;
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nwfilter: Handle libvirtd restart if nwfilter binding deleted
Posted by Daniel P. Berrangé 5 years, 7 months ago
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
> 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@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(-)
> 
> diff --git a/src/conf/domain_nwfilter.c b/src/conf/domain_nwfilter.c
> index f39c8a1f9b..3e6e462def 100644
> --- a/src/conf/domain_nwfilter.c
> +++ b/src/conf/domain_nwfilter.c
> @@ -85,16 +85,19 @@ int
>  virDomainConfNWFilterInstantiate(const char *vmname,
>                                   const unsigned char *vmuuid,
>                                   virDomainNetDefPtr net,
> -                                 bool ignoreExists)
> +                                 bool ignoreExists,
> +                                 bool ignoreDeleted)
>  {
>      virConnectPtr conn = virGetConnectNWFilter();
>      virNWFilterBindingDefPtr def = NULL;
>      virNWFilterBindingPtr binding = NULL;
> +    virNWFilterPtr nwfilter = NULL;
>      char *xml = NULL;
>      int ret = -1;
>  
> -    VIR_DEBUG("vmname=%s portdev=%s filter=%s ignoreExists=%d",
> -              vmname, NULLSTR(net->ifname), NULLSTR(net->filter), ignoreExists);
> +    VIR_DEBUG("vmname=%s portdev=%s filter=%s ignoreExists=%d ignoreDeleted=%d",
> +              vmname, NULLSTR(net->ifname), NULLSTR(net->filter),
> +              ignoreExists, ignoreDeleted);
>  
>      if (!conn)
>          goto cleanup;
> @@ -113,14 +116,34 @@ virDomainConfNWFilterInstantiate(const char *vmname,
>      if (!(xml = virNWFilterBindingDefFormat(def)))
>          goto cleanup;
>  
> -    if (!(binding = virNWFilterBindingCreateXML(conn, xml, 0)))
> -        goto cleanup;
> +    if (!(binding = virNWFilterBindingCreateXML(conn, xml, 0))) {
> +        virErrorPtr orig_err;
> +
> +        if (!ignoreDeleted)
> +            goto cleanup;
> +
> +        /* Let's determine if the error was because the filter was deleted.
> +         * Save the orig_err just in case it's not a failure to find the
> +         * filter by name. */
> +        orig_err = virSaveLastError();
> +        nwfilter = virNWFilterLookupByName(conn, def->filter);
> +        virSetError(orig_err);
> +        virFreeError(orig_err);
> +        if (nwfilter)
> +            goto cleanup;
> +
> +        VIR_WARN("filter '%s' for binding '%s' has been deleted while the "
> +                 "guest was running, ignoring for restart processing",
> +                 def->filter, def->portdevname);
> +        virResetLastError();
> +    }

I don't get what this code is trying to do. This method is about creating
nwfilter bindings. If virNWFilterBindingCreateXML() fails, that means the
filter binding already exists. But the stated problem was that the admin
had deleted the filter binding. The code is also checking a filter, not
a filter binding.

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nwfilter: Handle libvirtd restart if nwfilter binding deleted
Posted by John Ferlan 5 years, 7 months ago

On 08/23/2018 03:44 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
>> 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@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(-)
>>
>> diff --git a/src/conf/domain_nwfilter.c b/src/conf/domain_nwfilter.c
>> index f39c8a1f9b..3e6e462def 100644
>> --- a/src/conf/domain_nwfilter.c
>> +++ b/src/conf/domain_nwfilter.c
>> @@ -85,16 +85,19 @@ int
>>  virDomainConfNWFilterInstantiate(const char *vmname,
>>                                   const unsigned char *vmuuid,
>>                                   virDomainNetDefPtr net,
>> -                                 bool ignoreExists)
>> +                                 bool ignoreExists,
>> +                                 bool ignoreDeleted)
>>  {
>>      virConnectPtr conn = virGetConnectNWFilter();
>>      virNWFilterBindingDefPtr def = NULL;
>>      virNWFilterBindingPtr binding = NULL;
>> +    virNWFilterPtr nwfilter = NULL;
>>      char *xml = NULL;
>>      int ret = -1;
>>  
>> -    VIR_DEBUG("vmname=%s portdev=%s filter=%s ignoreExists=%d",
>> -              vmname, NULLSTR(net->ifname), NULLSTR(net->filter), ignoreExists);
>> +    VIR_DEBUG("vmname=%s portdev=%s filter=%s ignoreExists=%d ignoreDeleted=%d",
>> +              vmname, NULLSTR(net->ifname), NULLSTR(net->filter),
>> +              ignoreExists, ignoreDeleted);
>>  
>>      if (!conn)
>>          goto cleanup;
>> @@ -113,14 +116,34 @@ virDomainConfNWFilterInstantiate(const char *vmname,
>>      if (!(xml = virNWFilterBindingDefFormat(def)))
>>          goto cleanup;
>>  
>> -    if (!(binding = virNWFilterBindingCreateXML(conn, xml, 0)))
>> -        goto cleanup;
>> +    if (!(binding = virNWFilterBindingCreateXML(conn, xml, 0))) {
>> +        virErrorPtr orig_err;
>> +
>> +        if (!ignoreDeleted)
>> +            goto cleanup;
>> +
>> +        /* Let's determine if the error was because the filter was deleted.
>> +         * Save the orig_err just in case it's not a failure to find the
>> +         * filter by name. */
>> +        orig_err = virSaveLastError();
>> +        nwfilter = virNWFilterLookupByName(conn, def->filter);
>> +        virSetError(orig_err);
>> +        virFreeError(orig_err);
>> +        if (nwfilter)
>> +            goto cleanup;
>> +
>> +        VIR_WARN("filter '%s' for binding '%s' has been deleted while the "
>> +                 "guest was running, ignoring for restart processing",
>> +                 def->filter, def->portdevname);
>> +        virResetLastError();
>> +    }
> 
> I don't get what this code is trying to do. This method is about creating
> nwfilter bindings. If virNWFilterBindingCreateXML() fails, that means the
> filter binding already exists. But the stated problem was that the admin
> had deleted the filter binding. The code is also checking a filter, not
> a filter binding.
> 

virNWFilterBindingCreateXML eventually calls nwfilterBindingCreateXML,
which will virNWFilterBindingObjListAdd the binding, get the @ret
binding and attempt to virNWFilterInstantiateFilter the binding @def.

Instantiate will call virNWFilterInstantiateFilterInternal which calls
virNWFilterInstantiateFilterUpdate (eventually) which calls
virNWFilterObjListFindInstantiateFilter because someone deleted the
filter after deleting the portdev before hand.

As the bz shows, one cannot delete the filter because it's active in the
portdev; however, deleting the portdev allows one to delete the filter.
Once the port is deleted, the 'ebtables -t nat -L' will show it's gone.

Attempting to create the portdev again when libvirtd restarts (because
virDomainConfNWFilterInstantiate is called during that processing) will
cause a failure and the guest will be stopped.

Prior to the changes, adding a debug print in nwfilterBindingCreateXML
of the XML I see the following in a debug libvirtd session restart:

Detaching after fork from child process 13163.
2018-08-22 17:08:58.878+0000: 13162: warning :
nwfilterBindingCreateXML:748 : xml=<filterbinding>
  <owner>
    <name>f23</name>
    <uuid>43dc0cf8-809b-4adb-9bea-a9abb5f3d90e</uuid>
  </owner>
  <portdev name='vnet0'/>
  <mac address='52:54:00:f7:d4:f7'/>
  <filterref filter='clean-traffic'/>
</filterbinding>

2018-08-22 17:08:58.878+0000: 13162: error :
virNWFilterObjListFindInstantiateFilter:201 : internal error: referenced
filter 'clean-traffic' is missing
...

One finds the guest shutdown, the subsequent start would also fail:

[Thread 0x7fffa7fff700 (LWP 13083) exited]
2018-08-22 17:16:33.035+0000: 13068: warning :
nwfilterBindingCreateXML:748 : xml=<filterbinding>
  <owner>
    <name>f23</name>
    <uuid>43dc0cf8-809b-4adb-9bea-a9abb5f3d90e</uuid>
  </owner>
  <portdev name='vnet0'/>
  <mac address='52:54:00:f7:d4:f7'/>
  <filterref filter='clean-traffic'/>
</filterbinding>

2018-08-22 17:16:33.036+0000: 13068: error :
virNWFilterObjListFindInstantiateFilter:201 : internal error: referenced
filter 'clean-traffic' is missing
2018-08-22 17:16:33.065+0000: 13157: error : virFileReadAll:1435 :
Failed to open file '/sys/class/net/vnet0/operstate': No such file or
directory
2018-08-22 17:16:33.065+0000: 13157: error : virNetDevGetLinkInfo:2559 :
unable to read: /sys/class/net/vnet0/operstate: No such file or directory
....

With the patch in place, I'll now get:

Detaching after fork from child process 6919.
2018-08-22 21:29:45.201+0000: 6918: error :
virNWFilterObjListFindInstantiateFilter:201 : internal error: referenced
filter 'clean-traffic' is missing
2018-08-22 21:29:45.201+0000: 6918: warning :
virDomainConfNWFilterInstantiate:135 : filter 'clean-traffic' for
binding 'vnet0' has been deleted while the guest was running, ignoring
for restart processing
[Thread 0x7fff9dffb700 (LWP 6918) exited]

....

After restarting libvirtd, I can define the filter again and use the
*CreateXML API in order to reinstate the filter for the guest as shown
in the ebtables output.

And yes, I understand/agree that if some admin ends up using the
virNWFilterBindingDelete API they are on their own, but I think the
shutdown would be unexpected. Another "option" I considered was to alter
the live XML to remove the net->filter so that a subsequent libvirtd
restart wouldn't fail. That may be accomplish-able via some @flag usage
for the *Delete API to know the call didn't come from the hypervisor
induced call, but I wasn't sure if it was "fail safe" and altering the
live XML for the guest has it's own drawbacks/failures that would need
to be handled.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nwfilter: Handle libvirtd restart if nwfilter binding deleted
Posted by Daniel P. Berrangé 5 years, 7 months ago
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/

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


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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nwfilter: Handle libvirtd restart if nwfilter binding deleted
Posted by John Ferlan 5 years, 7 months ago

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

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.

Again, I could go back and figure out a way for the *Delete path to
remove the live net->filter based on the path taken so that when we get
here we wouldn't find the net->filter and thus wouldn't call the
instantiate function. It's just a different approach, but I'm hesitant
mainly because then one wouldn't be able to use the *CreateXML API to
reinstate the filter. I'm not as clear about all the (wacko) use cases.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] nwfilter: Handle libvirtd restart if nwfilter binding deleted
Posted by Daniel P. Berrangé 5 years, 7 months ago
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@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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list