Change in vdsm[master]: NetReload: netmodels for delNetwork
asegurap at redhat.com
asegurap at redhat.com
Mon May 27 21:03:12 UTC 2013
Antoni Segura Puimedon has posted comments on this change.
Change subject: NetReload: netmodels for delNetwork
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(5 inline comments)
Just a couple of simplifications that might be worth it.
....................................................
File vdsm/configNetwork.py
Line 60: if bonding:
Line 61: topObj = Bond.objectivize(bonding, configurator, bondingOptions, nics,
Line 62: mtu, _netinfo)
Line 63: if opts.get('implicitBonding'):
Line 64: topObj.destroyOnRemove = True
Since implicitBonding is something that is checked for everytime, I'd maybe add it to the method parameters (default None).
Line 65: elif nics:
Line 66: try:
Line 67: nic, = nics
Line 68: except ValueError:
Line 239:
Line 240:
Line 241: def _validateDelNetwork(network, vlan, bonding, nics, bridged, _netinfo):
Line 242: if bonding:
Line 243: Bond.validateName(bonding)
Wouldn't this check be automatically done at the Bonding objectivization step already?
Line 244: if set(nics) != set(_netinfo.bondings[bonding]["slaves"]):
Line 245: raise ConfigNetworkError(ne.ERR_BAD_NIC, 'delNetwork: %s are '
Line 246: 'not all nics enslaved to %s' %
Line 247: (nics, bonding))
Line 245: raise ConfigNetworkError(ne.ERR_BAD_NIC, 'delNetwork: %s are '
Line 246: 'not all nics enslaved to %s' %
Line 247: (nics, bonding))
Line 248: if vlan:
Line 249: Vlan.validateTag(vlan)
Same as comment in line 243.
Line 250: if bridged:
Line 251: assertBridgeClean(network, vlan, bonding, nics)
Line 252:
Line 253:
Line 252:
Line 253:
Line 254: def _delNonVdsmNetwork(network, vlan, bonding, nics, configWriter):
Line 255: if network in netinfo.bridges():
Line 256: configWriter.removeBridge(network)
Here we'll probably have to change it to use configurator.removeBridge.
Line 257: else:
Line 258: raise ConfigNetworkError(ne.ERR_BAD_BRIDGE, "Cannot delete network"
Line 259: " %r: It doesn't exist in the system" %
Line 260: network)
Line 273: configurator = Ifcfg(configWriter)
Line 274:
Line 275: if network not in _netinfo.networks:
Line 276: logging.info("Network %r: doesn't exist in libvirt database", network)
Line 277: _delNonVdsmNetwork(network, vlan, bonding, nics)
You forgot to pass configWriter, right? In any case, like I said before I'd substitute it for configurator.
Line 278: return
Line 279:
Line 280: nics, vlan, bonding = _netinfo.getNicsVlanAndBondingForNetwork(network)
Line 281: bridged = _netinfo.networks[network]['bridged']
--
To view, visit http://gerrit.ovirt.org/14873
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9e11fd923a4f5d07d069193278f3eadc94f327ec
Gerrit-PatchSet: 2
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: Giuseppe Vallarelli <gvallare 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