Edward Haas has posted comments on this change. Change subject: net: Use Linux bonds with OVS networks ......................................................................
Patch Set 16: (9 comments) https://gerrit.ovirt.org/#/c/63119/16//COMMIT_MSG Commit Message: PS16, Line 14: is kept > is it? you dropped the bond code. Not all the code, but you are right. https://gerrit.ovirt.org/#/c/63119/16/lib/vdsm/network/netswitch.py File lib/vdsm/network/netswitch.py: Line 157: Line 158: def _setup_ovs(networks, bondings, options, in_rollback): Line 159: _ovs_info = ovs_info.OvsInfo() Line 160: ovs_netinfo = ovs_info.create_netinfo(_ovs_info) Line 161: _netinfo = netinfo() > netinfo is requested also by _split_switch_type(), can we request it in set Screams: Make me a Setup object. Lets do it in a following patch. Line 162: Line 163: nets2add, nets2edit, nets2remove = _split_setup_actions( Line 164: networks, ovs_netinfo['networks']) Line 165: bonds2add, bonds2edit, bonds2remove = _split_setup_actions( Line 178: setup_bonds.remove_bonds() Line 179: setup_bonds.edit_bonds() Line 180: setup_bonds.add_bonds() Line 181: setup_ovs.add_nets(nets2add) Line 182: acq.acquire(setup_bonds.acquired_ifaces) > with OVS we were afraid NM will be faster than our acquiration and will rec Done Line 183: acq.acquire(setup_ovs.acquired_ifaces) Line 184: _update_networks_running_config(networks, config) Line 185: ovs_switch.cleanup() Line 186: setup_ipv6autoconf(networks) PS16, Line 214: sb = attrs.get('bonding') or attrs.get('nic') : address.disable_ipv6(sb) > this should be in a separate patch. plus i have a feeling we do already dis Ok, extracting it to a separate patch. I have not seen this done elsewhere, but I may have just missed it. PS16, Line 293: # Fake bond type to satisfy Engine. : for bond, bond_attrs in six.iteritems(RunningConfig().bonds): : if (bond_attrs['switch'] == ovs_switch.SWITCH_TYPE and : bond in _netinfo['bondings']): : _netinfo['bondings'][bond]['switch'] = ovs_switch.SWITCH_TYPE > move it to a separate function please Done https://gerrit.ovirt.org/#/c/63119/16/lib/vdsm/network/ovs/info.py File lib/vdsm/network/ovs/info.py: Line 258: def _get_network_info(northbound, bridge, southbound, ports, stp, addresses, Line 259: routes): Line 260: # OVS networks do not use the OVS bonds, they use external linux bonds. Line 261: bond = Bond(southbound) Line 262: bond_name = '' > it would make sense to have this in else branch Done Line 263: if bond.exists(): Line 264: bond_name = bond.master Line 265: nics = list(bond.slaves) Line 266: else: PS16, Line 357: nic_netinfo > nics Done PS16, Line 358: bond_netinfo > bonds Done Line 365: if iface_type == 'nics': Line 366: nic_netinfo[iface_name].update(_shared_net_attrs(net_attrs)) Line 367: elif iface_type == 'bondings': Line 368: bond_netinfo[iface_name].update(_shared_net_attrs(net_attrs)) Line 369: # The bond is used by OVS, so we mark it as OVS. > is not that already handled in netswitch:netinfo? or the another way around Yes, we can drop it from here, although it feels 'safer' doing it here (as we know more accurately that this bond is used by OVS). At the netswitch level, we check against running-config, which I trust less. I will remove it from here, it is more natural and easy to understand when this happens in one single place. Line 370: bond_netinfo[iface_name]['switch'] = 'ovs' Line 371: else: Line 372: ovs_netinfo[iface_type][iface_name].update( Line 373: _shared_net_attrs(net_attrs)) -- To view, visit https://gerrit.ovirt.org/63119 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6aeff335949a6e2996f7c3faa524df784dff1b01 Gerrit-PatchSet: 16 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Edward Haas <edwa...@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/admin/lists/vdsm-patches@lists.fedorahosted.org
