Change in vdsm[master]: NetReload: netmodels for addNetwork

danken at redhat.com danken at redhat.com
Tue May 28 10:33:11 UTC 2013


Dan Kenigsberg has posted comments on this change.

Change subject: NetReload: netmodels for addNetwork
......................................................................


Patch Set 12: (10 inline comments)

mostly minor comments

....................................................
File tests/configNetworkTests.py
Line 38:     return {'fakebridgenet': {'iface': 'fakebridge', 'bridged': True},
Line 39:             'fakenet': {'iface': 'fakeint', 'bridged': False}}
Line 40: 
Line 41: 
Line 42: def raiseInvalidOpException(*args, **kwargs):
this has to be declared _private. Raising a RuntimeError is a bit nicer.

also, add space between 'unit' and 'testing'.
Line 43:     return Exception('Attempted to apply network configuration during unit'
Line 44:                      'testing.')
Line 45: 
Line 46: 


....................................................
File vdsm/configNetwork.py
Line 51: def objectivizeNetwork(bridge=None, vlan=None, bonding=None,
Line 52:                        bondingOptions=None, nics=None, mtu=None, ipaddr=None,
Line 53:                        netmask=None, gateway=None, bootproto=None,
Line 54:                        _netinfo=None, configurator=None, **opts):
Line 55:     '''Takes a series of parameters that describe a network configuration and
http://www.python.org/dev/peps/pep-0257/
Line 56:     generates an object oriented representation of it. The object are set in
Line 57:     a hierarchical way and the ip configuration parametes (if any) are set on
Line 58:     the network device object that constitutes the top of the hierarchy.'''
Line 59:     if configurator is None:


Line 142:         _netinfo = netinfo.NetInfo()
Line 143:     bridged = utils.tobool(bridged)
Line 144: 
Line 145:     if mtu:
Line 146:         mtu = int(mtu)
I actually liked the breath-bearing newline that you removed
Line 147:     if prefix:
Line 148:         if netmask:
Line 149:             raise ConfigNetworkError(ne.ERR_BAD_PARAMS,
Line 150:                                      'Both PREFIX and NETMASK supplied')


....................................................
File vdsm/netconf/ifcfg.py
Line 70:                                     netmask=netmask, mtu=bridge.mtu,
Line 71:                                     gateway=gateway, bootproto=bootproto,
Line 72:                                     **opts)
Line 73:         ifdown(bridge.name)
Line 74:         if bridge.port:
(we would always need to check for niclessness)
Line 75:             bridge.port.configure(bridge=bridge.name, **opts)
Line 76:         ifup(bridge.name, bootproto == 'dhcp' and
Line 77:              not utils.tobool(opts.get('blockingdhcp')))
Line 78:         self.configWriter.createLibvirtNetwork(network, True)


Line 73:         ifdown(bridge.name)
Line 74:         if bridge.port:
Line 75:             bridge.port.configure(bridge=bridge.name, **opts)
Line 76:         ifup(bridge.name, bootproto == 'dhcp' and
Line 77:              not utils.tobool(opts.get('blockingdhcp')))
do we really want to push blockingdhcp down to the configurator level? it's not like iproute2 would be able to take a different policy for calculating "async".
Line 78:         self.configWriter.createLibvirtNetwork(network, True)
Line 79:         self._libvirtAdded.add(bridge.name)
Line 80: 
Line 81:     def configureVlan(self, vlan, network=None, bridge=None, **opts):


Line 100:                                      netmask=netmask, gateway=gateway,
Line 101:                                      bootproto=bootproto, **opts)
Line 102:         # take down nics that need to be changed
Line 103:         _netinfo = netinfo.NetInfo()
Line 104:         vlanedIfaces = [v['iface'] for v in _netinfo.vlans.values()]
...vlans.itervalues() would avoid an intermediate list()
Line 105:         if bond.name not in vlanedIfaces:
Line 106:             for slave in bond.slaves:
Line 107:                 if slave.name not in vlanedIfaces:
Line 108:                     ifdown(slave.name)


....................................................
File vdsm/netmodels.py
Line 106: 
Line 107: 
Line 108: class Bridge(NetDevice):
Line 109:     '''This class represents traditional kernel bridges.'''
Line 110:     MAX_BRIDGE_NAME_LEN = 15
this is currently defined twice - better drop the configNet.py definition.

Within this class, a better name is MAX_NAME_LEN.
Line 111:     ILLEGAL_BRIDGE_CHARS = frozenset(':. \t')
Line 112: 
Line 113:     def __init__(self, name, configurator, ipconfig=None, mtu=None, port=None,
Line 114:                  forwardDelay=0, stp=None):


Line 109:     '''This class represents traditional kernel bridges.'''
Line 110:     MAX_BRIDGE_NAME_LEN = 15
Line 111:     ILLEGAL_BRIDGE_CHARS = frozenset(':. \t')
Line 112: 
Line 113:     def __init__(self, name, configurator, ipconfig=None, mtu=None, port=None,
I need it for testing vdsm. Really. I do it all the time.

And I see a use case for an ensemble of VMs which are disconnected from the outer world.
Line 114:                  forwardDelay=0, stp=None):
Line 115:         self.validateName(name)
Line 116:         self.port = port
Line 117:         self.forwardDelay = forwardDelay


Line 118:         self.stp = stp
Line 119:         super(Bridge, self).__init__(name, configurator, ipconfig, mtu)
Line 120: 
Line 121:     def __repr__(self):
Line 122:         return 'Bridge(%s: %r)' % (self.name, self.ports)
ports -> port. Please add a unit test to catch this.
Line 123: 
Line 124:     def configure(self, network, **opts):
Line 125:         self.configurator.configureBridge(self, network=network, **opts)
Line 126: 


Line 245:         self.netmask = netmask
Line 246:         self.gateway = gateway
Line 247: 
Line 248:     def __str__(self):
Line 249:         return self.__repr__()
isn't this the default implementation for __str__()?
Line 250: 
Line 251:     def __repr__(self):
Line 252:         return 'IPv4(%s, %s, %s)' % (self.address, self.netmask, self.gateway)
Line 253: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iba5c5b84760e27245cbe34c3b290c54e51278e72
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Giuseppe Vallarelli <gvallare at redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer 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