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