Petr Horáček has posted comments on this change. Change subject: virt net: Support VM migration on OVS based networks ......................................................................
Patch Set 9: Code-Review-1 (13 comments) https://gerrit.ovirt.org/#/c/59645/9/vdsm/virt/vm_migrate_hook.py File vdsm/virt/vm_migrate_hook.py: PS9, Line 39: **kwargs this will never get a kwarg when executed from command line. *sys.argv[1:] will return list of strings. PS9, Line 65: space is not needed, will be included by print. the same for other prints. PS9, Line 80: target_vm_nets_by_mac target_vm_nets_by_vnic_mac? i read it as by_bridge_mac. sorry, just a nit. PS9, Line 89: elem_macaddr macaddr_elem? PS9, Line 92: target_vm_net = target_vm_nets_by_mac[mac_addr] why don't we read bridge name directly from interface? PS9, Line 123: elem_virtualport.set('type', 'openvswitch') : elem_virtualport.tail = ' ' this is overintended? Line 140: elem_source = interface.find('source') Line 141: elem_source.set('bridge', bridge) Line 142: Line 143: Line 144: def _set_graphics(devices, target_vm_conf): would not be better to do the check in caller function and pass here only graphics (if any)? Line 145: graphics = devices.find('graphics') Line 146: if graphics is None: Line 147: return Line 148: PS9, Line 153: ovs_bridge libvirt hooks are called by root? Line 151: target_display_network = target_vm_conf['displayNetwork'] Line 152: Line 153: if net_api.ovs_bridge(target_display_network): Line 154: graphics_listen.set('type', 'address') Line 155: graphics_listen.set('address', target_vm_conf['displayIp']) and what about 'network' attribute? don't we have to remove it, will it be just ignored with type=address? Line 156: else: Line 157: libvirt_network = net_api.netname_o2l(target_display_network) Line 158: graphics_listen.set('type', 'network') Line 159: graphics_listen.set('network', libvirt_network) PS9, Line 157: o2l eh this name, i was not sure if it is ovs2legacy or ovirt2libvirt. but i guess i gave it +1 PS9, Line 162: vm_id please use vm_id or vm_uuid everywhere. PS9, Line 162: _vmlist we can return the vm directly, it's always just one uuid, we expect one vm. PS9, Line 168: requestQueues use lower_snake_case -- To view, visit https://gerrit.ovirt.org/59645 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie7d32f9605f9ca99d1e07062108f2567806ac59c Gerrit-PatchSet: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Edward Haas <[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/admin/lists/[email protected]
