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

ibarkan at redhat.com ibarkan at redhat.com
Wed Sep 16 08:17:14 UTC 2015


Ido Barkan has posted comments on this change.

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


Patch Set 69: Code-Review+1

(25 comments)

many small comments, but none of them is a real blocker. I suggest we push this in, list them in a TODO somewhere, and work them out on subsequent patches.
more important right now is to pass all the tests and think about how this can become native in the future.

https://gerrit.ovirt.org/#/c/40312/69//COMMIT_MSG
Commit Message:

Line 9: This patch introduces new hook which allows us to configure OVS network
Line 10: instead of standard linux network.
Line 11: 
Line 12: Configuration is done by one big ovs-vsctl command executed at once, it
Line 13: apply the whole configuration iff all commands are correct.
ies
Line 14: 
Line 15: Change-Id: Id602b6cc87a663424d06c77d1847d2c2d60d289f


https://gerrit.ovirt.org/#/c/40312/69/tests/functional/networkTestsOVS.py
File tests/functional/networkTestsOVS.py:

Line 43: BRIDGE_NAME = 'ovsbr0'
Line 44: 
Line 45: # Tests which are not supported by OVS hook (because of OVS hook or because of
Line 46: # tests themselves). Some of these tests should be inherited and 'repaired'
Line 47: # for OVS, or rewritten.
most bridgeless tests use the permutations decorator. we should figure out how to inherit this and run all test in bridge only mode.
Line 48: not_supported = [
Line 49:     'testAddVlanedBridgeless',  # bridgeless
Line 50:     'testAddVlanedBridgeless_oneCommand',  # bridgeless
Line 51:     'testAfterNetworkSetupHook',  # bridgeless


Line 167:             for bond, attrs in bonds.items():
Line 168:                 if 'remove' not in attrs:
Line 169:                     bond_opts = bonds[bond].get('options', '').split()
Line 170:                     modified = False
Line 171:                     for i in range(len(bond_opts)):
use enumerate(bond_opts) here.
Line 172:                         if bond_opts[i].startswith('custom='):
Line 173:                             bond_opts[i] = ('custom=%s,ovs=True' %
Line 174:                                             bond_opts[i].split('=', 1)[1])
Line 175:                             modified = True


Line 208:             kwargs.pop('test_kernel_config')
Line 209:         return self.vdsm_net.setupNetworks(*args, **kwargs)
Line 210: 
Line 211:     @cleanupNet
Line 212:     def test_ovirtmgmtm_to_ovs(self):
ovirtmgmt ?
Line 213:         """ Test transformation of initial management network to OVS.
Line 214:         # TODO: test it with ovirtmgmt and test-network
Line 215:         # NOTE: without default route
Line 216:         # TODO: more asserts


https://gerrit.ovirt.org/#/c/40312/69/vdsm_hooks/ovs/README
File vdsm_hooks/ovs/README:

Line 80: TODO
Line 81: ----
Line 82: 
Line 83: - Implement rollback for case of failed non-ovs setup:
Line 84:   + When and error occurs during OVS networks setup, OVS hook is able to
an
Line 85:     rollback changes. When error happens during the following setup of non-OVS
Line 86:     networks, non-OVS networks are rolled back, but OVS has no idea about that
Line 87:     and is not able to rollback OVS networks.
Line 88:   + This could be implemented with after_network_fail hook (to be done).


Line 116:     certain sub-command.
Line 117: - Allow non-OVS=>OVS and OVS=>non-OVS changes:
Line 118:   + Now we do not handle the situation when OVS network is changed into
Line 119:     non-OVS and vice versa.
Line 120:   + non-OVS=>OVS editation should be easy, we could handle it within
editing
Line 121:     a before_network_setup hook.
Line 122:   + OVS=>non-OVS editation is harder, while we have to first remove non-OVS
Line 123:     network and then create OVS network after_network_setup.
Line 124: - Better handling of traceback and logging:


Line 118:   + Now we do not handle the situation when OVS network is changed into
Line 119:     non-OVS and vice versa.
Line 120:   + non-OVS=>OVS editation should be easy, we could handle it within
Line 121:     a before_network_setup hook.
Line 122:   + OVS=>non-OVS editation is harder, while we have to first remove non-OVS
editing
Line 123:     network and then create OVS network after_network_setup.
Line 124: - Better handling of traceback and logging:
Line 125:   + We need more logging.
Line 126:   + Traceback sometimes provides hooking mess, but not an initial error.


https://gerrit.ovirt.org/#/c/40312/69/vdsm_hooks/ovs/ovs_after_get_caps.py
File vdsm_hooks/ovs/ovs_after_get_caps.py:

Line 69: def networks_caps(running_config):
Line 70:     ovs_networks_caps = {}
Line 71:     dhcpv4ifaces, dhcpv6ifaces = netinfo._get_dhclient_ifaces()
Line 72:     routes = netinfo._get_routes()
Line 73:     for network, attrs in running_config.networks.items():
iteritems
Line 74:         if is_ovs_network(attrs):
Line 75:             interface = network if 'vlan' in attrs else BRIDGE_NAME
Line 76:             net_info = _get_net_info(attrs, interface, dhcpv4ifaces,
Line 77:                                      dhcpv6ifaces, routes)


Line 86: def bridges_caps(running_config):
Line 87:     ovs_bridges_caps = {}
Line 88:     dhcpv4ifaces, dhcpv6ifaces = netinfo._get_dhclient_ifaces()
Line 89:     routes = netinfo._get_routes()
Line 90:     for network, attrs in running_config.networks.items():
iteritems
Line 91:         if is_ovs_network(attrs):
Line 92:             interface = network if 'vlan' in attrs else BRIDGE_NAME
Line 93:             net_info = _get_net_info(attrs, interface, dhcpv4ifaces,
Line 94:                                      dhcpv6ifaces, routes)


Line 103: def vlans_caps(running_config):
Line 104:     ovs_vlans_caps = {}
Line 105:     dhcpv4ifaces, dhcpv6ifaces = netinfo._get_dhclient_ifaces()
Line 106:     routes = netinfo._get_routes()
Line 107:     for network, attrs in running_config.networks.items():
iteritems
Line 108:         if is_ovs_network(attrs):
Line 109:             vlan = attrs.get('vlan')
Line 110:             if vlan is not None:
Line 111:                 net_info = _get_net_info(attrs, network, dhcpv4ifaces,


Line 115: net_info['bridged'] = True
net_info['bridged'] = True
can be part of _get_net_info


Line 139: def bondings_caps(running_config):
Line 140:     ovs_bonding_caps = {}
Line 141:     dhcpv4ifaces, dhcpv6ifaces = netinfo._get_dhclient_ifaces()
Line 142:     routes = netinfo._get_routes()
Line 143:     for bonding, attrs in running_config.bonds.items():
iteritems
Line 144:         if is_ovs_bond(attrs):
Line 145:             options = get_bond_options(attrs.get('options'), keep_custom=True)
Line 146:             net_info = _get_net_info(attrs, bonding, dhcpv4ifaces,
Line 147:                                      dhcpv6ifaces, routes)


https://gerrit.ovirt.org/#/c/40312/69/vdsm_hooks/ovs/ovs_after_get_stats.py
File vdsm_hooks/ovs/ovs_after_get_stats.py:

Line 33:     """
Line 34:     ovs_networks_stats = {}
Line 35:     running_config = RunningConfig()
Line 36: 
Line 37:     for network, attrs in running_config.networks.items():
iteritems
Line 38:         if is_ovs_network(attrs):
Line 39:             vlan = attrs.get('vlan')
Line 40:             iface = attrs.get('nic') or attrs.get('bonding')
Line 41:             if vlan is None:


https://gerrit.ovirt.org/#/c/40312/69/vdsm_hooks/ovs/ovs_before_network_setup.py
File vdsm_hooks/ovs/ovs_before_network_setup.py:

Line 139:     print("> executing hook with fake json input: " + str(json_input))
Line 140:     hooks.before_network_setup(json_input)
Line 141:     print("hook finished")
Line 142:     _execCmd([EXT_OVS_VSCTL, 'show'])
Line 143:     _execCmd(['cat', '/var/run/vdsm/netconf/nets/ovs-test-net'])
Those files are json, so just load and decode them and then assert about some key/value
Line 144:     _execCmd(['cat', '/var/run/vdsm/netconf/bonds/bond1515'])
Line 145: 
Line 146: 
Line 147: def test_del():


Line 153:     print(json_input)
Line 154:     hooks.before_network_setup(json_input)
Line 155:     print("hook finished\n")
Line 156:     _execCmd([EXT_OVS_VSCTL, 'show'])
Line 157:     _execCmd(['cat', '/var/run/vdsm/netconf/nets/ovs-test-net'], exit=False)
see my comment in line 143
Line 158:     _execCmd(['cat', '/var/run/vdsm/netconf/bonds/bond1515'], exit=False)
Line 159:     _execCmd([EXT_IP, 'link', 'del', 'dummy_1'])
Line 160:     _execCmd([EXT_IP, 'link', 'del', 'dummy_2'])
Line 161: 


Line 168: 
Line 169: if __name__ == '__main__':
Line 170:     try:
Line 171:         if '--test' in sys.argv:
Line 172:             sys.path.extend(['../../vdsm', '../../lib'])
you can import hooks here once instead of in every test
Line 173:             if 'add' in sys.argv:
Line 174:                 test_add()
Line 175:             elif 'del' in sys.argv:
Line 176:                 test_del()


https://gerrit.ovirt.org/#/c/40312/69/vdsm_hooks/ovs/ovs_before_network_setup_mtu.py
File vdsm_hooks/ovs/ovs_before_network_setup_mtu.py:

Line 38:     """ Get MTUs for nics. Bondings MTUs will be changed as a consequence."""
Line 39:     changes = {}
Line 40:     for net, attrs in iter_ovs_nets(running_config.networks):
Line 41:         mtu = attrs.get('mtu')
Line 42:         if mtu is not None:
if mtu is None (although this is always set by the engine, at least for current engines) VDSM uses the default mtu of the system. This is done implicitly in ifcfg configurator simply by not setting the mtu value. in this case you can safely assume it is 1500 bytes.
Line 43:             nic = attrs.get('nic')
Line 44:             bonding = attrs.get('bonding')
Line 45:             if nic is not None:
Line 46:                 _update_mtu_changes(mtu, [nic], changes)


Line 42:         if mtu is not None:
Line 43:             nic = attrs.get('nic')
Line 44:             bonding = attrs.get('bonding')
Line 45:             if nic is not None:
Line 46:                 _update_mtu_changes(mtu, [nic], changes)
this is hard to understand. you mutate 'changes' as a side affect of _update_mtu_changes. all we need to do is to add to it the maximum mtu, if it is different than the current one right?
so:
current_mtu = netinfo.getMtu(nic)
if current_mtu != mtu:
  changes[nic] = max(mtu, current_mtu)
Line 47:             elif bonding is not None:
Line 48:                 slaves = running_config.bonds[bonding].get('nics')
Line 49:                 _update_mtu_changes(mtu, slaves, changes)
Line 50:     return changes


Line 45:             if nic is not None:
Line 46:                 _update_mtu_changes(mtu, [nic], changes)
Line 47:             elif bonding is not None:
Line 48:                 slaves = running_config.bonds[bonding].get('nics')
Line 49:                 _update_mtu_changes(mtu, slaves, changes)
same here, but do it for every slave.
Line 50:     return changes
Line 51: 
Line 52: 
Line 53: def _mtus_bonds(running_config):


Line 81: 
Line 82: def configure_mtu(running_config):
Line 83:     """ Setup MTUs for OVS networks. We have to do this in three iterations
Line 84:     because of change of a lesser device could influence an upper. This way we
Line 85:     keep MTU handling compatible with standard VDSM.
is this a specific ovs behaviour? IIRC this is not what happens in Linux. Maybe this behaviour could be shut down and make our life easyer?
Line 86:     """
Line 87:     changes_nics = _mtus_nics(running_config)
Line 88:     changes_bonds = _mtus_bonds(running_config)
Line 89:     changes_vlans = _mtus_vlans(running_config)


Line 83:     """ Setup MTUs for OVS networks. We have to do this in three iterations
Line 84:     because of change of a lesser device could influence an upper. This way we
Line 85:     keep MTU handling compatible with standard VDSM.
Line 86:     """
Line 87:     changes_nics = _mtus_nics(running_config)
I am a little confused here. why do you need all this logic in 3 iterations? assuming we cannot stop ovs from changing mtu's can't we just search for the max mtu required in the chain and then alter the chain in the order that ovs is happy with?
I am sure this complex logic works, I just wonder if we could simplify things here.
Line 88:     changes_bonds = _mtus_bonds(running_config)
Line 89:     changes_vlans = _mtus_vlans(running_config)
Line 90:     for mtu_changes in (changes_nics, changes_bonds, changes_vlans):
Line 91:         for iface, mtu in mtu_changes.iteritems():


https://gerrit.ovirt.org/#/c/40312/69/vdsm_hooks/ovs/ovs_before_network_setup_ovs.py
File vdsm_hooks/ovs/ovs_before_network_setup_ovs.py:

Line 77: 
Line 78: 
Line 79: def _remove_libvirt_nets(libvirt_remove):
Line 80:     for net in libvirt_remove:
Line 81:         with suppress():
isn't try-catch more readable? and why do you except exceptions here?
Line 82:             libvirt.removeNetwork(net)
Line 83: 
Line 84: 
Line 85: def _run_commands(commands):


Line 92:         hooking.log('Executing commands: %s' % ' '.join(commands))
Line 93:         rc, _, err = hooking.execCmd(commands)
Line 94:         if rc != 0:
Line 95:             raise Exception('Executing commands failed: %s' % '\n'.join(err))
Line 96: 
the removal of network from running config should happen at this point in time, upon success of the commands.
Line 97: 
Line 98: def _setup_ovs_net(net, attrs, running_config, nets_by_nic):
Line 99:     commands = []
Line 100:     nic = attrs.get('nic')


Line 304:                                                 nets_by_nic))
Line 305:     for bond, attrs in bonds.items():
Line 306:         if 'remove' in attrs:
Line 307:             commands.extend(['--', 'del-port', BRIDGE_NAME, bond])
Line 308:             running_config.removeBonding(bond)
this feels uncomfortable. on one hand, this function reads requested config and generates commands (not running then) and the the other hand, it removes them from running config. What happen if you fail to remove the nets? will you remember to add them back? In short- running_config.removeBonding(bond) should be part of a 'transaction' in a try catch block along with executing the commands.
Line 309:     return commands
Line 310: 
Line 311: 
Line 312: def prepare_ovs(nets, bonds, running_config):


Line 308:             running_config.removeBonding(bond)
Line 309:     return commands
Line 310: 
Line 311: 
Line 312: def prepare_ovs(nets, bonds, running_config):
can you move those 2 public functions to the top of the module? there is no way that this module would be run as a script anyway (only imported).
Line 313:     """ Prepare OVS commands and collect libvirt network to add and remove.
Line 314:     This function is separate because of no rollback is needed here.
Line 315:     """
Line 316:     libvirt_create, libvirt_remove = _get_libvirt_changes(nets, running_config)


-- 
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: 69
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