Dan Kenigsberg has posted comments on this change. Change subject: sparsify: integrating virt-sparsify into vdsm ......................................................................
Patch Set 11: Code-Review-1 (6 comments) Thanks for your patch and involvement with oVirt. See my inline comments. I am missing some kind of a test for the new API. Could you create a LOCALFS volume and spartify it? 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) 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. Do we need this wrapper outside the storage package? I think that it belogs there. 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 http://gerrit.ovirt.org/#/c/28328/11/vdsm/storage/image.py File vdsm/storage/image.py: 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. 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 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() - if dstVolume.prepare fails, we still want to teardown src. Line 557: dstVolume.prepare() Line 558: Line 559: try: Line 560: # Extend sizes 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. 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) -- 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
