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 -- 1.8.5.3 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

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

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.

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(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. > Yes. Please do push your patch. ACK.