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 <[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
