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

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(a)redhat.com&gt; >>>> --- >>>>   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(a)redhat.com&gt; >>> >>> 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.