Nir Soffer has posted comments on this change. Change subject: sparsify: integrating virt-sparsify into vdsm ......................................................................
Patch Set 13: (9 comments) Much nicer, some issues remain, mostly documentation issues. http://gerrit.ovirt.org/#/c/28328/13/vdsm/storage/image.py File vdsm/storage/image.py: Line 535: Line 536: # FIXME: Line 537: # sdCache.produce.produceVolume gives volumes that return getVolumePath Line 538: # with a colon (:) for NFS servers. So, we're using volClass(). Line 539: # RHBZ#1128942 The RHBZ is not needed now when we have a url - right? Line 540: # https://bugzilla.redhat.com/show_bug.cgi?id=1128942 Line 541: # If, and when the bug gets solved, remove the tmpVolClass and Line 542: # dstVolClass variables, and get tmpVolume and dstVolume using Line 543: # sdCache.produce(xyzSdUUID).produceVolume(xyzImgUUID, xyzVolUUID) Line 536: # FIXME: Line 537: # sdCache.produce.produceVolume gives volumes that return getVolumePath Line 538: # with a colon (:) for NFS servers. So, we're using volClass(). Line 539: # RHBZ#1128942 Line 540: # https://bugzilla.redhat.com/show_bug.cgi?id=1128942 The url can nicer using: https://bugzilla.redhat.com/1128942 Line 541: # If, and when the bug gets solved, remove the tmpVolClass and Line 542: # dstVolClass variables, and get tmpVolume and dstVolume using Line 543: # sdCache.produce(xyzSdUUID).produceVolume(xyzImgUUID, xyzVolUUID) Line 544: # Also, for baseVolume use Line 542: # dstVolClass variables, and get tmpVolume and dstVolume using Line 543: # sdCache.produce(xyzSdUUID).produceVolume(xyzImgUUID, xyzVolUUID) Line 544: # Also, for baseVolume use Line 545: # sdCache.produce(tmpSdUUID).produceVolume(tmpImgUUID, Line 546: # tmpVolume.getParent()) I think this can be shrinked to: When the bug is fixed, use sdCache.produce(...).produceVolume(...) to create the volumes. The person who will fix this can check the apis and write the code when needed, we don't have to write the new code now. Line 547: tmpVolClass = sdCache.produce(tmpSdUUID).getVolumeClass() Line 548: dstVolClass = sdCache.produce(dstSdUUID).getVolumeClass() Line 549: Line 550: # Get source, destination and temporary volumes Line 550: # Get source, destination and temporary volumes Line 551: tmpVolume = tmpVolClass(self.repoPath, tmpSdUUID, tmpImgUUID, Line 552: tmpVolUUID) Line 553: dstVolume = dstVolClass(self.repoPath, dstSdUUID, dstImgUUID, Line 554: dstVolUUID) It would be nice to remove this code and add a helper to return a volume from tmpSdUUID, imgUUID. Then this can be simplified to: tmpVolume = self._getSparsifyVolume(tmpSdUUID, tmpImgUUID) srcVolume = self._getSparsifyVolume(tmpSdUUID, tmpImgUUID, tmpVolume.getParent()) dstVolume = self._getSparsifyVolume(dstSdUUID, dstImgUUID) The documentation why we need to use this can be hidden inside the helper. 4 lines here instead of 34 lines. Line 555: Line 556: if not tmpVolume.isSparse() or not dstVolume.isSparse(): Line 557: raise se.VolumeNotSparse() Line 558: Line 556: if not tmpVolume.isSparse() or not dstVolume.isSparse(): Line 557: raise se.VolumeNotSparse() Line 558: Line 559: srcVolume = tmpVolClass(self.repoPath, tmpSdUUID, tmpImgUUID, Line 560: tmpVolume.getParent()) Should we check tmpVolume or srcVolume sparseness? or both? tmpVolume is empty snapshot - right? Line 561: Line 562: tmpVolume.prepare() Line 563: Line 564: try: Line 562: tmpVolume.prepare() Line 563: Line 564: try: Line 565: dstVolume.prepare() Line 566: The empty line bellow the prepare() and teardown() is not needed. Line 567: try: Line 568: # TODO: Some extra space may be needed for QCOW2 headers Line 569: tmpVolume.extend(tmpVolume.getSize()) Line 570: dstVolume.extend(tmpVolume.getSize()) Line 565: dstVolume.prepare() Line 566: Line 567: try: Line 568: # TODO: Some extra space may be needed for QCOW2 headers Line 569: tmpVolume.extend(tmpVolume.getSize()) Why do we need to extend tmpVolume? virt-sparsify will write into it? I asking this because you take only shared lock on this volume in sp.py - maybe we need an exclusive lock? Federico? Line 570: dstVolume.extend(tmpVolume.getSize()) Line 571: Line 572: srcFormat = volume.fmt2str(srcVolume.getFormat()) Line 573: dstFormat = volume.fmt2str(dstVolume.getFormat()) Line 586: Line 587: finally: Line 588: tmpVolume.teardown(sdUUID=tmpSdUUID, volUUID=tmpVolUUID) Line 589: Line 590: # Shrink to optimal size This comment is not adding any value. Line 591: tmpVolume.shrinkToOptimalSize() Line 592: dstVolume.shrinkToOptimalSize() Line 593: Line 594: def cloneStructure(self, sdUUID, imgUUID, dstSdUUID): http://gerrit.ovirt.org/#/c/28328/13/vdsm/storage/sp.py File vdsm/storage/sp.py: Line 1572: """ Line 1573: srcNamespace = sd.getNamespace(tmpSdUUID, IMAGE_NAMESPACE) Line 1574: dstNamespace = sd.getNamespace(dstSdUUID, IMAGE_NAMESPACE) Line 1575: Line 1576: with nested( Explain why you take shared lock for the tmp image and exclusive lock on the dst image. Line 1577: rmanager.acquireResource(srcNamespace, tmpImgUUID, Line 1578: rm.LockType.shared), Line 1579: rmanager.acquireResource(dstNamespace, dstImgUUID, Line 1580: rm.LockType.exclusive)): -- 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: 13 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
