Dan Kenigsberg has posted comments on this change. Change subject: virt net: Support VM migration on OVS based networks ......................................................................
Patch Set 4: (3 comments) very nice, but this is a partial review. https://gerrit.ovirt.org/#/c/59645/4/vdsm/virt/vm_migrate_hook.py File vdsm/virt/vm_migrate_hook.py: Line 29: Line 30: _DEBUG_MODE = False Line 31: Line 32: if _DEBUG_MODE: Line 33: LOG_FILE = '/tmp/libvirthook_ovs_migrate.log' this could result in a big mess if you enable DEBUG and run two VMs, as both would attempt to write to the same file. Generally, I've learned to be afraid of creating side effects in import time. Can you move logfile open/close into main()? Line 34: log = open(LOG_FILE, 'w') Line 35: Line 36: Line 37: def main(domain, event, phase, *args, **kwargs): PS4, Line 62: info I'd use vm_conf, since this the name all over virt. Line 66: target_vm_nets_by_mac = {dev['macAddr']: dev['network'] Line 67: for dev in target_vm_info['devices'] Line 68: if dev['type'] == 'interface'} Line 69: Line 70: for interface in devices.findall('interface'): I think that findall('./interface') is safer (in case there's a nested 'interface' in the future). but please check. Line 71: if interface.get('type') == 'bridge': Line 72: _bind_iface_to_bridge(interface, target_vm_nets_by_mac) Line 73: Line 74: -- 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: 4 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]
