Nir Soffer has posted comments on this change. Change subject: sparsify: integrating virt-sparsify into vdsm ......................................................................
Patch Set 11: Code-Review-1 (20 comments) Very good work, needs some more love, mostly minor issues. http://gerrit.ovirt.org/#/c/28328/11//COMMIT_MSG Commit Message: Line 16: volume is determined as the parent volume of source volume. A Line 17: destination volume is also specified. This destination volume must Line 18: exist, and should be writable. The sparsifyImage then runs Line 19: virt-sparsify with the source, base and destination volumes as Line 20: arguments. It is not clear from this what is the flow. It would be useful to understand what are you trying to do before we look at the code that implement it. Is the flow explained somewhere else? gsoc page? Line 21: Line 22: Change-Id: Id7bd2b4b6d45781fa27a128dd68d14b7561d0901 http://gerrit.ovirt.org/#/c/28328/11/lib/vdsm/virtsparsify.py File lib/vdsm/virtsparsify.py: Line 1: # Line 2: # Copyright 2012 Red Hat, Inc. > The year is wrong. Here we can add this later to vdsm-tool. Our current practice of moving code around when we want to change the place we invoke the code is bad. We should move to the concept of libvdsm - everything we want to to is provided by libvdsm, and we call and wrap this code from other places. For example, look at the useless work Yeela have to do here, moving code from multipath to lib/vdsm/tool/configurators.py: http://gerrit.ovirt.org/30909 From storage point of view, this patch is the right way. Line 3: # Line 4: # This program is free software; you can redistribute it and/or modify Line 5: # it under the terms of the GNU General Public License as published by Line 6: # the Free Software Foundation; either version 2 of the License, or Line 22: Line 23: from . import utils Line 24: Line 25: # Fedora, EL6 Line 26: _virtsparsify = utils.CommandPath("virt-sparsify", This is a constant, so lets use: _VIRTSPARSIFY This is how most other code use CommandPath. Line 27: "/usr/bin/virt-sparsify",) Line 28: Line 29: Line 30: class VirtSparsifyError(Exception): Line 26: _virtsparsify = utils.CommandPath("virt-sparsify", Line 27: "/usr/bin/virt-sparsify",) Line 28: Line 29: Line 30: class VirtSparsifyError(Exception): Why do we need to repeat the module name in the exception? virtsparsify.Error is very clear, and ensure that people will not try to import it into other modules. Line 31: def __init__(self, ecode, stdout, stderr, message=None): Line 32: self.ecode = ecode Line 33: self.stdout = stdout Line 34: self.stderr = stderr Line 27: "/usr/bin/virt-sparsify",) Line 28: Line 29: Line 30: class VirtSparsifyError(Exception): Line 31: def __init__(self, ecode, stdout, stderr, message=None): message is unused, remove it. Line 32: self.ecode = ecode Line 33: self.stdout = stdout Line 34: self.stderr = stderr Line 35: self.message = message Line 29: Line 30: class VirtSparsifyError(Exception): Line 31: def __init__(self, ecode, stdout, stderr, message=None): Line 32: self.ecode = ecode Line 33: self.stdout = stdout Why do we need stdout in the error? Line 34: self.stderr = stderr Line 35: self.message = message Line 36: Line 37: def __str__(self): Line 34: self.stderr = stderr Line 35: self.message = message Line 36: Line 37: def __str__(self): Line 38: return "ecode=%s, stdout=%s, stderr=%s, message=%s" % ( Lets have more streamlined format - similar to OSError "[Error %d] %s" % (self.ecode, self.stderr) Line 39: self.ecode, self.stdout, self.stderr, self.message) Line 40: Line 41: Line 42: def sparsify(srcVol, tmpVol, dstVol, srcFormat=None, dstFormat=None): Line 38: return "ecode=%s, stdout=%s, stderr=%s, message=%s" % ( Line 39: self.ecode, self.stdout, self.stderr, self.message) Line 40: Line 41: Line 42: def sparsify(srcVol, tmpVol, dstVol, srcFormat=None, dstFormat=None): This is new code - use pep8 style - src_vol, tmp_vol, dst_vol, src_format, dst_format Line 43: """ Line 44: Sparsify the 'srcVol' volume (srcFormat) to 'dstVol' volume (dstFormat) Line 45: using libguestfs virt-sparsify Line 46: """ Line 41: Line 42: def sparsify(srcVol, tmpVol, dstVol, srcFormat=None, dstFormat=None): Line 43: """ Line 44: Sparsify the 'srcVol' volume (srcFormat) to 'dstVol' volume (dstFormat) Line 45: using libguestfs virt-sparsify Please add documentation for the arguments. Some questions that this documentation should answer: - what is srcVol - a path? uuid? - what are the available format values? is this a string? integer? Line 46: """ Line 47: cmd = [_virtsparsify.cmd, '--tmp', 'prebuilt:' + tmpVol] Line 48: Line 49: if srcFormat: http://gerrit.ovirt.org/#/c/28328/11/vdsm.spec.in File vdsm.spec.in: Line 142: Line 143: %if 0%{?rhel} Line 144: Requires: libguestfs-tools-c >= 1:1.20.11-10 Line 145: %else # fedora Line 146: Requires: libguestfs-tools-c >= 1:1.26.7-2 Are these versions available in public repositories? we failed many times by depending on non-existing versions. Line 147: %endif Line 148: Line 149: %if 0%{?rhel} >= 7 || 0%{?fedora} >= 18 Line 150: Requires: libvirt-daemon >= 1.0.2-1 http://gerrit.ovirt.org/#/c/28328/11/vdsm/storage/hsm.py File vdsm/storage/hsm.py: Line 1630: Line 1631: if srcSdUUID == dstSdUUID: Line 1632: sdUUIDs = [srcSdUUID] Line 1633: else: Line 1634: sdUUIDs = sorted((srcSdUUID, dstSdUUID)) We can replace this with: sdUUIDS = sorted(set((dstSdUUID, srcSdUUID))) Line 1635: Line 1636: for dom in sdUUIDs: Line 1637: vars.task.getSharedLock(STORAGE, dom) Line 1638: http://gerrit.ovirt.org/#/c/28328/11/vdsm/storage/image.py File vdsm/storage/image.py: Line 532: argsStr = ("srcSdUUID=%s, srcImgUUID=%s, srcVolUUID=%s, dstSdUUID=%s," Line 533: " dstImgUUID=%s, dstVolUUID=%s" % Line 534: (srcSdUUID, srcImgUUID, srcVolUUID, dstSdUUID, dstImgUUID, Line 535: dstVolUUID)) Line 536: self.log.info(argsStr) We don't need argsStr, use: self.log.info("format", args, ...) Line 537: Line 538: srcVolClass = sdCache.produce(srcSdUUID).getVolumeClass() Line 539: dstVolClass = sdCache.produce(dstSdUUID).getVolumeClass() Line 540: Line 537: Line 538: srcVolClass = sdCache.produce(srcSdUUID).getVolumeClass() Line 539: dstVolClass = sdCache.produce(dstSdUUID).getVolumeClass() Line 540: Line 541: # Get source, destination and temporary volumes > Where's the temporary volume? I do not see virt-sperdify's --tmp exposed. Seems like "temporary" should be removed. Line 542: # sdCache.produce.produceVolume gives volumes that return getVolumePath Line 543: # with a colon (:) for NFS servers. So, we're using volClass(). Line 544: # RHBZ#1128942 Line 545: srcVolume = srcVolClass(self.repoPath, srcSdUUID, srcImgUUID, Line 540: Line 541: # Get source, destination and temporary volumes Line 542: # sdCache.produce.produceVolume gives volumes that return getVolumePath Line 543: # with a colon (:) for NFS servers. So, we're using volClass(). Line 544: # RHBZ#1128942 > Please give a hint here what should be done once this guestfs bug is solved This comment should be above line 538, explaining why we need to get volClass. Also put real url to the bug. The RHBZ# notation has few issues: - I have to guess that RHBS means red hat bugzilla - I have to copy manually the bug number by dragging, instead of double clicking if it was a separated word Line 545: srcVolume = srcVolClass(self.repoPath, srcSdUUID, srcImgUUID, Line 546: srcVolUUID) Line 547: dstVolume = dstVolClass(self.repoPath, dstSdUUID, dstImgUUID, Line 548: dstVolUUID) Line 556: srcVolume.prepare() Line 557: dstVolume.prepare() Line 558: Line 559: try: Line 560: # Extend sizes This comment is useless Line 561: # TODO: Some extra space may be needed for QCOW2 headers Line 562: srcVolume.extend(srcVolume.getSize()) Line 563: dstVolume.extend(srcVolume.getSize()) Line 564: Line 561: # TODO: Some extra space may be needed for QCOW2 headers Line 562: srcVolume.extend(srcVolume.getSize()) Line 563: dstVolume.extend(srcVolume.getSize()) Line 564: Line 565: # Sparsify This comment is useless too Line 566: virtsparsify.sparsify(baseVolume.getVolumePath(), Line 567: srcVolume.getVolumePath(), Line 568: dstVolume.getVolumePath(), Line 569: volume.fmt2str(srcVolume.getFormat()), Line 566: virtsparsify.sparsify(baseVolume.getVolumePath(), Line 567: srcVolume.getVolumePath(), Line 568: dstVolume.getVolumePath(), Line 569: volume.fmt2str(srcVolume.getFormat()), Line 570: volume.fmt2str(dstVolume.getFormat())) First, acording to lib/vdsm/virtsparsify: sparsify(srcVol, tmpVol, dstVol, srcFormat=None, dstFormat=None): You are calling this not in the correct order. src=baseVolume tmp=srcVolume dst=dstVolume Or the names chosen here or in sparsify function are not correct. Second, use key=value for optional parameters: func("required 1", "required 2", optional1=value1) Line 571: except Exception as e: Line 572: self.log.exception('Unexpected error sparsifying %s', srcVolUUID) Line 573: raise se.CannotSparsifyVolume(srcVolUUID) Line 574: finally: Line 572: self.log.exception('Unexpected error sparsifying %s', srcVolUUID) Line 573: raise se.CannotSparsifyVolume(srcVolUUID) Line 574: finally: Line 575: srcVolume.teardown(sdUUID=srcSdUUID, volUUID=srcVolUUID) Line 576: dstVolume.teardown(sdUUID=dstSdUUID, volUUID=dstVolUUID) We need separate try finally for teardowns. Line 577: Line 578: # Shrink to optimal size Line 579: srcVolume.shrinkToOptimalSize() Line 580: dstVolume.shrinkToOptimalSize() http://gerrit.ovirt.org/#/c/28328/11/vdsm/storage/sp.py File vdsm/storage/sp.py: Line 1570: :param dstVolUUID: The UUID of the destination volume for the Line 1571: sparsified volume. Line 1572: :type dstVolUUID: UUID Line 1573: """ Line 1574: srcImageResourcesNamespace = sd.getNamespace(srcSdUUID, Too long - we don't have other namespace this scope, so we don't care that this is an image namespace used for acquiring a resource. We care only about having src and dst namespaces. So: srcNamespace = ... dstNamespace = ... The rest of the code will be much readable now and will not need extra effort to comply with pep8. Line 1575: IMAGE_NAMESPACE) Line 1576: dstImageResourcesNamespace = sd.getNamespace(dstSdUUID, Line 1577: IMAGE_NAMESPACE) Line 1578: http://gerrit.ovirt.org/#/c/28328/11/vdsm/storage/storage_exception.py File vdsm/storage/storage_exception.py: Line 454: Line 455: Line 456: class SparsifyImageError(StorageException): Line 457: code = 269 Line 458: message = "Failed to run virt-sparsify on image" This is not correct - we *did* run virt-sparsify, but it failed. This should be something like: "virt-sparsify failed to sparsify image" Line 459: Line 460: Line 461: ################################################# Line 462: # Pool Exceptions -- To view, visit http://gerrit.ovirt.org/28328 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id7bd2b4b6d45781fa27a128dd68d14b7561d0901 Gerrit-PatchSet: 11 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Utkarsh Singh <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Utkarsh Singh <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
