[libvirt] [PATCH v3 04/36] network: add missing bandwidth limits for bridge forward type

[Michal Privoznik]

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