Change in vdsm[ovirt-3.5]: QoS: Remove broken libvirt-based host network QoS

phoracek at redhat.com phoracek at redhat.com
Tue Oct 21 14:28:54 UTC 2014


Hello Antoni Segura Puimedon, Dan Kenigsberg,

I'd like you to do a code review.  Please visit

    http://gerrit.ovirt.org/34319

to review the following change.

Change subject: QoS: Remove broken libvirt-based host network QoS
......................................................................

QoS: Remove broken libvirt-based host network QoS

Back when the current host network level QoSa was added, it was
thought that libvirt had a way of configuring and honoring the
Quality of Service definitions that we were giving it. Unfortunately
that was an unimplemented part of their api, which forced us to
design a new implementation with different semantics.

This patch removes most of the host based network QoS implementation
from the codebase to prepare vdsm for the following patches which
will use the new tc package to provide outbound host network QoS.

Conflicts:
	lib/vdsm/netinfo.py

Change-Id: I7b82cc6b656fb6af8caa2940c9ee2d14287f3526
Signed-off-by: Antoni S. Puimedon <asegurap at redhat.com>
Reviewed-on: http://gerrit.ovirt.org/30198
Reviewed-by: Dan Kenigsberg <danken at redhat.com>
---
M lib/vdsm/netinfo.py
M lib/vdsm/tool/unified_persistence.py
M tests/functional/networkTests.py
M tests/functional/utils.py
M tests/netconfTests.py
M tests/netinfoTests.py
M vdsm/network/api.py
M vdsm/network/configurators/__init__.py
M vdsm/network/configurators/ifcfg.py
M vdsm/network/configurators/iproute2.py
M vdsm/network/configurators/libvirt.py
M vdsm/rpc/vdsmapi-schema.json
12 files changed, 18 insertions(+), 174 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/19/34319/1

diff --git a/lib/vdsm/netinfo.py b/lib/vdsm/netinfo.py
index 5dc2cd6..5dfdc98 100644
--- a/lib/vdsm/netinfo.py
+++ b/lib/vdsm/netinfo.py
@@ -18,7 +18,7 @@
 # Refer to the README and COPYING files for full details of the license
 #
 
-from collections import defaultdict, namedtuple
+from collections import defaultdict
 import errno
 from glob import iglob
 from datetime import datetime
@@ -83,8 +83,6 @@
 LIBVIRT_NET_PREFIX = 'vdsm-'
 DEFAULT_MTU = '1500'
 
-_Qos = namedtuple('Qos', 'inbound outbound')
-
 OPERSTATE_UP = 'up'
 DUMMY_BRIDGE  # Appease flake8 since dummy bridge should be exported from here
 
@@ -108,11 +106,8 @@
 
     :returns: dict of networkname={properties}
     :rtype: dict of dict
-            { 'ovirtmgmt': { 'bridge': 'ovirtmgmt', 'bridged': True,
-                            'qosInbound': {'average': 1024, 'peak': 4096},
-                            'qosOutbound': {'average': 1024, 'burst': 2048}},
-              'red': { 'iface': 'red', 'bridged': False
-                       'qosOutbound': {'average': 1024, 'burst': 2048}}}
+            { 'ovirtmgmt': { 'bridge': 'ovirtmgmt', 'bridged': True}
+              'red': { 'iface': 'red', 'bridged': False}}
     """
     nets = {}
     conn = libvirtconnection.get()
@@ -122,11 +117,6 @@
             netname = netname[len(LIBVIRT_NET_PREFIX):]
             nets[netname] = {}
             xml = minidom.parseString(net.XMLDesc(0))
-            qos = _parseBandwidthQos(xml)
-            if qos.inbound:
-                nets[netname]['qosInbound'] = qos.inbound
-            if qos.outbound:
-                nets[netname]['qosOutbound'] = qos.outbound
             interfaces = xml.getElementsByTagName('interface')
             if len(interfaces) > 0:
                 nets[netname]['iface'] = interfaces[0].getAttribute('dev')
@@ -136,35 +126,6 @@
                     xml.getElementsByTagName('bridge')[0].getAttribute('name')
                 nets[netname]['bridged'] = True
     return nets
-
-
-def _parseBandwidthQos(networkXml):
-    """
-    Extract the Qos information
-    :param networkXml: instance of xml.dom.minidom.Document
-    :return: _Qos namedtuple containing inbound and outbound qos dicts.
-    """
-
-    qos = _Qos({}, {})
-
-    def extractQos(bandWidthElem, trafficType):
-        qos = {}
-        elem = bandWidthElem.getElementsByTagName(trafficType)
-        if elem:
-            qos['average'] = int(elem[0].getAttribute('average'))
-            if elem[0].hasAttribute('burst'):  # libvirt XML optional field
-                qos['burst'] = int(elem[0].getAttribute('burst'))
-            if elem[0].hasAttribute('peak'):  # libvirt XML optional field
-                qos['peak'] = int(elem[0].getAttribute('peak'))
-        return qos
-
-    bandwidthElem = networkXml.getElementsByTagName('bandwidth')
-    if bandwidthElem:
-        inbound = extractQos(bandwidthElem[0], "inbound")
-        outbound = extractQos(bandwidthElem[0], "outbound")
-        qos = _Qos(inbound, outbound)
-
-    return qos
 
 
 def slaves(bonding):
@@ -602,8 +563,7 @@
                      in sorted(opts.iteritems())))
 
 
-def _getNetInfo(iface, dhcp4, bridged, gateways, ipv6routes, ipaddrs,
-                qosInbound, qosOutbound):
+def _getNetInfo(iface, dhcp4, bridged, gateways, ipv6routes, ipaddrs):
     '''Returns a dictionary of properties about the network's interface status.
     Raises a KeyError if the iface does not exist.'''
     data = {}
@@ -626,10 +586,6 @@
                      'ipv6addrs': ipv6addrs,
                      'ipv6gateway': ipv6routes.get(iface, '::'),
                      'mtu': str(getMtu(iface))})
-        if qosInbound:
-            data['qosInbound'] = qosInbound
-        if qosOutbound:
-            data['qosOutbound'] = qosOutbound
     except (IOError, OSError) as e:
         if e.errno == errno.ENOENT:
             logging.info('Obtaining info for net %s.', iface, exc_info=True)
@@ -778,9 +734,7 @@
             d[net] = _getNetInfo(netAttr.get('iface', net),
                                  dhcp4,
                                  netAttr['bridged'], gateways,
-                                 ipv6routes, ipAddrs,
-                                 netAttr.get('qosInbound'),
-                                 netAttr.get('qosOutbound'))
+                                 ipv6routes, ipAddrs)
         except KeyError:
             continue  # Do not report missing libvirt networks.
     return d
diff --git a/lib/vdsm/tool/unified_persistence.py b/lib/vdsm/tool/unified_persistence.py
index 1733c97..5777ae4 100644
--- a/lib/vdsm/tool/unified_persistence.py
+++ b/lib/vdsm/tool/unified_persistence.py
@@ -29,7 +29,6 @@
 
 UPGRADE_NAME = 'upgrade-unified-persistence'
 NET_ATTR_WHITELIST = {'mtu': lambda value: int(value),
-                      'qosInbound': lambda value: value,
                       'qosOutbound': lambda value: value,
                       'stp': lambda value: utils.tobool(value)}
 
diff --git a/tests/functional/networkTests.py b/tests/functional/networkTests.py
index 2f1e035..558cbc4 100644
--- a/tests/functional/networkTests.py
+++ b/tests/functional/networkTests.py
@@ -594,39 +594,10 @@
                                                    opts={'bridged': bridged})
             self.assertEquals(status, SUCCESS, msg)
             self.assertNetworkExists(NETWORK_NAME, bridged=bridged)
-            qosInbound, qosOutbound = self.vdsm_net.networkQos(NETWORK_NAME)
             status, msg = self.vdsm_net.delNetwork(NETWORK_NAME, vlan=vlan_id,
                                                    bond=BONDING_NAME,
                                                    nics=nics)
-            self.assertEqual({}, qosInbound)
-            self.assertEqual({}, qosOutbound)
             self.assertEqual(status, SUCCESS, msg)
-
-    @cleanupNet
-    @RequireDummyMod
-    @ValidateRunningAsRoot
-    def testQosNetwork(self):
-        def testNetQoS(qos):
-            status, msg = self.vdsm_net.addNetwork(NETWORK_NAME, nics=nics,
-                                                   opts=qos)
-            self.assertEqual(status, SUCCESS, msg)
-
-            qosInbound, qosOutbound = self.vdsm_net.networkQos(NETWORK_NAME)
-            self.assertEqual(qos.get('qosInbound', {}), qosInbound)
-            self.assertEqual(qos.get('qosOutbound', {}), qosOutbound)
-
-            status, msg = self.vdsm_net.delNetwork(NETWORK_NAME)
-            self.assertEqual(status, SUCCESS, msg)
-
-        with dummyIf(1) as nics:
-            qosComplete = {'qosInbound': {'average': 1024, 'burst': 2048,
-                                          'peak': 42},
-                           'qosOutbound': {'average': 2400, 'burst': 2048,
-                                           'peak': 100}}
-            testNetQoS(qosComplete)
-
-            qosInbOnly = {'qosInbound': {'average': 1024}}
-            testNetQoS(qosInbOnly)
 
     @cleanupNet
     @permutations([[True], [False]])
diff --git a/tests/functional/utils.py b/tests/functional/utils.py
index 494be98..bc88be5 100644
--- a/tests/functional/utils.py
+++ b/tests/functional/utils.py
@@ -16,7 +16,6 @@
 #
 # Refer to the README and COPYING files for full details of the license
 #
-from collections import namedtuple
 from contextlib import contextmanager
 from functools import wraps
 import socket
@@ -33,7 +32,6 @@
 
 
 SUCCESS = 0
-Qos = namedtuple('Qos', 'inbound outbound')
 
 
 def cleanupRules(func):
@@ -192,10 +190,6 @@
             raise
         finally:
             done = True
-
-    def networkQos(self, networkName):
-        network = self.netinfo.networks[networkName]
-        return network.get('qosInbound', {}), network.get('qosOutbound', {})
 
     def getVdsStats(self):
         result = self.vdscli.getVdsStats()
diff --git a/tests/netconfTests.py b/tests/netconfTests.py
index 316eb19..ee3db9a 100644
--- a/tests/netconfTests.py
+++ b/tests/netconfTests.py
@@ -148,24 +148,3 @@
                                              iface=iface)
 
         self.assertEqualXml(expectedDoc, actualDoc)
-
-    def testCreateNetXmlBridgedQos(self):
-        inbound = {'average': '1024', 'burst': '5000'}
-        outbound = {'average': '666', 'burst': '666',
-                    'peak': '400'}
-        expectedDoc = ("""<network>
-                            <name>vdsm-awesome_net</name>
-                            <forward mode='bridge'/>
-                            <bridge name='awesome_net'/>
-                            <bandwidth>
-                                <inbound average='%s' burst='%s' />
-                                <outbound average='%s' burst='%s' peak='%s' />
-                            </bandwidth>
-                          </network>"""
-                       % (inbound['average'], inbound['burst'],
-                          outbound['average'], outbound['burst'],
-                          outbound['peak']))
-        actualDoc = libvirt.createNetworkDef('awesome_net', qosInbound=inbound,
-                                             qosOutbound=outbound)
-
-        self.assertEqualXml(expectedDoc, actualDoc)
diff --git a/tests/netinfoTests.py b/tests/netinfoTests.py
index a3a1e64..ac54f2b 100644
--- a/tests/netinfoTests.py
+++ b/tests/netinfoTests.py
@@ -22,7 +22,6 @@
 from datetime import datetime
 from functools import partial
 import time
-from xml.dom import minidom
 
 import ethtool
 
@@ -214,23 +213,6 @@
             self.assertFalse(hiddens.intersection(nics), 'Some of hidden '
                              'devices %s is shown in nics %s' % (hiddens,
                                                                  nics))
-
-    def testGetBandwidthQos(self):
-        notEmptyDoc = minidom.parseString("""<bandwidth>
-                            <inbound average='4500' burst='5400' />
-                            <outbound average='4500' burst='5400' peak='101' />
-                          </bandwidth>""")
-        expectedQosNotEmpty = netinfo._Qos(inbound={'average': 4500,
-                                                    'burst': 5400},
-                                           outbound={'average': 4500,
-                                                     'burst': 5400,
-                                                     'peak': 101})
-        emptyDoc = minidom.parseString("<whatever></whatever>")
-
-        self.assertEqual(expectedQosNotEmpty,
-                         netinfo._parseBandwidthQos(notEmptyDoc))
-        self.assertEqual(netinfo._Qos({}, {}),
-                         netinfo._parseBandwidthQos(emptyDoc))
 
     def testGetBootProtocolIfcfg(self):
         deviceName = "___This_could_never_be_a_device_name___"
diff --git a/vdsm/network/api.py b/vdsm/network/api.py
index 8b37808..dc586b3 100755
--- a/vdsm/network/api.py
+++ b/vdsm/network/api.py
@@ -247,8 +247,8 @@
                netmask=None, prefix=None, mtu=None, gateway=None,
                ipv6addr=None, ipv6gateway=None, ipv6autoconf=None, force=False,
                configurator=None, bondingOptions=None, bridged=True,
-               _netinfo=None, qosInbound=None, qosOutbound=None,
-               defaultRoute=None, blockingdhcp=False, **options):
+               _netinfo=None, qosOutbound=None, defaultRoute=None,
+               blockingdhcp=False, **options):
     nics = nics or ()
     if _netinfo is None:
         _netinfo = netinfo.NetInfo()
@@ -313,9 +313,7 @@
         _netinfo=_netinfo, configurator=configurator, opts=options)
 
     netEnt.configure(**options)
-    configurator.configureLibvirtNetwork(network, netEnt,
-                                         qosInbound=qosInbound,
-                                         qosOutbound=qosOutbound)
+    configurator.configureLibvirtNetwork(network, netEnt)
 
 
 def assertBridgeClean(bridge, vlan, bonding, nics):
diff --git a/vdsm/network/configurators/__init__.py b/vdsm/network/configurators/__init__.py
index 2923290..6e2f969 100644
--- a/vdsm/network/configurators/__init__.py
+++ b/vdsm/network/configurators/__init__.py
@@ -102,13 +102,10 @@
     def removeSourceRoute(self, routes, rules, device):
         raise NotImplementedError
 
-    def configureLibvirtNetwork(self, network, iface, qosInbound=None,
-                                qosOutbound=None):
+    def configureLibvirtNetwork(self, network, iface):
         self.configApplier.createLibvirtNetwork(network,
                                                 isinstance(iface, Bridge),
-                                                iface.name,
-                                                qosInbound=qosInbound,
-                                                qosOutbound=qosOutbound)
+                                                iface.name)
         self._libvirtAdded.add(network)
 
     def removeLibvirtNetwork(self, network):
diff --git a/vdsm/network/configurators/ifcfg.py b/vdsm/network/configurators/ifcfg.py
index 2b60253..5dbe9c1 100644
--- a/vdsm/network/configurators/ifcfg.py
+++ b/vdsm/network/configurators/ifcfg.py
@@ -281,11 +281,8 @@
         logging.debug("Removed file %s", filename)
 
     def createLibvirtNetwork(self, network, bridged=True, iface=None,
-                             skipBackup=False, qosInbound=None,
-                             qosOutbound=None):
-        netXml = libvirt.createNetworkDef(network, bridged, iface,
-                                          qosInbound=qosInbound,
-                                          qosOutbound=qosOutbound)
+                             skipBackup=False):
+        netXml = libvirt.createNetworkDef(network, bridged, iface)
         if not skipBackup:
             self._networkBackup(network)
         libvirt.createNetwork(netXml)
diff --git a/vdsm/network/configurators/iproute2.py b/vdsm/network/configurators/iproute2.py
index 2b9aca1..b06e0ad 100644
--- a/vdsm/network/configurators/iproute2.py
+++ b/vdsm/network/configurators/iproute2.py
@@ -326,10 +326,8 @@
             with open(netinfo.BONDING_OPT % (bond.name, key), 'w') as f:
                 f.write(value)
 
-    def createLibvirtNetwork(self, network, bridged, iface, qosInbound=None,
-                             qosOutbound=None):
-        netXml = libvirt.createNetworkDef(network, bridged, iface,
-                                          qosInbound, qosOutbound)
+    def createLibvirtNetwork(self, network, bridged, iface):
+        netXml = libvirt.createNetworkDef(network, bridged, iface)
         libvirt.createNetwork(netXml)
 
     def removeLibvirtNetwork(self, network):
diff --git a/vdsm/network/configurators/libvirt.py b/vdsm/network/configurators/libvirt.py
index f378641..3cca006 100644
--- a/vdsm/network/configurators/libvirt.py
+++ b/vdsm/network/configurators/libvirt.py
@@ -51,8 +51,7 @@
         raise
 
 
-def createNetworkDef(network, bridged=True, iface=None,
-                     qosInbound=None, qosOutbound=None):
+def createNetworkDef(network, bridged=True, iface=None):
     """
     Creates Network Xml e.g.:
     <network>
@@ -60,18 +59,12 @@
 
         <forward mode='bridge'/><bridge name='awesome_net'/> ||
         <forward mode='passthrough'><interface dev='incredible'/></forward>
-
-        [<bandwidth>]
-            [<inbound average='1000' [peak='5000'] [burst='1024']/>]
-            [<outbound average='1000' [burst='1024']/>]
-        [</bandwidth>]
     </network>
 
     Forward mode can be either bridge or passthrough,
     according to net if bridged or bridgeless this
     determines respectively the presence of bridge element
-    or interface subelement. Inbound or outbound element
-    can be optionally defined.
+    or interface subelement.
     """
 
     netName = netinfo.LIBVIRT_NET_PREFIX + network
@@ -96,16 +89,6 @@
         root.appendChild(XmlElement('bridge', name=network))
     else:
         forwardElem.appendChild(XmlElement('interface', dev=iface))
-
-    if qosInbound or qosOutbound:
-        bandwidthElem = XmlElement('bandwidth')
-        if qosInbound:
-            bandwidthElem.appendChild(XmlElement('inbound', **qosInbound))
-        if qosOutbound:
-            bandwidthElem.appendChild(XmlElement('outbound',
-                                                 **qosOutbound))
-        root.appendChild(bandwidthElem)
-
     return root.toxml()
 
 
diff --git a/vdsm/rpc/vdsmapi-schema.json b/vdsm/rpc/vdsmapi-schema.json
index a03eb56..fc5a9f5 100644
--- a/vdsm/rpc/vdsmapi-schema.json
+++ b/vdsm/rpc/vdsmapi-schema.json
@@ -63,8 +63,6 @@
 #
 # @bondingOptions:  #optional A string of space-separated <option>=<value> pairs
 #
-# @qosInbound:      #optional BandwidthParams for incoming traffic.
-#
 # @qosOutbound:     #optional BandwidthParams for outgoing traffic.
 #
 # Since: 4.10.0
@@ -72,7 +70,7 @@
 {'type': 'NetworkOptions',
  'data': {'*ipaddr': 'str', '*netmask': 'str', '*gateway': 'str',
           '*bootproto': 'str', '*bondingOptions', 'str',
-          '*blockingdhcp': 'bool', '*qosInbound': 'BandwidthParams',
+          '*blockingdhcp': 'bool',
           '*qosOutbound': 'BandwidthParams'}}
 
 ##
@@ -164,8 +162,6 @@
 #
 # @remove:        #optional If True, remove existing network only
 #
-# @qosInbound:    #optional BandwidthParams for incoming traffic.
-#
 # @qosOutbound:   #optional BandwidthParams for outgoing traffic.
 #
 # @defaultRoute:  #optional boolean - Is this network's gateway the host's
@@ -180,7 +176,6 @@
  'data': {'*vlan': 'str', '*bonding': 'str', '*nic': ['str'], '*ipaddr': 'str',
           '*netmask': 'str', '*gateway': 'str', '*bootproto': 'str',
           '*remove': 'bool',
-          '*qosInbound': 'BandwidthParams',
           '*qosOutbound': 'BandwidthParams',
           '*defaultRoute': 'bool',
           '*custom': 'StringMap'}}
@@ -658,8 +653,6 @@
 # @bridged:     Always True to distinguish this type from
 #               @NetInfoBridgelessNetwork
 #
-# @qosInbound:  #optional BandwidthParams for incoming traffic.
-#
 # @qosOutbound: #optional BandwidthParams for outgoing traffic.
 #
 # @opts:        A map containing information about the bridge specific options.
@@ -673,8 +666,7 @@
           'netmask': 'str', 'gateway': 'str', 'ipv4addrs': ['str'],
           'ipv6addrs': ['str'], 'ipv6gateway': 'str', 'mtu': 'uint',
           'cfg': 'StringMap', 'bridged': 'bool',
-          '*qosInbound': 'BandwidthParams', '*qosOutbound': 'BandwidthParams',
-          'opts': 'StringMap'}}
+          '*qosOutbound': 'BandwidthParams', 'opts': 'StringMap'}}
 
 ##
 # @NetInfoBridgelessNetwork:


-- 
To view, visit http://gerrit.ovirt.org/34319
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I7b82cc6b656fb6af8caa2940c9ee2d14287f3526
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.5
Gerrit-Owner: Petr Horáček <phoracek at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>


More information about the vdsm-patches mailing list