Nir Soffer has posted comments on this change. Change subject: kvm2ovirt: tool for copying images from libvirt ......................................................................
Patch Set 6: (16 comments) https://gerrit.ovirt.org/#/c/55797/6/helpers/kvm2ovirt File helpers/kvm2ovirt: Line 51: parser.add_argument('--source', dest='source', nargs='+', required=True, Line 52: help='Source remote volumes path') Line 53: parser.add_argument('--dest', dest='dest', nargs='+', required=True, Line 54: help='Destination local volumes path') Line 55: parser.add_argument('--bufsize', dest='bufsize', default=1048576, add type=int Line 56: help='Size of packets in bytes, default 1048676') Line 57: parser.add_argument('--verbose', action='store_true', Line 58: help='verbose output') Line 59: return parser.parse_args(sys.argv) Line 72: sys.stdout.write(' (%d/100%%)\r' % progress) Line 73: sys.stdout.flush() Line 74: Line 75: Line 76: def volume_progress(op, done): Add estimated_size argument Line 77: while op.done < op.size: Line 78: progress = op.done * 100 // op.size Line 79: write_progress(progress) Line 80: if done.wait(1): Line 73: sys.stdout.flush() Line 74: Line 75: Line 76: def volume_progress(op, done): Line 77: while op.done < op.size: op.size is None, we cannot use it, pass the estimated size as argument. Line 78: progress = op.done * 100 // op.size Line 79: write_progress(progress) Line 80: if done.wait(1): Line 81: break Line 74: Line 75: Line 76: def volume_progress(op, done): Line 77: while op.done < op.size: Line 78: progress = op.done * 100 // op.size progress = min(99, op.done * 100 // estimated_size) Line 79: write_progress(progress) Line 80: if done.wait(1): Line 81: break Line 82: write_progress(100) Line 82: write_progress(100) Line 83: Line 84: Line 85: @contextmanager Line 86: def progress(op): Add estimated_size argument, pass it to volume_progress thread Line 87: done = threading.Event() Line 88: th = concurrent.thread(volume_progress, args=(op, done)) Line 89: th.start() Line 90: try: Line 93: done.set() Line 94: th.join() Line 95: Line 96: Line 97: def download_volume(con, vol, dest, diskno, disks, bufsize, verbose): remove diskno, disks, verbose. Line 98: write_output('Copying disk %d/%d to %s' % (diskno, disks, dest)) Line 99: if verbose: Line 100: write_output('>>> disk %d, capacity: %d allocation %d' % Line 101: (diskno, vol.info()[1], vol.info()[2])) Line 97: def download_volume(con, vol, dest, diskno, disks, bufsize, verbose): Line 98: write_output('Copying disk %d/%d to %s' % (diskno, disks, dest)) Line 99: if verbose: Line 100: write_output('>>> disk %d, capacity: %d allocation %d' % Line 101: (diskno, vol.info()[1], vol.info()[2])) Move both to main loop Line 102: Line 103: stream = con.newStream() Line 104: try: Line 105: vol.download(stream, 0, 0, 0) Line 107: op = directio.Receive(dest, sr, buffersize=bufsize) Line 108: with progress(op): Line 109: op.run() Line 110: finally: Line 111: stream.finish() If op.run raises, and stream.finish raises, the error from stream finish will hide the original error making debugging much harder. I think we can simplify this by removing the all the try blocks here - if op.run fails, we don't care about finish since we are dieing anyway. Line 112: Line 113: Line 114: def get_password(options): Line 115: ret = '' Line 111: stream.finish() Line 112: Line 113: Line 114: def get_password(options): Line 115: ret = '' A better way would be: if not options.password_file: return ''' write output... with open(...) as f: return f.read() Line 116: if options.password_file: Line 117: if options.verbose: Line 118: write_output('>>> Reading password from file %s' % Line 119: options.password_file) Line 116: if options.password_file: Line 117: if options.verbose: Line 118: write_output('>>> Reading password from file %s' % Line 119: options.password_file) Line 120: with open(options.password_file, 'r') as fd: fd is a bad name file object, this is the common name for a file descriptor. The common name for this temporary is "f" Line 121: ret = fd.read() Line 122: return ret Line 123: Line 124: def main(): Line 121: ret = fd.read() Line 122: return ret Line 123: Line 124: def main(): Line 125: options = parse_arguments() Decide on the name - options or arguments? Line 126: Line 127: con = libvirtconnection.open_connection(options.uri, Line 128: options.username, Line 129: get_password(options)) Line 127: con = libvirtconnection.open_connection(options.uri, Line 128: options.username, Line 129: get_password(options)) Line 130: Line 131: diskno = 1 This works, but we have better ways in python. Line 132: disksitems = len(options.source) Line 133: bufsize = int(options.bufsize) Line 134: write_output('preparing for copy') Line 135: for item in itertools.izip(options.source, options.dest): Line 128: options.username, Line 129: get_password(options)) Line 130: Line 131: diskno = 1 Line 132: disksitems = len(options.source) disk_count? Line 133: bufsize = int(options.bufsize) Line 134: write_output('preparing for copy') Line 135: for item in itertools.izip(options.source, options.dest): Line 136: vol = con.storageVolLookupByPath(item[0]) Line 129: get_password(options)) Line 130: Line 131: diskno = 1 Line 132: disksitems = len(options.source) Line 133: bufsize = int(options.bufsize) Define the type of this argument to int instead, and remove this line. Line 134: write_output('preparing for copy') Line 135: for item in itertools.izip(options.source, options.dest): Line 136: vol = con.storageVolLookupByPath(item[0]) Line 137: download_volume(con, vol, item[1], diskno, disksitems, Line 135: for item in itertools.izip(options.source, options.dest): Line 136: vol = con.storageVolLookupByPath(item[0]) Line 137: download_volume(con, vol, item[1], diskno, disksitems, Line 138: bufsize, options.verbose) Line 139: diskno = diskno + 1 Use: disks = itertools.izip(options.source, options.dest) for diskno, (src, dst) in enumerate(disks, start=1): log about current disk (what you do in download_volume)... vol = con.storageVolLookupByPath(src) download_volume(con, vol, dst, options.bufsize) Why download_volume should care about diskno and disk_count? I think that you want to use these values here for logging the start and end message for each disk, instead of passing them around. Line 140: write_output('Finishing off') Line 141: Line 142: if __name__ == "__main__": Line 139: diskno = diskno + 1 Line 140: write_output('Finishing off') Line 141: Line 142: if __name__ == "__main__": Line 143: main() Bad indentation -- 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: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Shahar Havivi <[email protected]> Gerrit-Reviewer: Yaniv Kaul <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
