In the case of a network with forward=bridge, which has a bridge device
listed, we are capable of setting bandwidth limits but fail to call the
function to register them.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/network/bridge_driver.c | 39 ++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 1e6d1be32e..4770dd1e4e 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -3229,7 +3229,7 @@ networkValidate(virNetworkDriverStatePtr driver,
     virPortGroupDefPtr defaultPortGroup = NULL;
     virNetworkIPDefPtr ipdef;
     bool ipv4def = false, ipv6def = false;
-    bool bandwidthAllowed = true;
+    bool bandwidthAllowed = false;
     bool usesInterface = false, usesAddress = false;
 
     if (virXMLCheckIllegalChars("name", def->name, "\n") < 0)
@@ -3250,9 +3250,15 @@ networkValidate(virNetworkDriverStatePtr driver,
             return -1;
 
         virNetworkSetBridgeMacAddr(def);
+        bandwidthAllowed = true;
         break;
 
     case VIR_NETWORK_FORWARD_BRIDGE:
+        if (def->bridge != NULL)
+            bandwidthAllowed = true;
+
+        ATTRIBUTE_FALLTHROUGH;
+
     case VIR_NETWORK_FORWARD_PRIVATE:
     case VIR_NETWORK_FORWARD_VEPA:
     case VIR_NETWORK_FORWARD_PASSTHROUGH:
@@ -3293,15 +3299,6 @@ networkValidate(virNetworkDriverStatePtr driver,
                            virNetworkForwardTypeToString(def->forward.type));
             return -1;
         }
-        if (def->bandwidth) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("Unsupported network-wide <bandwidth> element "
-                             "in network %s with forward mode='%s'"),
-                           def->name,
-                           virNetworkForwardTypeToString(def->forward.type));
-            return -1;
-        }
-        bandwidthAllowed = false;
         break;
 
     case VIR_NETWORK_FORWARD_LAST:
@@ -3310,6 +3307,16 @@ networkValidate(virNetworkDriverStatePtr driver,
         return -1;
     }
 
+    if (def->bandwidth &&
+        !bandwidthAllowed) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("Unsupported network-wide <bandwidth> element "
+                         "in network %s with forward mode='%s'"),
+                       def->name,
+                       virNetworkForwardTypeToString(def->forward.type));
+        return -1;
+    }
+
     /* we support configs with a single PF defined:
      *   <pf dev='eth0'/>
      * or with a list of netdev names:
@@ -4578,6 +4585,9 @@ networkAllocateActualDevice(virNetworkPtr net,
                     goto error;
                 }
             }
+
+            if (networkPlugBandwidth(obj, iface) < 0)
+                goto error;
             break;
         }
 
@@ -5052,6 +5062,11 @@ networkReleaseActualDevice(virNetworkPtr net,
         break;
 
     case VIR_NETWORK_FORWARD_BRIDGE:
+        if (iface->data.network.actual &&
+            actualType == VIR_DOMAIN_NET_TYPE_BRIDGE &&
+            networkUnplugBandwidth(obj, iface) < 0)
+            goto error;
+        break;
     case VIR_NETWORK_FORWARD_PRIVATE:
     case VIR_NETWORK_FORWARD_VEPA:
     case VIR_NETWORK_FORWARD_PASSTHROUGH:
@@ -5460,7 +5475,9 @@ networkBandwidthGenericChecks(virDomainNetDefPtr iface,
     virNetDevBandwidthPtr ifaceBand;
     unsigned long long old_floor, new_floor;
 
-    if (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_NETWORK) {
+    if (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_NETWORK &&
+        (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_BRIDGE ||
+         iface->data.network.actual->data.bridge.brname == NULL)) {
         /* This is not an interface that's plugged into a network.
          * We don't care. Thus from our POV bandwidth change is allowed. */
         return false;
-- 
2.20.1

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

On 3/19/19 8:46 AM, Daniel P. Berrangé wrote:
> In the case of a network with forward=bridge, which has a bridge device
> listed, we are capable of setting bandwidth limits but fail to call the
> function to register them.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  src/network/bridge_driver.c | 39 ++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 

One thing missing is class_id XML reading in
virDomainActualNetDefParseXML, that needs to be adjusted for TYPE_BRIDGE

With that, code wise I'll give:

Reviewed-by: Cole Robinson <crobinso@redhat.com>

but I can't really comment on if there's any hidden pitfalls. But I'd
say push it and we can figure it out in git master

- Cole

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

On 3/21/19 8:58 PM, Cole Robinson wrote:
> On 3/19/19 8:46 AM, Daniel P. Berrangé wrote:
>> In the case of a network with forward=bridge, which has a bridge device
>> listed, we are capable of setting bandwidth limits but fail to call the
>> function to register them.
>>
>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> ---
>>   src/network/bridge_driver.c | 39 ++++++++++++++++++++++++++-----------
>>   1 file changed, 28 insertions(+), 11 deletions(-)
>>
> One thing missing is class_id XML reading in
> virDomainActualNetDefParseXML, that needs to be adjusted for TYPE_BRIDGE
>
> With that, code wise I'll give:
>
> Reviewed-by: Cole Robinson <crobinso@redhat.com>
>
> but I can't really comment on if there's any hidden pitfalls.


I seem to recall that Michal omitted bandwidth support on those types of 
networks for a reason (floor can't be supported because there isn't a 
single egress that we have exclusive control over, or something like 
that), but he should probably give the definitive response to that.


>   But I'd
> say push it and we can figure it out in git master
>
> - Cole
>
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list


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

On 3/21/19 9:52 PM, Laine Stump wrote:
> On 3/21/19 8:58 PM, Cole Robinson wrote:
>> On 3/19/19 8:46 AM, Daniel P. Berrangé wrote:
>>> In the case of a network with forward=bridge, which has a bridge device
>>> listed, we are capable of setting bandwidth limits but fail to call the
>>> function to register them.
>>>
>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>>>   src/network/bridge_driver.c | 39 
>>> ++++++++++++++++++++++++++-----------
>>>   1 file changed, 28 insertions(+), 11 deletions(-)
>>>
>> One thing missing is class_id XML reading in
>> virDomainActualNetDefParseXML, that needs to be adjusted for TYPE_BRIDGE
>>
>> With that, code wise I'll give:
>>
>> Reviewed-by: Cole Robinson <crobinso@redhat.com>
>>
>> but I can't really comment on if there's any hidden pitfalls.
>
>
> I seem to recall that Michal omitted bandwidth support on those types 
> of networks for a reason (floor can't be supported because there isn't 
> a single egress that we have exclusive control over, or something like 
> that), but he should probably give the definitive response to that.


Hmm, virNetdevBandwidthParse() appears to support it, while explicitly 
prohibiting floor, so maybe the skipping of class_id in the actualNetDef 
parse was already a bug?


>
>
>>   But I'd
>> say push it and we can figure it out in git master
>>
>> - Cole
>>
>> -- 
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>
>
>

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

On 3/22/19 3:02 AM, Laine Stump wrote:
> On 3/21/19 9:52 PM, Laine Stump wrote:
>> On 3/21/19 8:58 PM, Cole Robinson wrote:
>>> On 3/19/19 8:46 AM, Daniel P. Berrangé wrote:
>>>> In the case of a network with forward=bridge, which has a bridge device
>>>> listed, we are capable of setting bandwidth limits but fail to call the
>>>> function to register them.
>>>>
>>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>>> ---
>>>>   src/network/bridge_driver.c | 39 
>>>> ++++++++++++++++++++++++++-----------
>>>>   1 file changed, 28 insertions(+), 11 deletions(-)
>>>>
>>> One thing missing is class_id XML reading in
>>> virDomainActualNetDefParseXML, that needs to be adjusted for TYPE_BRIDGE
>>>
>>> With that, code wise I'll give:
>>>
>>> Reviewed-by: Cole Robinson <crobinso@redhat.com>
>>>
>>> but I can't really comment on if there's any hidden pitfalls.
>>
>>
>> I seem to recall that Michal omitted bandwidth support on those types 
>> of networks for a reason (floor can't be supported because there isn't 
>> a single egress that we have exclusive control over, or something like 
>> that), but he should probably give the definitive response to that.

Floor of an <interface/> is actually set on the bridge it's connected to 
because that is where all the traffic goes through so that's where we 
can set up traffic shaping so that floor can be honoured. By the way 
that is the reason why corresponding network is required to have 
<inbound/> at least. If there is no traffic shaping set on the bridge 
(in direction from bridge to VM interfaces) then:
a) we don't know where to place qdisc that guarantees the floor
b) what is the upper limit for the sum of all floor values

Long story short, qdiscs are a black box that can do various actions on 
packets that land in them. For traffic shaping I choose HTB which can 
delay packets so that required values are met (link rate, ceil, burst). 
In order to implement floor I needed to create some hierarchy of hose 
HTBs at a place where the whole traffic can be seen => the network 
bridge. Now, by default there is this qdisc named 1:1 aka 'class id' 
(there is some hierarchy in the naming too, unimportant for now) and 
whole traffic that's directed to <interface/>-s without floor goes 
through there. By default this qdisc is allowed to send packets at 
network's <inbound average/>. At the moment that new <interface/> with 
floor is plugged into the bridge, the default qdisc has its rate 
decreased and new qdisc is created (the traffic for the new <interface/> 
will go through there).

That is why we need a) and b). This is also the reason why we can't 
enable floor for network type='bridge'. Libvirt has no control over the 
bridge, nor traffic shaping rules on it.

> 
> 
> Hmm, virNetdevBandwidthParse() appears to support it, while explicitly 
> prohibiting floor, so maybe the skipping of class_id in the actualNetDef 
> parse was already a bug?
> 

class_id is a private attribute to keep mapping of <interface/> <--> 
qdisc and should never be set by user. Nor they need to know its value.

Michal

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

On 3/22/19 5:04 AM, Michal Privoznik wrote:
> On 3/22/19 3:02 AM, Laine Stump wrote:
>> On 3/21/19 9:52 PM, Laine Stump wrote:
>>> On 3/21/19 8:58 PM, Cole Robinson wrote:
>>>> On 3/19/19 8:46 AM, Daniel P. Berrangé wrote:
>>>>> In the case of a network with forward=bridge, which has a bridge
>>>>> device
>>>>> listed, we are capable of setting bandwidth limits but fail to call
>>>>> the
>>>>> function to register them.
>>>>>
>>>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>>>> ---
>>>>>   src/network/bridge_driver.c | 39
>>>>> ++++++++++++++++++++++++++-----------
>>>>>   1 file changed, 28 insertions(+), 11 deletions(-)
>>>>>
>>>> One thing missing is class_id XML reading in
>>>> virDomainActualNetDefParseXML, that needs to be adjusted for
>>>> TYPE_BRIDGE
>>>>
>>>> With that, code wise I'll give:
>>>>
>>>> Reviewed-by: Cole Robinson <crobinso@redhat.com>
>>>>
>>>> but I can't really comment on if there's any hidden pitfalls.
>>>
>>>
>>> I seem to recall that Michal omitted bandwidth support on those types
>>> of networks for a reason (floor can't be supported because there
>>> isn't a single egress that we have exclusive control over, or
>>> something like that), but he should probably give the definitive
>>> response to that.
> 
> Floor of an <interface/> is actually set on the bridge it's connected to
> because that is where all the traffic goes through so that's where we
> can set up traffic shaping so that floor can be honoured. By the way
> that is the reason why corresponding network is required to have
> <inbound/> at least. If there is no traffic shaping set on the bridge
> (in direction from bridge to VM interfaces) then:
> a) we don't know where to place qdisc that guarantees the floor
> b) what is the upper limit for the sum of all floor values
> 
> Long story short, qdiscs are a black box that can do various actions on
> packets that land in them. For traffic shaping I choose HTB which can
> delay packets so that required values are met (link rate, ceil, burst).
> In order to implement floor I needed to create some hierarchy of hose
> HTBs at a place where the whole traffic can be seen => the network
> bridge. Now, by default there is this qdisc named 1:1 aka 'class id'
> (there is some hierarchy in the naming too, unimportant for now) and
> whole traffic that's directed to <interface/>-s without floor goes
> through there. By default this qdisc is allowed to send packets at
> network's <inbound average/>. At the moment that new <interface/> with
> floor is plugged into the bridge, the default qdisc has its rate
> decreased and new qdisc is created (the traffic for the new <interface/>
> will go through there).
> 
> That is why we need a) and b). This is also the reason why we can't
> enable floor for network type='bridge'. Libvirt has no control over the
> bridge, nor traffic shaping rules on it.
> 

FWIW patch 12 deals with some stuff relating to the 'floor' bit, not
sure if it impacts this discussion at all

>>
>>
>> Hmm, virNetdevBandwidthParse() appears to support it, while explicitly
>> prohibiting floor, so maybe the skipping of class_id in the
>> actualNetDef parse was already a bug?
>>
> 
> class_id is a private attribute to keep mapping of <interface/> <-->
> qdisc and should never be set by user. Nor they need to know its value.
> 

Right, this subthread was just about the private runtime VM status XML,
where the class id is set so it can be restored on daemon restart.
Current code only sets it for actualType == TYPE_NETWORK but sounds like
it needs to do so now for actualType == TYPE_BRIDGE

Thanks,
Cole

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

On Fri, Mar 22, 2019 at 10:39:40AM -0400, Cole Robinson wrote:
> On 3/22/19 5:04 AM, Michal Privoznik wrote:
> > On 3/22/19 3:02 AM, Laine Stump wrote:
> >> On 3/21/19 9:52 PM, Laine Stump wrote:
> >>> On 3/21/19 8:58 PM, Cole Robinson wrote:
> >>>> On 3/19/19 8:46 AM, Daniel P. Berrangé wrote:
> >>>>> In the case of a network with forward=bridge, which has a bridge
> >>>>> device
> >>>>> listed, we are capable of setting bandwidth limits but fail to call
> >>>>> the
> >>>>> function to register them.
> >>>>>
> >>>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> >>>>> ---
> >>>>>   src/network/bridge_driver.c | 39
> >>>>> ++++++++++++++++++++++++++-----------
> >>>>>   1 file changed, 28 insertions(+), 11 deletions(-)
> >>>>>
> >>>> One thing missing is class_id XML reading in
> >>>> virDomainActualNetDefParseXML, that needs to be adjusted for
> >>>> TYPE_BRIDGE
> >>>>
> >>>> With that, code wise I'll give:
> >>>>
> >>>> Reviewed-by: Cole Robinson <crobinso@redhat.com>
> >>>>
> >>>> but I can't really comment on if there's any hidden pitfalls.
> >>>
> >>>
> >>> I seem to recall that Michal omitted bandwidth support on those types
> >>> of networks for a reason (floor can't be supported because there
> >>> isn't a single egress that we have exclusive control over, or
> >>> something like that), but he should probably give the definitive
> >>> response to that.
> > 
> > Floor of an <interface/> is actually set on the bridge it's connected to
> > because that is where all the traffic goes through so that's where we
> > can set up traffic shaping so that floor can be honoured. By the way
> > that is the reason why corresponding network is required to have
> > <inbound/> at least. If there is no traffic shaping set on the bridge
> > (in direction from bridge to VM interfaces) then:
> > a) we don't know where to place qdisc that guarantees the floor
> > b) what is the upper limit for the sum of all floor values
> > 
> > Long story short, qdiscs are a black box that can do various actions on
> > packets that land in them. For traffic shaping I choose HTB which can
> > delay packets so that required values are met (link rate, ceil, burst).
> > In order to implement floor I needed to create some hierarchy of hose
> > HTBs at a place where the whole traffic can be seen => the network
> > bridge. Now, by default there is this qdisc named 1:1 aka 'class id'
> > (there is some hierarchy in the naming too, unimportant for now) and
> > whole traffic that's directed to <interface/>-s without floor goes
> > through there. By default this qdisc is allowed to send packets at
> > network's <inbound average/>. At the moment that new <interface/> with
> > floor is plugged into the bridge, the default qdisc has its rate
> > decreased and new qdisc is created (the traffic for the new <interface/>
> > will go through there).
> > 
> > That is why we need a) and b). This is also the reason why we can't
> > enable floor for network type='bridge'. Libvirt has no control over the
> > bridge, nor traffic shaping rules on it.
> > 
> 
> FWIW patch 12 deals with some stuff relating to the 'floor' bit, not
> sure if it impacts this discussion at all

That patch is dealing with precisely the situation described here. It
ensures we only allow use of floor when we have type=network.

> > class_id is a private attribute to keep mapping of <interface/> <-->
> > qdisc and should never be set by user. Nor they need to know its value.
> > 
> 
> Right, this subthread was just about the private runtime VM status XML,
> where the class id is set so it can be restored on daemon restart.
> Current code only sets it for actualType == TYPE_NETWORK but sounds like
> it needs to do so now for actualType == TYPE_BRIDGE

Yes it does need to.


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