Change in vdsm[master]: kvm2ovirt: tool for copying images from libvirt
nsoffer at redhat.com
nsoffer at redhat.com
Sun May 1 11:28:59 UTC 2016
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 <shavivi at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <mskrivan at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: Yaniv Kaul <ykaul at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list