Edward Haas has posted comments on this change. Change subject: net: native ovs [1] ......................................................................
Patch Set 20: Code-Review-1 (6 comments) This is a partial review. My recomendation is to split this patch into small subjects: caps, stats, netswitch, validator... etc. I'm also missing the configurator layer here, at least as a skeleton.. It will make it clear why kernel interface and ip needs to be separated from the ovs parts. https://gerrit.ovirt.org/#/c/54890/20/lib/vdsm/network/api.py File lib/vdsm/network/api.py: Line 240: hooks.after_network_setup( Line 241: _build_setup_hook_dict(networks, bondings, options)) Line 242: Line 243: Line 244: def _setup_networks(networks, bondings, options): It will be best to have an additional patch, before this one, with the netswitch layer. It should split and check switch types, validate switch, call the correct switch setup... api.py should do little: Overall rollback logic, init stuff that we need in general (?), canonicalize part and the connectivity check. Regarding the rollback, it will be good to add a TODO here to complete the overall rollback. Line 245: canonicalize_networks(networks) Line 246: # TODO: Add canonicalize_bondings(bondings) Line 247: Line 248: running_config = RunningConfig() Line 250: separate_native_ovs_nets_bonds this is an example of netswitch responsibility https://gerrit.ovirt.org/#/c/54890/20/lib/vdsm/network/ovs_switch/caps.py File lib/vdsm/network/ovs_switch/caps.py: Line 88: supervdsm.getProxy Calling getProxy() from within the ovs switch is bad. We need a strategy on how to get caps as root, but as a minimum I would recomend creating a new verb under network.api: 'caps_network', register it (like we do with setupNetworks) and use it to complement the existing non-root caps. netinfo should be responsible for the collection of ovs information. caps can use netinfo and build a caps report, but that could even be a netinfo method if no special logic is required. https://gerrit.ovirt.org/#/c/54890/20/lib/vdsm/network/ovs_switch/stats.py File lib/vdsm/network/ovs_switch/stats.py: Line 28: networks_stats Seems to fit 'normalize' based on what it actually does. But I think it needs to be used differently, not to get stats as input but to generate fresh stats by itself. And what about placing this under ovs netinfo? https://gerrit.ovirt.org/#/c/54890/20/vdsm/caps.py File vdsm/caps.py: Line 166: Line 167: # TODO: Version requests by engine to ease handling of compatibility. Line 168: netinfo_data = netinfo_cache.get(compatibility=30600) Line 169: caps.update(netinfo_data) Line 170: caps = ovs_caps.caps(caps) The design included a netinfo hierarchy, passing caps of the switches through it seems appropriate. It deserves its own patch. Line 171: Line 172: try: Line 173: caps['hooks'] = hooks.installed() Line 174: except: https://gerrit.ovirt.org/#/c/54890/20/vdsm/supervdsmServer File vdsm/supervdsmServer: Line 380: except cmdutils.Error as e: Line 381: raise OSError(errno.EINVAL, 'Could not trigger change ' Line 382: 'out %s\nerr %s' % (e.out, e.err)) Line 383: Line 384: @logDecorator Are you using these new verbs because you cannot get those with a non-root? Doesn't ovs support non-root read only access? supervdsm verbs are now registered through vdsm/supervdsm_api/network.py and not here. Line 385: def ovs_get_stp(self, iface): Line 386: return ovs_caps.get_stp(iface) Line 387: Line 388: @logDecorator -- To view, visit https://gerrit.ovirt.org/54890 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iaaf958554c0f71cfe652869bd75eb17aea5ca676 Gerrit-PatchSet: 20 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Petr Horáček <phora...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Edward Haas <edwa...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Petr Horáček <phora...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
