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

[Daniel P. Berrangé]

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 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.
> > 
> 
> 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 :|