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