[NEW PATCH] Change getChain() to return dictionary with chain itself and its template. (via gerrit-bot)

[Igor Lvovsky]

New patch submitted by Igor Lvovsky ([e-mail])

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

commit 3a09f874889cb372ee84b04a234f7b653458e26f
Author: Igor Lvovsky 
Date:   Tue Jul 12 15:18:45 2011 +0300

Change getChain() to return dictionary with chain itself and its template.

This change will avoid unneeded produceVolume and improve
    behaviour of different flows.

Change-Id: Ife46d592171aedea087e0d6462b735cd7e0d75e0

diff --git a/vdsm/storage/image.py b/vdsm/storage/image.py
index 1c70a34..102ea6c 100644
--- a/vdsm/storage/image.py
+++ b/vdsm/storage/image.py
@@ -104,7 +104,7 @@ class Image:
         # 1. Remove template's image:  Create 'fake' template instead of deleted one
         # 2. Remove regular image:  Remove parent-'fake' template if nobody need it already
         try:
-            pvol = self.getTemplate(sdUUID=sdUUID, imgUUID=imgUUID)
+            pvol = self.getChain(sdUUID, imgUUID)['template']
             # 1. If we required to delete template's image that have VMs
             # based on it, we should create similar 'fake' template instead
             if pvol:
@@ -269,17 +269,16 @@ class Image:

return newImgUUID

-    def __chainSizeCalc(self, sdUUID, imgUUID, volUUID, size):
+    def __chainSizeCalc(self, sdUUID, imgUUID, size):
         """
         Compute an estimate of the whole chain size
         using the sum of the actual size of the chain's volumes
         """
-        chain = self.getChain(sdUUID, imgUUID, volUUID)
+        chainDict = self.getChain(sdUUID, imgUUID)
         newsize = 0
-        template = chain[0].getParentVolume()
-        if template:
-            newsize = template.getVolumeSize()
-        for vol in chain:
+        if chainDict['template']:
+            newsize = chainDict['template'].getVolumeSize()
+        for vol in chainDict['chain']:
             newsize += vol.getVolumeSize()
         if newsize > size:
             newsize = size
@@ -301,12 +300,14 @@ class Image:
         newsize = int(newsize * 1.1)    # allocate %10 more for cow metadata
         return newsize

-    def getChain(self, sdUUID, imgUUID, volUUID=None):
+    def getChain(self, sdUUID, imgUUID):
         """
-        Return the chain of volumes of image as a sorted list
-        (not including a shared base (template) if any)
+        Return the dictionary with chain of volumes of image as a sorted list
+        (not including a shared base (template)) and the template if any
         """
-        chain = []
+        # 'chain' - chain of volumes of image as a sorted list
+        # 'template' - shared base (template) if any
+        chainDict = {'chain':[], 'template':None}
         # Find all volumes of image
         volclass = SDF.produce(sdUUID).getVolumeClass()
         uuidlist = volclass.getImageVolumes(self.repoPath, sdUUID, imgUUID)
@@ -316,14 +317,13 @@ class Image:
         srcVol = volclass(self.repoPath, sdUUID, imgUUID, uuidlist[0])
         # For template image include only one volume (template itself)
         if len(uuidlist) == 1 and srcVol.isShared():
-            return [srcVol]
+            return {'chain':[srcVol], 'template':srcVol}

# find the leaf
         for vol in uuidlist:
             srcVol = volclass(self.repoPath, sdUUID, imgUUID, vol)
             if srcVol.isLeaf():
-                if not volUUID or volUUID == srcVol.volUUID:
-                    break
+                break
             srcVol = None

if not srcVol:
@@ -332,38 +332,25 @@ class Image:

# Build up the sorted (parent->child) chain
         while not srcVol.isShared():
-            chain.insert(0, srcVol)
+            chainDict['chain'].insert(0, srcVol)
             if srcVol.getParent() == volume.BLANK_UUID:
                 break
             srcVol = srcVol.getParentVolume()

-        self.log.info("sdUUID=%s imgUUID=%s chain=%s ", sdUUID, imgUUID, str(chain))
-        return chain
+        if srcVol.isShared():
+            chainDict['template'] = srcVol

-    def getTemplate(self, sdUUID, imgUUID):
-        """
-        Return template of the image
-        """
-        tmpl = None
-        # Find all volumes of image (excluding template)
-        chain = self.getChain(sdUUID, imgUUID)
-        # check if the chain is build above a template, or it is a standalone
-        pvol = chain[0].getParentVolume()
-        if pvol:
-            tmpl = pvol
-        elif chain[0].isShared():
-            tmpl = chain[0]
-
-        return tmpl
+        self.log.info("sdUUID=%s imgUUID=%s chainDict=%s ", sdUUID, imgUUID, str(chainDict))
+        return chainDict

def validate(self, srcSdUUID, dstSdUUID, imgUUID, op=MOVE_OP):
         """
         Validate template on destination domain
         """
-        # Find all volumes of source image
-        chain = self.getChain(srcSdUUID, imgUUID)
-        leafVol = chain[-1]
         srcDom = SDF.produce(srcSdUUID)
+        # Find all volumes of source image
+        chainDict = self.getChain(srcSdUUID, imgUUID)
+        leafVol = chainDict['chain'][-1]
         # Avoid move template's image if there is a VM based on it (except 'Backup' domain)
         if op == MOVE_OP and leafVol.isShared() and not srcDom.isBackup():
             chList = leafVol.getAllChildrenList(self.repoPath, srcSdUUID, imgUUID, leafVol.volUUID)
@@ -371,18 +358,16 @@ class Image:
                 raise se.MoveTemplateImageError(imgUUID)

# check if the chain is build above a template, or it is a standalone
-        pvol = chain[0].getParentVolume()
+        pvol = chainDict['template']
         if pvol:    # this is a shared template based chain
-            if not pvol.isShared():
-                raise se.ImageIsNotLegalChain("Base image parent vol %s is not shared" % pvol.volUUID)
             pimg = pvol.getImage()      # pimg == template image
             try:
                 volclass = SDF.produce(dstSdUUID).getVolumeClass()
                 # Validate that the destination template exists and accessible
                 volclass(self.repoPath, dstSdUUID, pimg, pvol.volUUID)
-            except se.StorageException, e:
-                self.log.error("Unexpected error", exc_info=True)
-                raise se.CouldNotValideTemplateOnTargetDomain("Template %s Destination domain %s: %s" % (pimg, dstSdUUID, str(e)))
+            except se.StorageException:
+                self.log.error("Cannot validate template %s on target domain %s", pimg, dstSdUUID, exc_info=True)
+                raise se.CouldNotValideTemplateOnTargetDomain()

def __templateRelink(self, destDom, imgUUID, volUUID):
         """
@@ -473,20 +458,15 @@ class Image:
     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
-            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))
+
+        # Find all volumes of source image
+        srcChainDict = self.getChain(srcSdUUID, imgUUID)
+        srcChain = srcChainDict['chain']

fakeTemplate = False
         pimg = volume.BLANK_UUID    # standalone chain
         # check if the chain is build above a template, or it is a standalone
-        pvol = srcChain[0].getParentVolume()
+        pvol = srcChainDict['template']
         if pvol:
             # find out parent volume parameters
             volParams = pvol.getVolumeParams()
@@ -686,11 +666,9 @@ class Image:
         """
         if not self.isLegal(sdUUID, imgUUID):
             raise se.ImageIsNotLegalChain(imgUUID)
-        chain = self.getChain(sdUUID, imgUUID)
-        # check if the chain is build above a template, or it is a standalone
-        pvol = chain[0].getParentVolume()
-        if pvol:
-            if not pvol.isLegal() or pvol.isFake():
+        chainDict = self.getChain(sdUUID, imgUUID)
+        if chainDict['template']:
+            if not chainDict['template'].isLegal() or chainDict['template'].isFake():
                 raise se.ImageIsNotLegalChain(imgUUID)

def copy(self, sdUUID, vmUUID, srcImgUUID, srcVolUUID, dstImgUUID, dstVolUUID,
@@ -735,8 +713,7 @@ class Image:
                     # using the sum of the actual size of the chain's volumes
                     if volParams['volFormat'] != volume.COW_FORMAT or volParams['prealloc'] != volume.SPARSE_VOL:
                         raise se.IncorrectFormat(self)
-                    volParams['apparentsize'] = self.__chainSizeCalc(sdUUID, srcImgUUID,
-                                                                   srcVolUUID, volParams['size'])
+                    volParams['apparentsize'] = self.__chainSizeCalc(sdUUID, srcImgUUID, volParams['size'])

# Find out dest volume parameters
                 if preallocate in [volume.PREALLOCATED_VOL, volume.SPARSE_VOL]:
diff --git a/vdsm/storage/resourceFactories.py b/vdsm/storage/resourceFactories.py
index ce3ec70..07df0f0 100644
--- a/vdsm/storage/resourceFactories.py
+++ b/vdsm/storage/resourceFactories.py
@@ -106,20 +106,18 @@ class ImageResourceFactory(rm.SimpleResourceFactory):
         # Get the list of the volumes
         repoPath = os.path.join(self.storage_repository, dom.getPools()[0])
         try:
-            chain = image.Image(repoPath).getChain(sdUUID=self.sdUUID, imgUUID=resourceName)
+            chainDict = image.Image(repoPath).getChain(sdUUID=self.sdUUID, imgUUID=resourceName)
+            chain = chainDict['chain']
         except se.ImageDoesNotExistInSD:
             log.debug("Image %s does not exist in domain %s", resourceName, self.sdUUID)
             return []

-        # check if the chain is build above a template, or it is a standalone
-        pvol = chain[0].getParentVolume()
-        if pvol:
-            template = pvol.volUUID
-        elif chain[0].isShared():
-            # Image of template itself,
-            # with no other volumes in chain
-            template = chain[0].volUUID
-            del chain[:]
+        # Image of template itself,
+        # with no other volumes in chain
+        if chainDict['template']:
+            template = chainDict['template'].volUUID
+            if chainDict['template'] in chain:
+                del chain[:]

volUUIDChain = [vol.volUUID for vol in chain]
         volUUIDChain.sort()

Change in vdsm[master]: Change getChain() to return dictionary with chain itself and...

Saggi Mizrahi has posted comments on this change.

Change subject: Change getChain() to return dictionary with chain itself and its template.
......................................................................

Patch Set 1: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.usersys.redhat.com/692
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife46d592171aedea087e0d6462b735cd7e0d75e0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: Change getChain() to return dictionary with chain itself and...

Saggi Mizrahi has posted comments on this change.

Change subject: Change getChain() to return dictionary with chain itself and its template.
......................................................................

--
To view, visit http://gerrit.usersys.redhat.com/692
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife46d592171aedea087e0d6462b735cd7e0d75e0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: Change getChain() to return dictionary with chain itself and...

[David Naori]

David Naori has posted comments on this change.

Change subject: Change getChain() to return dictionary with chain itself and its template.
......................................................................

Patch Set 1: Verified

Tested basic operations. Verified

--
To view, visit http://gerrit.usersys.redhat.com/692
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife46d592171aedea087e0d6462b735cd7e0d75e0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: David Naori 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: Change getChain() to return dictionary with chain itself and...

Eduardo Warszawski has posted comments on this change.

Change subject: Change getChain() to return dictionary with chain itself and its template.
......................................................................

Patch Set 1: I would prefer that you didn't submit this

(2 inline comments)

....................................................
File vdsm/storage/image.py
Line 307:         """
Update doc.

Line 310:         chainDict = {'chain':[], 'template':None}
This is a dict, no need to remark it in the name.
Why a dict?
Most use cases need the template also.
IMHO is better to return the whole chain as an ordered list.
Consumer can do with the template the right thing.
Please consider receive/return volumes instead of UUIDs in order to avoid to re-produce many times the volumes.

--
To view, visit http://gerrit.usersys.redhat.com/692
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife46d592171aedea087e0d6462b735cd7e0d75e0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: David Naori 
Gerrit-Reviewer: Eduardo Warszawski 
Gerrit-Reviewer: Saggi Mizrahi

Change in vdsm[master]: Change getChain() to return dictionary with chain itself and...

[Igor Lvovsky]

Igor Lvovsky has abandoned this change.

Change subject: Change getChain() to return dictionary with chain itself and its template.
......................................................................

Patch Set 1: Abandoned

--
To view, visit http://gerrit.usersys.redhat.com/692
To unsubscribe, visit http://gerrit.usersys.redhat.com/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: Ife46d592171aedea087e0d6462b735cd7e0d75e0
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Igor Lvovsky 
Gerrit-Reviewer: Dan Kenigsberg 
Gerrit-Reviewer: David Naori 
Gerrit-Reviewer: Eduardo Warszawski 
Gerrit-Reviewer: Saggi Mizrahi