Change in vdsm[master]: net: native ovs [3]: validate networks and bonds

edwardh at redhat.com edwardh at redhat.com
Fri Apr 1 21:00:56 UTC 2016


Edward Haas has posted comments on this change.

Change subject: net: native ovs [3]: validate networks and bonds
......................................................................


Patch Set 4: Code-Review-1

(10 comments)

https://gerrit.ovirt.org/#/c/55310/4/lib/vdsm/common/contextlib.py
File lib/vdsm/common/contextlib.py:

Line 24: @contextmanager
Line 25: def suppress(*exceptions):
Line 26:     """Python >= 3.4 suppress context manager.
Line 27: 
Line 28: <<<<<<< HEAD:lib/vdsm/common/contextlib.py
Some leftover I guess
Line 29:     https://docs.python.org/3/library/contextlib.html#contextlib.suppress
Line 30:     """
Line 31:     try:
Line 32:         yield


https://gerrit.ovirt.org/#/c/55310/4/lib/vdsm/network/ovs/validator.py
File lib/vdsm/network/ovs/validator.py:

PS4, Line 34: running_config
Maybe it will be better to pass here the needed info only: current_nets (other name?)

In the future we can replace the source of this list, with the ones read from the ovs switch itself without touching these funcs.


PS4, Line 43: nic = attrs.get('nic')
            :     bond = attrs.get('bonding')
Only one can exist, if both appear its a problem.
It will be nice if it can be shown as such.
Maybe: iface = attrs.get('nic') or attrs.get('bonding')

Just throwing an idea for another patch: Merge these two in the canonicalize step...


Line 43:     nic = attrs.get('nic')
Line 44:     bond = attrs.get('bonding')
Line 45:     vlan = attrs.get('vlan')
Line 46: 
Line 47:     if vlan is None:
We will need to add support for this, probably using multiple ovs switches.
Please add a comment here with a TODO, mentioning that we should add support for multi-untag nets.
Line 48:         untagged_net = _get_untagged_net(running_config)
Line 49:         if untagged_net not in (None, net):
Line 50:             raise ne.ConfigNetworkError(
Line 51:                 ne.ERR_BAD_VLAN,


PS4, Line 53: else
Why under 'else'?
Isn't this check in addition to the vlan one?


PS4, Line 59: netinfo
Maybe it will be better to pass here only the needed info: kernel_nics


PS4, Line 74: You have to define at least 2 slaves for OVS bonding
OVS bond requires at least 2 slaves


https://gerrit.ovirt.org/#/c/55310/4/tests/network/ovs_test.py
File tests/network/ovs_test.py:

Line 56:         self.assertFalse(
Line 57:             ovs_switch.is_ovs_bond({}))
Line 58: 
Line 59: 
Line 60: class ValidationTests(TestCaseBase):
Please separate between unit, integration and functional tests and mark/tag them accordingly.
I would prefer to see each in its own class.
Line 61:     """Test network setup validation performed by validator and setup."""
Line 62: 
Line 63:     class FakeRunningConfig(netconfpersistence.BaseConfig):
Line 64:         def __init__(self, *args, **kwargs):


Line 64:         def __init__(self, *args, **kwargs):
Line 65:             self.networks = {'net1': {'nic': 'eth0'}}
Line 66:             self.bonds = {}
Line 67: 
Line 68:     def test_net_only_one_untagged_networks(self):
Better split it into separated tests, then you can name them properly instead of the comments.
Line 69:         fake_config = self.FakeRunningConfig()
Line 70: 
Line 71:         # no initial untagged network
Line 72:         fake_config.networks = {


Line 95:         with self.assertRaises(ne.ConfigNetworkError):
Line 96:             ovs_validator.validate_net_configuration(
Line 97:                 'net1', {'vlan': 10, 'custom': {'ovs': True}}, fake_config)
Line 98: 
Line 99:     @brokentest('OVS setup is not implemented yet.')
We better add the tests with the implementation, not before and preferably not after.
Line 100:     def test_net_nic_must_exist(self):
Line 101:         # TODO: ovs.setup(...) should raise ConfigNetworkError
Line 102:         raise NotImplemented
Line 103: 


-- 
To view, visit https://gerrit.ovirt.org/55310
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5bceae0aada964c47aacfcfdbe93ca19eb8f3d14
Gerrit-PatchSet: 4
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