Ido Barkan has posted comments on this change. Change subject: hooks: Open vSwitch configurator ......................................................................
Patch Set 57: Code-Review-1 (24 comments) a few more stuff https://gerrit.ovirt.org/#/c/40312/57/vdsm_hooks/ovs/ovs_before_network_setup.py File vdsm_hooks/ovs/ovs_before_network_setup.py: Line 129: running_config = RunningConfig() Line 130: initial_config = deepcopy(running_config) Line 131: Line 132: ovs_nets, non_ovs_nets, ovs_bonds, non_ovs_bonds = \ Line 133: _separate_ovs_nets_bonds(nets, bonds, running_config) configure should not call seperate_ovs_nets_bonds(). it should configure! call it before you call configure() and give configure only ovs_bonds and ovs_nets. Line 134: Line 135: # NOTE: temporary Line 136: # with _rollback(nets, bonds, running_config, initial_config, in_rollback): Line 137: configure_ovs(ovs_nets, ovs_bonds, running_config) Line 221: Line 222: if __name__ == '__main__': Line 223: try: Line 224: if '--test' in sys.argv: Line 225: # These tests creates|removes|cleanups simple bonded and vlaned this will make us sad. either except an importError and print this usage string or handle sys.path yourself here (only for test case) Line 226: # network using request formated in VDSM way. Line 227: # Usage: PYTHONPATH=vdsm:vdsm/vdsm \ Line 228: # ./ovs_before_network_setup.py -t add|del|clean Line 229: if 'add' in sys.argv: https://gerrit.ovirt.org/#/c/40312/57/vdsm_hooks/ovs/ovs_before_network_setup_ip.py File vdsm_hooks/ovs/ovs_before_network_setup_ip.py: Line 91: iproute2._addSourceRoute(net_dev) Line 92: Line 93: Line 94: def _remove_ip_config(iface, ipv4, ipv6): Line 95: net_dev = NetDevice(iface, iproute2, ipv4=ipv4) why not telling NetDevice about ipv6. can it do harm? one day we will support source routing for ipv6 Line 96: DynamicSourceRoute.addInterfaceTracking(net_dev) Line 97: dhclient = DhcpClient(iface) Line 98: dhclient.shutdown() Line 99: iproute2._removeSourceRoute(net_dev, DynamicSourceRoute) Line 94: def _remove_ip_config(iface, ipv4, ipv6): Line 95: net_dev = NetDevice(iface, iproute2, ipv4=ipv4) Line 96: DynamicSourceRoute.addInterfaceTracking(net_dev) Line 97: dhclient = DhcpClient(iface) Line 98: dhclient.shutdown() nit: you can merge those 2 lines Line 99: iproute2._removeSourceRoute(net_dev, DynamicSourceRoute) Line 100: if ipv4.address or ipv6.address: Line 101: with suppress(ipwrapper.IPRoute2Error): # device does not exist Line 102: ipwrapper.addrFlush(iface) Line 95: net_dev = NetDevice(iface, iproute2, ipv4=ipv4) Line 96: DynamicSourceRoute.addInterfaceTracking(net_dev) Line 97: dhclient = DhcpClient(iface) Line 98: dhclient.shutdown() Line 99: iproute2._removeSourceRoute(net_dev, DynamicSourceRoute) make it public :) Line 100: if ipv4.address or ipv6.address: Line 101: with suppress(ipwrapper.IPRoute2Error): # device does not exist Line 102: ipwrapper.addrFlush(iface) Line 103: Line 101: with suppress(ipwrapper.IPRoute2Error): # device does not exist Line 102: ipwrapper.addrFlush(iface) Line 103: Line 104: Line 105: def configure_ip(nets, init_nets): why not just call it running_config? Line 106: ip_config_to_set = {} Line 107: ip_config_to_remove = {} Line 108: Line 109: for net, attrs in iter_ovs_nets(nets): Line 123: ipv4, ipv6, port, 'blockingdhcp' in attrs) Line 124: Line 125: hooking.log('Remove IP configuration of: %s' % ip_config_to_remove) Line 126: hooking.log('Set IP configuration: %s' % ip_config_to_set) Line 127: for iface, (ipv4, ipv6) in ip_config_to_remove.items(): itreitems Line 128: _remove_ip_config(iface, ipv4, ipv6) Line 129: for iface, (ipv4, ipv6, blockingdhcp, port) in ip_config_to_set.items(): https://gerrit.ovirt.org/#/c/40312/57/vdsm_hooks/ovs/ovs_before_network_setup_mtu.py File vdsm_hooks/ovs/ovs_before_network_setup_mtu.py: Line 34: changes[device] = mtu Line 35: Line 36: Line 37: def _mtus_nics_slaves(running_config): Line 38: """ Get MTUs for nics and bondings slaves. Bondings MTUs will be bonding slaves can be only nics Line 39: changed as a consequence. Line 40: """ Line 41: changes = {} Line 42: for net, attrs in iter_ovs_nets(running_config.networks): Line 83: Line 84: def configure_mtu(running_config): Line 85: """ Setup MTUs for OVS networks. We have to do this in three iterations Line 86: because of change of a lesser device could influence an upper. This way we Line 87: keep MTU handling compatible with standard VDSM. can't we do it in a single pass? Line 88: """ Line 89: mtu_changes = _mtus_nics_slaves(running_config) Line 90: for iface, mtu in mtu_changes.items(): Line 91: _set_iface_mtu(iface, mtu) Line 85: """ Setup MTUs for OVS networks. We have to do this in three iterations Line 86: because of change of a lesser device could influence an upper. This way we Line 87: keep MTU handling compatible with standard VDSM. Line 88: """ Line 89: mtu_changes = _mtus_nics_slaves(running_config) you could nest to for loops Line 90: for iface, mtu in mtu_changes.items(): Line 91: _set_iface_mtu(iface, mtu) Line 92: mtu_changes = _mtus_bonds(running_config) Line 93: for iface, mtu in mtu_changes.items(): Line 86: because of change of a lesser device could influence an upper. This way we Line 87: keep MTU handling compatible with standard VDSM. Line 88: """ Line 89: mtu_changes = _mtus_nics_slaves(running_config) Line 90: for iface, mtu in mtu_changes.items(): iteritems Line 91: _set_iface_mtu(iface, mtu) Line 92: mtu_changes = _mtus_bonds(running_config) Line 93: for iface, mtu in mtu_changes.items(): Line 94: _set_iface_mtu(iface, mtu) https://gerrit.ovirt.org/#/c/40312/57/vdsm_hooks/ovs/ovs_before_network_setup_ovs.py File vdsm_hooks/ovs/ovs_before_network_setup_ovs.py: Line 44: hooking.exit_hook('\n'.join(err)) Line 45: Line 46: Line 47: def _remove_redundant_ovs_bridge(): Line 48: " Remove OVS Bridge if there is no underlying port anymore. " triple double quotes Line 49: rc, ifaces, err = execCmd([EXT_OVS_VSCTL, 'list-ifaces', BRIDGE_NAME]) Line 50: if len(ifaces) == 0: Line 51: hooking.log('Removing redundant OVS bridge') Line 52: destroy_ovs_bridge() Line 52: destroy_ovs_bridge() Line 53: Line 54: Line 55: def _get_nets_by_nic(running_config): Line 56: " Transform running config into {nic: set(networks)}. " triple double quotes Line 57: nets_by_nic = {} Line 58: for net, attrs in running_config.networks.items(): Line 59: nic = attrs.get('nic') Line 60: if nic is not None: Line 54: Line 55: def _get_nets_by_nic(running_config): Line 56: " Transform running config into {nic: set(networks)}. " Line 57: nets_by_nic = {} Line 58: for net, attrs in running_config.networks.items(): itreitems to avoid copying the dictionary Line 59: nic = attrs.get('nic') Line 60: if nic is not None: Line 61: if nic in nets_by_nic: Line 62: nets_by_nic[nic].add(net) Line 56: " Transform running config into {nic: set(networks)}. " Line 57: nets_by_nic = {} Line 58: for net, attrs in running_config.networks.items(): Line 59: nic = attrs.get('nic') Line 60: if nic is not None: this is just the pattern handled by defaultDict(set) or dict.setdefault(key[, default]) Line 61: if nic in nets_by_nic: Line 62: nets_by_nic[nic].add(net) Line 63: else: Line 64: nets_by_nic[nic] = set([net]) Line 60: if nic is not None: Line 61: if nic in nets_by_nic: Line 62: nets_by_nic[nic].add(net) Line 63: else: Line 64: nets_by_nic[nic] = set([net]) nit: you can use a set literal in python 2.7: {net} Line 65: return nets_by_nic Line 66: Line 67: Line 68: def _get_libvirt_changes(nets, running_config): Line 67: Line 68: def _get_libvirt_changes(nets, running_config): Line 69: libvirt_create = {} Line 70: libvirt_remove = set() Line 71: for net, attrs in nets.items(): itreitems to avoid copying the dictionary Line 72: if 'remove' in attrs: Line 73: libvirt_remove.add(net) Line 74: for net, attrs in nets.items(): Line 75: if 'remove' not in attrs: Line 109: Line 110: def _handle_nets(nets, running_config): Line 111: nets_by_nic = _get_nets_by_nic(running_config) Line 112: commands = [] Line 113: for net, attrs in nets.items(): use itreitems to avoid copying Line 114: if 'remove' in attrs: Line 115: commands.extend(_remove_ovs_network(net, running_config, Line 116: nets_by_nic)) Line 117: for net, attrs in nets.items(): Line 117: t use itreitems to avoid copying Line 222: 'OVS network') Line 223: if not bridged: Line 224: hooking.exit_hook('OVS does not support bridgeless networks') Line 225: if bonding: Line 226: if bonding in running_config.bonds: nit: you can merge those 2 lines Line 227: if not is_ovs_bond(running_config.bonds[bonding]): Line 228: hooking.exit_hook('%s is not OVS bonding' % bonding) Line 229: Line 230: for another_net, another_attrs in running_config.networks.items(): Line 226: if bonding in running_config.bonds: Line 227: if not is_ovs_bond(running_config.bonds[bonding]): Line 228: hooking.exit_hook('%s is not OVS bonding' % bonding) Line 229: Line 230: for another_net, another_attrs in running_config.networks.items(): existing_net? Line 231: if (another_net != net and Line 232: another_attrs.get('nic') == nic and Line 233: another_attrs.get('bond') == bonding and Line 234: another_attrs.get('vlan') == vlan): Line 240: if untagged_net not in (None, net): can net be None? if not, then: if untagged_net != net: Line 247: def _handle_bonds(bonds, running_config): Line 248: commands = [] Line 249: for bond, attrs in bonds.items(): Line 250: if 'remove' in attrs: Line 251: bonds.pop(bond) this is bad practice. try to avoid changing the parameters. why do you need it? Line 252: commands.extend(['--', 'del-port', BRIDGE_NAME, bond]) Line 253: running_config.removeBonding(bond) Line 254: for bond, attrs in bonds.items(): Line 255: if len(attrs.get('nics')) < 2: Line 304: commands.extend(_setup_ovs_bond(bond, attrs, running_config)) Line 305: return commands Line 306: Line 307: Line 308: def _check_bond_options(bond_mode, lacp): _validate_bond_options? Line 309: if bond_mode: Line 310: if bond_mode not in VALID_MODES: Line 311: hooking.exit_hook('%s is not valid ovs bond mode' % bond_mode) Line 312: if lacp: -- 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: 57 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
