Change in vdsm[master]: hooks: Open vSwitch configurator
ibarkan at redhat.com
ibarkan at redhat.com
Sun Aug 9 06:37:39 UTC 2015
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 <phoracek at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <toni+ovirt at midokura.com>
Gerrit-Reviewer: Barak Azulay <bazulay at gmail.com>
Gerrit-Reviewer: Barak Azulay <bazulay at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Ido Barkan <ibarkan at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list