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