On 11/8/21 5:18 AM, Michal Prívozník wrote:

On 11/3/21 4:11 AM, jx8zjs wrote:

...

From: zhangjl02 <zhangjl02@inspur.com>

Fix bug 1826168: bridge type network with ovs bridge can start with Qos setting which do not take any effect

Resolves:https://bugzilla.redhat.com/show_bug.cgi?id=1826168 Signed-off-by: zhangjl02 <zhangjl02@inspur.com> --- src/network/bridge_driver.c | 65 +++++++++++++++++++++++++++++++------ 1 file changed, 55 insertions(+), 10 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 498c45d0a7..d0627848cd 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2305,6 +2305,15 @@ networkAddRouteToBridge(virNetworkObj *obj, }

+static int +networkDefIsOvsBridge(virNetworkDef *def)

This @def should have been const too. And function returns a bool, effectively.

...

+{ + const virNetDevVPortProfile *vport = def->virtPortProfile; + return vport && + vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH; +} + + static int networkStartNetworkVirtual(virNetworkDriverState *driver, virNetworkObj *obj) @@ -2320,6 +2329,7 @@ networkStartNetworkVirtual(virNetworkDriverState *driver, bool dnsmasqStarted = false; bool devOnline = false; bool firewalRulesAdded = false; + bool ovsType = networkDefIsOvsBridge(def);

/* Check to see if any network IP collides with an existing route */ if (networkCheckRouteCollision(def) < 0) @@ -2439,15 +2449,29 @@ networkStartNetworkVirtual(virNetworkDriverState *driver, if (v6present && networkStartRadvd(driver, obj) < 0) goto error;

- if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0) - goto error; + if (ovsType) { + if (virNetDevOpenvswitchInterfaceSetQos(def->bridge, def->bandwidth, + def->uuid, + true) < 0) + goto error; + } else { + if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0) + goto error; + }

I don't think this should be here. At this point, the bridge was created using netlink (definitely NOT ovs-vsctl add-br). Therefore, virNetDevOpenvswitchInterfaceSetQos() must fail because it won't be able to find any ports/queues/...

Beyond that, networks of the type that would end up calling networkStartNetworkVirtual (forward modes 'route' and "nat') don't support using OVS bridges anyway, so this code in networkStartNetworkVirtual and networkShutdownNetworkVirtual will never be executed anyway.

...

return 0;

error: virErrorPreserveLast(&save_err); - if (def->bandwidth) - virNetDevBandwidthClear(def->bridge); + if (ovsType) { + if (virNetDevOpenvswitchInterfaceClearQos(def->bridge, def->uuid) < 0) + VIR_WARN("cannot clear bandwidth setting for ovs bridge : %s", + def->bridge); + } else { + if (def->bandwidth) { + virNetDevBandwidthClear(def->bridge); + } + }

Similarly, this shouldn't be here either.

...

if (dnsmasqStarted) { pid_t dnsmasqPid = virNetworkObjGetDnsmasqPid(obj); @@ -2536,13 +2560,21 @@ static int networkStartNetworkBridge(virNetworkObj *obj) { virNetworkDef *def = virNetworkObjGetDef(obj); + bool ovsType = networkDefIsOvsBridge(def);

/* put anything here that needs to be done each time a network of * type BRIDGE, is started. On failure, undo anything you've done, * and return -1. On success return 0. */ - if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0) - goto error; + if (ovsType) { + if (virNetDevOpenvswitchInterfaceSetQos(def->bridge, def->bandwidth, + def->uuid, + true) < 0) + goto error; + } else { + if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0) + goto error; + }

if (networkStartHandleMACTableManagerMode(obj) < 0) goto error; @@ -2550,8 +2582,15 @@ networkStartNetworkBridge(virNetworkObj *obj) return 0;

error: - if (def->bandwidth) - virNetDevBandwidthClear(def->bridge); + if (ovsType) { + if (virNetDevOpenvswitchInterfaceClearQos(def->bridge, def->uuid) < 0) + VIR_WARN("cannot clear bandwidth setting for ovs bridge : %s", + def->bridge); + } else { + if (def->bandwidth) { + virNetDevBandwidthClear(def->bridge); + } + } return -1; }

@@ -2565,9 +2604,15 @@ networkShutdownNetworkBridge(virNetworkObj *obj G_GNUC_UNUSED) * type BRIDGE is shutdown. On failure, undo anything you've done, * and return -1. On success return 0. */ - if (def->bandwidth) - virNetDevBandwidthClear(def->bridge);

+ if (networkDefIsOvsBridge(def)) { + if (virNetDevOpenvswitchInterfaceClearQos(def->bridge, def->uuid) < 0) + VIR_WARN("cannot clear bandwidth setting for ovs bridge : %s", + def->bridge); + } else { + if (def->bandwidth) + virNetDevBandwidthClear(def->bridge); + } return 0; }

Alright, these last three hunks are important because they set QoS on the correct type of network. But I wonder if they belong in libvirt at all. I mean, the sole purpose of having openvswitch type of network is that libvirt just plugs TAPs into an OVS controlled bridge without touching it in any other way. IOW, the OVS bridge is created and controlled outside of libvirt. That may include configuring QoS. Merging this patch would open a pandora's box: what should happen if <bandwidth/> is configured in network XML and there's some QoS already set on the bridge?

That was my thought too. Although I looked back in the git history and saw that support for setting the bandwidth of forward mode='bridge' (aka "unamanaged bridge network") was added incidentally as a part of decoupling the network driver from the hypervisor drivers: https://gitlab.com/libvirt/libvirt/-/commit/0a85aad582322034b758f8aa0199641b... which was a bugfix on top of https://gitlab.com/libvirt/libvirt/-/commit/42a92ee93d5432ebd9ebfd409903b528... So setting the QoS for an unmanaged bridge goes against the original philosophy of that type of network, but we already do it in the case of Linux host bridges. I suppose if someone goes to the effort of adding the extra config to set bandwidth, then they must know that this is going to trash any existing bandwidth setting for the bridge, and are willing to accept that (most likely because they *haven't* set anything directly via OVS).

On Mon, Nov 08, 2021 at 10:13:13AM -0500, Laine Stump wrote:

On 11/8/21 5:18 AM, Michal Prívozník wrote:

...

On 11/3/21 4:11 AM, jx8zjs wrote:

...

From: zhangjl02 <zhangjl02@inspur.com>

Fix bug 1826168: bridge type network with ovs bridge can start with Qos setting which do not take any effect

Resolves:https://bugzilla.redhat.com/show_bug.cgi?id=1826168 Signed-off-by: zhangjl02 <zhangjl02@inspur.com> --- src/network/bridge_driver.c | 65 +++++++++++++++++++++++++++++++------ 1 file changed, 55 insertions(+), 10 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 498c45d0a7..d0627848cd 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -2305,6 +2305,15 @@ networkAddRouteToBridge(virNetworkObj *obj, }

+static int +networkDefIsOvsBridge(virNetworkDef *def)

This @def should have been const too. And function returns a bool, effectively.

...

+{ + const virNetDevVPortProfile *vport = def->virtPortProfile; + return vport && + vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH; +} + + static int networkStartNetworkVirtual(virNetworkDriverState *driver, virNetworkObj *obj) @@ -2320,6 +2329,7 @@ networkStartNetworkVirtual(virNetworkDriverState *driver, bool dnsmasqStarted = false; bool devOnline = false; bool firewalRulesAdded = false; + bool ovsType = networkDefIsOvsBridge(def);

/* Check to see if any network IP collides with an existing route */ if (networkCheckRouteCollision(def) < 0) @@ -2439,15 +2449,29 @@ networkStartNetworkVirtual(virNetworkDriverState *driver, if (v6present && networkStartRadvd(driver, obj) < 0) goto error;

- if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0) - goto error; + if (ovsType) { + if (virNetDevOpenvswitchInterfaceSetQos(def->bridge, def->bandwidth, + def->uuid, + true) < 0) + goto error; + } else { + if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0) + goto error; + }

I don't think this should be here. At this point, the bridge was created using netlink (definitely NOT ovs-vsctl add-br). Therefore, virNetDevOpenvswitchInterfaceSetQos() must fail because it won't be able to find any ports/queues/...

Beyond that, networks of the type that would end up calling networkStartNetworkVirtual (forward modes 'route' and "nat') don't support using OVS bridges anyway, so this code in networkStartNetworkVirtual and networkShutdownNetworkVirtual will never be executed anyway.

...

...

return 0;

error: virErrorPreserveLast(&save_err); - if (def->bandwidth) - virNetDevBandwidthClear(def->bridge); + if (ovsType) { + if (virNetDevOpenvswitchInterfaceClearQos(def->bridge, def->uuid) < 0) + VIR_WARN("cannot clear bandwidth setting for ovs bridge : %s", + def->bridge); + } else { + if (def->bandwidth) { + virNetDevBandwidthClear(def->bridge); + } + }

Similarly, this shouldn't be here either.

...

if (dnsmasqStarted) { pid_t dnsmasqPid = virNetworkObjGetDnsmasqPid(obj); @@ -2536,13 +2560,21 @@ static int networkStartNetworkBridge(virNetworkObj *obj) { virNetworkDef *def = virNetworkObjGetDef(obj); + bool ovsType = networkDefIsOvsBridge(def);

/* put anything here that needs to be done each time a network of * type BRIDGE, is started. On failure, undo anything you've done, * and return -1. On success return 0. */ - if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0) - goto error; + if (ovsType) { + if (virNetDevOpenvswitchInterfaceSetQos(def->bridge, def->bandwidth, + def->uuid, + true) < 0) + goto error; + } else { + if (virNetDevBandwidthSet(def->bridge, def->bandwidth, true, true) < 0) + goto error; + }

if (networkStartHandleMACTableManagerMode(obj) < 0) goto error; @@ -2550,8 +2582,15 @@ networkStartNetworkBridge(virNetworkObj *obj) return 0;

error: - if (def->bandwidth) - virNetDevBandwidthClear(def->bridge); + if (ovsType) { + if (virNetDevOpenvswitchInterfaceClearQos(def->bridge, def->uuid) < 0) + VIR_WARN("cannot clear bandwidth setting for ovs bridge : %s", + def->bridge); + } else { + if (def->bandwidth) { + virNetDevBandwidthClear(def->bridge); + } + } return -1; }

@@ -2565,9 +2604,15 @@ networkShutdownNetworkBridge(virNetworkObj *obj G_GNUC_UNUSED) * type BRIDGE is shutdown. On failure, undo anything you've done, * and return -1. On success return 0. */ - if (def->bandwidth) - virNetDevBandwidthClear(def->bridge);

+ if (networkDefIsOvsBridge(def)) { + if (virNetDevOpenvswitchInterfaceClearQos(def->bridge, def->uuid) < 0) + VIR_WARN("cannot clear bandwidth setting for ovs bridge : %s", + def->bridge); + } else { + if (def->bandwidth) + virNetDevBandwidthClear(def->bridge); + } return 0; }

Alright, these last three hunks are important because they set QoS on the correct type of network. But I wonder if they belong in libvirt at all. I mean, the sole purpose of having openvswitch type of network is that libvirt just plugs TAPs into an OVS controlled bridge without touching it in any other way. IOW, the OVS bridge is created and controlled outside of libvirt. That may include configuring QoS. Merging this patch would open a pandora's box: what should happen if <bandwidth/> is configured in network XML and there's some QoS already set on the bridge?

That was my thought too. Although I looked back in the git history and saw that support for setting the bandwidth of forward mode='bridge' (aka "unamanaged bridge network") was added incidentally as a part of decoupling the network driver from the hypervisor drivers:

https://gitlab.com/libvirt/libvirt/-/commit/0a85aad582322034b758f8aa0199641b...

which was a bugfix on top of

https://gitlab.com/libvirt/libvirt/-/commit/42a92ee93d5432ebd9ebfd409903b528...

So setting the QoS for an unmanaged bridge goes against the original philosophy of that type of network, but we already do it in the case of Linux host bridges. I suppose if someone goes to the effort of adding the extra config to set bandwidth, then they must know that this is going to trash any existing bandwidth setting for the bridge, and are willing to accept that (most likely because they *haven't* set anything directly via OVS).

I am a bit hesitant on this front. Clearly the answer is to document what we do and do not support. I guess most of us remember various occasions when we added support for new feature saying that for sure people can shoot themselves in the foot with this if it is used incorrectly, only to find out that people not only will do that, but they will expect libvirt to handle this gracefully. I am not against this particular feature. I just want to say that we should be very cautious if we do things like this because unless we document exactly what the purpose is and what usage scenarios we are (not) supporting, then there will rise new expectations which we might not be able to fulfil, not only because there might not be enough maintenance work on the new code, but also because some scenarios might not be feasible from libvirt's point of view. For example I am not sure if it would be worth it to spend time on updating our running network state based on events from ovs and similar things, if that is even possible. Just my two cents. Have a nice day, Martin