Antoni Segura Puimedon has posted comments on this change. Change subject: network/api.py: delete network's orphans ......................................................................
Patch Set 4: Code-Review-1 (4 comments) http://gerrit.ovirt.org/#/c/31018/4/vdsm/network/api.py File vdsm/network/api.py: Line 588: Line 589: Line 590: def _emergencyNetworkCleanup(network, networkAttrs, configurator): Line 591: """Remove all leftovers after failed setupNetwork""" Line 592: d = dict(networkAttrs) isn't networkAttrs already a dictionary? Why the casting/renaming? Line 593: _netinfo = netinfo.NetInfo() Line 594: Line 595: def _used(devName): Line 596: masters = _netinfo.ifaceUsers(devName) Line 609: topNetDev = Bond.objectivize(d['bonding'], configurator, None, Line 610: None, None, _netinfo, True) Line 611: elif 'nic' in d: Line 612: if d['nic'] in _netinfo.nics: Line 613: if not _used(d['nic']): Will this not be checked already by the configurator removeNic code? Line 614: topNetDev = Nic(d['nic'], configurator, _netinfo=_netinfo) Line 615: if 'vlan' in d: Line 616: vlan_name = '%s.%s' % (topNetDev.name, d['vlan']) Line 617: if vlan_name in _netinfo.vlans: Line 611: elif 'nic' in d: Line 612: if d['nic'] in _netinfo.nics: Line 613: if not _used(d['nic']): Line 614: topNetDev = Nic(d['nic'], configurator, _netinfo=_netinfo) Line 615: if 'vlan' in d: Note that vlan will only happen if 'bonding' in d or 'nic' in d. Line 616: vlan_name = '%s.%s' % (topNetDev.name, d['vlan']) Line 617: if vlan_name in _netinfo.vlans: Line 618: topNetDev = Vlan(topNetDev, d['vlan'], configurator) Line 619: if d['bridged']: Line 738: addNetwork(network, configurator=configurator, Line 739: implicitBonding=True, _netinfo=_netinfo, **d) Line 740: except ConfigNetworkError: Line 741: logger.debug("Adding network %r failed. Running " Line 742: "orphan-devices cleanup", network) If I understand correctly _emergencyNetworkCleanup was designed for when ConfigNetworkError is raised with the network/error FAILED_IFUP. Here you are calling it for any ConfigNetworkErrror. It would probably be best if you check error codes that might need it and error codes that don't. Line 743: _emergencyNetworkCleanup(network, networkAttrs, configurator) Line 744: raise Line 745: Line 746: _netinfo.updateDevices() # Things like a bond mtu can change -- To view, visit http://gerrit.ovirt.org/31018 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id14d109d67c5906bc85a2fcf98dfabd61d96da13 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Petr Horáček <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
