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

edwardh at redhat.com edwardh at redhat.com
Sun Mar 27 21:27:17 UTC 2016


Edward Haas has posted comments on this change.

Change subject: net: native ovs [1]: ovs switch skeleton
......................................................................


Patch Set 1: Code-Review-1

(9 comments)

The logic here is important enough to deserve unit tests coverage.
I'll add soon a netswitch basic coverage, please add over it.

Sorry for the so many comments.

https://gerrit.ovirt.org/#/c/55308/1/lib/vdsm/network/netswitch.py
File lib/vdsm/network/netswitch.py:

Line 31: from .errors import ConfigNetworkError
Line 32: from .ovs import switch as ovs_switch
Line 33: 
Line 34: 
Line 35: def _split_legacy_ovs(networks, bondings):
Lets split this into two:
_split_switch_type_nets()
_split_switch_type_bonds()

Calling each from validate.
Line 36:     """TODO: Use something smarter."""
Line 37:     legacy_nets = {}
Line 38:     ovs_nets = {}
Line 39:     legacy_bonds = {}


PS1, Line 45:         if (('remove' in attrs and net in running_config.networks and
            :                 ovs_switch.is_ovs_net(running_config.networks[net])) or
            :                 ovs_switch.is_ovs_net(attrs)):
Why is this so complex?
Each input network/bond should be marked with its type.. isn't this what we planned? If so, the logic should be simple...

In another place we should check if the request is valid (like 'remove' an ovs network while it is currently a legacy one)


PS1, Line 65:     legacy = legacy_nets or legacy_bonds
            :     ovs = ovs_nets or ovs_bonds
Not sure if its worth a variable, but if you decide to use one, better call it is_legacy_switch and is_ovs_switch

BTW, What about this scenario:
- All current nets are legacy.
- Requests: Remove one legacy net and setup the rest as ovs.
This is a mix which seems valid, should we support it or not?
(If we decide to handle such a scenario, it should be in a different patch)


PS1, Line 68: ConfigNetworkError
Better use ne.ConfigNetworkError, it will save you an import line and a possibly module to mock.


Line 68:         raise ConfigNetworkError(
Line 69:             ne.ERR_BAD_PARAMS,
Line 70:             'Cannot mix OVS and legacy networks in one networkSetup.')
Line 71: 
Line 72:     legacy_switch.validate_network_setup(legacy_nets, legacy_bonds)
Why do we call both? It seems to collide with the previous error.
Don't we need here an if() .. elif() ?

(Although I do see a scenario which both may be needed, see remarks above)
Line 73:     ovs_switch.validate_network_setup(ovs_nets, legacy_bonds)
Line 74: 
Line 75: 
Line 76: def setup(networks, bondings, options, in_rollback):


PS1, Line 73: legacy
ovs


PS1, Line 78: _split_legacy_ovs
This shouts... netswitch should have a state/cahce, netswitch should be a class... :)

What other alternative do we have here for not calling the split twice?


PS1, Line 106: rollback_manager
Can we find a better name for this one? 'manager', 'controller' say nothing about what it does.


https://gerrit.ovirt.org/#/c/55308/1/lib/vdsm/network/ovs/switch.py
File lib/vdsm/network/ovs/switch.py:

Line 21: from contextlib import contextmanager
Line 22: 
Line 23: 
Line 24: def is_ovs_net(attrs):
Line 25:     pass
We can add here the key that classify the request switch type.

Same for the bond
Line 26: 
Line 27: 
Line 28: def is_ovs_bond(attrs):
Line 29:     pass


-- 
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: 1
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: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list