Utkarsh Singh has posted comments on this change. Change subject: sparsify: integrating virt-sparsify into vdsm ......................................................................
Patch Set 11: -Verified (23 comments) 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 understan Included some relevant details in the commit message. Line 21: Line 22: Change-Id: Id7bd2b4b6d45781fa27a128dd68d14b7561d0901 http://gerrit.ovirt.org/#/c/28328/11/lib/vdsm/constants.py.in File lib/vdsm/constants.py.in: Line 160: Line 161: EXT_VDSM_RESTORE_NET_CONFIG = '@VDSMDIR@/vdsm-restore-net-config' Line 162: EXT_VDSM_STORE_NET_CONFIG = '@VDSMDIR@/vdsm-store-net-config' Line 163: Line 164: EXT_VIRTSPARSIFY = '@VIRTSPARSIFY_PATH@' > This seems to be unused - please drop (from here and from configure.ac) Done Line 165: Line 166: EXT_WGET = '@WGET_PATH@' Line 167: Line 168: EXT_VDSM_TOOL = os.path.join('@BINDIR@', 'vdsm-tool') 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. Done 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: Done 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.Err Done 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. Done 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? Done 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 Done 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, Done 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. Done Line 46: """ Line 47: cmd = [_virtsparsify.cmd, '--tmp', 'prebuilt:' + tmpVol] Line 48: Line 49: if srcFormat: http://gerrit.ovirt.org/#/c/28328/11/vdsm/rpc/vdsmapi-schema.json File vdsm/rpc/vdsmapi-schema.json: Line 4150: # Line 4151: # Returns: Line 4152: # A task UUID Line 4153: # Line 4154: # Since: x.y.z > Fill this in. Done Line 4155: ## Line 4156: {'command': {'class': 'Image', 'name': 'sparsify'}, Line 4157: 'data': {'srcSdUUID': 'UUID', 'srcImgUUID': 'UUID', 'srcVolUUID': 'UUID', Line 4158: 'dstSdUUID': 'UUID', 'dstImgUUID': 'UUID', 'dstVolUUID': 'UUID'}, 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: Done 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: Done 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 > Seems like "temporary" should be removed. Done 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 > This comment should be above line 538, explaining why we need to get volCla Done Line 545: srcVolume = srcVolClass(self.repoPath, srcSdUUID, srcImgUUID, Line 546: srcVolUUID) Line 547: dstVolume = dstVolClass(self.repoPath, dstSdUUID, dstImgUUID, Line 548: dstVolUUID) Line 552: Line 553: baseVolume = srcVolClass(self.repoPath, srcSdUUID, srcImgUUID, Line 554: srcVolume.getParent()) Line 555: Line 556: srcVolume.prepare() > Let's be puritans, adding a standalone try-finally foe srcVolume.prepare() Done Line 557: dstVolume.prepare() Line 558: Line 559: try: Line 560: # Extend sizes Line 556: srcVolume.prepare() Line 557: dstVolume.prepare() Line 558: Line 559: try: Line 560: # Extend sizes > This comment is useless Done 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 Done 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())) > I think you should change the names so it is clear what is going on. Since Done 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 567: srcVolume.getVolumePath(), Line 568: dstVolume.getVolumePath(), Line 569: volume.fmt2str(srcVolume.getFormat()), Line 570: volume.fmt2str(dstVolume.getFormat())) Line 571: except Exception as e: > e is unused. better drop it. Done 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 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. Done 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 Done 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 shoul Removing this. Not used. 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: Michal Skrivanek <[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
