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

phoracek at redhat.com phoracek at redhat.com
Tue Apr 12 15:57:48 UTC 2016


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

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


Patch Set 10:

(8 comments)

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

PS10, Line 35: 'legacy'
> It will be nice to see these in a constant: legacy_switch.SWITCH_TYPE, ovs_
Done


PS10, Line 42: _split_legacy_ovs
> Nice merge of nets and bonds!
Done


Line 44:     ovs_entries = {}
Line 45: 
Line 46:     for name, attrs in six.iteritems(entries):
Line 47:         if 'remove' in attrs:
Line 48:             # If a network/bond should be removed, we have to find out what
> How about extracting a method out of this block?
Done
Line 49:             # switch type has to be used on our own. If a network/bond was not
Line 50:             # found in running_config, we pass it to legacy switch which is
Line 51:             # unlike OVS switch able to remove broken networks/bonds.
Line 52:             # TODO: Try to find out which switch type should be used for broken


PS10, Line 51: unlike OVS switch
> In parenthesis..
Done


PS10, Line 56: legacy_entries[name] = attrs
> I would extract this to a method and name it something like: _add_broken_en
Done


Line 57:             else:
Line 58:                 if _is_legacy(running_attrs):
Line 59:                     legacy_entries[name] = attrs
Line 60:                 elif _is_ovs(running_attrs):
Line 61:                     ovs_entries[name] = attrs
> An invalid switch exception fits here as well.
Done
Line 62:         elif _is_legacy(attrs):
Line 63:             legacy_entries[name] = attrs
Line 64:         elif _is_ovs(attrs):
Line 65:             ovs_entries[name] = attrs


PS10, Line 94:     running_config = RunningConfig()
             :     legacy_nets, ovs_nets = _split_legacy_ovs(
             :         networks, running_config.networks)
             :     legacy_bonds, ovs_bonds = _split_legacy_ovs(bondings, running_config.bonds)
             : 
             :     use_legacy_switch = legacy_nets or legacy_bonds
             :     use_ovs_switch = ovs_nets or ovs_bonds
> Looks like the exact code from the previous method, better extract to a 3rd
Done. I did not place use_*_switch code outside as it will be probably removed with migration support.


PS10, Line 130: running_config
> Why does the contextmanager returns this?
I guess now we don't need it.


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