Change in vdsm[master]: Refactor configNetwork module

asegurap at redhat.com asegurap at redhat.com
Thu Nov 15 16:09:56 UTC 2012


Antoni Segura Puimedon has posted comments on this change.

Change subject: Refactor configNetwork module
......................................................................


Patch Set 3: (18 inline comments)

Thanks a lot for the work you put into this. I would like to take the refactoring you are doing as an opportunity to structure the network configuration in a more flexible and semantically organized way. Thus, the comments that follow.

....................................................
File tests/configNetworkTests.py
Line 27: import shutil
Line 28: import pwd
Line 29: from contextlib import contextmanager
Line 30: import inspect
Line 31: 
I'd move around some methods as decribed in configNetworks/nativeNetConfig.
Line 32: import netConfig
Line 33: import nativeNetConfig
Line 34: from vdsm import netinfo
Line 35: from vdsm.utils import memoized


....................................................
File vdsm/clientIF.py
Line 36: from vdsm import libvirtconnection
Line 37: import vm
Line 38: from vdsm import constants
Line 39: from vdsm import utils
Line 40: from nativeNetConfig import ConfigWriter
as with restore, I'm not sure if we should not get which netconfig to use from vdsm.conf
Line 41: import caps
Line 42: from vmChannels import Listener
Line 43: from libvirtvm import LibvirtVm
Line 44: import blkid


....................................................
File vdsm/configNetwork.py
Line 112: 
Line 113: 
Line 114: def setSafeNetworkConfig():
Line 115:     """Declare current network configuration as 'safe'"""
Line 116:     execCmd([constants.EXT_VDSM_STORE_NET_CONFIG])
I'm not entirely sure we shouldn't put this as network backend dependent, and thus, make it use the dispatch method. A different backend might require a different way of persisting the conf.
Line 117: 
Line 118: 
Line 119: def usage():
Line 120:     print """Usage:


....................................................
File vdsm/nativeNetConfig.py
Line 66:         rc, out, err = _ifup(iface)
Line 67:         return rc
Line 68: 
Line 69: 
Line 70: def ifaceUsers(iface):
I'm quite favorable to moving this method to netConfig, as it is only netinfo dependant, so all the NetConfig modules are likely to do it the same way.

Is this part of the configNetwork "stable" api? If not, I would really prefer it to be in NetInfo itself.
Line 71:     "Returns a list of entities using the interface"
Line 72:     _netinfo = netinfo.NetInfo()
Line 73:     users = set()
Line 74:     for n, ndict in _netinfo.networks.iteritems():


Line 84:             users.add(v)
Line 85:     return users
Line 86: 
Line 87: 
Line 88: def nicOtherUsers(bridge, vlan, bonding, nic):
I'm quite favorable to moving this method to netConfig, as it is only netinfo dependant, so all the NetConfig modules are likely to do it the same way.

Is this part of the configNetwork "stable" api? If not, I would really prefer it to be in NetInfo itself.
Line 89:     """
Line 90:     Returns a list of interfaces using a nic,
Line 91:     other than the specified one (used for validation)
Line 92:     """


Line 102:     users.discard(owner)
Line 103:     return users
Line 104: 
Line 105: 
Line 106: def bondingOtherUsers(bridge, vlan, bonding):
I'm quite favorable to moving this method to netConfig, as it is only netinfo dependant, so all the NetConfig modules are likely to do it the same way.

Is this part of the configNetwork "stable" api? If not, I would really prefer it to be in NetInfo itself.
Line 107:     """
Line 108:     Return a list of nics/interfaces using a bonding,
Line 109:     other than the specified one (used for validation)
Line 110:     """


Line 660:                 cf = self.NET_CONF_PREF + nics[0]
Line 661:                 self._updateConfigValue(cf, 'MTU', str(newmtu), newmtu is None)
Line 662: 
Line 663: 
Line 664: def isBridgeNameValid(bridgeName):
I'm quite favorable to moving this method to netConfig as all the NetConfig modules are likely to do it the same way.
Line 665:     return (bridgeName and len(bridgeName) <= MAX_BRIDGE_NAME_LEN and
Line 666:             len(set(bridgeName) & ILLEGAL_BRIDGE_CHARS) == 0 and
Line 667:             not bridgeName.startswith('-'))
Line 668: 


Line 666:             len(set(bridgeName) & ILLEGAL_BRIDGE_CHARS) == 0 and
Line 667:             not bridgeName.startswith('-'))
Line 668: 
Line 669: 
Line 670: def validateBridgeName(bridgeName):
I'm quite favorable to moving this method to netConfig as all the NetConfig modules are likely to do it the same way.
Line 671:     if not isBridgeNameValid(bridgeName):
Line 672:         raise netConfig.ConfigNetworkError(ne.ERR_BAD_BRIDGE,
Line 673:                                  "Bridge name isn't valid: %r" % bridgeName)
Line 674: 


Line 672:         raise netConfig.ConfigNetworkError(ne.ERR_BAD_BRIDGE,
Line 673:                                  "Bridge name isn't valid: %r" % bridgeName)
Line 674: 
Line 675: 
Line 676: def _validateIpAddress(address):
I'm quite favorable to moving this method to netConfig as all the NetConfig modules are likely to do it the same way.
Line 677:     try:
Line 678:         parts = address.split(".")
Line 679:         if len(parts) != 4:
Line 680:             return False


Line 685:         return False
Line 686:     return True
Line 687: 
Line 688: 
Line 689: def validateIpAddress(ipAddr):
I'm quite favorable to moving this method to netConfig as all the NetConfig modules are likely to do it the same way.
Line 690:     if not _validateIpAddress(ipAddr):
Line 691:         raise netConfig.ConfigNetworkError(ne.ERR_BAD_ADDR,
Line 692:                                  "Bad IP address: %r" % ipAddr)
Line 693: 


Line 691:         raise netConfig.ConfigNetworkError(ne.ERR_BAD_ADDR,
Line 692:                                  "Bad IP address: %r" % ipAddr)
Line 693: 
Line 694: 
Line 695: def validateNetmask(netmask):
I'm quite favorable to moving this method to netConfig as all the NetConfig modules are likely to do it the same way.
Line 696:     if not _validateIpAddress(netmask):
Line 697:         raise netConfig.ConfigNetworkError(ne.ERR_BAD_ADDR,
Line 698:                                  "Bad netmask: %r" % netmask)
Line 699: 


Line 697:         raise netConfig.ConfigNetworkError(ne.ERR_BAD_ADDR,
Line 698:                                  "Bad netmask: %r" % netmask)
Line 699: 
Line 700: 
Line 701: def validateGateway(gateway):
I'm quite favorable to moving this method to netConfig as all the NetConfig modules are likely to do it the same way.
Line 702:     if not _validateIpAddress(gateway):
Line 703:         raise netConfig.ConfigNetworkError(ne.ERR_BAD_ADDR,
Line 704:                                  "Bad gateway: %r" % gateway)
Line 705: 


Line 703:         raise netConfig.ConfigNetworkError(ne.ERR_BAD_ADDR,
Line 704:                                  "Bad gateway: %r" % gateway)
Line 705: 
Line 706: 
Line 707: def validateBondingOptions(bonding, bondingOptions):
I'm quite favorable to moving this method to netConfig as all the NetConfig modules are likely to do it the same way (although this one with ovs/team might differ, so it is not a priority to move it there).
Line 708:     'Example: BONDING_OPTS="mode=802.3ad miimon=150"'
Line 709:     try:
Line 710:         for option in bondingOptions.split():
Line 711:             key, value = option.split('=')


Line 717:         raise netConfig.ConfigNetworkError(ne.ERR_BAD_BONDING,
Line 718:                 "Error parsing bonding options: %r" % bondingOptions)
Line 719: 
Line 720: 
Line 721: def _validateInterNetworkCompatibility(ni, vlan, iface, bridged):
I'm quite favorable to moving this method to netConfig as all the NetConfig modules are likely to do it the same way (although this one with ovs might differ, so it is not a priority to move it there).
Line 722:     """
Line 723:     Verify network compatibility with other networks on iface (bond/nic).
Line 724: 
Line 725:     Only following combinations allowed:


Line 758:                         "interface %r already has networks" %
Line 759:                         (iface))
Line 760: 
Line 761: 
Line 762: def _addNetworkValidation(_netinfo, network, vlan, bonding, nics, ipaddr,
I'm quite favorable to moving this method to netConfig as all the NetConfig modules are likely to do it the same way (although this one with ovs might differ, so it is not a priority to move it there).
Line 763:                           netmask, gateway, bondingOptions, bridged=True,
Line 764:                           implicitBonding=False, **options):
Line 765:     # The (relatively) new setupNetwork verb allows to specify a network on
Line 766:     # top of an existing bonding device. The nics of this bonds are taken


Line 840:         else:
Line 841:             _validateInterNetworkCompatibility(_netinfo, vlan, nic, bridged)
Line 842: 
Line 843: 
Line 844: def _validateNetworkSetup(networks={}, bondings={}):
I'm quite favorable to moving this method to configNetwork as all the NetConfig modules are likely to do it the same way.
Line 845:     _netinfo = netinfo.NetInfo()
Line 846: 
Line 847:     for network, networkAttrs in networks.iteritems():
Line 848:         if networkAttrs.get('remove', False):


Line 890:         raise netConfig.ConfigNetworkError(ne.ERR_USED_BRIDGE,
Line 891:                  'bridge %s has interfaces %s connected' % (bridge, brifs))
Line 892: 
Line 893: 
Line 894: def clientSeen(timeout):
I'm quite favorable to moving this method to NetConfig as all the NetConfig modules are likely to do it the same way.
Line 895:     start = time.time()
Line 896:     while timeout >= 0:
Line 897:         if os.stat(constants.P_VDSM_CLIENT_LOG).st_mtime > start:
Line 898:             return True


....................................................
File vdsm/vdsm-restore-net-config
Line 21: 
Line 22: import os
Line 23: import glob
Line 24: 
Line 25: import nativeNetConfig
It nicely preserves the current functionality. However, I think it'd be better if if we would use vdsm.conf to store which NetConfig the system is using and invoke a restore method of that specific module.
Line 26: 
Line 27: 
Line 28: def main():
Line 29:     # this should NOT be used in ovirt-node, where configuration persistence is


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I052955a8e2b7b92b123a5179afd15b79798e6c50
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Igor Lvovsky <ilvovsky at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list