Federico Simoncelli has posted comments on this change. Change subject: sparsify: integrating virt-sparsify into vdsm ......................................................................
Patch Set 6: Code-Review-1 (25 comments) http://gerrit.ovirt.org/#/c/28328/6/client/vdsClient.py File client/vdsClient.py: Line 1313: tmpVolUUID, dstSdUUID, dstImgUUID, dstVolUUID) = args Line 1314: status = self.s.sparsifyImage(spUUID, srcSdUUID, srcImgUUID, Line 1315: srcVolUUID, tmpSdUUID, tmpImgUUID, Line 1316: tmpVolUUID, dstSdUUID, dstImgUUID, Line 1317: dstVolUUID) I think this should be just: status = self.s.sparsifyImage(spUUID, srcSdUUID, srcImgUUID, srcVolUUID, dstSdUUID, dstImgUUID, dstVolUUID) Line 1318: if status['status']['code']: Line 1319: return status['status']['code'], status['status']['message'] Line 1320: return 0, status['uuid'] Line 1321: Line 2448: )), Line 2449: 'sparsifyImage': (serv.sparsifyImage, Line 2450: ('<spUUID> <srcSdUUID> <srcImgUUID> <srcVolUUID> ' Line 2451: '<tmpSdUUID> <tmpImgUUID> <tmpVolUUID> ' Line 2452: '<dstSdUUID> <dstImgUUID> <dstVolUUID>', Please fix description as well. Line 2453: 'Reduce the size of a sparse image by converting ' Line 2454: 'free space on image to free space on storage ' Line 2455: 'domain using virt-sparsify' Line 2456: )), http://gerrit.ovirt.org/#/c/28328/6/lib/vdsm/virtsparsify.py File lib/vdsm/virtsparsify.py: Line 26: _virtsparsify = utils.CommandPath("virt-sparsify", Line 27: "/usr/bin/virt-sparsify",) Line 28: Line 29: Line 30: class VSprsError(Exception): VirtSparsifyError 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 43: """ Line 44: Sparsify the 'srcVol' volume to 'dstVol' volume using libguestfs Line 45: virt-sparsify Line 46: """ Line 47: cmd = [_virtsparsify.cmd] cmd = (_virtsparsify.cmd, '--tmp', 'prebuilt:' + tmpVol, srcVol, dstVol) And then you can get rid of all the rest (lines 48-55) Line 48: Line 49: tmpPrebuilt = "prebuilt:%s" % (tmpVol) Line 50: Line 51: cmd.append(srcVol) Line 55: Line 56: rc, out, err = utils.execCmd(cmd, deathSignal=signal.SIGKILL) Line 57: Line 58: if rc != 0: Line 59: raise VSprsError(rc, out, err) VirtSparsifyError http://gerrit.ovirt.org/#/c/28328/6/vdsm/API.py File vdsm/API.py: Line 866: self._UUID, vmUUID, operation, postZero, Line 867: force) Line 868: Line 869: def sparsify(self, srcVolUUID, tmpSdUUID, tmpImgUUID, tmpVolUUID, Line 870: dstSdUUID, dstImgUUID, dstVolUUID): Signature here could be: def sparsify(self, srcSdUUID, srcImgUUID, srcVolUUID, dstSdUUID, dstImgUUID, dstVolUUID): No need for the "tmp" arguments, we can obtain them later. Line 871: return self._irs.sparsifyImage(self._spUUID, self._sdUUID, self._UUID, Line 872: srcVolUUID, tmpSdUUID, tmpImgUUID, Line 873: tmpVolUUID, dstSdUUID, dstImgUUID, Line 874: dstVolUUID) Line 867: force) Line 868: Line 869: def sparsify(self, srcVolUUID, tmpSdUUID, tmpImgUUID, tmpVolUUID, Line 870: dstSdUUID, dstImgUUID, dstVolUUID): Line 871: return self._irs.sparsifyImage(self._spUUID, self._sdUUID, self._UUID, You need to fix this as well. Line 872: srcVolUUID, tmpSdUUID, tmpImgUUID, Line 873: tmpVolUUID, dstSdUUID, dstImgUUID, Line 874: dstVolUUID) Line 875: http://gerrit.ovirt.org/#/c/28328/6/vdsm/rpc/BindingXMLRPC.py File vdsm/rpc/BindingXMLRPC.py: Line 682: return image.move(dstDomUUID, op, postZero, force) Line 683: Line 684: def imageSparsify(self, spUUID, sdUUID, imgUUID, srcVolUUID, tmpSdUUID, Line 685: tmpImgUUID, tmpVolUUID, dstSdUUID, dstImgUUID, Line 686: dstVolUUID): This needs to be fixed: def imageSparsify(self, spUUID, srcSdUUID, srcImgUUID, srcVolUUID, dstSdUUID, dstImgUUID, dstVolUUID): Line 687: image = API.Image(imgUUID, spUUID, sdUUID) Line 688: return image.sparsify(srcVolUUID, tmpSdUUID, tmpImgUUID, tmpVolUUID, Line 689: dstSdUUID, dstImgUUID, dstVolUUID) Line 690: http://gerrit.ovirt.org/#/c/28328/6/vdsm/rpc/vdsmapi-schema.json File vdsm/rpc/vdsmapi-schema.json: Line 4100: # @srcImgUUID: The UUID of the source image Line 4101: # Line 4102: # @srcVolUUID: The UUID of the source volume Line 4103: # Line 4104: # @tmpSdUUID: The UUID of the temporary storage domain Remove these three as they can be obtained. Line 4105: # Line 4106: # @tmpImgUUID: The UUID of the temporary image Line 4107: # Line 4108: # @tmpVolUUID: The UUID of the temporary volume Line 4118: # Line 4119: # Since: x.y.z Line 4120: ## Line 4121: {'command': {'class': 'Image', 'name': 'sparsify'}, Line 4122: 'data': {'srcSdUUID': 'UUID', 'srcImgUUID': 'UUID', 'srcVolUUID': 'UUID', From here as well. Line 4123: 'tmpSdUUID': 'UUID', 'tmpImgUUID': 'UUID', 'tmpVolUUID': 'UUID', Line 4124: 'dstSdUUID': 'UUID', 'dstImgUUID': 'UUID', 'dstVolUUID': 'UUID'}, Line 4125: 'returns': 'UUID'} Line 4126: http://gerrit.ovirt.org/#/c/28328/6/vdsm/storage/hsm.py File vdsm/storage/hsm.py: Line 1619: misc.parseBool(force)) Line 1620: Line 1621: @public Line 1622: def sparsifyImage(self, spUUID, srcSdUUID, srcImgUUID, srcVolUUID, Line 1623: tmpSdUUID, tmpImgUUID, tmpVolUUID, Remove the unneeded arguments. Line 1624: dstSdUUID, dstImgUUID, dstVolUUID): Line 1625: """ Line 1626: Reduce sparse image size by converting free space on image to free Line 1627: space on storage domain using virt-sparsify. Line 1638: vars.task.getSharedLock(STORAGE, dom) Line 1639: Line 1640: self._spmSchedule(spUUID, "sparsifyImage", pool.sparsifyImage, Line 1641: srcSdUUID, srcImgUUID, srcVolUUID, tmpSdUUID, Line 1642: tmpImgUUID, tmpVolUUID, dstSdUUID, dstImgUUID, Remove the unneeded arguments. Line 1643: dstVolUUID) Line 1644: Line 1645: @public Line 1646: def cloneImageStructure(self, spUUID, sdUUID, imgUUID, dstSdUUID): http://gerrit.ovirt.org/#/c/28328/6/vdsm/storage/image.py File vdsm/storage/image.py: Line 511: self.log.info("%s task on image %s was successfully finished", Line 512: OP_TYPES[op], imgUUID) Line 513: return True Line 514: Line 515: def _activateVolume(self, sdUUID, imgUUID, volUUID): Remove this, it's unneeded. Line 516: domain = sdCache.produce(sdUUID) Line 517: domain.activateVolumes(imgUUID, volUUIDs=[volUUID]) Line 518: return domain.getVolumeClass()(self.repoPath, sdUUID, imgUUID, volUUID) Line 519: Line 516: domain = sdCache.produce(sdUUID) Line 517: domain.activateVolumes(imgUUID, volUUIDs=[volUUID]) Line 518: return domain.getVolumeClass()(self.repoPath, sdUUID, imgUUID, volUUID) Line 519: Line 520: def sparsify(self, srcSdUUID, srcImgUUID, srcVolUUID, tmpSdUUID, Remove the unneeded arguments. Idea is: srcImage: baseVolume <= srcVolume dstImage: dstVolume baseVolume is the source for sparsify, srcVolume is the temporary volume that *engine* created, dstVolume is the destination. Line 521: tmpImgUUID, tmpVolUUID, dstSdUUID, dstImgUUID, dstVolUUID): Line 522: """ Line 523: Reduce sparse image size by converting free space on image to free Line 524: space on storage domain using virt-sparsify. Line 531: self.log.info(argsStr) Line 532: Line 533: # Activate source and destination volume links Line 534: srcVolume = self._activateVolume(srcSdUUID, srcImgUUID, srcVolUUID) Line 535: dstVolume = self._activateVolume(dstSdUUID, dstImgUUID, dstVolUUID) These two should be: srcVolume = sdCache.produce(srcSdUUID).produceVolume(srcImgUUID, srcVolUUID) dstVolume = sdCache.produce(dstSdUUID).produceVolume(dstImgUUID, dstVolUUID) Line 536: Line 537: if not srcVolume.isSparse() or not dstVolume.isSparse(): Line 538: raise se.VolumeNotSparse() Line 539: Line 546: 0, volume.COW_FORMAT, Line 547: volume.SPARSE_VOL, TEMP_DISK_TYPE, Line 548: tmpVolUUID, Line 549: 'Temporary Volume for virt-sparsify', Line 550: volume.BLANK_UUID, srcVolUUID) Creation of the temporary volume is engine-driven. Engine should send the createVolume(...) request in advance. So you have to remove this. Line 551: # volume.create with backing volume sets the backing volume as INTERNAL Line 552: sdCache.produce(srcSdUUID).getVolumeClass()(self.repoPath, srcSdUUID, Line 553: srcImgUUID, srcVolUUID Line 554: ).setLeaf() Line 550: volume.BLANK_UUID, srcVolUUID) Line 551: # volume.create with backing volume sets the backing volume as INTERNAL Line 552: sdCache.produce(srcSdUUID).getVolumeClass()(self.repoPath, srcSdUUID, Line 553: srcImgUUID, srcVolUUID Line 554: ).setLeaf() Remove this. Line 555: Line 556: # Activate temporary volume links Line 557: tmpVolume = self._activateVolume(tmpSdUUID, srcImgUUID, tmpVolUUID) Line 558: Line 553: srcImgUUID, srcVolUUID Line 554: ).setLeaf() Line 555: Line 556: # Activate temporary volume links Line 557: tmpVolume = self._activateVolume(tmpSdUUID, srcImgUUID, tmpVolUUID) This should be: baseVolume = sdCache.produce(srcSdUUID).produceVolume(srcImgUUID, srcVolume.getParent()) Line 558: Line 559: srcVolume.prepare(rw=False) Line 560: tmpVolume.prepare(rw=True, justme=True, setrw=True) Line 561: dstVolume.prepare(rw=True, justme=True, setrw=True) Line 557: tmpVolume = self._activateVolume(tmpSdUUID, srcImgUUID, tmpVolUUID) Line 558: Line 559: srcVolume.prepare(rw=False) Line 560: tmpVolume.prepare(rw=True, justme=True, setrw=True) Line 561: dstVolume.prepare(rw=True, justme=True, setrw=True) Lines 556-561 should be substituted just with: srcVolume.prepare() dstVolume.prepare() Line 562: Line 563: # Extend sizes Line 564: dstVolume.extend(srcVolume.getSize()) Line 565: tmpVolume.extend(srcVolume.getSize()) Line 561: dstVolume.prepare(rw=True, justme=True, setrw=True) Line 562: Line 563: # Extend sizes Line 564: dstVolume.extend(srcVolume.getSize()) Line 565: tmpVolume.extend(srcVolume.getSize()) This is: srcVolume.extend(srcVolume.getSize()) dstVolume.extend(srcVolume.getSize()) Line 566: Line 567: try: Line 568: self.log.info("start sparsify %s %s", dstImgUUID, dstVolUUID) Line 569: virtsparsify.sparsify(srcVolume.getVolumePath(), Line 567: try: Line 568: self.log.info("start sparsify %s %s", dstImgUUID, dstVolUUID) Line 569: virtsparsify.sparsify(srcVolume.getVolumePath(), Line 570: tmpVolume.getVolumePath(), Line 571: dstVolume.getVolumePath()) This is: virtsparsify.sparsify(baseVolume.getVolumePath(), srcVolume.getVolumePath(), dstVolume.getVolumePath()) Line 572: self.log.info("end sparsify %s %s", dstImgUUID, dstVolUUID) Line 573: Line 574: dstVolume.shrinkToOptimalSize() Line 575: dstVolume.syncMetadata() Line 571: dstVolume.getVolumePath()) Line 572: self.log.info("end sparsify %s %s", dstImgUUID, dstVolUUID) Line 573: Line 574: dstVolume.shrinkToOptimalSize() Line 575: dstVolume.syncMetadata() No need for this. Line 576: except Exception as e: Line 577: self.log.info("fail sparsify %s %s", dstImgUUID, dstVolUUID) Line 578: self.log.error(e, exc_info=True) Line 579: raise se.CannotSparsifyVolume(srcVolUUID) Line 574: dstVolume.shrinkToOptimalSize() Line 575: dstVolume.syncMetadata() Line 576: except Exception as e: Line 577: self.log.info("fail sparsify %s %s", dstImgUUID, dstVolUUID) Line 578: self.log.error(e, exc_info=True) Only: self.log.exception("fail sparsify %s %s", dstImgUUID, dstVolUUID) Line 579: raise se.CannotSparsifyVolume(srcVolUUID) Line 580: finally: Line 581: # Delete temporary volume Line 582: tmpVolume.delete(False, True) http://gerrit.ovirt.org/#/c/28328/6/vdsm/storage/sp.py File vdsm/storage/sp.py: Line 1548: imgUUID, rm.LockType.exclusive)): Line 1549: image.Image(self.poolPath).move(srcDomUUID, dstDomUUID, imgUUID, Line 1550: vmUUID, op, postZero, force) Line 1551: Line 1552: def sparsifyImage(self, srcSdUUID, srcImgUUID, srcVolUUID, tmpSdUUID, Remove the unneeded arguments. Line 1553: tmpImgUUID, tmpVolUUID, dstSdUUID, dstImgUUID, Line 1554: dstVolUUID): Line 1555: """ Line 1556: Reduce sparse image size by converting free space on image to free Line 1598: rm.LockType.exclusive)): Line 1599: # rmanager.acquireResource(tmpImageResourcesNamespace, dstImgUUID, Line 1600: # rm.LockType.exclusive)): Line 1601: image.Image(self.poolPath).sparsify( Line 1602: srcSdUUID, srcImgUUID, srcVolUUID, tmpSdUUID, tmpImgUUID, Remove the unneeded arguments. Line 1603: tmpVolUUID, dstSdUUID, dstImgUUID, dstVolUUID) Line 1604: Line 1605: def cloneImageStructure(self, sdUUID, imgUUID, dstSdUUID): Line 1606: """ -- 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: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Utkarsh Singh <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[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
