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