Nir Soffer has posted comments on this change. Change subject: kvm@ovirt: tool for streaming images from libvirt ......................................................................
Patch Set 2: (5 comments) https://gerrit.ovirt.org/#/c/55797/2/helpers/kvm2ovirt File helpers/kvm2ovirt: Line 82: write_output('Creating output metadata') Line 83: write_output('Finishing off') Line 84: Line 85: Line 86: def volume_progress(op): Here we wait using time.sleep, so we always have to wait 1 second (or more if we decide) before finishing. We can instead wait on a threading.Event, which will allow waking the progress thread when we finish the operation. When starting the operation, create an event object: done = threading.Event() Send it to the progress thread: t = concurrent.thread(volume_progress, args=(op, done)) t.start() op.run() done.set() # This will wake up the progress thread immediately. And in volume_progress, do: def volume_progress(op, done): while op.done < op.size: progress = op.done * 100 // op.size write_progress(progress) done.wait(1) write_progress(100) Note that you should write progress before sleeping, otherwise you will have empty output for the first second, instead of seeing (0/100%) immediately. Using an event we can not sleep for 5 seconds since we query progress only every 15 seconds from engine, and we will not have useless delay when the operation is done. Line 87: while op.done < op.size: Line 88: time.sleep(1) Line 89: progress = op.done * 100 // op.size Line 90: write_progress(progress) Line 106: op = directio.Receive(dest, sr, size, buffersize=pksize) Line 107: t = concurrent.thread(volume_progress, args=(op,)) Line 108: t.start() Line 109: op.run() Line 110: t.join() > works. A nicer alternative th.stop? I guess you mean join() Line 111: finally: Line 112: try: Line 113: # TODO: remove that after libvirt answer??? Line 114: stream.finish() Line 112: try: Line 113: # TODO: remove that after libvirt answer??? Line 114: stream.finish() Line 115: except Exception as ex: Line 116: write_output(str(ex)) We probably need a helper for logging errors: def write_error(e): write_output("ERROR: %s" % e) Line 117: Line 118: Line 119: def get_password(): Line 120: ret = '' Line 125: fd = open(options.password_file, 'r') Line 126: try: Line 127: ret = fd.read() Line 128: finally: Line 129: fd.close() > why not just +1 Line 130: return ret Line 131: Line 132: Line 133: options = parse_arguments() Line 143: for item in itertools.izip(options.source, options.dest): Line 144: vol = con.storageVolLookupByPath(item[0]) Line 145: download_volume(vol, item[1], diskno, disksitems, pksize, options.verbose) Line 146: diskno = diskno + 1 Line 147: finish_progress() > please wrap it into a function like _main() There is no need to use _main(), the normal idiom is main() This makes it possible to import and write unit tests for this module. -- 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
