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

phoracek at redhat.com phoracek at redhat.com
Tue Mar 29 12:12:18 UTC 2016


Petr Horáček has posted comments on this change.

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


Patch Set 1:

(8 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:
Done
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?
As soon as we have 'switch' property. Until that time, we have to handle 'custom' property on setup and on removal check if existing network is legacy or ovs.


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
we want all networks legacy OR ovs, i think we should not take care of such scenario.


PS1, Line 68: ConfigNetworkError
> Better use ne.ConfigNetworkError, it will save you an import line and a pos
Done


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.
Done
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
Done


PS1, Line 78: _split_legacy_ovs
> This shouts... netswitch should have a state/cahce, netswitch should be a c
Not sure. We have to think in context of smarter netswitch, with implemented migration.


PS1, Line 106: rollback_manager
> Can we find a better name for this one? 'manager', 'controller' say nothing
Done


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