[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

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,
       } 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,
+            return -1;
+        }
           if (virNetworkDefGetIpByIndex(def, AF_UNSPEC, 0)) {
                              _("Unsupported <ip> element in network %s "
@@ -2433,6 +2442,14 @@ networkValidate(virNetworkDriverStatePtr driver,

               return -1;
+        if (def->bandwidth) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           _("Unsupported network-wide <bandwidth>
element "
+                             "in network %s with forward mode='%s'"),
+                           def->name,
+            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


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.


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]