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