Nir Soffer has posted comments on this change. Change subject: kvm@ovirt: tool for streaming images from libvirt ......................................................................
Patch Set 2: (14 comments) Partial review https://gerrit.ovirt.org/#/c/55797/2//COMMIT_MSG Commit Message: Line 3: AuthorDate: 2016-04-06 16:13:50 +0300 Line 4: Commit: Shahar Havivi <shah...@redhat.com> Line 5: CommitDate: 2016-04-20 10:19:35 +0300 Line 6: Line 7: kvm@ovirt: tool for streaming images from libvirt kvm2ovirt Line 8: Line 9: v2v is needs to a streaming/download tool for importing kvm based Line 10: images from Libvirt to vdsm. Line 11: for other such as xen and vmware virt-v2v provide the solution. Line 6: Line 7: kvm@ovirt: tool for streaming images from libvirt Line 8: Line 9: v2v is needs to a streaming/download tool for importing kvm based Line 10: images from Libvirt to vdsm. This sentence needs rewrite. Line 11: for other such as xen and vmware virt-v2v provide the solution. Line 12: this tool mimic virt-v2v output in order to use it smoothly in v2v.py Line 13: module. Line 14: https://gerrit.ovirt.org/#/c/55797/2/helpers/Makefile.am File helpers/Makefile.am: Line 12: kvm2ovirt \ Line 13: $(NULL) Line 14: Line 15: all-local: \ Line 16: $(nodist_vdsm_SCRIPTS) \ Why do we need this? https://gerrit.ovirt.org/#/c/55797/2/helpers/kvm2ovirt File helpers/kvm2ovirt: Line 19: # Line 20: Line 21: import sys Line 22: import time Line 23: Unneeded blank line Line 24: import argparse Line 25: import itertools Line 26: Line 27: from vdsm import libvirtconnection Line 21: import sys Line 22: import time Line 23: Line 24: import argparse Line 25: import itertools Please sort the imports Line 26: Line 27: from vdsm import libvirtconnection Line 28: from vdsm import concurrent Line 29: from vdsm import utils Line 25: import itertools Line 26: Line 27: from vdsm import libvirtconnection Line 28: from vdsm import concurrent Line 29: from vdsm import utils Sort Line 30: Line 31: # TODO: change when directio lib is ready Line 32: from ovirt_image_daemon import directio Line 33: Line 28: from vdsm import concurrent Line 29: from vdsm import utils Line 30: Line 31: # TODO: change when directio lib is ready Line 32: from ovirt_image_daemon import directio This is now in ovirt_image_common Please move this import to above the vdsm library imports (stdlib, 3rd party, vdsm, current package) Line 33: Line 34: Line 35: start = utils.monotonic_time() Line 36: Line 36: Line 37: Line 38: # TODO: write tests first Line 39: # mock libvirtstream that gets ['aa', 'bbbbb'] and return each string Line 40: # in the recv Not needed now, directio handles this. Line 41: class StreamAdapter(): Line 42: def __init__(self, stream): Line 43: self.read = stream.recv Line 44: Line 37: Line 38: # TODO: write tests first Line 39: # mock libvirtstream that gets ['aa', 'bbbbb'] and return each string Line 40: # in the recv Line 41: class StreamAdapter(): Always inherit from object Line 42: def __init__(self, stream): Line 43: self.read = stream.recv Line 44: Line 45: Line 55: parser.add_argument('--source', dest='source', nargs='+', required=True, Line 56: help='Source remote volumes path') Line 57: parser.add_argument('--dest', dest='dest', nargs='+', required=True, Line 58: help='Destination local volumes path') Line 59: parser.add_argument('--packet-size', dest='pksize', default=1048576, packet-size is not a good name, use --buffer-size and bufffer_size or if you like shorter name, bufsize Line 60: help='Size of packets in bytes') Line 61: parser.add_argument('--verbose', action='store_true', Line 62: help='verbose output') Line 63: return parser.parse_args(sys.argv) Line 56: help='Source remote volumes path') Line 57: parser.add_argument('--dest', dest='dest', nargs='+', required=True, Line 58: help='Destination local volumes path') Line 59: parser.add_argument('--packet-size', dest='pksize', default=1048576, Line 60: help='Size of packets in bytes') Show the default in the help message (default 1048676) Line 61: parser.add_argument('--verbose', action='store_true', Line 62: help='verbose output') Line 63: return parser.parse_args(sys.argv) Line 64: Line 73: sys.stdout.flush() Line 74: Line 75: Line 76: def init_progress(): Line 77: write_output('Opening the source -i libvirt') -i? Line 78: write_output('Creating an overlay to protect') Line 79: Line 80: Line 81: def finish_progress(): Line 74: Line 75: Line 76: def init_progress(): Line 77: write_output('Opening the source -i libvirt') Line 78: write_output('Creating an overlay to protect') What? I don't think we should copy virt-v2v messages, they do not make sense. We should have our own messages like "preparing for copy". Line 79: Line 80: Line 81: def finish_progress(): Line 82: write_output('Creating output metadata') Line 79: Line 80: Line 81: def finish_progress(): Line 82: write_output('Creating output metadata') Line 83: write_output('Finishing off') Same, don't copy virt-v2v messages like this. Hopefully you do not depend on the text in these messages. Line 84: Line 85: Line 86: def volume_progress(op): Line 87: while op.done < op.size: -- To view, visit https://gerrit.ovirt.org/55797 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9d95c3bf4b2605e71f899171259d4721204eb8e2 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek <mskri...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Yaniv Kaul <yk...@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/mailman/listinfo/vdsm-patches
