Change in vdsm[master]: [WIP] Simplifiying hsm.copyImage logic.

Eduardo has uploaded a new change for review.

Change subject: [WIP] Simplifiying hsm.copyImage logic.
......................................................................

[WIP] Simplifiying hsm.copyImage logic.

Consider further optimization moving the
*.validateCreateVolumeParams() to the domain level.

Change-Id: I5db9053dabb97423611634e2bdfbdd09ec02876b
Signed-off-by: Eduardo 
---
M vdsm/storage/hsm.py
M vdsm/storage/sd.py
2 files changed, 14 insertions(+), 27 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/09/8509/1

diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index 2a8bc59..2c031f8 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -1386,35 +1386,29 @@
         Create new template/volume from VM.
         Do it by collapse and copy the whole chain (baseVolUUID->srcVolUUID)
         """
-        argsStr = "sdUUID=%s, spUUID=%s, vmUUID=%s, srcImgUUID=%s, srcVolUUID=%s, dstImgUUID=%s, "\
-                   "dstVolUUID=%s, description=%s, dstSdUUID=%s, volType=%s, volFormat=%s, "\
-                   "preallocate=%s force=%s, postZero=%s" % (sdUUID, spUUID, vmUUID,
-                   srcImgUUID, srcVolUUID, dstImgUUID, dstVolUUID, description,
-                   dstSdUUID, volType, volFormat, preallocate, force, postZero)
+        argsStr = str(locals())
         vars.task.setDefaultException(se.TemplateCreationError("%s" % argsStr))
+        if dstSdUUID == sd.BLANK_UUID:
+            dstSdUUID = sdUUID
+
+        if dstSdUUID != sdUUID:
+            domains = (sdUUID, dstSdUUID)
         # Validate imgUUID in case of copy inside source domain itself
-        if dstSdUUID in (sdUUID, sd.BLANK_UUID):
-            if srcImgUUID == dstImgUUID:
+        elif srcImgUUID == dstImgUUID:
                 raise se.InvalidParameterException("dstImgUUID", dstImgUUID)
+        else:
+            domains = (sdUUID)
+
         pool = self.getPool(spUUID)
-        self.validateSdUUID(sdUUID)
+        for dom in domains:
+            self.validateSdUUID(domains)

# Avoid VM copy if one of its volume (including template if exists) ILLEGAL/FAKE
         pool.validateVolumeChain(sdUUID, srcImgUUID)
         # Validate volume type and format
-        if dstSdUUID != sd.BLANK_UUID:
-            dom = dstSdUUID
-        else:
-            dom = sdUUID
-        sdCache.produce(dom).validateCreateVolumeParams(volFormat, preallocate, volume.BLANK_UUID)
+        sdCache.produce(dstSdUUID).validateCreateVolumeParams(volFormat, preallocate, volume.BLANK_UUID)

-        # If dstSdUUID defined, means we copy image to it
-        domains = [sdUUID]
-        if dstSdUUID not in [sdUUID, sd.BLANK_UUID]:
-            self.validateSdUUID(dstSdUUID)
-            domains.append(dstSdUUID)
-            domains.sort()
-
+        domains.sort()
         for dom in domains:
             vars.task.getSharedLock(STORAGE, dom)

diff --git a/vdsm/storage/sd.py b/vdsm/storage/sd.py
index fcb796c..9c7fd52 100644
--- a/vdsm/storage/sd.py
+++ b/vdsm/storage/sd.py
@@ -369,13 +369,6 @@
         """
         pass

-    @classmethod
-    def validateCreateVolumeParams(cls, volFormat, preallocate, srcVolUUID):
-        """
-        Validate create volume parameters
-        """
-        pass
-
     def createVolume(self, imgUUID, size, volFormat, preallocate, diskType,
                      volUUID, desc, srcImgUUID, srcVolUUID):
         """

--
To view, visit http://gerrit.ovirt.org/8509
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5db9053dabb97423611634e2bdfbdd09ec02876b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo

Ayal Baron has posted comments on this change.

Change subject: [WIP] Simplifiying hsm.copyImage logic.
......................................................................

Patch Set 1: (2 inline comments)

....................................................
File vdsm/storage/hsm.py
Line 1403:         for dom in domains:
Line 1404:             self.validateSdUUID(domains)
Line 1405: 
Line 1406:         # Avoid VM copy if one of its volume (including template if exists) ILLEGAL/FAKE
Line 1407:         pool.validateVolumeChain(sdUUID, srcImgUUID)
as far as I'm concerned we can also get rid of this validate, just copy the entire image (as a tree, not chain).
Line 1408:         # Validate volume type and format
Line 1409:         sdCache.produce(dstSdUUID).validateCreateVolumeParams(volFormat, preallocate, volume.BLANK_UUID)
Line 1410: 
Line 1411:         domains.sort()

Line 1405: 
Line 1406:         # Avoid VM copy if one of its volume (including template if exists) ILLEGAL/FAKE
Line 1407:         pool.validateVolumeChain(sdUUID, srcImgUUID)
Line 1408:         # Validate volume type and format
Line 1409:         sdCache.produce(dstSdUUID).validateCreateVolumeParams(volFormat, preallocate, volume.BLANK_UUID)
why produce again?
Line 1410: 
Line 1411:         domains.sort()
Line 1412:         for dom in domains:
Line 1413:             vars.task.getSharedLock(STORAGE, dom)

--
To view, visit http://gerrit.ovirt.org/8509
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5db9053dabb97423611634e2bdfbdd09ec02876b
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Ayal Baron

Ayal Baron has posted comments on this change.

Change subject: [WIP] Simplifiying hsm.copyImage logic.
......................................................................

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

(2 inline comments)

....................................................
File vdsm/storage/hsm.py
Line 1403:         for dom in domains:
Line 1404:             self.validateSdUUID(domains)
Line 1405: 
Line 1406:         # Avoid VM copy if one of its volume (including template if exists) ILLEGAL/FAKE
Line 1407:         pool.validateVolumeChain(sdUUID, srcImgUUID)
previous comment is still valid
Line 1408:         # Validate volume type and format
Line 1409:         sdCache.produce(dstSdUUID).validateCreateVolumeParams(volFormat, preallocate, volume.BLANK_UUID)
Line 1410: 
Line 1411:         domains.sort()

Line 1405: 
Line 1406:         # Avoid VM copy if one of its volume (including template if exists) ILLEGAL/FAKE
Line 1407:         pool.validateVolumeChain(sdUUID, srcImgUUID)
Line 1408:         # Validate volume type and format
Line 1409:         sdCache.produce(dstSdUUID).validateCreateVolumeParams(volFormat, preallocate, volume.BLANK_UUID)
previous comment is still valid
Line 1410: 
Line 1411:         domains.sort()
Line 1412:         for dom in domains:
Line 1413:             vars.task.getSharedLock(STORAGE, dom)

--
To view, visit http://gerrit.ovirt.org/8509
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5db9053dabb97423611634e2bdfbdd09ec02876b
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Eduardo 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Eduardo

Itamar Heim has posted comments on this change.

Change subject: [WIP] Simplifiying hsm.copyImage logic.
......................................................................

Patch Set 10:

pinf

Itamar Heim has abandoned this change.

Change subject: [WIP] Simplifiying hsm.copyImage logic.
......................................................................

Abandoned

abandoning - old. no reply. not touched for a while. please restore if relevant