[PATCH 1/1] bridge_driver: use ovs-vsctl to setup and clean Qos when

[Michal Prívozník]

On 11/3/21 4:11 AM, jx8zjs wrote:
> From: zhangjl02 <zhangjl02 at 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 at 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/...

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

Sorry for not realizing this in v1.

Michal