On 01/30/2014 12:29 PM, Michal Privoznik wrote: > On 30.01.2014 11:26, Laine Stump wrote: >> On 01/27/2014 06:08 PM, Michal Privoznik wrote: >>> On 24.01.2014 13:18, Laine Stump wrote: >>>> https://bugzilla.redhat.com/show_bug.cgi?id=1057321 pointed out that >>>> we weren't honoring the <bandwidth> element in libvirt networks using >>>> <forward mode='bridge'/>. In fact, these networks are just a method of >>>> giving a libvirt network name to an existing Linux host bridge on the >>>> system, and even if it were technically possible for us to set >>>> network-wide bandwidth limits for all the taps on a bridge, it's >>>> probably not a polite thing to do since libvirt is just using a bridge >>>> that was created by someone else for other purposes. So the proper >>>> thing is to just log an error when someone tries to put a <bandwidth> >>>> element in that type of network. >>>> >>>> While looking through the network XML documentation and comparing it >>>> to the networkValidate function, I noticed that we also ignore the >>>> presence of a mac address in the config, even though we do nothing >>>> with it in this case either. >>>> >>>> This patch updates networkValidate() (which is called any time a >>>> persistent network is defined, or a transient network created) to log >>>> an error and fail if it finds either a <bandwidth> or <mac> element >>>> and the network forward mode is anything except 'route'. 'nat', or >>>> nothing. (Yes, neither of those elements is acceptable for any macvtap >>>> mode, nor for a hostdev network). >>>> >>>> NB: This does *not* cause failure to start any existing network that >>>> contains one of those elements, so someone might have erroneously >>>> defined such a network in the past, and that network will continue to >>>> function unmodified. I considered it too disruptive to suddenly break >>>> working configs on the next reboot after a libvirt upgrade. >>>> --- >>>> src/network/bridge_driver.c | 19 ++++++++++++++++++- >>>> 1 file changed, 18 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c >>>> index 0b43a67..3b9b58d 100644 >>>> --- a/src/network/bridge_driver.c >>>> +++ b/src/network/bridge_driver.c >>>> @@ -2407,8 +2407,17 @@ networkValidate(virNetworkDriverStatePtr >>>> driver, >>>> virNetworkSetBridgeMacAddr(def); >>>> } else { >>>> /* They are also the only types that currently support >>>> setting >>>> - * an IP address for the host-side device (bridge) >>>> + * a MAC or IP address for the host-side device (bridge), DNS >>>> + * configuration, or network-wide bandwidth limits. >>>> */ >>>> + if (def->mac_specified) { >>>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >>>> + _("Unsupported <mac> element in network >>>> %s " >>>> + "with forward mode='%s'"), >>>> + def->name, >>>> + >>>> virNetworkForwardTypeToString(def->forward.type)); >>>> + return -1; >>>> + } >>>> if (virNetworkDefGetIpByIndex(def, AF_UNSPEC, 0)) { >>>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >>>> _("Unsupported <ip> element in >>>> network %s " >>>> @@ -2433,6 +2442,14 @@ networkValidate(virNetworkDriverStatePtr >>>> driver, >>>> >>>> virNetworkForwardTypeToString(def->forward.type)); >>>> return -1; >>>> } >>>> + if (def->bandwidth) { >>>> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >>>> + _("Unsupported network-wide <bandwidth> >>>> element " >>>> + "in network %s with forward mode='%s'"), >>>> + def->name, >>>> + >>>> virNetworkForwardTypeToString(def->forward.type)); >>>> + return -1; >>>> + } >>>> } >>>> >>>> /* We only support dhcp on one IPv4 address and >>>> >>> >>> I think think this is exactly the opposite of what I've just pushed :) >> >> Indeed :-) I really should get myself Cc'ed on more bugzilla >> copmonents/products so that I notice these things sooner. >> >>> I mean: >>> >>> commit 2996e6be19a13199ded7c2aa21039cca97318e01 >>> Author: Michal Privoznik <mprivozn@xxxxxxxxxx> >>> AuthorDate: Wed Jan 22 18:58:33 2014 +0100 >>> Commit: Michal Privoznik <mprivozn@xxxxxxxxxx> >>> CommitDate: Mon Jan 27 12:11:27 2014 +0100 >>> >>> networkAllocateActualDevice: Set QoS for bridgeless networks too >>> >>> https://bugzilla.redhat.com/show_bug.cgi?id=1055484 >>> >>> >>> >>> In the commit I'm trying to inherit network QoS to the interface that >>> is just being created. Yes, it involves some magic but it works. I >>> guess we need to agree if we want this approach or mine as they seem >>> to be contradictionary. >> >> Since you've reverted yours, should I push this? >> >> After that, we may want to talk about 1) supporting use of the "dev" >> attribute in <forward> to name a *single* forwarding interface, and >> applying a network's <bandwidth> to that interface (while still failing >> in other cases), and 2) maybe supporting Open vSwitch in a more thorough >> manner so that projects like ovirt can use it to create intermediate >> bridges and manage their bandwidth via libvirt <network> xml. >> > > Yes. Please do push your patch. ACK. Okay, after a delay while considering the ramifications this could have on vdsm's current network configs when upgrading to a newer libvirt, I've added some text to formatnetwork.html describing what is and isn't supported (and further clarifying exactly what is the function of <bandwidth> in a network definition vs. <bandwidth> in a network's portgroup), and pushed the result. We decided that it is safer to fail users who were unaware of the limitations of <bandwidth> and possible break some working setups, rather than to silently ignore config and falsely lead them to the conclusion that their config is actually doing something. For some simpler config scenarios, we may add more built-in support for network-wide QoS to libvirt, but Michal has also posted some patches which implement hook scripts for network lifecycle events which should allow setting up QoS on any arbitrary network setup (as long as it is technically possible to do so with available host system commands). -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list