Federico Simoncelli has posted comments on this change. Change subject: sparsify: integrating virt-sparsify into vdsm ......................................................................
Patch Set 2: (9 comments) Overall it's good. Thanks Utkarsh! We'll have to do some minor changes here and there for the block storage domains but nothing major. Keep up the good work! http://gerrit.ovirt.org/#/c/28328/2//COMMIT_MSG Commit Message: Line 14: Since virt-sparsify included with CentOS does not allow in-place Line 15: sparsification, a temporary image file is created when sparsify is Line 16: being executed. If sparsification is successful, the temporary file is Line 17: swapped with the original image file. If the swapping is successful Line 18: the original image is removed. This won't work on block domains. I also prefer engine to orchestrate the flow: * engine sends a new createVolume to prepare a new volume as destination * engine sends sparsifyImage specifying source and (the newly created) destination (here we need few checks to make sure that destination is usable, e.g. correct format, etc.) * on completion engine sends (optionally) a deleteImage on the source In this way it's also possible to test the new image before deleting the source. Line 19: Line 20: If at any point the sparsify task fails, a rollback task has been Line 21: implemented which depending upon if a rollback is possible, will Line 22: attempt to restore the image to a state before attempting sparsify. http://gerrit.ovirt.org/#/c/28328/2/vdsm/BindingXMLRPC.py File vdsm/BindingXMLRPC.py: Line 669: op, postZero=False, force=False): Line 670: image = API.Image(imgUUID, spUUID, srcDomUUID) Line 671: return image.move(dstDomUUID, op, postZero, force) Line 672: Line 673: def imageSparsify(self, spUUID, sdUUID, imgUUID): The API should be documented in the json file. Given my previous comment on source/destination I think this will become something like: imageSparsify(self, spUUID, srcSdUUID, srcImgUUID, srcVolUUID, dstSdUUID, dstImgUUID, dstVolUUID) dstVolUUID could be optional but I'd prefer to make it explicit. Line 674: image = API.Image(imgUUID, spUUID, sdUUID) Line 675: return image.sparsify() Line 676: Line 677: def imageCloneStructure(self, spUUID, sdUUID, imgUUID, dstSdUUID): http://gerrit.ovirt.org/#/c/28328/2/vdsm/storage/hsm.py File vdsm/storage/hsm.py: Line 1596: Reduce sparse image size by converting free space on image to free Line 1597: space on storage domain using virt-sparsify. Line 1598: """ Line 1599: Line 1600: pool = self.getPool(spUUID) Shared lock on storage domain and then lock on image source and destination. Line 1601: self._spmSchedule(spUUID, "sparsifyImage", pool.sparsifyImage, sdUUID, Line 1602: imgUUID) Line 1603: Line 1604: @public http://gerrit.ovirt.org/#/c/28328/2/vdsm/storage/image.py File vdsm/storage/image.py: Line 521: Line 522: #TODO: self.isLegal(sdUUID, imgUUID)? Line 523: Line 524: chain = self.getChain(sdUUID, imgUUID) Line 525: leafVol = chain[-1] Making the volUUID explicit in the API will save us from doing the lookup. Line 526: Line 527: if not leafVol.isLeaf(): Line 528: self.log.error("Cannot determine leaf volume for image %s in " Line 529: "storage domain %s", imgUUID, sdUUID) http://gerrit.ovirt.org/#/c/28328/2/vdsm/storage/volume.py File vdsm/storage/volume.py: Line 583: Reduce sparse volume size by converting free space on volume to free Line 584: space on storage domain using virt-sparsify. Line 585: """ Line 586: if not self.isLeaf() or self.isShared(): Line 587: raise se.VolumeNonWritable(self.volUUID) Nice, we can get rid of this because the destination is another image and we are not risking to corrupt anything. Line 588: Line 589: volFormat = self.getFormat() Line 590: if volFormat != RAW_FORMAT and volFormat != COW_FORMAT: Line 591: raise se.IncorrectFormat(self.volUUID) Line 587: raise se.VolumeNonWritable(self.volUUID) Line 588: Line 589: volFormat = self.getFormat() Line 590: if volFormat != RAW_FORMAT and volFormat != COW_FORMAT: Line 591: raise se.IncorrectFormat(self.volUUID) This is overly pedantic, do we have this somewhere else? Line 592: Line 593: if not self.isSparse(): Line 594: raise se.VolumeNotSparse(self.volUUID) Line 595: Line 597: Line 598: path = self.getVolumePath() Line 599: Line 600: try: Line 601: vars.task.pushRecovery( I haven't looked into this but it seems to me that this will be redundant after the other changes (and it can be removed together with sparsifyRollback). Line 602: task.Recovery("Sparsify volume rollback: %s" % self.volUUID, Line 603: clsModule, clsName, "sparsifyRollback", [path]) Line 604: ) Line 605: Line 1054: """ Line 1055: pass Line 1056: Line 1057: Line 1058: def virtSparsify(vol, stop): Let's leave this here for now but I think that we'll have to move it before merging the patch. Line 1059: """ Line 1060: Sparsify the 'vol' volume using libguestfs virt-sparsify Line 1061: """ Line 1062: log.debug('(virtSparsify): virt-sparsify %s START' % vol) Line 1057: Line 1058: def virtSparsify(vol, stop): Line 1059: """ Line 1060: Sparsify the 'vol' volume using libguestfs virt-sparsify Line 1061: """ There's probably more to review in this function here but I'll leave it for the next round. Line 1062: log.debug('(virtSparsify): virt-sparsify %s START' % vol) Line 1063: Line 1064: vol_split = os.path.splitext(vol) Line 1065: dst_tmp = "%s_sparsify_tmp%s" % (vol_split[0], vol_split[1]) -- 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: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Utkarsh Singh <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[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
