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@redhat.com> AuthorDate: Wed Jan 22 18:58:33 2014 +0100 Commit: Michal Privoznik <mprivozn@redhat.com> 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).