Ido Barkan has posted comments on this change. Change subject: hooks: Open vSwitch configurator ......................................................................
Patch Set 51: Code-Review-1 (16 comments) code review is still WIP, but It would be nice if you start working on it. Can you please respond with 'done' to my comments so I could keep track? https://gerrit.ovirt.org/#/c/40312/51/vdsm_hooks/ovs/ovs_before_network_setup.py File vdsm_hooks/ovs/ovs_before_network_setup.py: Line 83: def __init__(self, in_rollback): Line 84: self.running_config = RunningConfig() Line 85: self.initial_config = deepcopy(self.running_config) Line 86: self.in_rollback = in_rollback Line 87: self.configurator = Iproute2() called configApplier in other configurators Line 88: Line 89: self.ip_configurator = IP_Configurator(self.running_config, Line 90: self.configurator) Line 91: self.ovs_configurator = OVS_Configurator(self.running_config, Line 103: else: Line 104: hooking.log('ROLLBACK: Config failed. Entering rollback') Line 105: # Remove Libvirt OVS networks Line 106: for network, attrs in self.running_config.networks.items(): Line 107: if rget(attrs, ('custom', 'ovs')): this repeats itself many times. how about: def _is_ovs(net_attrs): return _rget(net_attrs, ('custom', 'ovs')) def libvirt_nets(nets): for network, attrs in nets.iteritems(): if _is_ovs(attrs): yield network, attrs Line 108: try: Line 109: libvirt.removeNetwork(network) Line 110: except: Line 111: pass Line 130: self.initial_config.bonds) Line 131: hooking.log('ROLLBACK: Successfully finished, initial error:') Line 132: hooking.exit_hook(traceback.format_exc()) Line 133: finally: Line 134: pass is that necessary? Line 135: Line 136: def run(self, networks, bondings): Line 137: with self.rollback(): Line 138: non_ovs_bondings = self.ovs_configurator.handle_bondings(bondings) Line 157: if is_ovs_bond(attrs): Line 158: running_config.removeBonding(bonding) Line 159: Line 160: Line 161: class OVS_Configurator: pop8 OVSConfigurator + pls use new style classes Line 162: VALID_MODES = frozenset(['active-backup', 'balance-tcp', 'balance-slb']) Line 163: VALID_LACP = frozenset(['active', 'passive', 'off']) Line 164: Line 165: def __init__(self, running_config, ip_configurator): Line 166: self.running_config = running_config Line 167: self.ip_configurator = ip_configurator Line 168: self.commands = [] Line 169: # We support only one vlanless network Line 170: self.vlanless_network = self.get_vlanless_network() 'untagged_net' is a better more common name in vdsm Line 171: # Remove nic-port only when it is not used by another network Line 172: self.nic_counter = self.get_nic_counter() Line 173: self.new_libvirt_networks = {} Line 174: Line 168: self.commands = [] Line 169: # We support only one vlanless network Line 170: self.vlanless_network = self.get_vlanless_network() Line 171: # Remove nic-port only when it is not used by another network Line 172: self.nic_counter = self.get_nic_counter() 'networks_by_nics'? Line 173: self.new_libvirt_networks = {} Line 174: Line 175: @staticmethod Line 176: def destroy_ovs_bridge(): Line 204: if 'vlan' not in attrs: Line 205: return network Line 206: return None Line 207: Line 208: def run(self): this method should be declared first after __init__ Line 209: """ If there are any needed changes in OVS network listed in Line 210: self.commands, apply them. Otherwise do nothing. Line 211: """ Line 212: if len(self.commands) > 0: Line 208: def run(self): Line 209: """ If there are any needed changes in OVS network listed in Line 210: self.commands, apply them. Otherwise do nothing. Line 211: """ Line 212: if len(self.commands) > 0: you can safely drop len() here: "if self.commands" Line 213: self.commands = [EXT_OVS_VSCTL, '--', '--may-exist', 'add-br', Line 214: BRIDGE_NAME] + self.commands Line 215: hooking.log('Executing commands: %s' % ' '.join(self.commands)) Line 216: rc, _, err = hooking.execCmd(self.commands) Line 211: """ Line 212: if len(self.commands) > 0: Line 213: self.commands = [EXT_OVS_VSCTL, '--', '--may-exist', 'add-br', Line 214: BRIDGE_NAME] + self.commands Line 215: hooking.log('Executing commands: %s' % ' '.join(self.commands)) maybe appending a newline here will make sense Line 216: rc, _, err = hooking.execCmd(self.commands) Line 217: if rc != 0: Line 218: hooking.log('Executing commands failed: %s' % '\n'.join(err)) Line 219: hooking.exit_hook('\n'.join(err)) Line 460: if lacp not in OVS_Configurator.VALID_LACP: Line 461: hooking.exit_hook('%s is not valid ovs lacp value' % lacp) Line 462: Line 463: Line 464: class MTU_Configurator: new style classes pep8 Line 465: Line 466: def __init__(self, running_config): Line 467: self.running_config = running_config Line 468: Line 463: Line 464: class MTU_Configurator: Line 465: Line 466: def __init__(self, running_config): Line 467: self.running_config = running_config there is no real reason to be object oriented in mtu configurator, since this object has no real state. I think it will be a lot cleaner to convert it into a flat module and code in a completely functional manner here. Line 468: Line 469: @staticmethod Line 470: def set_iface_mtu(iface, mtu): Line 471: ipwrapper.linkSet(iface, ['mtu', str(mtu)]) Line 470: def set_iface_mtu(iface, mtu): Line 471: ipwrapper.linkSet(iface, ['mtu', str(mtu)]) Line 472: Line 473: @staticmethod Line 474: def get_mtu_for_nic(nic, mtu, changes): it is a bad name, since this method has a side effect. Also, this is very "c++ style". Lets try to be pure. get the list of devices, and return a dictionary with changes (single dict comprehension). The caller should compute the list of devices it wants mtu info about (as already being done in mtus_nics_slaves or mtus_bonds for example). Line 475: """ Compare parameter mtu and MTU of an existing nic. If mtu is bigger, Line 476: save its value to changes dictionary {nic: mtu}. Line 477: """ Line 478: current_nic_mtu = netinfo.getMtu(nic) Line 481: changes[nic] = nic_mtu Line 482: Line 483: def change_mtus(self): Line 484: """ Setup MTUs for OVS networks. We have to do this in three iterations Line 485: because of change of lesser device could influence an upper. so why not starting from lesser devices? Line 486: """ Line 487: mtu_changes = self.mtus_nics_slaves() Line 488: for iface, mtu in mtu_changes.items(): Line 489: MTU_Configurator.set_iface_mtu(iface, mtu) Line 485: because of change of lesser device could influence an upper. Line 486: """ Line 487: mtu_changes = self.mtus_nics_slaves() Line 488: for iface, mtu in mtu_changes.items(): Line 489: MTU_Configurator.set_iface_mtu(iface, mtu) self.set_iface_mtu is better Line 490: Line 491: mtu_changes = self.mtus_bonds() Line 492: for iface, mtu in mtu_changes.items(): Line 493: MTU_Configurator.set_iface_mtu(iface, mtu) Line 500: """ Get MTUs for bonds' slaves and nics under networks. Bondings' MTUs Line 501: will be changed as a consequence. Line 502: """ Line 503: changes = {} Line 504: for network, attrs in self.running_config.networks.items(): see comment on get_mtu_for_nic Line 505: ovs = rget(attrs, ('custom', 'ovs')) Line 506: mtu = attrs.get('mtu') Line 507: if ovs and mtu is not None: Line 508: nic = attrs.get('nic') Line 673: def main(): Line 674: setup_nets_config = hooking.read_json() Line 675: hooking.log('Hook started, handling networks: %s' % setup_nets_config) Line 676: Line 677: configurator = Configurator( IMO the separation of ovs networks from 'regular' vdsm networks should take place here, before calling configurator.run. this will allow the configurator to stop filtering for them each and every time , and clean your code. Line 678: setup_nets_config['request']['options'].get('_inRollback', False)) Line 679: non_ovs_networks, non_ovs_bondings = configurator.run( Line 680: setup_nets_config['request']['networks'], Line 681: setup_nets_config['request']['bondings']) -- To view, visit https://gerrit.ovirt.org/40312 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id602b6cc87a663424d06c77d1847d2c2d60d289f Gerrit-PatchSet: 51 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Petr Horáček <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Barak Azulay <[email protected]> Gerrit-Reviewer: Barak Azulay <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Ido Barkan <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Petr Horáček <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
