[libvirt] [PATCH 3/9] bridge_driver: Introduce networkBandwidthChangeAllowed

[John Ferlan]

On 08/03/2015 02:39 AM, Michal Privoznik wrote:
> When a domain vNIC's bandwidth is to be changed (at runtime) it is
> possible that guaranteed minimal bandwidth (@floor) will change too.
> Well, so far it is, because we still don't have an implementation that
> allows setting it dynamically, so it's effectively erased on:
> 
>     #virsh domiftune $dom vnet0 --inbound 0
> 
> However, that's slightly unfortunate. We do some checks on domain
> startup to see if @floor can be guaranteed. We ought do the same if
> QoS is changed at runtime.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/network/bridge_driver.c | 72 +++++++++++++++++++++++++++++++++++++++++++--
>  src/network/bridge_driver.h | 12 ++++++++
>  2 files changed, 82 insertions(+), 2 deletions(-)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 17fc430..fa60ba4 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -4686,9 +4686,18 @@ networkGetNetworkAddress(const char *netname, char **netaddr)
>   * networkCheckBandwidth:
>   * @net: network QoS
>   * @ifaceBand: interface QoS (may be NULL if no QoS)
> + * @oldBandwidth: new interface QoS (may be NULL if no QoS)
>   * @ifaceMac: interface MAC (used in error messages for identification)
>   * @new_rate: new rate for non guaranteed class
>   *
> + * Function checks if @ifaceBand can be satisfied on @net. However, sometimes it
> + * may happen that the interface that @ifaceBand corresponds to is already
> + * plugged into the @net and the bandwidth is to be updated. In that case we
> + * need to check if new bandwidth can be satisfied. If that's the case
> + * @ifaceBand should point to new bandwidth settings and @oldBandwidth to
> + * current ones. If you want to suppress this functionality just pass
> + * @oldBandwidth == NULL.
> + *
>   * Returns: -1 if plugging would overcommit network QoS
>   *           0 if plugging is safe (@new_rate updated)
>   *           1 if no QoS is set (@new_rate untouched)

new_rate is unchanged if passed as NULL too

Part of me says - who cares - just set it and let the caller decide -
both paths will "always have" that extra "if" check now... and it only
ever matters if plugging is safe.

I'm OK keeping as is - so I'll ACK and let you decide. At the very least
point out that new_rate can be NULL in the comments. I'm guessing that
if the caller that has it NULL had a value, then there'd be a warning
about an unused variable...

John
> @@ -4696,6 +4705,7 @@ networkGetNetworkAddress(const char *netname, char **netaddr)
>  static int
>  networkCheckBandwidth(virNetworkObjPtr net,
>                        virNetDevBandwidthPtr ifaceBand,
> +                      virNetDevBandwidthPtr oldBandwidth,
>                        virMacAddr ifaceMac,
>                        unsigned long long *new_rate)
>  {
> @@ -4723,6 +4733,8 @@ networkCheckBandwidth(virNetworkObjPtr net,
>      }
>  
>      tmp_new_rate = netBand->in->average;
> +    if (oldBandwidth && oldBandwidth->in)
> +        tmp_floor_sum -= oldBandwidth->in->floor;
>      tmp_floor_sum += ifaceBand->in->floor;
>  
>      /* check against peak */
> @@ -4749,7 +4761,8 @@ networkCheckBandwidth(virNetworkObjPtr net,
>          goto cleanup;
>      }
>  
> -    *new_rate = tmp_new_rate;
> +    if (new_rate)
> +        *new_rate = tmp_new_rate;
>      ret = 0;
>  
>   cleanup:
> @@ -4791,7 +4804,7 @@ networkPlugBandwidth(virNetworkObjPtr net,
>      char ifmac[VIR_MAC_STRING_BUFLEN];
>      virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface);
>  
> -    if ((plug_ret = networkCheckBandwidth(net, ifaceBand,
> +    if ((plug_ret = networkCheckBandwidth(net, ifaceBand, NULL,
>                                            iface->mac, &new_rate)) < 0) {
>          /* helper reported error */
>          goto cleanup;
> @@ -4917,3 +4930,58 @@ networkNetworkObjTaint(virNetworkObjPtr net,
>                   virNetworkTaintTypeToString(taint));
>      }
>  }
> +
> +
> +static bool
> +networkBandwidthGenericChecks(virDomainNetDefPtr iface,
> +                              virNetDevBandwidthPtr newBandwidth)
> +{
> +    virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface);
> +    unsigned long long old_floor, new_floor;
> +
> +    if (virDomainNetGetActualType(iface) != VIR_DOMAIN_NET_TYPE_NETWORK) {
> +        /* 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;
> +    }
> +
> +    old_floor = new_floor = 0;
> +
> +    if (ifaceBand && ifaceBand->in)
> +        old_floor = ifaceBand->in->floor;
> +    if (newBandwidth && newBandwidth->in)
> +        new_floor = newBandwidth->in->floor;
> +
> +    return new_floor != old_floor;
> +}
> +
> +
> +bool
> +networkBandwidthChangeAllowed(virDomainNetDefPtr iface,
> +                              virNetDevBandwidthPtr newBandwidth)
> +{
> +    virNetworkDriverStatePtr driver = networkGetDriver();
> +    virNetworkObjPtr network = NULL;
> +    virNetDevBandwidthPtr ifaceBand = virDomainNetGetActualBandwidth(iface);
> +    bool ret = false;
> +
> +    if (!networkBandwidthGenericChecks(iface, newBandwidth))
> +        return true;
> +
> +    network = virNetworkObjFindByName(driver->networks, iface->data.network.name);
> +    if (!network) {
> +        virReportError(VIR_ERR_NO_NETWORK,
> +                       _("no network with matching name '%s'"),
> +                       iface->data.network.name);
> +        return false;
> +    }
> +
> +    if (networkCheckBandwidth(network, newBandwidth, ifaceBand, iface->mac, NULL) < 0)
> +        goto cleanup;
> +
> +    ret = true;
> +
> + cleanup:
> +    virNetworkObjEndAPI(&network);
> +    return ret;
> +}
> diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h
> index 513ccf7..cce9237 100644
> --- a/src/network/bridge_driver.h
> +++ b/src/network/bridge_driver.h
> @@ -52,6 +52,11 @@ int networkDnsmasqConfContents(virNetworkObjPtr network,
>                          char **configstr,
>                          dnsmasqContext *dctx,
>                          dnsmasqCapsPtr caps);
> +
> +bool networkBandwidthChangeAllowed(virDomainNetDefPtr iface,
> +                                   virNetDevBandwidthPtr newBandwidth)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> +
>  # else
>  /* Define no-op replacements that don't drag in any link dependencies.  */
>  #  define networkAllocateActualDevice(dom, iface) 0
> @@ -73,6 +78,13 @@ networkReleaseActualDevice(virDomainDefPtr dom ATTRIBUTE_UNUSED,
>      return 0;
>  }
>  
> +static inline bool
> +networkBandwidthChangeAllowed(virDomainNetDefPtr iface ATTRIBUTE_UNUSED,
> +                              virNetDevBandwidthPtr newBandwidth ATTRIBUTE_UNUSED)
> +{
> +    return true;
> +}
> +
>  # endif
>  
>  typedef char *(*networkDnsmasqLeaseFileNameFunc)(const char *netname);
>