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 <utkarshsins(a)gmail.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce(a)redhat.com>
Gerrit-Reviewer: automation(a)ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes