Hello Ayal Baron,
I'd like you to do a code review. Please visit
http://gerrit.ovirt.org/9209
to review the following change.
Change subject: BZ#836161 - Changed code obsoleted by deleteImage() rewrite.
......................................................................
BZ#836161 - Changed code obsoleted by deleteImage() rewrite.
Change image.move and image.multipleMove to call the new
deleteImage implementation instead of image.delete
Change-Id: Icee221a3e8282194971afefd21540dc759a02aab
Signed-off-by: Eduardo <ewarszaw(a)redhat.com>
Reviewed-on:
https://gerrit.eng.lab.tlv.redhat.com/3141
Reviewed-by: Ayal Baron <abaron(a)redhat.com>
Tested-by: Daniel Paikov <dpaikov(a)redhat.com>
---
M vdsm/storage/image.py
M vdsm/storage/sp.py
2 files changed, 30 insertions(+), 83 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/09/9209/1
diff --git a/vdsm/storage/image.py b/vdsm/storage/image.py
index 4cd9785..73274fe 100644
--- a/vdsm/storage/image.py
+++ b/vdsm/storage/image.py
@@ -67,6 +67,19 @@
TEMPORARY_VOLUME_SIZE = 20480 # in sectors (10M)
+def _deleteImage(dom, imgUUID, postZero):
+ """This ancillary function will be removed.
+
+ Replaces Image.delete() in Image.[copy(), move(),multimove()].
+ """
+ allVols = dom.getAllVolumes()
+ imgVols = sd.getVolsOfImage(allVols, imgUUID)
+ if postZero:
+ dom.zeroImage(dom.sdUUID, imgUUID, imgVols)
+ else:
+ dom.deleteImage(dom.sdUUID, imgUUID, imgVols)
+
+
class Image:
""" Actually represents a whole virtual disk.
Consist from chain of volumes.
@@ -612,7 +625,7 @@
# in destination domain, if we got the overwrite command
if force:
self.log.info("delete image %s on domain %s before overwriting",
imgUUID, destDom.sdUUID)
- self.delete(destDom.sdUUID, imgUUID, postZero, force=True)
+ _deleteImage(destDom, imgUUID, postZero)
chains = self._createTargetImage(destDom, srcSdUUID, imgUUID)
self._interImagesCopy(destDom, srcSdUUID, imgUUID, chains)
@@ -627,7 +640,12 @@
vars.task.clearRecoveries()
# If it's 'move' operation, we should delete src image after copying
if op == MOVE_OP:
- self.delete(srcSdUUID, imgUUID, postZero, force=True)
+ try:
+ dom = sdCache.produce(srcSdUUID)
+ _deleteImage(dom, imgUUID, postZero)
+ except se.StorageException:
+ self.log.warning("Failed to remove img: %s from srcDom %s: "
+ "after it was copied to: %s", imgUUID, srcSdUUID,
dstSdUUID)
self.log.info("%s task on image %s was successfully finished",
OP_TYPES[op], imgUUID)
return True
@@ -676,9 +694,11 @@
"""
for imgUUID in imgList:
try:
- self.delete(sdUUID, imgUUID, postZero, force=True)
- except Exception:
- pass
+ dom = sdCache.produce(sdUUID)
+ _deleteImage(dom, imgUUID, postZero)
+ except se.StorageException:
+ self.log.warning("Delete image failed for image: %s in SD:
%s",
+ imgUUID, sdUUID, exc_info=True)
def multiMove(self, srcSdUUID, dstSdUUID, imgDict, vmUUID, force):
"""
@@ -706,9 +726,11 @@
# Remove images from source domain only after successfull copying of all images
to the destination domain
for (imgUUID, postZero) in imgDict.iteritems():
try:
- self.delete(srcSdUUID, imgUUID, postZero, force=True)
- except Exception:
- pass
+ dom = sdCache.produce(srcSdUUID)
+ _deleteImage(dom, imgUUID, postZero)
+ except se.StorageException:
+ self.log.warning("Delete image failed for image %s in SD: %s",
+ imgUUID, dom.sdUUID, exc_info=True)
def __cleanupCopy(self, srcVol, dstVol):
"""
diff --git a/vdsm/storage/sp.py b/vdsm/storage/sp.py
index c9489b8..fdd146c 100644
--- a/vdsm/storage/sp.py
+++ b/vdsm/storage/sp.py
@@ -1823,73 +1823,6 @@
image.Image(repoPath).multiMove(srcDomUUID, dstDomUUID, imgDict, vmUUID,
force)
- def deleteImage(self, sdUUID, imgUUID, postZero, force):
- """
- Deletes an Image folder with all its volumes.
-
- This function assumes that imgUUID is locked.
- In addition to removing image, this function also does the following:
- If removing a template from a backup SD which has dependent images:
- - creates a fake template.
- If removing the last image which depends on a fake template:
- - removes the fake template as well
- :param sdUUID: The UUID of the storage domain that contains the images.
- :type sdUUID: UUID
- :param imgUUID: The UUID of the image you want to delete.
- :type imgUUID: UUID
- :param postZero: bool
- :param force: Should the operation be forced.
- :type force: bool
- """
- # TODO: This function works on domains. No relation with pools.
- # Therefore move this to the relevant *sd module
- repoPath = os.path.join(self.storage_repository, self.spUUID)
- img = image.Image(repoPath)
- dom = sdCache.produce(sdUUID)
- allVols = dom.getAllVolumes()
- # Filter volumes related to this image
- imgsByVol = sd.getVolsOfImage(allVols, imgUUID)
- if all(len(v.imgs) == 1 for k, v in imgsByVol.iteritems()):
- # This is a self contained regular image, i.e. it's either an image
- # which is not based on a template or a template which has no
- # derived images, e.g. not derived from a template
- img.delete(sdUUID=sdUUID, imgUUID=imgUUID, postZero=postZero, force=force)
- else:
- # This is either a template with derived images or a derived image
- # so needs further scrutiny
- ts = tuple((volName, vol.imgs) for volName, vol in
- imgsByVol.iteritems() if len(vol.imgs) > 1)
- if len(ts) != 1:
- raise se.ImageValidationError("Image points to multiple"
- "templates %s in %s from %s" %
- (ts, imgsByVol, allVols))
- # TODO: Lock the template, reload allVols.
- # template = ts[0] = [(tName, tImgs)]
- tName, tImgs = ts[0]
- # getAllVolumes makes the template self img the 1st one in tImgs
- templateImage = tImgs[0]
- numOfDependentImgs = len(tImgs) - 1
- if templateImage != imgUUID:
- # Removing an image based on a template
- img.delete(sdUUID=sdUUID, imgUUID=imgUUID, postZero=postZero,
force=force)
- if numOfDependentImgs == 1 and dom.produceVolume(templateImage,
tName).isFake():
- # Remove the fake template since last consumer was removed
- img.delete(sdUUID=sdUUID, imgUUID=templateImage, postZero=False,
force=True)
-
- # Removing a template with dependencies
- elif force:
- img.delete(sdUUID=sdUUID, imgUUID=templateImage, postZero=postZero,
- force=force)
- elif not dom.isBackup():
- raise se.ImagesActionError("Can't remove template with children
%s" %
- allVols)
- else:
- # Removing a template with dependencies in backup domain
- # A fake template will be created
- img.delete(sdUUID=sdUUID, imgUUID=imgUUID, postZero=postZero,
force=True)
- tParams = dom.produceVolume(imgUUID, tName).getVolumeParams()
- img.createFakeTemplate(sdUUID=sdUUID, volParams=tParams)
-
def mergeSnapshots(self, sdUUID, vmUUID, imgUUID, ancestor, successor, postZero):
"""
Merges the source volume to the destination volume.
@@ -2065,14 +1998,6 @@
vol.teardown(sdUUID, volUUID)
except:
self.log.warning("SP %s SD %s img %s Vol %s - teardown
failed")
-
- def preDeleteRename(self, sdUUID, imgUUID):
- repoPath = os.path.join(self.storage_repository, self.spUUID)
- return image.Image(repoPath).preDeleteRename(sdUUID, imgUUID)
-
- def validateDelete(self, sdUUID, imgUUID):
- repoPath = os.path.join(self.storage_repository, self.spUUID)
- image.Image(repoPath).validateDelete(sdUUID, imgUUID)
def validateVolumeChain(self, sdUUID, imgUUID):
repoPath = os.path.join(self.storage_repository, self.spUUID)
--
To view, visit
http://gerrit.ovirt.org/9209
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icee221a3e8282194971afefd21540dc759a02aab
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo <ewarszaw(a)redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron(a)redhat.com>