[NEW PATCH] BZ#712106 - Add lock for template during moveImage operation (via gerrit-bot)

[Igor Lvovsky]

New patch submitted by Igor Lvovsky (ilvovsky(a)redhat.com)

You can review this change at: http://gerrit.usersys.redhat.com/630

commit 74ec9aede8460a5cb69269ff5420bb183b3d3110
Author: Igor Lvovsky <ilvovsky(a)redhat.com&gt;
Date:   Tue Jun 21 13:52:20 2011 +0300

    BZ#712106 - Add lock for template during moveImage operation
    
    Change-Id: Ib87649cf0aebd5a35d56faa7e8f165d10d715331

diff --git a/vdsm/constants.py b/vdsm/constants.py
index ae9a79b..3a800a4 100644
--- a/vdsm/constants.py
+++ b/vdsm/constants.py
@@ -139,6 +139,11 @@ CMD_LOWPRIO = [EXT_NICE, '-n', '19', EXT_IONICE,
'-c', '2', '-n', '7']
 #
 # Storage constants
 #
+
+LVM_ACTIVATION_NAMESPACE = 'lvmActivationNS'
+IMAGE_NAMESPACE = 'imageNS'
+VOLUME_NAMESPACE = 'volumeNS'
+
 STRG_ISCSI_HOST = "iscsi_host/"
 STRG_SCSI_HOST = "scsi_host/"
 STRG_ISCSI_SESSION = "iscsi_session/"
diff --git a/vdsm/storage/blockSD.py b/vdsm/storage/blockSD.py
index 214e8a4..01b2182 100644
--- a/vdsm/storage/blockSD.py
+++ b/vdsm/storage/blockSD.py
@@ -27,7 +27,7 @@ import safelease
 import blockVolume
 import multipath
 import resourceFactories
-from resourceFactories import LVM_ACTIVATION_NAMESPACE
+from constants import LVM_ACTIVATION_NAMESPACE
 from persistentDict import PersistentDict, DictValidator
 import iscsi
 import storage_exception as se
diff --git a/vdsm/storage/blockVolume.py b/vdsm/storage/blockVolume.py
index ae411be..cebdd6b 100644
--- a/vdsm/storage/blockVolume.py
+++ b/vdsm/storage/blockVolume.py
@@ -24,7 +24,7 @@ import lvm
 import resourceManager as rm
 from threadLocal import vars
 from sdf import StorageDomainFactory as SDF
-from resourceFactories import LVM_ACTIVATION_NAMESPACE
+from constants import LVM_ACTIVATION_NAMESPACE
 import fileUtils
 
 TAG_PREFIX_MD = "MD_"
diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index 2aaf822..c00cb70 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -38,7 +38,7 @@ from threadLocal import vars
 import constants
 from storageConstants import STORAGE
 from task import Job
-from resourceFactories import IMAGE_NAMESPACE
+from constants import IMAGE_NAMESPACE
 import resourceManager as rm
 import devicemapper
 
diff --git a/vdsm/storage/image.py b/vdsm/storage/image.py
index 40a82c9..f655e44 100644
--- a/vdsm/storage/image.py
+++ b/vdsm/storage/image.py
@@ -22,6 +22,11 @@ from config import config
 import storage_exception as se
 import task
 from threadLocal import vars
+from constants import IMAGE_NAMESPACE
+import resourceManager as rm
+from contextlib import contextmanager
+
+rmanager = rm.ResourceManager.getInstance()
 
 # Disk type
 UNKNOWN_DISK_TYPE = 0
@@ -315,7 +320,8 @@ class Image:
             srcVol = None
 
         if not srcVol:
-            raise se.ImageIsNotLegalChain("Image %s is not a leaf" % imgUUID)
+            self.log.error("There is no leaf in the image %s" % imgUUID)
+            raise se.ImageIsNotLegalChain(imgUUID)
 
         # Build up the sorted (parent->child) chain
         while not srcVol.isShared():
@@ -450,48 +456,29 @@ class Image:
         """
         try:
             if srcVol:
-                srcVol.teardown(sdUUID=srcVol.sdUUID, volUUID=srcVol.volUUID,
justme=True)
+                srcVol.teardown(sdUUID=srcVol.sdUUID, volUUID=srcVol.volUUID)
             if dstVol:
-                dstVol.teardown(sdUUID=dstVol.sdUUID, volUUID=dstVol.volUUID,
justme=True)
+                dstVol.teardown(sdUUID=dstVol.sdUUID, volUUID=dstVol.volUUID)
         except Exception:
             self.log.error("Unexpected error", exc_info=True)
 
-    def move(self, srcSdUUID, dstSdUUID, imgUUID, vmUUID, op, postZero, force):
-        """
-        Move/Copy image between storage domains within same storage pool
-        """
-        self.log.info("srcSdUUID=%s dstSdUUID=%s "\
-            "imgUUID=%s vmUUID=%s op=%s force=%s postZero=%s",
-            srcSdUUID, dstSdUUID, imgUUID, vmUUID, OP_TYPES[op], str(force),
str(postZero))
-
-        srcVol = leafVol = dstVol = None
+    def _createTargetImage(self, destDom, srcSdUUID, imgUUID):
+        # Before actual data copying we need perform several operation
+        # such as: create all volumes, create fake template if needed, ...
         try:
             # Find all volumes of source image
-            chain = self.getChain(srcSdUUID, imgUUID)
-            leafVol = chain[-1]
-        except se.StorageException, e:
+            srcChain = self.getChain(srcSdUUID, imgUUID)
+        except se.StorageException:
             self.log.error("Unexpected error", exc_info=True)
             raise
         except Exception, e:
             self.log.error("Unexpected error", exc_info=True)
             raise se.SourceImageActionError(imgUUID, srcSdUUID, str(e))
 
-        destDom = SDF.produce(dstSdUUID)
+        fakeTemplate = False
         pimg = volume.BLANK_UUID    # standalone chain
-        # If image already exists check whether it illegal/fake, overwrite it
-        if not self.isLegal(dstSdUUID, imgUUID):
-            force = True
-        # We must first remove the previous instance of image (if exists)
-        # in destination domain, if we got the overwrite command
-        if force:
-            try:
-                self.log.info("delete image %s on domain %s before
overwriting", imgUUID, dstSdUUID)
-                self.delete(dstSdUUID, imgUUID, postZero, force=True)
-            except Exception, e:
-                self.log.error("Unexpected error", exc_info=True)
-                raise e
         # check if the chain is build above a template, or it is a standalone
-        pvol = chain[0].getParentVolume()
+        pvol = srcChain[0].getParentVolume()
         if pvol:
             # find out parent volume parameters
             volParams = pvol.getVolumeParams()
@@ -500,81 +487,145 @@ class Image:
                 # FIXME: This workaround help as copy VM to the backup domain without its
template
                 # We will create fake template for future VM creation and mark it as FAKE
volume
                 # This situation is relevant for backup domain only
-                self.createFakeTemplate(dstSdUUID, volParams)
+                fakeTemplate = True
+
+        @contextmanager
+        def justLogIt(pimg):
+            self.log.debug("You don't really need lock for image %s",
pimg)
+            yield
+
+        dstTmplResourcesNamespace = sd.getNamespace(destDom.sdUUID, IMAGE_NAMESPACE)
+        # In destination domain we need to lock image's template if exists
+        with rmanager.acquireResource(dstTmplResourcesNamespace, pimg,
rm.LockType.shared) \
+                        if pimg != volume.BLANK_UUID else justLogIt(pimg):
+            if fakeTemplate:
+                self.createFakeTemplate(destDom.sdUUID, volParams)
+
+            dstChain = []
+            for srcVol in srcChain:
+                # Create the dst volume
+                try:
+                    # find out src volume parameters
+                    volParams = srcVol.getVolumeParams(bs=1)
+
+                    # To avoid 'prezeroing' preallocated volume on NFS domain,
+                    # we create the target volume with minimal size and after that
w'll change
+                    # its metadata back to the original size.
+                    tmpSize = 20480 # in sectors (10M)
+                    destDom.createVolume(imgUUID=imgUUID, size=tmpSize,
+                                         volFormat=volParams['volFormat'],
preallocate=volParams['prealloc'],
+                                         diskType=volParams['disktype'],
volUUID=srcVol.volUUID, desc=volParams['descr'],
+                                         srcImgUUID=pimg,
srcVolUUID=volParams['parent'])
+                    dstVol = destDom.produceVolume(imgUUID=imgUUID,
volUUID=srcVol.volUUID)
+                    # Extend volume (for LV only) size to the actual size
+                    dstVol.extend((volParams['apparentsize'] + 511) / 512)
+                    # Change destination volume metadata back to the original size.
+                    dstVol.setSize(volParams['size'])
+                    dstChain.append(dstVol)
+                except se.StorageException:
+                    self.log.error("Unexpected error", exc_info=True)
+                    raise
+                except Exception, e:
+                    self.log.error("Unexpected error", exc_info=True)
+                    raise se.DestImageActionError(imgUUID, destDom.sdUUID, str(e))
 
-        for srcVol in chain:
-            # Create the dst volume
-            try:
-                # find out src volume parameters
-                volParams = srcVol.getVolumeParams(bs=1)
+                # only base may have a different parent image
+                pimg = imgUUID
 
-                # To avoid 'prezeroing' preallocated volume on NFS domain,
-                # we create the target volume with minimal size and after that w'll
change
-                # its metadata back to the original size.
-                tmpSize = 20480 # in sectors (10M)
-                destDom.createVolume(imgUUID=imgUUID, size=tmpSize,
-                                     volFormat=volParams['volFormat'],
preallocate=volParams['prealloc'],
-                                     diskType=volParams['disktype'],
volUUID=srcVol.volUUID, desc=volParams['descr'],
-                                     srcImgUUID=pimg,
srcVolUUID=volParams['parent'])
-                # Extend volume (for LV only) size to the actual size
-                dstVol = destDom.produceVolume(imgUUID=imgUUID, volUUID=srcVol.volUUID)
-                dstVol.extend((volParams['apparentsize'] + 511) / 512)
-                # Change destination volume metadata back to the original size.
-                dstVol.setSize(volParams['size'])
-            except se.StorageException, e:
-                self.log.error("Unexpected error", exc_info=True)
-                raise
-            except Exception, e:
-                self.log.error("Unexpected error", exc_info=True)
-                raise se.DestImageActionError(imgUUID, dstSdUUID, str(e))
+        return {'srcChain':srcChain, 'dstChain':dstChain}
 
-            # Do the actual copy
-            try:
+    def _interImagesCopy(self, destDom, srcSdUUID, imgUUID, chains):
+        srcLeafVol = chains['srcChain'][-1]
+        dstLeafVol = chains['dstChain'][-1]
+        try:
+            # Prepare the whole chains before the copy
+            srcLeafVol.prepare(rw=False)
+            dstLeafVol.prepare(rw=True, chainrw=True, setrw=True)
+        except Exception, e:
+            self.log.error("Unexpected error", exc_info=True)
+            # teardown volumes
+            self.__cleanupMove(srcLeafVol, dstLeafVol)
+            raise
+
+        try:
+            for srcVol in chains['srcChain']:
+                # Do the actual copy
                 try:
+                    dstVol = destDom.produceVolume(imgUUID=imgUUID,
volUUID=srcVol.volUUID)
                     srcSize = srcVol.getVolumeSize(bs=1)
-                    # Prepare relevant volumes before copy and teardown after that
-                    srcVol.prepare(rw=False, justme=True)
-                    dstVol.prepare(rw=True, justme=True, setrw=True)
                     misc.ddWatchCopy(srcVol.getVolumePath(), dstVol.getVolumePath(),
vars.task.aborting, self.idle, size=srcSize)
-                    # In case of copying template, we should set the destination volume
-                    #  as SHARED (after copy because otherwise prepare as RW would fail)
-                    if srcVol.isShared():
-                        dstVol.setShared()
-                    elif srcVol.isInternal():
-                        dstVol.setInternal()
-                except se.ActionStopped, e:
-                    raise e
-                except se.StorageException, e:
+                except se.ActionStopped:
+                    raise
+                except se.StorageException:
                     self.log.error("Unexpected error", exc_info=True)
                     raise
                 except Exception, e:
                     self.log.error("Unexpected error", exc_info=True)
-                    raise se.CopyImageError("image=%s, src domain=%s, dst domain=%s:
msg %s" % (imgUUID, srcSdUUID, dstSdUUID, str(e)))
-            finally:
-                # teardown volumes
-                self.__cleanupMove(srcVol, dstVol)
-                srcVol = dstVol = None
+                    raise se.CopyImageError("image=%s, src domain=%s, dst domain=%s:
msg %s" % (imgUUID, srcSdUUID, destDom.sdUUID, str(e)))
+        finally:
+            # teardown volumes
+            self.__cleanupMove(srcLeafVol, dstLeafVol)
 
-            # only base may have a different parent image
-            pimg = imgUUID
+    def _finalizeDestinationImage(self, destDom, imgUUID, chains, force):
+        for srcVol in chains['srcChain']:
+            try:
+                dstVol = destDom.produceVolume(imgUUID=imgUUID, volUUID=srcVol.volUUID)
+                # In case of copying template, we should set the destination volume
+                #  as SHARED (after copy because otherwise prepare as RW would fail)
+                if srcVol.isShared():
+                    dstVol.setShared()
+                elif srcVol.isInternal():
+                    dstVol.setInternal()
+            except se.StorageException:
+                self.log.error("Unexpected error", exc_info=True)
+                raise
+            except Exception, e:
+                self.log.error("Unexpected error", exc_info=True)
+                raise se.DestImageActionError(imgUUID, destDom.sdUUID, str(e))
 
         if force:
+            leafVol = chains['dstChain'][-1]
             # Now we should re-link all deleted hardlinks, if exists
-            self.__templateRelink(dstSdUUID, imgUUID, leafVol.volUUID)
+            self.__templateRelink(destDom.sdUUID, imgUUID, leafVol.volUUID)
 
         # At this point we successfully finished the 'copy' part of the
operation
         # and we can clear all recoveries.
         vars.task.clearRecoveries()
+
+    def _removeImage(self, sdUUID, imgUUID, postZero, force):
+        try:
+            self.delete(sdUUID, imgUUID, postZero, force)
+        except se.StorageException:
+            self.log.error("Unexpected error", exc_info=True)
+            raise
+        except Exception, e:
+            self.log.error("Unexpected error", exc_info=True)
+            raise se.ImageDeleteError("%s: %s" % (imgUUID), str(e))
+
+    def move(self, srcSdUUID, dstSdUUID, imgUUID, vmUUID, op, postZero, force):
+        """
+        Move/Copy image between storage domains within same storage pool
+        """
+        self.log.info("srcSdUUID=%s dstSdUUID=%s "\
+            "imgUUID=%s vmUUID=%s op=%s force=%s postZero=%s",
+            srcSdUUID, dstSdUUID, imgUUID, vmUUID, OP_TYPES[op], str(force),
str(postZero))
+
+        destDom = SDF.produce(dstSdUUID)
+        # If image already exists check whether it illegal/fake, overwrite it
+        if not self.isLegal(destDom.sdUUID, imgUUID):
+            force = True
+        # We must first remove the previous instance of image (if exists)
+        # 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._removeImage(destDom.sdUUID, imgUUID, postZero, force=True)
+
+        chains = self._createTargetImage(destDom, srcSdUUID, imgUUID)
+        self._interImagesCopy(destDom, srcSdUUID, imgUUID, chains)
+        self._finalizeDestinationImage(destDom, imgUUID, chains, force)
         # If it's 'move' operation, we should delete src image after copying
         if op == MOVE_OP:
-            try:
-                self.delete(srcSdUUID, imgUUID, postZero, force=True)
-            except se.StorageException, e:
-                self.log.error("Unexpected error", exc_info=True)
-                raise
-            except Exception, e:
-                self.log.error("Unexpected error", exc_info=True)
-                raise se.ImageDeleteError("%s: %s" % (imgUUID), str(e))
+            self._removeImage(srcSdUUID, imgUUID, postZero, force=True)
 
         self.log.info("%s task on image %s was successfully finished",
OP_TYPES[op], imgUUID)
         return True
diff --git a/vdsm/storage/resourceFactories.py b/vdsm/storage/resourceFactories.py
index ce3ec70..4836217 100644
--- a/vdsm/storage/resourceFactories.py
+++ b/vdsm/storage/resourceFactories.py
@@ -16,10 +16,7 @@ import storage_exception as se
 from sdf import StorageDomainFactory as SDF
 import sd
 import image
-
-LVM_ACTIVATION_NAMESPACE = 'lvmActivationNS'
-IMAGE_NAMESPACE = 'imageNS'
-VOLUME_NAMESPACE = 'volumeNS'
+from constants import VOLUME_NAMESPACE
 
 rmanager = rm.ResourceManager.getInstance()
 
diff --git a/vdsm/storage/sd.py b/vdsm/storage/sd.py
index 325da5d..4cdaef9 100644
--- a/vdsm/storage/sd.py
+++ b/vdsm/storage/sd.py
@@ -17,7 +17,7 @@ from glob import glob
 import storage_exception as se
 import misc
 import resourceFactories
-from resourceFactories import IMAGE_NAMESPACE, VOLUME_NAMESPACE
+from constants import IMAGE_NAMESPACE, VOLUME_NAMESPACE
 import resourceManager as rm
 import constants
 import safelease
diff --git a/vdsm/storage/spm.py b/vdsm/storage/spm.py
index b186d39..6ed480b 100644
--- a/vdsm/storage/spm.py
+++ b/vdsm/storage/spm.py
@@ -36,7 +36,7 @@ import logging
 import storage_exception as se
 from threadLocal import vars
 from storageConstants import STORAGE
-from resourceFactories import IMAGE_NAMESPACE
+from constants import IMAGE_NAMESPACE
 import resourceManager as rm
 from contextlib import nested
 import fileUtils
diff --git a/vdsm/storage/storage_exception.py b/vdsm/storage/storage_exception.py
index b1cbcdf..c9df109 100644
--- a/vdsm/storage/storage_exception.py
+++ b/vdsm/storage/storage_exception.py
@@ -274,7 +274,7 @@ class ImageValidationError(StorageException):
 
 class ImageDeleteError(StorageException):
     code = 256
-    message = "Could not remove all files"
+    message = "Could not remove all image's volumes"
 
 #REMOVED in 2.3.
 #This class was not in use in 2.2 nor in 2.3.
diff --git a/vdsm/storage/volume.py b/vdsm/storage/volume.py
index 123ed37..0052e84 100644
--- a/vdsm/storage/volume.py
+++ b/vdsm/storage/volume.py
@@ -21,8 +21,9 @@ import misc
 import fileUtils
 import task
 from threadLocal import vars
-import resourceFactories
 import resourceManager as rm
+from constants import IMAGE_NAMESPACE
+
 rmanager = rm.ResourceManager.getInstance()
 
 
@@ -150,7 +151,7 @@ class Volume:
         cls.log.info("sdUUID=%s srcImg=%s srcVol=%s dstFormat=%s
srcParent=%s",
                      sdUUID, srcImg, srcVol, dstFormat, srcParent)
 
-        imageResourcesNamespace = sd.getNamespace(sdUUID,
resourceFactories.IMAGE_NAMESPACE)
+        imageResourcesNamespace = sd.getNamespace(sdUUID, IMAGE_NAMESPACE)
         with rmanager.acquireResource(imageResourcesNamespace, srcImg,
rm.LockType.exclusive):
             try:
                 vol = SDF.produce(sdUUID).produceVolume(imgUUID=srcImg, volUUID=srcVol)