Change in vdsm[master]: storage: perform image deletion in task

laravot at redhat.com laravot at redhat.com
Thu Nov 12 09:30:05 UTC 2015


Liron Aravot has uploaded a new change for review.

Change subject: storage: perform image deletion in task
......................................................................

storage: perform image deletion in task

deleteImage() always schedules a task after it's synchronous part, the
task is currently used to wipe the image (if requested) and in case of
deletion without wipe it performs nothing.

The synchronous part of deleteImage() renames the image to clarify that
it's about to be deleted by adding a prefix and then delets the image.
On storage under heavy load any of those operations can be slow, as the
engine has a default timeout of 180s for vdsm operation on some cases we
might fail on timeout. In order to lower the chance to get that timeout
this patch moves the image deletion to occur always on the task (same as
happens today when wiping the image).

The assumption that the engine relies on currently is that when a call to
deleteImage() returns the image is no longer available and that it's id
is "free" to use (whether it was deleted on the synchronous part or a
wipe is ongoing), as we still rename the image in the synchronous part
this assumption is still valid and the engine shouldn't be affected by
it.

Change-Id: I1e61d38439192d193f3f806e08661b61a65e7836
Signed-off-by: Liron Aravot <laravot at redhat.com>
---
M vdsm/storage/blockSD.py
M vdsm/storage/fileSD.py
M vdsm/storage/hsm.py
M vdsm/storage/sd.py
M vdsm/storage/sp.py
5 files changed, 40 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/77/48477/1

diff --git a/vdsm/storage/blockSD.py b/vdsm/storage/blockSD.py
index 2621f74..f09af4a 100644
--- a/vdsm/storage/blockSD.py
+++ b/vdsm/storage/blockSD.py
@@ -637,9 +637,16 @@
             self.log.debug("removed image dir: %s", imgPath)
         return imgPath
 
-    def deleteImage(self, sdUUID, imgUUID, volsImgs):
-        toDel = self._getImgExclusiveVols(imgUUID, volsImgs)
+    def _getVolsForDeletion(self, imgUUID, volsImgs):
+        return self._getImgExclusiveVols(imgUUID, volsImgs)
+
+    def markImageForDeletion(self, sdUUID, imgUUID, volsImgs):
+        toDel = self._getVolsForDeletion(imgUUID, volsImgs)
         self._markForDelVols(sdUUID, imgUUID, toDel, sd.REMOVED_IMAGE_PREFIX)
+
+    def deleteImage(self, sdUUID, imgUUID, volsImgs):
+        self.log.debug("Romoving image %s", imgUUID)
+        toDel = self._getVolsForDeletion(imgUUID, volsImgs)
         deleteVolumes(sdUUID, toDel)
         self.rmDCImgDir(imgUUID, volsImgs)
 
diff --git a/vdsm/storage/fileSD.py b/vdsm/storage/fileSD.py
index dc4a68e..73749bc 100644
--- a/vdsm/storage/fileSD.py
+++ b/vdsm/storage/fileSD.py
@@ -201,16 +201,24 @@
         """
         return fileVolume.FileVolume
 
-    def deleteImage(self, sdUUID, imgUUID, volsImgs):
+    def _getImageDirs(self, imgUUID):
         currImgDir = self.getImagePath(imgUUID)
         dirName, baseName = os.path.split(currImgDir)
         toDelDir = os.path.join(dirName, sd.REMOVED_IMAGE_PREFIX + baseName)
+        return currImgDir, toDelDir
+
+    def markImageForDeletion(self, sdUUID, imgUUID, volsImgs):
+        currImgDir, toDelDir = self._getImageDirs(imgUUID)
         self.log.debug("Renaming dir %s to %s", currImgDir, toDelDir)
         try:
             self.oop.os.rename(currImgDir, toDelDir)
         except OSError as e:
             self.log.error("image: %s can't be moved", currImgDir)
             raise se.ImageDeleteError("%s %s" % (imgUUID, str(e)))
+
+    def deleteImage(self, sdUUID, imgUUID, volsImgs):
+        self.log.debug("Romoving image %s", imgUUID)
+        currImgDir, toDelDir = self._getImageDirs(imgUUID)
         for volUUID in volsImgs:
             volPath = os.path.join(toDelDir, volUUID)
             try:
diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index 23d325b..2fb2722 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -1533,7 +1533,8 @@
             if fakeTUUID:
                 tParams = dom.produceVolume(imgUUID, fakeTUUID).\
                     getVolumeParams()
-            pool.deleteImage(dom, imgUUID, volsByImg)
+
+            pool.markImageForDeletion(sdUUID, imgUUID, volsByImg)
             # This is a hack to keep the interface consistent
             # We currently have race conditions in delete image, to quickly fix
             # this we delete images in the "synchronous" state. This only works
@@ -1545,7 +1546,8 @@
                 img = image.Image(os.path.join(self.storage_repository,
                                                spUUID))
                 img.createFakeTemplate(sdUUID=sdUUID, volParams=tParams)
-            self._spmSchedule(spUUID, "deleteImage_%s" % imgUUID, lambda: True)
+            self._spmSchedule(spUUID, "deleteImage_%s" % imgUUID,
+                              pool.deleteImage, sdUUID, imgUUID, volsByImg)
 
     def validateImageMove(self, srcDom, dstDom, imgUUID):
         """
diff --git a/vdsm/storage/sd.py b/vdsm/storage/sd.py
index 661da0d..3585ed6 100644
--- a/vdsm/storage/sd.py
+++ b/vdsm/storage/sd.py
@@ -520,6 +520,9 @@
     def getVAllocSize(self, imgUUID, volUUID):
         return self._manifest.getVAllocSize(imgUUID, volUUID)
 
+    def markImageForDeletion(self, sdUUID, imgUUID, volsImgs):
+        self._manifest.markImageForDeletion(sdUUID, imgUUID, volsImgs)
+
     def deleteImage(self, sdUUID, imgUUID, volsImgs):
         self._manifest.deleteImage(sdUUID, imgUUID, volsImgs)
 
diff --git a/vdsm/storage/sp.py b/vdsm/storage/sp.py
index b8fd8f3..1e9ebeb 100644
--- a/vdsm/storage/sp.py
+++ b/vdsm/storage/sp.py
@@ -1899,17 +1899,24 @@
                 dom.produceVolume(imgUUID, volUUID).delete(
                     postZero=postZero, force=force)
 
-    def deleteImage(self, domain, imgUUID, volsByImg):
+    def deleteImage(self, sdUUID, imgUUID, volsByImg):
         """
         Deletes a given list of volumes belonging to imgUUID.
 
-        .. note::
-            This function cannot be scheduled as it takes the domain object
-            (for performance reasons) instead of the sdUUID.
+        :param domain: The UUID of the relevant domain containing the image.
+        :type sdUUID: UUID
+        :param imgUUID: The UUID of the relevant image.
+        :type imgUUID: UUID
+        :param volsByImg: List of the volumes to remove.
+        :type volsByImg: list
+        """
+        domain = sdCache.produce(sdUUID=sdUUID)
+        domain.deleteImage(sdUUID, imgUUID, volsByImg)
 
-            Few arguments could be evetually optimzed out and normalized but
-            it requires some refactoring.
-
+    def markImageForDeletion(self, domain, imgUUID, volsByImg):
+        """
+        Marks a given list of volumes belonging to imgUUID for deletion.
+.
         :param domain: The object of the domain containing the image.
         :type sdUUID: StorageDomain
         :param imgUUID: The UUID of the relevant image.
@@ -1917,7 +1924,7 @@
         :param volsByImg: List of the volumes to remove.
         :type volsByImg: list
         """
-        domain.deleteImage(domain.sdUUID, imgUUID, volsByImg)
+        domain.markImageForDeletion(domain.sdUUID, imgUUID, volsByImg)
 
     def setMaxHostID(self, spUUID, maxID):
         """


-- 
To view, visit https://gerrit.ovirt.org/48477
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I1e61d38439192d193f3f806e08661b61a65e7836
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <laravot at redhat.com>


More information about the vdsm-patches mailing list