Antoni Segura Puimedon has posted comments on this change. Change subject: Add iproute2 configurator ......................................................................
Patch Set 22: Code-Review-1 (5 comments) Some minor things related to compatibility in error messages. .................................................... File vdsm/netconf/iproute2.py Line 32: from vdsm.netconfpersistence import RunningConfig Line 33: from vdsm.utils import execCmd, CommandPath Line 34: Line 35: Line 36: _BRCTL = CommandPath('brctl', '/usr/sbin/brctl') The path is defined in constants.EXT_BRCTL Line 37: Line 38: Line 39: class Iproute2(Configurator): Line 40: def __init__(self, inRollback=False): Line 77: self.runningConfig.setBonding( Line 78: bond.name, {'options': bond.options, Line 79: 'nics': [slave.name for slave in bond.slaves]}) Line 80: Line 81: def editBonding(self, bond, _netinfo): I would like to keep the same docstring as in ifcfg.py: """ Modifies the bond so that the bond in the system ends up with the same slave and options configuration that are requested. Makes a best effort not to interrupt connectivity. """ Line 82: nicsToSet = frozenset(nic.name for nic in bond.slaves) Line 83: currentNics = frozenset(_netinfo.getNicsForBonding(bond.name)) Line 84: nicsToAdd = nicsToSet Line 85: nicsToRemove = currentNics Line 212: Line 213: def addBridge(self, bridge): Line 214: rc, _, err = execCmd([_BRCTL.cmd, 'addbr', bridge.name]) Line 215: if rc != 0: Line 216: raise ConfigNetworkError(ERR_USED_BRIDGE, err) I think the current neterror code that is closer to what happens here is: ERR_FAILED_IFUP. In the future we should have more fine grained errors though. Line 217: Line 218: def addBridgePort(self, bridge): Line 219: rc, _, err = execCmd([_BRCTL.cmd, 'addif', bridge.name, Line 220: bridge.port.name]) Line 218: def addBridgePort(self, bridge): Line 219: rc, _, err = execCmd([_BRCTL.cmd, 'addif', bridge.name, Line 220: bridge.port.name]) Line 221: if rc != 0: Line 222: raise ConfigNetworkError(ERR_USED_BRIDGE, err) I think the current neterror code that is closer to what happens here is: FAILED_IFUP. In the future we should have more fine grained errors though. Line 223: Line 224: def removeBridge(self, bridge): Line 225: rc, _, err = execCmd([_BRCTL.cmd, 'delbr', bridge.name]) Line 226: if rc != 0: Line 223: Line 224: def removeBridge(self, bridge): Line 225: rc, _, err = execCmd([_BRCTL.cmd, 'delbr', bridge.name]) Line 226: if rc != 0: Line 227: raise ConfigNetworkError(ERR_USED_BRIDGE, err) This doesn't raise right now with ifcfg. We simply swallow ifdown erroneous rc. If we want to raise, and I tend to agree with it, we should add a ERR_FAILED_IFDOWN Line 228: Line 229: def addVlan(self, vlan): Line 230: ipwrapper.linkAdd(name=vlan.name, linkType='vlan', Line 231: link=vlan.device.name, args=['id', str(vlan.tag)]) -- To view, visit http://gerrit.ovirt.org/15301 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I762cfa30f78c5a46507b86f53e98bcf79dfc5844 Gerrit-PatchSet: 22 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Mark Wu <wu...@linux.vnet.ibm.com> Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com> Gerrit-Reviewer: Assaf Muller <amul...@redhat.com> Gerrit-Reviewer: Ayal Baron <aba...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Itamar Heim <ih...@redhat.com> Gerrit-Reviewer: Mark Wu <wu...@linux.vnet.ibm.com> Gerrit-Reviewer: Petr Ĺ ebek <pse...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches