Edward Haas has posted comments on this change. Change subject: virt net: Support VM migration on OVS based networks ......................................................................
Patch Set 9: (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:] Ok then, I can use stdin and atdout instead. PS9, Line 65: > space is not needed, will be included by print. the same for other prints. Done 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. Done PS9, Line 89: elem_macaddr > macaddr_elem? This is the pattern I used in this patch all over, I'm lazy and prefer not to change all now. PS9, Line 92: target_vm_net = target_vm_nets_by_mac[mac_addr] > why don't we read bridge name directly from interface? Sorry, I do not fully understand. Which interface? This is the network, not the bridge. We perform a lookup on data previously retrieved from the VM object to get the network of the interface. The mac address is our interface identifier. Please elaborate on your alternative proposal. PS9, Line 123: elem_virtualport.set('type', 'openvswitch') : elem_virtualport.tail = ' ' > this is overintended? Seems like line 124 is not needed now, it is a leftover. 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 g Caller is at the 'devices' level, it seems appropriate to 'hide' the validation here, but both approaches seem valid. It is consistent with _set_bridge_interfaces, so I prefer this approach in this case. 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? They are called by libvirt, which is running as 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 Good catch, now also covered by tests. 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 gu It's in the func docstring. PS9, Line 162: _vmlist > we can return the vm directly, it's always just one uuid, we expect one vm. Done PS9, Line 162: vm_id > please use vm_id or vm_uuid everywhere. Done PS9, Line 168: requestQueues > use lower_snake_case Done -- 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]
