Change in vdsm[master]: net: native ovs [1]: ovs switch skeleton

edwardh at redhat.com edwardh at redhat.com
Sun Apr 10 14:21:29 UTC 2016


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 <phoracek at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Edward Haas <edwardh at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Petr Horáček <phoracek at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list