Re: [libvirt] [PATCH] network: disallow <bandwidth>/<mac> for bridged/macvtap networks

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