[PATCH 0/5] Clear QoS for OVS more frequently

*** BLURB HERE *** Michal Prívozník (5): virnetdevopenvswitch: Fix comment to virNetDevOpenvswitchInterfaceGetMaster() hypervisor: Introduce and use virDomainInterfaceVportRemove() virnetdevopenvswitch: Drop @brname arg from virNetDevOpenvswitchRemovePort() conf: Move virDomainClearNetBandwidth() to src/hypervisor/ domain_interface: Introduce and use virDomainInterfaceClearQoS() src/conf/netdev_bandwidth_conf.c | 14 ------ src/conf/netdev_bandwidth_conf.h | 3 -- src/hypervisor/domain_interface.c | 78 ++++++++++++++++++++++++++----- src/hypervisor/domain_interface.h | 5 ++ src/libvirt_private.syms | 4 +- src/lxc/lxc_driver.c | 14 ++---- src/lxc/lxc_process.c | 18 +++---- src/qemu/qemu_hotplug.c | 40 ++-------------- src/util/virnetdevopenvswitch.c | 4 +- src/util/virnetdevopenvswitch.h | 4 +- src/util/virnetdevtap.c | 2 +- 11 files changed, 95 insertions(+), 91 deletions(-) -- 2.43.2

The comment to virNetDevOpenvswitchInterfaceGetMaster() contains wrong function name. Fix this. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virnetdevopenvswitch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index d836d05845..7d1cd25171 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -427,7 +427,7 @@ virNetDevOpenvswitchInterfaceStats(const char *ifname, /** - * virNetDeOpenvswitchGetMaster: + * virNetDevOpenvswitchInterfaceGetMaster: * @ifname: name of interface we're interested in * @master: used to return a string containing the name of @ifname's "master" * (this is the bridge or bond device that this device is attached to) -- 2.43.2

On Fri, Apr 12, 2024 at 12:19:01 +0200, Michal Privoznik wrote:
The comment to virNetDevOpenvswitchInterfaceGetMaster() contains wrong function name. Fix this.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

Both LXC and QEMU drivers have the same code to remove vport when removing a domain's interface. Instead of repeating the same pattern in both drivers, move the code into hypervisor agnostic location (src/hypervisor/) and switch to calling this new function. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/hypervisor/domain_interface.c | 38 ++++++++++++++++++++++--------- src/hypervisor/domain_interface.h | 1 + src/libvirt_private.syms | 1 + src/lxc/lxc_driver.c | 9 +++----- src/lxc/lxc_process.c | 18 +++++---------- src/qemu/qemu_hotplug.c | 22 ++---------------- 6 files changed, 40 insertions(+), 49 deletions(-) diff --git a/src/hypervisor/domain_interface.c b/src/hypervisor/domain_interface.c index 853814fe78..0003412065 100644 --- a/src/hypervisor/domain_interface.c +++ b/src/hypervisor/domain_interface.c @@ -374,6 +374,32 @@ virDomainInterfaceStopDevices(virDomainDef *def) return 0; } + +/** + * virDomainInterfaceVportRemove: + * @net: a net definition in the VM + * + * Removes vport profile from corresponding bridge. + * NOP if no vport profile is present in @net. + */ +void +virDomainInterfaceVportRemove(virDomainNetDef *net) +{ + const virNetDevVPortProfile *vport = virDomainNetGetActualVirtPortProfile(net); + const char *brname; + + if (!vport) + return; + + if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_MIDONET) { + ignore_value(virNetDevMidonetUnbindPort(vport)); + } else if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) { + brname = virDomainNetGetActualBridgeName(net); + ignore_value(virNetDevOpenvswitchRemovePort(brname, net->ifname)); + } +} + + /** * virDomainInterfaceDeleteDevice: * @def: domain definition @@ -390,10 +416,8 @@ virDomainInterfaceDeleteDevice(virDomainDef *def, bool priv_net_created, char *stateDir) { - const virNetDevVPortProfile *vport = NULL; g_autoptr(virConnect) conn = NULL; - vport = virDomainNetGetActualVirtPortProfile(net); switch (virDomainNetGetActualType(net)) { case VIR_DOMAIN_NET_TYPE_DIRECT: if (priv_net_created) { @@ -435,15 +459,7 @@ virDomainInterfaceDeleteDevice(virDomainDef *def, /* release the physical device (or any other resources used by * this interface in the network driver */ - if (vport) { - if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_MIDONET) { - ignore_value(virNetDevMidonetUnbindPort(vport)); - } else if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) { - ignore_value(virNetDevOpenvswitchRemovePort( - virDomainNetGetActualBridgeName(net), - net->ifname)); - } - } + virDomainInterfaceVportRemove(net); /* kick the device out of the hostdev list too */ virDomainNetRemoveHostdev(def, net); diff --git a/src/hypervisor/domain_interface.h b/src/hypervisor/domain_interface.h index 3d15e000cc..8047fdadfa 100644 --- a/src/hypervisor/domain_interface.h +++ b/src/hypervisor/domain_interface.h @@ -39,6 +39,7 @@ int virDomainInterfaceStartDevice(virDomainNetDef *net); int virDomainInterfaceStartDevices(virDomainDef *def); int virDomainInterfaceStopDevice(virDomainNetDef *net); int virDomainInterfaceStopDevices(virDomainDef *def); +void virDomainInterfaceVportRemove(virDomainNetDef *net); void virDomainInterfaceDeleteDevice(virDomainDef *def, virDomainNetDef *net, bool priv_net_created, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 84e30b711c..8642305a8b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1642,6 +1642,7 @@ virDomainInterfaceStartDevice; virDomainInterfaceStartDevices; virDomainInterfaceStopDevice; virDomainInterfaceStopDevices; +virDomainInterfaceVportRemove; # hypervisor/virclosecallbacks.h diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 39992bdf96..89ef9416aa 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -70,6 +70,7 @@ #include "virhostdev.h" #include "netdev_bandwidth_conf.h" #include "virutil.h" +#include "domain_interface.h" #define VIR_FROM_THIS VIR_FROM_LXC @@ -4042,7 +4043,6 @@ lxcDomainDetachDeviceNetLive(virDomainObj *vm, int detachidx, ret = -1; virDomainNetType actualType; virDomainNetDef *detach = NULL; - const virNetDevVPortProfile *vport = NULL; virErrorPtr save_err = NULL; if ((detachidx = virDomainNetFindIdx(vm->def, dev->data.net)) < 0) @@ -4098,11 +4098,8 @@ lxcDomainDetachDeviceNetLive(virDomainObj *vm, virDomainConfNWFilterTeardown(detach); - vport = virDomainNetGetActualVirtPortProfile(detach); - if (vport && vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) - ignore_value(virNetDevOpenvswitchRemovePort( - virDomainNetGetActualBridgeName(detach), - detach->ifname)); + virDomainInterfaceVportRemove(detach); + ret = 0; cleanup: if (!ret) { diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index bfdcefd01b..30ff4eb3d0 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -49,6 +49,7 @@ #include "virprocess.h" #include "netdev_bandwidth_conf.h" #include "virutil.h" +#include "domain_interface.h" #define VIR_FROM_THIS VIR_FROM_LXC @@ -149,7 +150,6 @@ static void virLXCProcessCleanup(virLXCDriver *driver, { size_t i; virLXCDomainObjPrivate *priv = vm->privateData; - const virNetDevVPortProfile *vport = NULL; g_autoptr(virLXCDriverConfig) cfg = virLXCDriverGetConfig(driver); g_autoptr(virConnect) conn = NULL; @@ -210,13 +210,9 @@ static void virLXCProcessCleanup(virLXCDriver *driver, for (i = 0; i < vm->def->nnets; i++) { virDomainNetDef *iface = vm->def->nets[i]; - vport = virDomainNetGetActualVirtPortProfile(iface); + if (iface->ifname) { - if (vport && - vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) - ignore_value(virNetDevOpenvswitchRemovePort( - virDomainNetGetActualBridgeName(iface), - iface->ifname)); + virDomainInterfaceVportRemove(iface); ignore_value(virNetDevVethDelete(iface->ifname)); } if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK) { @@ -637,11 +633,9 @@ virLXCProcessSetupInterfaces(virLXCDriver *driver, virErrorPreserveLast(&save_err); for (i = 0; i < def->nnets; i++) { virDomainNetDef *iface = def->nets[i]; - const virNetDevVPortProfile *vport = virDomainNetGetActualVirtPortProfile(iface); - if (vport && vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) - ignore_value(virNetDevOpenvswitchRemovePort( - virDomainNetGetActualBridgeName(iface), - iface->ifname)); + + virDomainInterfaceVportRemove(iface); + if (iface->type == VIR_DOMAIN_NET_TYPE_NETWORK && netconn) virDomainNetReleaseActualDevice(netconn, iface); } diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 62dc879ed4..054053729c 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1091,24 +1091,6 @@ qemuDomainAttachDeviceDiskLive(virQEMUDriver *driver, } -static void -qemuDomainNetDeviceVportRemove(virDomainNetDef *net) -{ - const virNetDevVPortProfile *vport = virDomainNetGetActualVirtPortProfile(net); - const char *brname; - - if (!vport) - return; - - if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_MIDONET) { - ignore_value(virNetDevMidonetUnbindPort(vport)); - } else if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) { - brname = virDomainNetGetActualBridgeName(net); - ignore_value(virNetDevOpenvswitchRemovePort(brname, net->ifname)); - } -} - - static int qemuDomainAttachNetDevice(virQEMUDriver *driver, virDomainObj *vm, @@ -1414,7 +1396,7 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver, cfg->stateDir); } - qemuDomainNetDeviceVportRemove(net); + virDomainInterfaceVportRemove(net); } if (teardownlabel && @@ -4895,7 +4877,7 @@ qemuDomainRemoveNetDevice(virQEMUDriver *driver, VIR_WARN("Unable to restore security label on vhostuser char device"); } - qemuDomainNetDeviceVportRemove(net); + virDomainInterfaceVportRemove(net); if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK) { g_autoptr(virConnect) conn = virGetConnectNetwork(); -- 2.43.2

On Fri, Apr 12, 2024 at 12:19:02 +0200, Michal Privoznik wrote:
Both LXC and QEMU drivers have the same code to remove vport when removing a domain's interface. Instead of repeating the same pattern in both drivers, move the code into hypervisor agnostic location (src/hypervisor/) and switch to calling this new function.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

The @brname argument of virNetDevOpenvswitchRemovePort() is and was unused ever since its introduction in v0.9.11-rc1~257. Just remove it. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/hypervisor/domain_interface.c | 4 +--- src/util/virnetdevopenvswitch.c | 2 +- src/util/virnetdevopenvswitch.h | 4 ++-- src/util/virnetdevtap.c | 2 +- 4 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/hypervisor/domain_interface.c b/src/hypervisor/domain_interface.c index 0003412065..ccf4cb94bd 100644 --- a/src/hypervisor/domain_interface.c +++ b/src/hypervisor/domain_interface.c @@ -386,7 +386,6 @@ void virDomainInterfaceVportRemove(virDomainNetDef *net) { const virNetDevVPortProfile *vport = virDomainNetGetActualVirtPortProfile(net); - const char *brname; if (!vport) return; @@ -394,8 +393,7 @@ virDomainInterfaceVportRemove(virDomainNetDef *net) if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_MIDONET) { ignore_value(virNetDevMidonetUnbindPort(vport)); } else if (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) { - brname = virDomainNetGetActualBridgeName(net); - ignore_value(virNetDevOpenvswitchRemovePort(brname, net->ifname)); + ignore_value(virNetDevOpenvswitchRemovePort(net->ifname)); } } diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index 7d1cd25171..f1765ae1c8 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -206,7 +206,7 @@ int virNetDevOpenvswitchAddPort(const char *brname, const char *ifname, * * Returns 0 in case of success or -1 in case of failure. */ -int virNetDevOpenvswitchRemovePort(const char *brname G_GNUC_UNUSED, const char *ifname) +int virNetDevOpenvswitchRemovePort(const char *ifname) { g_autofree char *errbuf = NULL; g_autoptr(virCommand) cmd = virNetDevOpenvswitchCreateCmd(&errbuf); diff --git a/src/util/virnetdevopenvswitch.h b/src/util/virnetdevopenvswitch.h index e6ee985f17..a20fb5b029 100644 --- a/src/util/virnetdevopenvswitch.h +++ b/src/util/virnetdevopenvswitch.h @@ -38,8 +38,8 @@ int virNetDevOpenvswitchAddPort(const char *brname, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) G_GNUC_WARN_UNUSED_RESULT; -int virNetDevOpenvswitchRemovePort(const char *brname, const char *ifname) - ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; +int virNetDevOpenvswitchRemovePort(const char *ifname) + ATTRIBUTE_NONNULL(1) G_GNUC_WARN_UNUSED_RESULT; int virNetDevOpenvswitchInterfaceGetMaster(const char *ifname, char **master) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index 3bfd36fc23..9a4866a4e4 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -543,7 +543,7 @@ virNetDevTapReattachBridge(const char *tapname, int ret; VIR_INFO("Removing %s from %s", tapname, master); if (useOVS) - ret = virNetDevOpenvswitchRemovePort(master, tapname); + ret = virNetDevOpenvswitchRemovePort(tapname); else ret = virNetDevBridgeRemovePort(master, tapname); if (ret < 0) -- 2.43.2

On Fri, Apr 12, 2024 at 12:19:03 +0200, Michal Privoznik wrote:
The @brname argument of virNetDevOpenvswitchRemovePort() is and was unused ever since its introduction in v0.9.11-rc1~257. Just remove it.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

The reason virDomainClearNetBandwidth() exists in src/conf/ is that at the time its introduction we did not have a better place. But now we do. Firstly, virDomainClearNetBandwidth() is hypervisor agnostic code, but really has nothing to do with domain configuration (it doesn't parse/format XML). Secondly, in near future it'll call another function from src/hypervisor/ and that's not really allowed from src/conf/. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/netdev_bandwidth_conf.c | 14 -------------- src/conf/netdev_bandwidth_conf.h | 3 --- src/hypervisor/domain_interface.c | 16 ++++++++++++++++ src/hypervisor/domain_interface.h | 2 ++ src/libvirt_private.syms | 2 +- 5 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/conf/netdev_bandwidth_conf.c b/src/conf/netdev_bandwidth_conf.c index cdf289270a..9faa46a27f 100644 --- a/src/conf/netdev_bandwidth_conf.c +++ b/src/conf/netdev_bandwidth_conf.c @@ -199,20 +199,6 @@ virNetDevBandwidthFormat(const virNetDevBandwidth *def, return 0; } -void -virDomainClearNetBandwidth(virDomainDef *def) -{ - size_t i; - virDomainNetType type; - - for (i = 0; i < def->nnets; i++) { - type = virDomainNetGetActualType(def->nets[i]); - if (virDomainNetGetActualBandwidth(def->nets[i]) && - virNetDevSupportsBandwidth(type)) - virNetDevBandwidthClear(def->nets[i]->ifname); - } -} - bool virNetDevSupportsBandwidth(virDomainNetType type) { diff --git a/src/conf/netdev_bandwidth_conf.h b/src/conf/netdev_bandwidth_conf.h index c698cc9dbc..b679b0f51f 100644 --- a/src/conf/netdev_bandwidth_conf.h +++ b/src/conf/netdev_bandwidth_conf.h @@ -34,9 +34,6 @@ int virNetDevBandwidthFormat(const virNetDevBandwidth *def, unsigned int class_id, virBuffer *buf); -void virDomainClearNetBandwidth(virDomainDef *def) - ATTRIBUTE_NONNULL(1); - bool virNetDevSupportsBandwidth(virDomainNetType type); bool virNetDevBandwidthHasFloor(const virNetDevBandwidth *b); bool virNetDevBandwidthSupportsFloor(virNetworkForwardType type); diff --git a/src/hypervisor/domain_interface.c b/src/hypervisor/domain_interface.c index ccf4cb94bd..0a9cad8011 100644 --- a/src/hypervisor/domain_interface.c +++ b/src/hypervisor/domain_interface.c @@ -27,6 +27,7 @@ #include "domain_driver.h" #include "domain_interface.h" #include "domain_nwfilter.h" +#include "netdev_bandwidth_conf.h" #include "network_conf.h" #include "viralloc.h" #include "virconftypes.h" @@ -469,3 +470,18 @@ virDomainInterfaceDeleteDevice(virDomainDef *def, } } + + +void +virDomainClearNetBandwidth(virDomainDef *def) +{ + size_t i; + virDomainNetType type; + + for (i = 0; i < def->nnets; i++) { + type = virDomainNetGetActualType(def->nets[i]); + if (virDomainNetGetActualBandwidth(def->nets[i]) && + virNetDevSupportsBandwidth(type)) + virNetDevBandwidthClear(def->nets[i]->ifname); + } +} diff --git a/src/hypervisor/domain_interface.h b/src/hypervisor/domain_interface.h index 8047fdadfa..bb212cf3b8 100644 --- a/src/hypervisor/domain_interface.h +++ b/src/hypervisor/domain_interface.h @@ -44,3 +44,5 @@ void virDomainInterfaceDeleteDevice(virDomainDef *def, virDomainNetDef *net, bool priv_net_created, char *stateDir); +void virDomainClearNetBandwidth(virDomainDef *def) + ATTRIBUTE_NONNULL(1); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8642305a8b..328f5b347b 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -813,7 +813,6 @@ virDomainMomentDefPostParse; # conf/netdev_bandwidth_conf.h -virDomainClearNetBandwidth; virNetDevBandwidthFormat; virNetDevBandwidthHasFloor; virNetDevBandwidthParse; @@ -1635,6 +1634,7 @@ virDomainDriverParseBlkioDeviceStr; virDomainDriverSetupPersistentDefBlkioParams; # hypervisor/domain_interface.h +virDomainClearNetBandwidth; virDomainInterfaceDeleteDevice; virDomainInterfaceEthernetConnect; virDomainInterfaceIsVnetCompatModel; -- 2.43.2

On Fri, Apr 12, 2024 at 12:19:04 +0200, Michal Privoznik wrote:
The reason virDomainClearNetBandwidth() exists in src/conf/ is that at the time its introduction we did not have a better place. But now we do. Firstly, virDomainClearNetBandwidth() is hypervisor agnostic code, but really has nothing to do with domain configuration (it doesn't parse/format XML). Secondly, in near future it'll call another function from src/hypervisor/ and that's not really allowed from src/conf/.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> ---
Reviewed-by: Peter Krempa <pkrempa@redhat.com>

In QEMU and LXC drivers in a few places just virNetDevBandwidthClear() is called. This means that if an interface is of openvswitch vport profile, its QoS is not removed. And to make matters worse - OVS is designed to remember state even when corresponding interface is gone. This leads to stale QoS settings piling up in OVS database. To resolve this, introduce virDomainInterfaceClearQoS() which looks at given interface and calls corresponding QoS clear function. Then, basically replace virNetDevBandwidthClear() calls in those hypervisor drivers with this new function. Resolves: https://issues.redhat.com/browse/RHEL-30373 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/hypervisor/domain_interface.c | 36 ++++++++++++++++++++++++++----- src/hypervisor/domain_interface.h | 2 ++ src/libvirt_private.syms | 1 + src/lxc/lxc_driver.c | 5 +---- src/qemu/qemu_hotplug.c | 18 +++------------- 5 files changed, 38 insertions(+), 24 deletions(-) diff --git a/src/hypervisor/domain_interface.c b/src/hypervisor/domain_interface.c index 0a9cad8011..cc6aa8551a 100644 --- a/src/hypervisor/domain_interface.c +++ b/src/hypervisor/domain_interface.c @@ -472,16 +472,42 @@ virDomainInterfaceDeleteDevice(virDomainDef *def, } +/** + * virDomainInterfaceClearQoS + * @def: domain definition + * @net: a net definition in the VM + * + * For given interface @net clear its QoS settings in the + * host. NOP if @net has no QoS or is of a type that doesn't + * support QoS. + * + * Returns: 0 on success, + * -1 otherwise (with appropriate error reported) + */ +int +virDomainInterfaceClearQoS(virDomainDef *def, + virDomainNetDef *net) +{ + if (!virDomainNetGetActualBandwidth(net)) + return 0; + + if (!virNetDevSupportsBandwidth(virDomainNetGetActualType(net))) + return 0; + + if (virDomainNetDefIsOvsport(net)) { + return virNetDevOpenvswitchInterfaceClearQos(net->ifname, def->uuid); + } + + return virNetDevBandwidthClear(net->ifname); +} + + void virDomainClearNetBandwidth(virDomainDef *def) { size_t i; - virDomainNetType type; for (i = 0; i < def->nnets; i++) { - type = virDomainNetGetActualType(def->nets[i]); - if (virDomainNetGetActualBandwidth(def->nets[i]) && - virNetDevSupportsBandwidth(type)) - virNetDevBandwidthClear(def->nets[i]->ifname); + virDomainInterfaceClearQoS(def, def->nets[i]); } } diff --git a/src/hypervisor/domain_interface.h b/src/hypervisor/domain_interface.h index bb212cf3b8..572b4dd8c5 100644 --- a/src/hypervisor/domain_interface.h +++ b/src/hypervisor/domain_interface.h @@ -44,5 +44,7 @@ void virDomainInterfaceDeleteDevice(virDomainDef *def, virDomainNetDef *net, bool priv_net_created, char *stateDir); +int virDomainInterfaceClearQoS(virDomainDef *def, + virDomainNetDef *net); void virDomainClearNetBandwidth(virDomainDef *def) ATTRIBUTE_NONNULL(1); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 328f5b347b..839fe4f545 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1635,6 +1635,7 @@ virDomainDriverSetupPersistentDefBlkioParams; # hypervisor/domain_interface.h virDomainClearNetBandwidth; +virDomainInterfaceClearQoS; virDomainInterfaceDeleteDevice; virDomainInterfaceEthernetConnect; virDomainInterfaceIsVnetCompatModel; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 89ef9416aa..1842ae8f0e 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4052,10 +4052,7 @@ lxcDomainDetachDeviceNetLive(virDomainObj *vm, actualType = virDomainNetGetActualType(detach); /* clear network bandwidth */ - if (virDomainNetGetActualBandwidth(detach) && - virNetDevSupportsBandwidth(actualType) && - virNetDevBandwidthClear(detach->ifname)) - goto cleanup; + virDomainInterfaceClearQoS(vm->def, detach); switch (actualType) { case VIR_DOMAIN_NET_TYPE_BRIDGE: diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 054053729c..774962b0df 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -4059,11 +4059,8 @@ qemuDomainChangeNet(virQEMUDriver *driver, goto cleanup; } } else { - /* - * virNetDevBandwidthSet() doesn't clear any existing - * setting unless something new is being set. - */ - virNetDevBandwidthClear(newdev->ifname); + if (virDomainInterfaceClearQoS(vm->def, newdev) < 0) + goto cleanup; } /* If the old bandwidth was cleared out, restore qdisc. */ @@ -4800,16 +4797,7 @@ qemuDomainRemoveNetDevice(virQEMUDriver *driver, if (!(charDevAlias = qemuAliasChardevFromDevAlias(net->info.alias))) return -1; - if (virNetDevSupportsBandwidth(virDomainNetGetActualType(net))) { - if (virDomainNetDefIsOvsport(net)) { - if (virNetDevOpenvswitchInterfaceClearQos(net->ifname, vm->def->uuid) < 0) - VIR_WARN("cannot clear bandwidth setting for ovs device : %s", - net->ifname); - } else if (virNetDevBandwidthClear(net->ifname) < 0) { - VIR_WARN("cannot clear bandwidth setting for device : %s", - net->ifname); - } - } + virDomainInterfaceClearQoS(vm->def, net); /* deactivate the tap/macvtap device on the host, which could also * affect the parent device (e.g. macvtap passthrough mode sets -- 2.43.2

On Fri, Apr 12, 2024 at 12:19:05 +0200, Michal Privoznik wrote:
In QEMU and LXC drivers in a few places just
s/just/only/ so that's more clear that something is missing.
virNetDevBandwidthClear() is called. This means that if an interface is of openvswitch vport profile, its QoS is not removed. And to make matters worse - OVS is designed to remember state even when corresponding interface is gone. This leads to stale QoS settings piling up in OVS database.
To resolve this, introduce virDomainInterfaceClearQoS() which looks at given interface and calls corresponding QoS clear function. Then, basically replace virNetDevBandwidthClear() calls in those hypervisor drivers with this new function.
Resolves: https://issues.redhat.com/browse/RHEL-30373 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/hypervisor/domain_interface.c | 36 ++++++++++++++++++++++++++----- src/hypervisor/domain_interface.h | 2 ++ src/libvirt_private.syms | 1 + src/lxc/lxc_driver.c | 5 +---- src/qemu/qemu_hotplug.c | 18 +++------------- 5 files changed, 38 insertions(+), 24 deletions(-)

On 4/12/24 16:33, Peter Krempa wrote:
On Fri, Apr 12, 2024 at 12:19:05 +0200, Michal Privoznik wrote:
In QEMU and LXC drivers in a few places just
s/just/only/ so that's more clear that something is missing.
Fixed locally. Does it warrant your Reviewed-by? ;-) Michal

On Fri, Apr 12, 2024 at 17:18:41 +0200, Michal Prívozník wrote:
On 4/12/24 16:33, Peter Krempa wrote:
On Fri, Apr 12, 2024 at 12:19:05 +0200, Michal Privoznik wrote:
In QEMU and LXC drivers in a few places just
s/just/only/ so that's more clear that something is missing.
Fixed locally. Does it warrant your Reviewed-by? ;-)
oops, yes ... I forgot to invoke the magic macro to add it before sending :/
participants (3)
-
Michal Privoznik
-
Michal Prívozník
-
Peter Krempa