Edward Haas has posted comments on this change. Change subject: net: native ovs [1]: ovs switch skeleton ......................................................................
Patch Set 10: Code-Review-1 (12 comments) https://gerrit.ovirt.org/#/c/55308/10/lib/vdsm/network/netswitch.py File lib/vdsm/network/netswitch.py: PS10, Line 35: 'legacy' It will be nice to see these in a constant: legacy_switch.SWITCH_TYPE, ovs_switch.SWITCH_TYPE ? Consider it for this or any following patch PS10, Line 42: _split_legacy_ovs Nice merge of nets and bonds! Naming nit: _split_switch_types sounds nicer and perhaps changing the second argument to running_config_entries Line 44: ovs_entries = {} Line 45: Line 46: for name, attrs in six.iteritems(entries): Line 47: if 'remove' in attrs: Line 48: # If a network/bond should be removed, we have to find out what How about extracting a method out of this block? There is also duplicate code here that we better take care of. Line 49: # switch type has to be used on our own. If a network/bond was not Line 50: # found in running_config, we pass it to legacy switch which is Line 51: # unlike OVS switch able to remove broken networks/bonds. Line 52: # TODO: Try to find out which switch type should be used for broken PS10, Line 51: unlike OVS switch In parenthesis.. PS10, Line 56: legacy_entries[name] = attrs I would extract this to a method and name it something like: _add_broken_entry() Line 57: else: Line 58: if _is_legacy(running_attrs): Line 59: legacy_entries[name] = attrs Line 60: elif _is_ovs(running_attrs): Line 61: ovs_entries[name] = attrs An invalid switch exception fits here as well. Line 62: elif _is_legacy(attrs): Line 63: legacy_entries[name] = attrs Line 64: elif _is_ovs(attrs): Line 65: ovs_entries[name] = attrs PS10, Line 94: running_config = RunningConfig() : legacy_nets, ovs_nets = _split_legacy_ovs( : networks, running_config.networks) : legacy_bonds, ovs_bonds = _split_legacy_ovs(bondings, running_config.bonds) : : use_legacy_switch = legacy_nets or legacy_bonds : use_ovs_switch = ovs_nets or ovs_bonds Looks like the exact code from the previous method, better extract to a 3rd method. PS10, Line 130: running_config Why does the contextmanager returns this? https://gerrit.ovirt.org/#/c/55308/10/tests/network/netswitch_test.py File tests/network/netswitch_test.py: Sorry for this, but there seem to me a disagreement regrading the mock library addition to VDSM. Lets take netswitch_test.py out of the equation until a decision will be taken, so we will not get stuck. Please move changes made here to its own patch over the one originally created (https://gerrit.ovirt.org/#/c/55342/). Nevertheless, these tests should be one of the criteria for marking 'verify' on this patch. Line 1: # Line 2: # Copyright 2016 Red Hat, Inc. Line 3: # Line 4: # This program is free software; you can redistribute it and/or modify Line 31: Line 32: @attr(type='unit') Line 33: class TestNetSwitch(VdsmTestCase): Line 34: Line 35: LEGACY_NET_TEST = {'legacynet': {'switch': 'legacy'}} The _TEST tail seems to be redundant. Line 36: LEGACY_BOND_TEST = {'legacybond': {'switch': 'legacy'}} Line 37: OVS_NET_TEST = {'ovsnet': {'switch': 'ovs'}} Line 38: OVS_BOND_TEST = {'ovsbond': {'switch': 'ovs'}} Line 39: OPT_TEST = {'optionname': {'O': 'o'}} PS10, Line 60: NET_TEST MIX_SWITCH_NETS (same for bonds) PS10, Line 95: netswitch mocking netswitch itself does not make sense, netswitch is the unit under test. -- To view, visit https://gerrit.ovirt.org/55308 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic056fcf8c0d36625328a90a339d4a09658683056 Gerrit-PatchSet: 10 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Petr Horáček <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Edward Haas <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Petr Horáček <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
