Change in vdsm[master]: kvm at ovirt: tool for streaming images from libvirt
fromani at redhat.com
fromani at redhat.com
Thu Apr 21 12:19:53 UTC 2016
Francesco Romani has posted comments on this change.
Change subject: kvm at ovirt: tool for streaming images from libvirt
......................................................................
Patch Set 2:
(10 comments)
minor comments, since the heavy lifting is done now by the directio module.
https://gerrit.ovirt.org/#/c/55797/2//COMMIT_MSG
Commit Message:
Line 3: AuthorDate: 2016-04-06 16:13:50 +0300
Line 4: Commit: Shahar Havivi <shaharh at redhat.com>
Line 5: CommitDate: 2016-04-20 10:19:35 +0300
Line 6:
Line 7: kvm at ovirt: tool for streaming images from libvirt
> kvm2ovirt
+1
Line 8:
Line 9: v2v is needs to a streaming/download tool for importing kvm based
Line 10: images from Libvirt to vdsm.
Line 11: for other such as xen and vmware virt-v2v provide the solution.
https://gerrit.ovirt.org/#/c/55797/2/configure.ac
File configure.ac:
Line 377: Makefile
Line 378: client/Makefile
Line 379: contrib/Makefile
Line 380: debian/Makefile
Line 381: helpers/Makefile
missing tab
Line 382: init/Makefile
Line 383: init/systemd/Makefile
Line 384: lib/Makefile
Line 385: lib/vdsm/Makefile
https://gerrit.ovirt.org/#/c/55797/2/helpers/Makefile.am
File helpers/Makefile.am:
PS2, Line 1: 2008
wrong copyright year
Line 12: kvm2ovirt \
Line 13: $(NULL)
Line 14:
Line 15: all-local: \
Line 16: $(nodist_vdsm_SCRIPTS) \
> Why do we need this?
probably unneeded, yes
https://gerrit.ovirt.org/#/c/55797/2/helpers/kvm2ovirt
File helpers/kvm2ovirt:
Line 55: parser.add_argument('--source', dest='source', nargs='+', required=True,
Line 56: help='Source remote volumes path')
Line 57: parser.add_argument('--dest', dest='dest', nargs='+', required=True,
Line 58: help='Destination local volumes path')
Line 59: parser.add_argument('--packet-size', dest='pksize', default=1048576,
> packet-size is not a good name, use --buffer-size and bufffer_size or if yo
+1
Line 60: help='Size of packets in bytes')
Line 61: parser.add_argument('--verbose', action='store_true',
Line 62: help='verbose output')
Line 63: return parser.parse_args(sys.argv)
PS2, Line 97: capasity
typo: capacity
PS2, Line 100: size = vol.info()[2]
could this raise? If so, it is ok if we explode?
PS2, Line 107: t = concurrent.thread(volume_progress, args=(op,))
: t.start()
: op.run()
: t.join()
works. A nicer alternative
@contextmanager
def running(th):
th.start()
try:
yield th
finally:
th.stop()
adepted from utils.running(), which unfortunately doesn't really fit here. Perhaps we could improve it? (in a later patch of course)
use it like
with running(concurrent.thread(volume_progress, args=(op,))):
op.run()
PS2, Line 125: fd = open(options.password_file, 'r')
: try:
: ret = fd.read()
: finally:
: fd.close()
why not just
with open(options.password_file, 'r'):
ret = fd.read()
?
PS2, Line 139: diskno = 1
: disksitems = len(options.source)
: pksize = int(options.pksize)
: init_progress()
: for item in itertools.izip(options.source, options.dest):
: vol = con.storageVolLookupByPath(item[0])
: download_volume(vol, item[1], diskno, disksitems, pksize, options.verbose)
: diskno = diskno + 1
: finish_progress()
please wrap it into a function like _main()
and make sure it runs using this idiom:
if __name__ == "__main__":
_main()
the above two should be the last two lines of the 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 <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