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