Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
Dan Kenigsberg has submitted this change and it was merged. Change subject: kvm2ovirt: tool for copying images from libvirt .. kvm2ovirt: tool for copying images from libvirt tool for downloading/copying images from libvirt to ovrit Change-Id: I9d95c3bf4b2605e71f899171259d4721204eb8e2 Signed-off-by: Shahar Havivi Reviewed-on: https://gerrit.ovirt.org/55797 Tested-by: Shahar Havivi Continuous-Integration: Jenkins CI Reviewed-by: Francesco Romani --- M Makefile.am M configure.ac A helpers/Makefile.am A helpers/kvm2ovirt M vdsm.spec.in 5 files changed, 157 insertions(+), 0 deletions(-) Approvals: Shahar Havivi: Verified Jenkins CI: Passed CI tests Francesco Romani: Looks good to me, approved -- To view, visit https://gerrit.ovirt.org/55797 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: I9d95c3bf4b2605e71f899171259d4721204eb8e2 Gerrit-PatchSet: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Yaniv Kaul Gerrit-Reviewer: gerrit-hooks ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
gerrit-hooks has posted comments on this change. Change subject: kvm2ovirt: tool for copying images from libvirt .. Patch Set 9: * Update tracker: IGNORE, no Bug-Url found * Set MODIFIED::IGNORE, no Bug-Url found. -- 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: 9 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Yaniv Kaul Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
Francesco Romani has posted comments on this change. Change subject: kvm2ovirt: tool for copying images from libvirt .. Patch Set 8: Code-Review+2 -- 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Yaniv Kaul Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
Shahar Havivi has posted comments on this change. Change subject: kvm2ovirt: tool for copying images from libvirt .. Patch Set 8: Verified+1 -- 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Yaniv Kaul Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
gerrit-hooks has posted comments on this change. Change subject: kvm2ovirt: tool for copying images from libvirt .. Patch Set 8: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6']) -- 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: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Yaniv Kaul Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
Nir Soffer has posted comments on this change. Change subject: kvm2ovirt: tool for copying images from libvirt .. Patch Set 7: Code-Review+2 Verified+1 Shahar, please verify -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Yaniv Kaul Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
Francesco Romani has posted comments on this change. Change subject: kvm2ovirt: tool for copying images from libvirt .. Patch Set 7: Code-Review+2 well since this is needed by virt flows, I'm raising my score to +2 -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Yaniv Kaul Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
Francesco Romani has posted comments on this change. Change subject: kvm2ovirt: tool for copying images from libvirt .. Patch Set 7: Code-Review+1 nice! -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Yaniv Kaul Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
Nir Soffer has posted comments on this change. Change subject: kvm2ovirt: tool for copying images from libvirt .. Patch Set 7: Code-Review+1 Waiting for Francesco ack. -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Yaniv Kaul Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
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():
Lin
Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
gerrit-hooks has posted comments on this change. Change subject: kvm2ovirt: tool for copying images from libvirt .. Patch Set 7: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6']) -- 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: 7 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Yaniv Kaul Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
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(opti
Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
Nir Soffer has posted comments on this change. Change subject: kvm2ovirt: tool for copying images from libvirt .. Patch Set 5: (1 comment) 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: try: Line 112: # TODO: remove that after libvirt answer??? > we need the first try but not the one around the finish If you don't cache exceptions in stream.finish, and both stream.finish and op.run raise, the error from stream.finish will hide the original error, making debuging much harder. Line 113: stream.finish() Line 114: except Exception as ex: Line 115: write_error(str(ex)) Line 116: -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Yaniv Kaul Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
gerrit-hooks has posted comments on this change. Change subject: kvm2ovirt: tool for copying images from libvirt .. Patch Set 6: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6']) -- 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 Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Yaniv Kaul Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
Shahar Havivi has posted comments on this change.
Change subject: kvm2ovirt: tool for copying images from libvirt
..
Patch Set 5:
(5 comments)
https://gerrit.ovirt.org/#/c/55797/5/configure.ac
File configure.ac:
Line 376: AC_OUTPUT([
Line 377: Makefile
Line 378: client/Makefile
Line 379: contrib/Makefile
Line 380: helpers/Makefile
> missing tab
Done
Line 381: init/Makefile
Line 382: init/systemd/Makefile
Line 383: lib/Makefile
Line 384: lib/vdsm/Makefile
https://gerrit.ovirt.org/#/c/55797/5/helpers/kvm2ovirt
File helpers/kvm2ovirt:
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 may be None, causing the progress thread to crash (see vdms log for
Done
Line 78: progress = op.done * 100 // op.size
Line 79: write_progress(progress)
Line 80: if done.wait(1):
Line 81: break
Line 108: with progress(op):
Line 109: op.run()
Line 110: finally:
Line 111: try:
Line 112: # TODO: remove that after libvirt answer???
> Actually we need the try block so we don't hide the real error raise before
we need the first try but not the one around the finish
Line 113: stream.finish()
Line 114: except Exception as ex:
Line 115: write_error(str(ex))
Line 116:
PS5, Line 129: options = parse_arguments()
:
: con = libvirtconnection.open_connection(options.uri,
: options.username,
: get_password())
:
: diskno = 1
: disksitems = len(options.source)
: bufsize = int(options.bufsize)
: write_output('preparing for copy')
: for item in itertools.izip(options.source, options.dest):
: vol = con.storageVolLookupByPath(item[0])
: download_volume(vol, item[1], diskno, disksitems, bufsize,
options.verbose)
: diskno = diskno + 1
: write_output('Finishing off')
> please wrap this in a main() function, like I commented few versions ago.
Done
Line 140: vol = con.storageVolLookupByPath(item[0])
Line 141: download_volume(vol, item[1], diskno, disksitems, bufsize,
options.verbose)
Line 142: diskno = diskno + 1
Line 143: write_output('Finishing off')
Line 144
> call the new main() function using this idiom
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Shahar Havivi
Gerrit-Reviewer: Yaniv Kaul
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
Nir Soffer has posted comments on this change. Change subject: kvm2ovirt: tool for copying images from libvirt .. Patch Set 5: (2 comments) https://gerrit.ovirt.org/#/c/55797/5/helpers/kvm2ovirt File helpers/kvm2ovirt: Line 73: sys.stdout.flush() Line 74: Line 75: Line 76: def volume_progress(op, done): Line 77: while op.done < op.size: > you mean min() Right, using max() to limit the maximum is a common error :-) Line 78: progress = op.done * 100 // op.size Line 79: write_progress(progress) Line 80: if done.wait(1): Line 81: break Line 108: with progress(op): Line 109: op.run() Line 110: finally: Line 111: try: Line 112: # TODO: remove that after libvirt answer??? > The comment and try block are not needed, this was bug in the download code Actually we need the try block so we don't hide the real error raise before. Line 113: stream.finish() Line 114: except Exception as ex: Line 115: write_error(str(ex)) Line 116: -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Yaniv Kaul Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
Shahar Havivi has posted comments on this change. Change subject: kvm2ovirt: tool for copying images from libvirt .. Patch Set 5: (1 comment) https://gerrit.ovirt.org/#/c/55797/5/helpers/kvm2ovirt File helpers/kvm2ovirt: 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 may be None, causing the progress thread to crash (see vdms log for you mean min() max() returns 99/100 immediately... Line 78: progress = op.done * 100 // op.size Line 79: write_progress(progress) Line 80: if done.wait(1): Line 81: break -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Yaniv Kaul Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
Nir Soffer has posted comments on this change. Change subject: kvm2ovirt: tool for copying images from libvirt .. Patch Set 5: (2 comments) https://gerrit.ovirt.org/#/c/55797/5/helpers/kvm2ovirt File helpers/kvm2ovirt: 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 may be None, causing the progress thread to crash (see vdms log for the traceback). We don't know the size of the upload, since libvirt does provide any clue. I would use the virtual size for this - send it to the progress function as argument. Should be: while True: progress = max(100, op.done * 100 / virtual_size) ... Note that upload may be bigger than virtual size since qcow file includes internal structures (e.g. 10G image that can hold 10G user data will be bigger then 10G when full). It means that progress can report 100% multiple cycles. If this is a problem use: max(99, op.done * 100 / virtual_size) Line 78: progress = op.done * 100 // op.size Line 79: write_progress(progress) Line 80: if done.wait(1): Line 81: break Line 108: with progress(op): Line 109: op.run() Line 110: finally: Line 111: try: Line 112: # TODO: remove that after libvirt answer??? The comment and try block are not needed, this was bug in the download code. Line 113: stream.finish() Line 114: except Exception as ex: Line 115: write_error(str(ex)) Line 116: -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Yaniv Kaul Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
Francesco Romani has posted comments on this change.
Change subject: kvm2ovirt: tool for copying images from libvirt
..
Patch Set 5: Code-Review-1
(3 comments)
minor things, looks good. -1 for visibility only.
https://gerrit.ovirt.org/#/c/55797/5/configure.ac
File configure.ac:
Line 376: AC_OUTPUT([
Line 377: Makefile
Line 378: client/Makefile
Line 379: contrib/Makefile
Line 380: helpers/Makefile
missing tab
Line 381: init/Makefile
Line 382: init/systemd/Makefile
Line 383: lib/Makefile
Line 384: lib/vdsm/Makefile
https://gerrit.ovirt.org/#/c/55797/5/helpers/kvm2ovirt
File helpers/kvm2ovirt:
PS5, Line 129: options = parse_arguments()
:
: con = libvirtconnection.open_connection(options.uri,
: options.username,
: get_password())
:
: diskno = 1
: disksitems = len(options.source)
: bufsize = int(options.bufsize)
: write_output('preparing for copy')
: for item in itertools.izip(options.source, options.dest):
: vol = con.storageVolLookupByPath(item[0])
: download_volume(vol, item[1], diskno, disksitems, bufsize,
options.verbose)
: diskno = diskno + 1
: write_output('Finishing off')
please wrap this in a main() function, like I commented few versions ago.
Line 140: vol = con.storageVolLookupByPath(item[0])
Line 141: download_volume(vol, item[1], diskno, disksitems, bufsize,
options.verbose)
Line 142: diskno = diskno + 1
Line 143: write_output('Finishing off')
Line 144
call the new main() function using this idiom
if __name__ == "__main__":
main()
those should be the last lines of the file.
--
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Shahar Havivi
Gerrit-Reviewer: Francesco Romani
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek
Gerrit-Reviewer: Nir Soffer
Gerrit-Reviewer: Shahar Havivi
Gerrit-Reviewer: Yaniv Kaul
Gerrit-Reviewer: gerrit-hooks
Gerrit-HasComments: Yes
___
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
gerrit-hooks has posted comments on this change. Change subject: kvm2ovirt: tool for copying images from libvirt .. Patch Set 5: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6']) -- 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: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Yaniv Kaul Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
gerrit-hooks has posted comments on this change. Change subject: kvm2ovirt: tool for copying images from libvirt .. Patch Set 4: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6']) -- 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: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Yaniv Kaul Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
Shahar Havivi has posted comments on this change. Change subject: kvm2ovirt: tool for copying images from libvirt .. Patch Set 3: When canceling the script e.g. ctrl+c we are hang in infinite loop, I think its related to the contectmanager, I did try that without the event and we still hang. -- 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: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Yaniv Kaul Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
gerrit-hooks has posted comments on this change. Change subject: kvm2ovirt: tool for copying images from libvirt .. Patch Set 3: * Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6']) -- 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: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shahar Havivi Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Yaniv Kaul Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
