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