Change in vdsm[master]: kvmstream: tool for streaming images from libvirt

shavivi at redhat.com shavivi at redhat.com
Mon Apr 11 07:05:20 UTC 2016


Shahar Havivi has posted comments on this change.

Change subject: kvmstream: tool for streaming images from libvirt
......................................................................


Patch Set 1:

(10 comments)

https://gerrit.ovirt.org/#/c/55797/1//COMMIT_MSG
Commit Message:

PS1, Line 9: v2v is needed to a streaming/download tool f
> "v2v needs"?
Done


https://gerrit.ovirt.org/#/c/55797/1/kvmstream.py
File kvmstream.py:

PS1, Line 22: from itertools import izip
> nit: better to avoid from ... imports and just import modules unless the im
Done


PS1, Line 23: import libvirt
> please group the non-standard-library imports below all the standard-librar
Done


PS1, Line 27: elapsed_time = 0
> I'd try to avoid global state unless absolutely needed. Here I think we can
Done


Line 30: def parse_arguments():
Line 31:     parser = argparse.ArgumentParser()
Line 32:     parser.add_argument('args')
Line 33:     parser.add_argument('-u', dest='uri',
Line 34:                         help='Libvirt URI')
> This should be required argument
Done
Line 35:     parser.add_argument('-s', dest='source', nargs='+',
Line 36:                         help='Source remote volumes path')
Line 37:     parser.add_argument('-d', dest='dest', nargs='+',
Line 38:                         help='Destination local volumes path')


Line 32:     parser.add_argument('args')
Line 33:     parser.add_argument('-u', dest='uri',
Line 34:                         help='Libvirt URI')
Line 35:     parser.add_argument('-s', dest='source', nargs='+',
Line 36:                         help='Source remote volumes path')
> This should be required argument
Done
Line 37:     parser.add_argument('-d', dest='dest', nargs='+',
Line 38:                         help='Destination local volumes path')
Line 39:     return parser.parse_args(sys.argv)
Line 40: 


Line 34:                         help='Libvirt URI')
Line 35:     parser.add_argument('-s', dest='source', nargs='+',
Line 36:                         help='Source remote volumes path')
Line 37:     parser.add_argument('-d', dest='dest', nargs='+',
Line 38:                         help='Destination local volumes path')
> This should be required argument
Done
Line 39:     return parser.parse_args(sys.argv)
Line 40: 
Line 41: 
Line 42: def write_output(msg):


Line 46: 
Line 47: def init_progres():
Line 48:     global elapsed_time
Line 49:     write_output('[   %d.0] Opening the source -i libvirt\n' % elapsed_time)
Line 50:     elapsed_time = elapsed_time + 1
> This is left over from fake progress code
No, I increase it intentionally.
Line 51:     write_output('[   %d.0] Creating an overlay to protect\n' % elapsed_time)
Line 52:     elapsed_time = elapsed_time + 1
Line 53: 
Line 54: 


Line 69:     vol.download(stream, 0, 0, 0)
Line 70:     write_output('    (0/100%%)\r')
Line 71:     counter = 0
Line 72:     while pos < size:
Line 73:         buf = s.recv(1024)
> We use 1M in ovirt-imageio and vdsm when copying images. We should have a d
Done
Line 74:         f.write(buf)
Line 75:         f.flush()
Line 76:         if not pos % jump:
Line 77:             counter = counter + 1


Line 82:     write_output('    (100/100%%)\r')
Line 83: 
Line 84: 
Line 85: options = parse_arguments()
Line 86: con = libvirt.open(options.uri)
> Some error handling?
Done
Line 87: 
Line 88: diskno = 1
Line 89: disksitems = len(options.source)
Line 90: init_progres()


-- 
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: 1
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