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