Edward Haas has posted comments on this change. Change subject: virt net: Support VM migration on OVS based networks ......................................................................
Patch Set 5: (8 comments) https://gerrit.ovirt.org/#/c/59645/5//COMMIT_MSG Commit Message: PS5, Line 10: betweeni > between Done PS5, Line 12: king > kind Done https://gerrit.ovirt.org/#/c/59645/5/vdsm/virt/vm_migrate_hook.py File vdsm/virt/vm_migrate_hook.py: Line 27: from vdsm.config import config Line 28: from vdsm.network import api as net_api Line 29: Line 30: Line 31: _DEBUG_MODE = False > this is used only once. would not be better to use just kwarg? It is here to allow easy access to enable/disable debugging logs. It makes it harder and unclear what needs to be set if done at usage. Line 32: LOG_FILE = '/tmp/libvirthook_ovs_migrate.log' Line 33: Line 34: Line 35: def main(domain, event, phase, *args, **kwargs): Line 37: sys.exit(0) Line 38: Line 39: with _logging(_DEBUG_MODE) as log: Line 40: if log: Line 41: print('Hook input args are: ', domain, event, phase, file=log) > what about args and kwargs? you use them, you should log them. I do not use them, they are there for gathering derbies that may have been thrown at it by the caller (script). The stdin that may exist in kwargs is a hack and there is no need to log it. Line 42: Line 43: stdin = kwargs.get('stdin', sys.stdin) Line 44: Line 45: tree = ET.parse(stdin) PS5, Line 130: target_vm_info > i'd use targed_vm_info or target_vm_conf everywhere in the script Done Line 133: return Line 134: Line 135: graphics_listen = graphics.find('listen') Line 136: Line 137: target_display_network = target_vm_info['displayNetwork'] > so, just to be sure, we still use displayNetwork, even with displayIp? We use it if the target display network is a legacy network. The display IP is used only for OVS networks. This is due to a limitation we have with the migration: If the target host has no libvirt hook (older VDSM version), we cannot translate the source IP to a new target IP. We have no way of knowing if all the hosts in the cluster are with a minimum version, we use OVS Engine logic that all hosts in the cluster should be OVS enabled, which means all have this hook. Line 138: Line 139: if net_api.ovs_bridge(target_display_network): Line 140: graphics_listen.set('type', 'address') Line 141: graphics_listen.set('address', target_vm_info['displayIp']) PS5, Line 155: "," > single quotes single quotes!! copy paste copy paste :) Done PS5, Line 161: w > why not 'a'? 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: 5 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]
