[libvirt] [PATCH v2] process: Ignore nwfilter binding instantiation issues during reconnect
https://bugzilla.redhat.com/show_bug.cgi?id=1607202 It's essentially stated in the nwfilterBindingDelete that we will allow the admin to shoot themselves in the foot by deleting the nwfilter binding which then allows them to undefine the nwfilter that is in use for the running guest... However, by allowing this we cause a problem for libvirtd restart reconnect processing which would then try to recreate the missing binding attempting to use the deleted filter resulting in an error and thus shutting the guest down. So rather than keep adding virDomainConfNWFilterInstantiate flags to "ignore" specific error conditions and since (so far) this is the only path that cared about checking if the filter already exists and ignoring, let's just ignore all errors and make the qemuProcessFiltersInstantiate be a void which will attempt to check all networks for bindings and reload all filters that exist. Using the VIR_INFO in order to at least "log" the avoided issue. This also means virDomainConfNWFilterInstantiate no longer needs to handle/check the ignoreExists possbility. Signed-off-by: John Ferlan <jferlan@redhat.com> --- v1: https://www.redhat.com/archives/libvir-list/2018-August/msg01407.html Changes - removed the ignoreExists and just change the logic for reconnect processing to essentially ignore all errors. If it's felt the VIR_INFO would be too chatty (especially since it'll be generated for every already defined filter binding), I can remove it. Another option would be to keep the ignoreExists logic and only generate that VIR_INFO for "other" messages. In that case, I'd probably want to change it to a VIR_WARN. Still figured I'd post the remove it all option first for consideration with this caveat so that "option" can be considered as well. src/conf/domain_nwfilter.c | 15 +++------------ src/conf/domain_nwfilter.h | 3 +-- src/lxc/lxc_process.c | 2 +- src/qemu/qemu_hotplug.c | 4 ++-- src/qemu/qemu_interface.c | 4 ++-- src/qemu/qemu_process.c | 23 +++++++++++++++-------- src/uml/uml_conf.c | 2 +- 7 files changed, 25 insertions(+), 28 deletions(-) diff --git a/src/conf/domain_nwfilter.c b/src/conf/domain_nwfilter.c index f39c8a1f9b..51c9063ca7 100644 --- a/src/conf/domain_nwfilter.c +++ b/src/conf/domain_nwfilter.c @@ -84,8 +84,7 @@ virNWFilterBindingDefForNet(const char *vmname, int virDomainConfNWFilterInstantiate(const char *vmname, const unsigned char *vmuuid, - virDomainNetDefPtr net, - bool ignoreExists) + virDomainNetDefPtr net) { virConnectPtr conn = virGetConnectNWFilter(); virNWFilterBindingDefPtr def = NULL; @@ -93,20 +92,12 @@ virDomainConfNWFilterInstantiate(const char *vmname, 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", + vmname, NULLSTR(net->ifname), NULLSTR(net->filter)); if (!conn) goto cleanup; - if (ignoreExists) { - binding = virNWFilterBindingLookupByPortDev(conn, net->ifname); - if (binding) { - ret = 0; - goto cleanup; - } - } - if (!(def = virNWFilterBindingDefForNet(vmname, vmuuid, net))) goto cleanup; diff --git a/src/conf/domain_nwfilter.h b/src/conf/domain_nwfilter.h index 6bda228fc8..d2ebeff853 100644 --- a/src/conf/domain_nwfilter.h +++ b/src/conf/domain_nwfilter.h @@ -25,8 +25,7 @@ int virDomainConfNWFilterInstantiate(const char *vmname, const unsigned char *vmuuid, - virDomainNetDefPtr net, - bool ignoreExists); + virDomainNetDefPtr net); void virDomainConfNWFilterTeardown(virDomainNetDefPtr net); void virDomainConfVMNWFilterTeardown(virDomainObjPtr vm); diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 33c806630b..86f7463e53 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -303,7 +303,7 @@ virLXCProcessSetupInterfaceTap(virDomainDefPtr vm, } if (net->filter && - virDomainConfNWFilterInstantiate(vm->name, vm->uuid, net, false) < 0) + virDomainConfNWFilterInstantiate(vm->name, vm->uuid, net) < 0) goto cleanup; ret = containerVeth; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 0b84a503bb..38c74bd9b1 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -3435,7 +3435,7 @@ qemuDomainChangeNetFilter(virDomainObjPtr vm, if (newdev->filter && virDomainConfNWFilterInstantiate(vm->def->name, - vm->def->uuid, newdev, false) < 0) { + vm->def->uuid, newdev) < 0) { virErrorPtr errobj; virReportError(VIR_ERR_OPERATION_FAILED, @@ -3444,7 +3444,7 @@ qemuDomainChangeNetFilter(virDomainObjPtr vm, olddev->ifname); virErrorPreserveLast(&errobj); ignore_value(virDomainConfNWFilterInstantiate(vm->def->name, - vm->def->uuid, olddev, false)); + vm->def->uuid, olddev)); virErrorRestore(&errobj); return -1; } diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c index a3f13093f5..5d54a85c53 100644 --- a/src/qemu/qemu_interface.c +++ b/src/qemu/qemu_interface.c @@ -467,7 +467,7 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def, goto cleanup; if (net->filter && - virDomainConfNWFilterInstantiate(def->name, def->uuid, net, false) < 0) { + virDomainConfNWFilterInstantiate(def->name, def->uuid, net) < 0) { goto cleanup; } @@ -586,7 +586,7 @@ qemuInterfaceBridgeConnect(virDomainDefPtr def, goto cleanup; if (net->filter && - virDomainConfNWFilterInstantiate(def->name, def->uuid, net, false) < 0) { + virDomainConfNWFilterInstantiate(def->name, def->uuid, net) < 0) { goto cleanup; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index ab749389ee..48d9bab128 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3160,22 +3160,30 @@ qemuProcessNotifyNets(virDomainDefPtr def) } } -static int -qemuProcessFiltersInstantiate(virDomainDefPtr def, bool ignoreExists) + +/* Attempt to instantiate the filters. Ignore failures because it's + * (primarily) possible that a filter binding either already exists + * or someone deleted it and the associated filter while the guest + * was running and we don't want that action to cause failure to + * keep the guest running during the reconnection processing. */ +static void +qemuProcessFiltersInstantiate(virDomainDefPtr def) { 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) - return 1; + if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net) < 0) { + VIR_INFO("filter '%s' instantiation for '%s' failed '%s'", + net->filter, net->ifname, virGetLastErrorMessage()); + virResetLastError(); + } } } - - return 0; } + static int qemuProcessUpdateState(virQEMUDriverPtr driver, virDomainObjPtr vm) { @@ -7892,8 +7900,7 @@ qemuProcessReconnect(void *opaque) qemuProcessNotifyNets(obj->def); - if (qemuProcessFiltersInstantiate(obj->def, true)) - goto error; + qemuProcessFiltersInstantiate(obj->def); if (qemuProcessRefreshDisks(driver, obj, QEMU_ASYNC_JOB_NONE) < 0) goto error; diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index f116e619ef..9c548f0e80 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -137,7 +137,7 @@ umlConnectTapDevice(virDomainDefPtr vm, } if (net->filter) { - if (virDomainConfNWFilterInstantiate(vm->name, vm->uuid, net, false) < 0) { + if (virDomainConfNWFilterInstantiate(vm->name, vm->uuid, net) < 0) { if (template_ifname) VIR_FREE(net->ifname); goto error; -- 2.17.1
On Fri, Aug 24, 2018 at 08:30:50AM -0400, John Ferlan wrote:
+static void +qemuProcessFiltersInstantiate(virDomainDefPtr def) { 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) - return 1; + if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net) < 0) { + VIR_INFO("filter '%s' instantiation for '%s' failed '%s'", + net->filter, net->ifname, virGetLastErrorMessage());
Won't this cause a log message on every single running guests on every libvirtd restart, since the normal scenario is that the filter binding exists ?
+ virResetLastError(); + }
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 :|
On 08/24/2018 09:04 AM, Daniel P. Berrangé wrote:
On Fri, Aug 24, 2018 at 08:30:50AM -0400, John Ferlan wrote:
+static void +qemuProcessFiltersInstantiate(virDomainDefPtr def) { 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) - return 1; + if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net) < 0) { + VIR_INFO("filter '%s' instantiation for '%s' failed '%s'", + net->filter, net->ifname, virGetLastErrorMessage());
Won't this cause a log message on every single running guests on every libvirtd restart, since the normal scenario is that the filter binding exists ?
Yes, that was explained under the --- John
On Fri, Aug 24, 2018 at 09:05:06AM -0400, John Ferlan wrote:
On 08/24/2018 09:04 AM, Daniel P. Berrangé wrote:
On Fri, Aug 24, 2018 at 08:30:50AM -0400, John Ferlan wrote:
+static void +qemuProcessFiltersInstantiate(virDomainDefPtr def) { 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) - return 1; + if (virDomainConfNWFilterInstantiate(def->name, def->uuid, net) < 0) { + VIR_INFO("filter '%s' instantiation for '%s' failed '%s'", + net->filter, net->ifname, virGetLastErrorMessage());
Won't this cause a log message on every single running guests on every libvirtd restart, since the normal scenario is that the filter binding exists ?
Yes, that was explained under the ---
Next time I will actually read the text :-) 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 :|
On Fri, Aug 24, 2018 at 08:30:50AM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1607202
It's essentially stated in the nwfilterBindingDelete that we will allow the admin to shoot themselves in the foot by deleting the nwfilter binding which then allows them to undefine the nwfilter that is in use for the running guest...
However, by allowing this we cause a problem for libvirtd restart reconnect processing which would then try to recreate the missing binding attempting to use the deleted filter resulting in an error and thus shutting the guest down.
So rather than keep adding virDomainConfNWFilterInstantiate flags to "ignore" specific error conditions and since (so far) this is the only path that cared about checking if the filter already exists and ignoring, let's just ignore all errors and make the qemuProcessFiltersInstantiate be a void which will attempt to check all networks for bindings and reload all filters that exist. Using the VIR_INFO in order to at least "log" the avoided issue.
This also means virDomainConfNWFilterInstantiate no longer needs to handle/check the ignoreExists possbility.
Signed-off-by: John Ferlan <jferlan@redhat.com> ---
v1: https://www.redhat.com/archives/libvir-list/2018-August/msg01407.html
Changes - removed the ignoreExists and just change the logic for reconnect processing to essentially ignore all errors. If it's felt the VIR_INFO would be too chatty (especially since it'll be generated for every already defined filter binding), I can remove it. Another option would be to keep the ignoreExists logic and only generate that VIR_INFO for "other" messages. In that case, I'd probably want to change it to a VIR_WARN. Still figured I'd post the remove it all option first for consideration with this caveat so that "option" can be considered as well.
I guess the difference with 'ignore exists' is that this situation is harmless. The binding is still active and thus network traffic is confined. In the other error scenarios, the failure indicates a problem that likely means the network traffic is not confined. So it probably makes sense to keep the ignoreExists flag and use VIR_WARN for other errors. 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 :|
participants (2)
-
Daniel P. Berrangé -
John Ferlan