[libvirt] [PATCH v3 22/36] network: remove the virDomainNetBandwidthChangeAllowed callback

[Daniel P. Berrangé]

On Tue, Apr 02, 2019 at 09:22:38PM -0400, Laine Stump wrote:
> On 3/19/19 8:46 AM, Daniel P. Berrangé wrote:
> > The current qemu driver code for changing bandwidth on a NIC first asks
> > the network driver if the change is supported, then changes the
> > bandwidth on the VIF, and then tells the network driver to update the
> > bandwidth on the bridge.
> > 
> > This is potentially racing if a parallel API call causes the network
> > driver to allocate bandwidth on the bridge between the check and the
> > update phases.
> > 
> > Change the code to just try to apply the network bridge update
> > immediately and rollback at the end if something failed.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange at redhat.com>

> > @@ -5391,6 +5334,7 @@ networkBandwidthUpdate(virDomainNetDefPtr iface,
> >       unsigned long long tmp_floor_sum;
> >       virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface);
> >       unsigned long long new_rate = 0;
> > +    unsigned long long old_floor, new_floor;
> >       int plug_ret;
> >       int ret = -1;
> > @@ -5400,7 +5344,21 @@ networkBandwidthUpdate(virDomainNetDefPtr iface,
> >           return -1;
> >       }
> > -    if (!networkBandwidthGenericChecks(iface, newBandwidth))
> > +    if (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. */
> 
> 
> This comment is more confusing than it is helpful. Especially since it's
> wrong :-)

It was correct originally, then i made bandwidth stuff apply to any
bridged nic.

I'll s/into a network/into a bridge/


> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index b81c411007..baf188ae40 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -11729,17 +11729,17 @@ qemuDomainSetInterfaceParameters(virDomainPtr dom,
> >           }
> >           if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> > -            !virDomainNetBandwidthChangeAllowed(net, newBandwidth))
> > +            virDomainNetBandwidthUpdate(net, newBandwidth) < 0)
> >               goto endjob;
> >           if (virNetDevBandwidthSet(net->ifname, newBandwidth, false,
> > -                                  !virDomainNetTypeSharesHostView(net)) < 0 ||
> > -            (net->type == VIR_DOMAIN_NET_TYPE_NETWORK &&
> > -             virDomainNetBandwidthUpdate(net, newBandwidth) < 0)) {
> > +                                  !virDomainNetTypeSharesHostView(net)) < 0) {
> >               ignore_value(virNetDevBandwidthSet(net->ifname,
> >                                                  net->bandwidth,
> >                                                  false,
> >                                                  !virDomainNetTypeSharesHostView(net)));
> > +            ignore_value(virDomainNetBandwidthUpdate(net,
> > +                                                     net->bandwidth));
> >               goto endjob;
> >           }
> 
> 
> The new version of the code is calling virDomainNetBandwidthUpdate() and
> virNetDevBandwidthSet() in reverse order from what it previously did. If
> either of the functions were to modify newBandwidth's contents, then the
> results would change. Fortunately it looks like neither of them do that, so
> I think we're safe.

Yes, the previous code did a two phase change. It first checked if change
was allowed by the network, then changed the local NIC & updated the
network.

That's a time of check vs time of update race wrt to the network, so I
had to merge them into one step.

Which ever way we update the local NIC vs the network has unplesant
failure scenarios, where we can merely do a best effort to roll back.

I think this is as good as we'll get.

> Reviewed-by: Laine Stump <laine at laine.org>

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