Change in vdsm[master]: hooks: Open vSwitch configurator

phoracek at redhat.com phoracek at redhat.com
Wed Aug 12 11:14:24 UTC 2015


Petr Horáček has posted comments on this change.

Change subject: hooks: Open vSwitch configurator
......................................................................


Patch Set 57:

(24 comments)

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! c
Done
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 st
We cannot except ImportError here, in case vdsm/lib is not in PATH, it will be raised while importing. I cannot handle sys.path while I don't know where are those dirs located (if i'm in source code dir or in libexec/hooks), and again, handling sys.path here is too late, it explodes while importing.
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 suppo
Done
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
Done
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 :)
not that easy while there already is public removeSourceRoute. IIUC, we call _removeSourceRoute and after some internal handling  (in sourceroute.py) it calls configurators removeSourceRoute.
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?
it's not running config, just networks from running config in the state before ovs configuration.
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
Done
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
Done
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?
We can, but in this case, we have to create a list of changes like this: {'nic': mtu, 'nic': mtu, ...., 'bond': mtu, 'bond': mtu, ..., 'vlan': mtu}.

We have to follow two rules: Bond and its slaves have to have the same MTU. MTU can  be only raised, not lowered. And we have to work with OVS's automatic MTU configuration (of upper devices).

For example, this problem could happen if we don't run _mtus_vlans as the last function: We have vlan and one nic, both with 1500 MTU, we replace the nic with a new one with 1000 MTU. MTU of vlan is lowered by OVS and breakes the rules.
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
Done
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
Done
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
Done
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
Done
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
Done
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
Done
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}
Done
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
Done
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
Done
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 113:     for net, attrs in nets.items():
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():
> use itreitems to avoid copying
Done
Line 118:         if 'remove' not in attrs:
Line 119:             _validate_net_configuration(net, attrs, running_config)
Line 120:             if net in running_config.networks:
Line 121:                 commands.extend(_edit_ovs_net(net, attrs, running_config,


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
Done
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?
yes, that sounds better
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 236:             hooking.exit_hook('%s is already used by network %s' %
Line 237:                               ((nic or bonding), another_net))
Line 238:     if vlan is None:
Line 239:         untagged_net = _get_untagged_net(running_config)
Line 240:         if untagged_net not in (None, net):
> can net be None? if not, then:
net can't be None, but this 'untagged_net' is returned by _get_untagged_net function, which returns None, when there is no untagged net.
Line 241:             hooking.exit_hook('Untagged network already defined with name %s' %
Line 242:                               untagged_net)
Line 243:     if stp and vlan is not None:
Line 244:         hooking.exit_hook('STP could be set only on untagged networks')


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
with this i don't have to test `'remove' not in attrs`, but you're right.
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?
Done
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