Shahar Havivi has posted comments on this change. Change subject: kvm2ovirt: tool for copying images from libvirt ......................................................................
Patch Set 6: (17 comments) https://gerrit.ovirt.org/#/c/55797/5/helpers/kvm2ovirt File helpers/kvm2ovirt: Line 108: with progress(op): Line 109: op.run() Line 110: finally: Line 111: stream.finish() Line 112: > If you don't cache exceptions in stream.finish, and both stream.finish and Done Line 113: Line 114: def get_password(options): Line 115: ret = '' Line 116: if options.password_file: 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 Done 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 did it, its looks like the wrong patch 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. Done 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) Done 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 Done 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. Done 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 Done 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 wi Done 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: Done 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 Done 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? Done 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. such? 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? Done 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. Done 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: Done 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 Done -- 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
