Change in vdsm[master]: net: remove business logic out of CachingNetInfo

ibarkan at redhat.com ibarkan at redhat.com
Mon Dec 7 13:14:28 UTC 2015


Ido Barkan has uploaded a new change for review.

Change subject: net: remove business logic out of CachingNetInfo
......................................................................

net: remove business logic out of CachingNetInfo

This object should not be able to do anything that is not related to
caching the result of assumed slow libvirt network query. Its
incorporated logic was spread in netinfo package and is now used
directly by CachingNetInfo instances, but can also be used directly
by anyone without creating such an instance. Function names were also
pep8'ed.
Logic that is not directly dependant in CachingNetInfo.networks
should be reevaluated if it can be implemented directly using netinfo
package existing logic (e.g. bonding.getBondingForNic).

Change-Id: I1bae19df6721c7559e8eeac1db590026b401f76b
Signed-off-by: Ido Barkan <ibarkan at redhat.com>
---
M lib/vdsm/kernelconfig.py
M lib/vdsm/netinfo/__init__.py
M lib/vdsm/netinfo/bonding.py
M lib/vdsm/netinfo/misc.py
M lib/vdsm/netinfo/vlans.py
M lib/vdsm/network/api.py
M lib/vdsm/network/configurators/ifcfg.py
M lib/vdsm/network/configurators/iproute2.py
M lib/vdsm/network/models.py
9 files changed, 148 insertions(+), 129 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/00/50000/1

diff --git a/lib/vdsm/kernelconfig.py b/lib/vdsm/kernelconfig.py
index e0e625f..89fcb6c 100644
--- a/lib/vdsm/kernelconfig.py
+++ b/lib/vdsm/kernelconfig.py
@@ -25,16 +25,13 @@
 from .netinfo import addresses
 from .netinfo import bonding
 from .netinfo import bridges
+from .netinfo import misc
 from .netinfo import mtus
 from . import utils
 from .netconfpersistence import BaseConfig
 
 
 class KernelConfig(BaseConfig):
-    # TODO: after the netinfo API is refactored, we should decide if we need
-    # TODO: the dependency of KernelConfig in a NetInfo object.
-    # TODO: The only real dependency is on the products of
-    # TODO: NetInfo.getNicsVlanAndBondingForNetwork and on NetInfo.Bondings
     def __init__(self, netinfo):
         super(KernelConfig, self).__init__({}, {})
         self._netinfo = netinfo
@@ -75,7 +72,9 @@
 
 def _translate_netinfo_net(net, net_attr, netinfo_):
     nics, _, vlan_id, bond = \
-        netinfo_.getNicsVlanAndBondingForNetwork(net)
+        misc.nics_vlan_and_bonding_for_network(
+            net, netinfo_.networks, netinfo_.nics, netinfo_.vlans,
+            netinfo_.bondings)
     attributes = {}
     _translate_bridged(attributes, net_attr)
     _translate_mtu(attributes, net_attr)
diff --git a/lib/vdsm/netinfo/__init__.py b/lib/vdsm/netinfo/__init__.py
index 97686fb..ec00c0a 100644
--- a/lib/vdsm/netinfo/__init__.py
+++ b/lib/vdsm/netinfo/__init__.py
@@ -19,7 +19,6 @@
 #
 
 from __future__ import absolute_import
-from itertools import chain
 import logging
 import os
 import errno
@@ -138,8 +137,8 @@
 
 
 def ifaceUsed(iface):
-    """Lightweight implementation of bool(Netinfo.ifaceUsers()) that does not
-    require a NetInfo object."""
+    """Lightweight implementation of bool(netinfo.misc.ifaceUsers()) that does
+    not require a NetInfo object."""
     if os.path.exists(os.path.join(NET_PATH, iface, 'brport')):  # Is it a port
         return True
     for linkDict in nl_link.iter_links():
@@ -243,104 +242,3 @@
 
     def del_bonding(self, bonding):
         del self.bondings[bonding]
-
-    def getNetworksAndVlansForIface(self, iface):
-        """ Returns tuples of (bridge/network, vlan) connected to  nic/bond """
-        return chain(self._getBridgedNetworksAndVlansForIface(iface),
-                     self._getBridgelessNetworksAndVlansForIface(iface))
-
-    def _getBridgedNetworksAndVlansForIface(self, iface):
-        """ Returns tuples of (bridge, vlan) connected to nic/bond """
-        for network, netdict in self.networks.iteritems():
-            if netdict['bridged']:
-                for interface in netdict['ports']:
-                    if iface == interface:
-                        yield (network, None)
-                    elif interface.startswith(iface + '.'):
-                        yield (network, vlans.vlan_id(interface))
-
-    def _getBridgelessNetworksAndVlansForIface(self, iface):
-        """ Returns tuples of (network, vlan) connected to nic/bond """
-        for network, netdict in self.networks.iteritems():
-            if not netdict['bridged']:
-                if iface == netdict['iface']:
-                    yield (network, None)
-                elif netdict['iface'].startswith(iface + '.'):
-                    yield (network, vlans.vlan_id(netdict['iface']))
-
-    def getVlansForIface(self, iface):
-        for vlandict in six.itervalues(self.vlans):
-            if iface == vlandict['iface']:
-                yield vlandict['vlanid']
-
-    def getNetworkForIface(self, iface):
-        """ Return the network attached to nic/bond """
-        for network, netdict in self.networks.iteritems():
-            if ('ports' in netdict and iface in netdict['ports'] or
-                    iface == netdict['iface']):
-                return network
-
-    def getBridgedNetworkForIface(self, iface):
-        """ Return all bridged networks attached to nic/bond """
-        for bridge, netdict in self.networks.iteritems():
-            if netdict['bridged'] and iface in netdict['ports']:
-                return bridge
-
-    def getNicsForBonding(self, bond):
-        bondAttrs = self.bondings[bond]
-        return bondAttrs['slaves']
-
-    def getBondingForNic(self, nic):
-        bondings = [b for (b, attrs) in self.bondings.iteritems() if
-                    nic in attrs['slaves']]
-        if bondings:
-            assert len(bondings) == 1, \
-                "Unexpected configuration: More than one bonding per nic"
-            return bondings[0]
-        return None
-
-    def getNicsVlanAndBondingForNetwork(self, network):
-        vlan = None
-        vlanid = None
-        bonding = None
-        lnics = []
-
-        if self.networks[network]['bridged']:
-            ports = self.networks[network]['ports']
-        else:
-            ports = []
-            interface = self.networks[network]['iface']
-            ports.append(interface)
-
-        for port in ports:
-            if port in self.vlans:
-                assert vlan is None
-                nic = vlans.vlan_device(port)
-                vlanid = vlans.vlan_id(port)
-                vlan = port  # vlan devices can have an arbitrary name
-                assert self.vlans[port]['iface'] == nic
-                port = nic
-            if port in self.bondings:
-                assert bonding is None
-                bonding = port
-                lnics += self.bondings[bonding]['slaves']
-            elif port in self.nics:
-                lnics.append(port)
-
-        return lnics, vlan, vlanid, bonding
-
-    def ifaceUsers(self, iface):
-        "Returns a list of entities using the interface"
-        users = set()
-        for n, ndict in self.networks.iteritems():
-            if ndict['bridged'] and iface in ndict['ports']:
-                users.add(n)
-            elif not ndict['bridged'] and iface == ndict['iface']:
-                users.add(n)
-        for b, bdict in self.bondings.iteritems():
-            if iface in bdict['slaves']:
-                users.add(b)
-        for v, vdict in self.vlans.iteritems():
-            if iface == vdict['iface']:
-                users.add(v)
-        return users
diff --git a/lib/vdsm/netinfo/bonding.py b/lib/vdsm/netinfo/bonding.py
index 321f365..1c8c124 100644
--- a/lib/vdsm/netinfo/bonding.py
+++ b/lib/vdsm/netinfo/bonding.py
@@ -184,3 +184,13 @@
                 if line.startswith('Permanent HW addr: '):
                     paddr[slave] = line[len('Permanent HW addr: '):-1]
     return paddr
+
+
+def bonding_for_nic(nic, bondings):
+    bondings = [b for (b, attrs) in bondings.iteritems() if
+                nic in attrs['slaves']]
+    if bondings:
+        assert len(bondings) == 1, \
+            "Unexpected configuration: More than one bonding per nic"
+        return bondings[0]
+    return None
diff --git a/lib/vdsm/netinfo/misc.py b/lib/vdsm/netinfo/misc.py
index ffee477..698e474 100644
--- a/lib/vdsm/netinfo/misc.py
+++ b/lib/vdsm/netinfo/misc.py
@@ -17,6 +17,7 @@
 #
 # Refer to the README and COPYING files for full details of the license
 from __future__ import absolute_import
+from itertools import chain
 import shlex
 
 from ..ipwrapper import getLinks
@@ -49,3 +50,95 @@
     predicate is True"""
     return [dev.name for dev in getLinks() if predicate(dev) and
             not dev.isHidden()]
+
+
+def networks_and_vlans_for_iface(iface, networks, vlans):
+    """ Returns tuples of (bridge/network, vlan) connected to  nic/bond """
+    return chain(
+        _bridged_networks_and_vlans_for_iface(iface, networks, vlans),
+        _bridgeless_networks_and_vlans_for_iface(iface, networks, vlans))
+
+
+def _bridged_networks_and_vlans_for_iface(iface, networks, vlans):
+    """ Returns tuples of (bridge, vlan) connected to nic/bond """
+    for network, netdict in networks.iteritems():
+        if netdict['bridged']:
+            for interface in netdict['ports']:
+                if iface == interface:
+                    yield (network, None)
+                elif interface.startswith(iface + '.'):
+                    yield (network, vlans.vlan_id(interface))
+
+
+def _bridgeless_networks_and_vlans_for_iface(iface, networks, vlans):
+    """ Returns tuples of (network, vlan) connected to nic/bond """
+    for network, netdict in networks.iteritems():
+        if not netdict['bridged']:
+            if iface == netdict['iface']:
+                yield (network, None)
+            elif netdict['iface'].startswith(iface + '.'):
+                yield (network, vlans.vlan_id(netdict['iface']))
+
+
+def network_for_iface(iface, networks):
+    """ Return the network attached to nic/bond """
+    for net_name, net_attr in networks.iteritems():
+        if ('ports' in net_attr and iface in net_attr['ports'] or
+                iface == net_attr['iface']):
+            return net_name
+
+
+def bridged_network_for_iface(iface, networks):
+    """ Return all bridged networks attached to nic/bond """
+    for bridge, netdict in networks.iteritems():
+        if netdict['bridged'] and iface in netdict['ports']:
+            return bridge
+
+
+def nics_vlan_and_bonding_for_network(network, networks, nics, vlans,
+                                      bondings):
+    vlan = None
+    vlanid = None
+    bonding = None
+    lnics = []
+
+    if networks[network]['bridged']:
+        ports = networks[network]['ports']
+    else:
+        ports = []
+        interface = networks[network]['iface']
+        ports.append(interface)
+
+    for port in ports:
+        if port in vlans:
+            assert vlan is None
+            nic = vlans.vlan_device(port)
+            vlanid = vlans.vlan_id(port)
+            vlan = port  # vlan devices can have an arbitrary name
+            assert vlans[port]['iface'] == nic
+            port = nic
+        if port in bondings:
+            assert bonding is None
+            bonding = port
+            lnics += bondings[bonding]['slaves']
+        elif port in nics:
+            lnics.append(port)
+
+    return lnics, vlan, vlanid, bonding
+
+
+def iface_users(iface, networks, bondings, vlans):
+    """Returns a list of entities using the interface"""
+    users = set()
+    for n, ndict in networks.iteritems():
+        if ndict['bridged'] and iface in ndict['ports']:
+            users.add(n)
+        elif not ndict['bridged'] and iface == ndict['iface']:
+            users.add(n)
+    for b, bdict in bondings.iteritems():
+        if iface in bdict['slaves']:
+            users.add(b)
+    for v, vdict in vlans.iteritems():
+        if iface == vdict['iface']:
+            users.add(v)
+    return users
diff --git a/lib/vdsm/netinfo/vlans.py b/lib/vdsm/netinfo/vlans.py
index 4222d90..73de077 100644
--- a/lib/vdsm/netinfo/vlans.py
+++ b/lib/vdsm/netinfo/vlans.py
@@ -18,6 +18,7 @@
 # Refer to the README and COPYING files for full details of the license
 from __future__ import absolute_import
 from functools import partial
+import six
 
 from . import bonding
 from .misc import visible_devs
@@ -35,6 +36,12 @@
             yield linkDict['name']
 
 
+def find_iface_connected_vlans(iface, all_vlans):
+    for vlan in six.itervalues(all_vlans):
+        if iface == vlan['iface']:
+            yield vlan['vlanid']
+
+
 def is_vlanned(device_name):
     return any(vlan.startswith(device_name + '.') for vlan in vlans())
 
diff --git a/lib/vdsm/network/api.py b/lib/vdsm/network/api.py
index 4ba04c3..cdeca6f 100755
--- a/lib/vdsm/network/api.py
+++ b/lib/vdsm/network/api.py
@@ -32,8 +32,9 @@
 from vdsm import constants
 from vdsm import kernelconfig
 from vdsm import netconfpersistence
-from vdsm.netinfo import (addresses, libvirtNets2vdsm, bridges,
-                          get as netinfo_get, CachingNetInfo,
+from vdsm.netinfo import (addresses, libvirtNets2vdsm,
+                          bonding as netinfo_bonding, bridges,
+                          get as netinfo_get, CachingNetInfo, misc,
                           networks as netinfo_networks, nics as netinfo_nics,
                           NET_PATH)
 from vdsm import udevadm
@@ -147,7 +148,7 @@
             raise ConfigNetworkError(ne.ERR_BAD_BONDING, 'Multiple nics '
                                      'require a bonding device')
         else:
-            bond = _netinfo.getBondingForNic(nic)
+            bond = netinfo_bonding.bonding_for_nic(nic, _netinfo.bondings)
             if bond:
                 raise ConfigNetworkError(ne.ERR_USED_NIC, 'nic %s already '
                                          'enslaved to %s' % (nic, bond))
@@ -185,9 +186,10 @@
 
 
 def _validateInterNetworkCompatibility(ni, vlan, iface):
-    iface_nets_by_vlans = dict((vlan, net)  # None is also a valid key
-                               for (net, vlan)
-                               in ni.getNetworksAndVlansForIface(iface))
+    iface_nets_by_vlans = dict(
+        (vlan, net)  # None is also a valid key
+        for (net, vlan)
+        in misc.networks_and_vlans_for_iface(iface, ni.networks, ni.vlans))
 
     if vlan in iface_nets_by_vlans:
         raise ConfigNetworkError(ne.ERR_BAD_PARAMS,
@@ -362,8 +364,9 @@
     bridged = _netinfo.networks[network]['bridged']
     print("Network %s(Bridged: %s):" % (network, bridged))
 
-    nics, vlan, vlan_id, bonding = _netinfo.getNicsVlanAndBondingForNetwork(
-        network)
+    nics, vlan, vlan_id, bonding = misc.nics_vlan_and_bonding_for_network(
+        network, _netinfo.networks, _netinfo.nics, _netinfo.vlans,
+        _netinfo.bondings)
 
     if bridged:
         ipaddr = _netinfo.networks[network]['addr']
@@ -457,8 +460,9 @@
                            configurator)
         return
 
-    nics, vlan, vlan_id, bonding = _netinfo.getNicsVlanAndBondingForNetwork(
-        network)
+    nics, vlan, vlan_id, bonding = misc.nics_vlan_and_bonding_for_network(
+        network, _netinfo.networks, _netinfo.nics, _netinfo.vlans,
+        _netinfo.bondings)
     bridged = _netinfo.networks[network]['bridged']
 
     logging.info("Removing network %s with vlan=%s, bonding=%s, nics=%s,"
@@ -661,7 +665,8 @@
                     raise ConfigNetworkError(
                         ne.ERR_BAD_BONDING,
                         "Cannot remove bonding %s: does not exist" % name)
-            bond_users = _netinfo.ifaceUsers(name)
+            bond_users = misc.iface_users(name, _netinfo.networks,
+                                          _netinfo.bondings, _netinfo.vlans)
             # networks removal takes place before bondings handling, therefore
             # all assigned networks (bond_users) should be already removed
             if bond_users:
diff --git a/lib/vdsm/network/configurators/ifcfg.py b/lib/vdsm/network/configurators/ifcfg.py
index bd727a4..1240ecb 100644
--- a/lib/vdsm/network/configurators/ifcfg.py
+++ b/lib/vdsm/network/configurators/ifcfg.py
@@ -134,6 +134,7 @@
         """
         nicsToSet = frozenset(nic.name for nic in bond.slaves)
         currentNics = frozenset(_netinfo.getNicsForBonding(bond.name))
+        currentNics = frozenset(_netinfo.bondings[bond.name]['slaves'])
         nicsToAdd = nicsToSet - currentNics
 
         # Create bond configuration in case it was a non ifcfg controlled bond.
@@ -143,7 +144,8 @@
         isIfcfgControlled = os.path.isfile(NET_CONF_PREF + bond.name)
         areOptionsApplied = bond.areOptionsApplied()
         if not isIfcfgControlled or not areOptionsApplied:
-            bridgeName = _netinfo.getBridgedNetworkForIface(bond.name)
+            bridgeName = misc.bridged_network_for_iface(
+                bond.name, _netinfo.networks)
             if isIfcfgControlled and bridgeName:
                 bond.master = Bridge(bridgeName, self, port=bond)
             self.configApplier.addBonding(bond)
diff --git a/lib/vdsm/network/configurators/iproute2.py b/lib/vdsm/network/configurators/iproute2.py
index e3d1f07..1dc8d14 100644
--- a/lib/vdsm/network/configurators/iproute2.py
+++ b/lib/vdsm/network/configurators/iproute2.py
@@ -114,7 +114,7 @@
         best effort not to interrupt connectivity.
         """
         nicsToSet = frozenset(nic.name for nic in bond.slaves)
-        currentNics = frozenset(_netinfo.getNicsForBonding(bond.name))
+        currentNics = frozenset(_netinfo.bondings[bond.name]['slaves'])
         nicsToAdd = nicsToSet
         nicsToRemove = currentNics
 
diff --git a/lib/vdsm/network/models.py b/lib/vdsm/network/models.py
index a5609fd..f9b4831 100644
--- a/lib/vdsm/network/models.py
+++ b/lib/vdsm/network/models.py
@@ -22,7 +22,8 @@
 import socket
 import struct
 
-from vdsm.netinfo import bonding, ifaceUsed, mtus, nics, CachingNetInfo
+from vdsm.netinfo import (bonding, ifaceUsed, misc, mtus, nics, vlans,
+                          CachingNetInfo)
 
 from .errors import ConfigNetworkError
 from . import errors as ne
@@ -81,7 +82,8 @@
         if name not in _netinfo.nics:
             raise ConfigNetworkError(ne.ERR_BAD_NIC, 'unknown nic: %s' % name)
 
-        if _netinfo.ifaceUsers(name):
+        if misc.iface_users(name, _netinfo.networks, _netinfo.bondings,
+                            _netinfo.vlans):
             mtu = max(mtu, mtus.getMtu(name))
 
         super(Nic, self).__init__(name, configurator, ipv4, ipv6, blockingdhcp,
@@ -260,9 +262,10 @@
     def _objectivizeSlaves(cls, name, configurator, nics, mtu, _netinfo):
         slaves = []
         for nic in nics:
-            nicVlans = tuple(_netinfo.getVlansForIface(nic))
-            nicNet = _netinfo.getNetworkForIface(nic)
-            nicBond = _netinfo.getBondingForNic(nic)
+            nicVlans = tuple(vlans.find_iface_connected_vlans(
+                nic, _netinfo.vlans))
+            nicNet = misc.network_for_iface(nic, _netinfo.networks)
+            nicBond = bonding.bonding_for_nic(nic, _netinfo.bondings)
             if nicVlans or nicNet or nicBond and nicBond != name:
                 raise ConfigNetworkError(
                     ne.ERR_USED_NIC, 'nic %s already used by %s' %
@@ -279,14 +282,16 @@
             slaves = cls._objectivizeSlaves(name, configurator, _nicSort(nics),
                                             mtu, _netinfo)
             if name in _netinfo.bondings:
-                if _netinfo.ifaceUsers(name):
+                if misc.iface_users(name, _netinfo.networks,
+                                    _netinfo.bondings, _netinfo.vlans):
                     mtu = max(mtu, mtus.getMtu(name))
 
                 if not options:
                     options = _netinfo.bondings[name]['cfg'].get(
                         'BONDING_OPTS')
         elif name in _netinfo.bondings:  # Implicit bonding.
-            if _netinfo.ifaceUsers(name):
+            if misc.iface_users(name, _netinfo.networks, _netinfo.bondings,
+                                _netinfo.vlans):
                 mtu = max(mtu, mtus.getMtu(name))
 
             slaves = [Nic(nic, configurator, mtu=mtu, _netinfo=_netinfo)


-- 
To view, visit https://gerrit.ovirt.org/50000
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I1bae19df6721c7559e8eeac1db590026b401f76b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ido Barkan <ibarkan at redhat.com>


More information about the vdsm-patches mailing list