Change in vdsm[master]: image: copying a template is always allowed

[Federico Simoncelli]

Federico Simoncelli has uploaded a new change for review.

Change subject: image: copying a template is always allowed
......................................................................

image: copying a template is always allowed

To copy a template from a domain to another is a safe action even if
it has images that are based on it. This patch relaxes the check during
moveImage if the operation is a copy.

Signed-off-by: Federico Simoncelli 
Change-Id: I9e07d569eec02ac4dcd386a7c576342f640ec242
---
M vdsm/storage/hsm.py
1 file changed, 19 insertions(+), 11 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/08/8408/1

diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index 50d6ce1..730ee03 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -1288,32 +1288,39 @@
             self._spmSchedule(spUUID, "deleteImage", lambda : True)

-    def validateImageMove(self, srcDom, dstDom, imgUUID):
+    def validateImageCopy(self, srcDom, dstDom, imgUUID, safeToMove=False):
         """
-        Determines if the image move is legal.
+        Determines if it is possible to copy (or move) the image.

-        Moving an image based on a template to a data domain is only allowed if
-        the template exists on the target domain.
-        Moving a template from a data domain is only allowed if there are no
-        images based on it in the source data domain.
+        Copying or moving an image based on a template to a data domain is
+        allowed only if the template already exists on the target domain.
+        When the option safeToMove (default: False) is active (True) an
+        extra check makes sure that if the image is a template (on a data
+        domain) then there are no other images based on it and therefore it
+        is safe to remove it.
         """
         srcAllVols = srcDom.getAllVolumes()
         dstAllVols = dstDom.getAllVolumes()

# Filter volumes related to this image
         srcVolsImgs = sd.getVolsOfImage(srcAllVols, imgUUID)
+
         # Find the template
         for volName, imgsPar in srcVolsImgs.iteritems():
             if len(imgsPar.imgs) > 1:
                 # This is the template. Should be only one.
                 tName, tImgs = volName, imgsPar.imgs
+
                 # Template self image is the 1st entry
                 if imgUUID != tImgs[0] and tName not in dstAllVols.keys():
-                    self.log.error("img %s can't be moved to dom %s because "
-                    "template %s is absent on it", imgUUID, dstDom.sdUUID, tName)
+                    self.log.error("Cannot move image %s to domain %s "
+                            "because the template %s is missing on the "
+                            "destination", imgUUID, dstDom.sdUUID, tName)
                     raise se.ImageDoesNotExistInSD(imgUUID, dstDom.sdUUID)
-                elif imgUUID == tImgs[0] and not srcDom.isBackup():
+                elif (safeToMove and imgUUID == tImgs[0]
+                                     and not srcDom.isBackup()):
                     raise se.MoveTemplateImageError(imgUUID)
+
                 break

return True
@@ -1333,7 +1340,8 @@
         srcDom = self.validateSdUUID(srcDomUUID)
         dstDom = self.validateSdUUID(dstDomUUID)
         pool = self.getPool(spUUID) #Validates that the pool is connected. WHY?
-        self.validateImageMove(srcDom, dstDom, imgUUID)
+        self.validateImageCopy(srcDom, dstDom, imgUUID,
+                               safeToMove=(op != image.COPY_OP))
         domains = [srcDomUUID, dstDomUUID]
         domains.sort()

@@ -1364,7 +1372,7 @@
         images = {}
         for (imgUUID, pZero) in imgDict.iteritems():
             images[imgUUID.strip()] = misc.parseBool(pZero)
-            self.validateImageMove(srcDom, dstDom, imgUUID)
+            self.validateImageCopy(srcDom, dstDom, imgUUID, safeToMove=True)

domains = sorted([srcDomUUID, dstDomUUID])
         for dom in domains:

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I9e07d569eec02ac4dcd386a7c576342f640ec242
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: image: copying a template is always allowed
......................................................................

Patch Set 1: (1 inline comment)

....................................................
File vdsm/storage/hsm.py
Line 1316:                     self.log.error("Cannot move image %s to domain %s "
Line 1317:                             "because the template %s is missing on the "
Line 1318:                             "destination", imgUUID, dstDom.sdUUID, tName)
Line 1319:                     raise se.ImageDoesNotExistInSD(imgUUID, dstDom.sdUUID)
Line 1320:                 elif (safeToMove and imgUUID == tImgs[0]
To keep this method optimized (few heavy calls and one cycle) we should accept to have this additional flag here (instead of probably two cleaner methods validateImageMove and validateImageCopy). Eventually I could make those two as frontend for this (making it private).
Line 1321:                                      and not srcDom.isBackup()):
Line 1322:                     raise se.MoveTemplateImageError(imgUUID)
Line 1323: 
Line 1324:                 break

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9e07d569eec02ac4dcd386a7c576342f640ec242
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Federico Simoncelli

Ayal Baron has posted comments on this change.

Change subject: image: copying a template is always allowed
......................................................................

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

(1 inline comment)

....................................................
File vdsm/storage/hsm.py
Line 1316:                     self.log.error("Cannot move image %s to domain %s "
Line 1317:                             "because the template %s is missing on the "
Line 1318:                             "destination", imgUUID, dstDom.sdUUID, tName)
Line 1319:                     raise se.ImageDoesNotExistInSD(imgUUID, dstDom.sdUUID)
Line 1320:                 elif (safeToMove and imgUUID == tImgs[0]
Actually after reviewing the method I think we should just remove the entire thing.
i.e. get rid of validateImageCopy altogether.
I mean, what's the point? we allow having images without the template on the export domain, why not allow it on the data domain? engine does not permit this today anyway and on vdsm side this would remove 1 of 2 differences we have between data domain and export domain (on the way to getting rid of the export domain).
The second difference being that not all data domains can contain OVF files, which we should address, but not relevant to this patch.
Line 1321:                                      and not srcDom.isBackup()):
Line 1322:                     raise se.MoveTemplateImageError(imgUUID)
Line 1323: 
Line 1324:                 break

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

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

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: image: copying a template is always allowed
......................................................................

Patch Set 1: (1 inline comment)

....................................................
File vdsm/storage/hsm.py
Line 1316:                     self.log.error("Cannot move image %s to domain %s "
Line 1317:                             "because the template %s is missing on the "
Line 1318:                             "destination", imgUUID, dstDom.sdUUID, tName)
Line 1319:                     raise se.ImageDoesNotExistInSD(imgUUID, dstDom.sdUUID)
Line 1320:                 elif (safeToMove and imgUUID == tImgs[0]
I agree on removing this validation, we shouldn't miss this opportunity to remove an extra validation (that we hate so much) which is even preventing us from addressing more use cases.
Line 1321:                                      and not srcDom.isBackup()):
Line 1322:                     raise se.MoveTemplateImageError(imgUUID)
Line 1323: 
Line 1324:                 break

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

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

Shu Ming has posted comments on this change.

Change subject: image: copying a template is always allowed
......................................................................

Patch Set 1: (1 inline comment)

The comments are quite confusing to me.

....................................................
File vdsm/storage/hsm.py
Line 1296:         allowed only if the template already exists on the target domain.
Line 1297:         When the option safeToMove (default: False) is active (True) an
Line 1298:         extra check makes sure that if the image is a template (on a data
Line 1299:         domain) then there are no other images based on it and therefore it
Line 1300:         is safe to remove it.
I think it would be better to explain the two cases more clearly. Like, case I:  coping or moving an image based on a template
case II: moving an template, an extra check make sure that ...
Line 1301:         """
Line 1302:         srcAllVols = srcDom.getAllVolumes()
Line 1303:         dstAllVols = dstDom.getAllVolumes()
Line 1304:

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9e07d569eec02ac4dcd386a7c576342f640ec242
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming

Shu Ming has posted comments on this change.

Change subject: image: copying a template is always allowed
......................................................................

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9e07d569eec02ac4dcd386a7c576342f640ec242
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming

Eduardo has posted comments on this change.

Change subject: image: copying a template is always allowed
......................................................................

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

(1 inline comment)

....................................................
File vdsm/storage/hsm.py
Line 1317:                             "because the template %s is missing on the "
Line 1318:                             "destination", imgUUID, dstDom.sdUUID, tName)
Line 1319:                     raise se.ImageDoesNotExistInSD(imgUUID, dstDom.sdUUID)
Line 1320:                 elif (safeToMove and imgUUID == tImgs[0]
Line 1321:                                      and not srcDom.isBackup()):
1) This method have at maximum two IO calls. glob or vgs according to file or block based storage.

2) When copying a template the hsm.copyImage() should be called instead of calling hsm.moveImage(op="copy"). Therefore this validation is not called.
This function was written to preserve all the old (broken) semantics of VDSM.
Copying a template to another data SD was verified (succeed) with the actual code, since hsm.copy image is called.

3) Allowing broken images in a regular (data) SD is a very deep change that breaks the fact that (actually) PDIV images are totally contained in a data SD.
In spite that this change is desirable, can't and shouldn't be done only here.
Line 1322:                     raise se.MoveTemplateImageError(imgUUID)
Line 1323: 
Line 1324:                 break
Line 1325:

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9e07d569eec02ac4dcd386a7c576342f640ec242
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming

Ayal Baron has posted comments on this change.

Change subject: image: copying a template is always allowed
......................................................................

Patch Set 1: (2 inline comments)

....................................................
File vdsm/storage/hsm.py
Line 1287:             # an eliminate all race conditions
Line 1288:             self._spmSchedule(spUUID, "deleteImage", lambda : True)
Line 1289: 
Line 1290: 
Line 1291:     def validateImageCopy(self, srcDom, dstDom, imgUUID, safeToMove=False):
'safeToMove' doesn't mean anything and doesn't really belong here.
The copy should succeed and the delete should fail.
There should be no 'move' operation to begin with.
Line 1292:         """
Line 1293:         Determines if it is possible to copy (or move) the image.
Line 1294: 
Line 1295:         Copying or moving an image based on a template to a data domain is

Line 1317:                             "because the template %s is missing on the "
Line 1318:                             "destination", imgUUID, dstDom.sdUUID, tName)
Line 1319:                     raise se.ImageDoesNotExistInSD(imgUUID, dstDom.sdUUID)
Line 1320:                 elif (safeToMove and imgUUID == tImgs[0]
Line 1321:                                      and not srcDom.isBackup()):
this is just a reply to my comment, not about the patch itself.
anyway, I'm ok with keeping the validation in light of concerns of regressions and flows going awry.
Line 1322:                     raise se.MoveTemplateImageError(imgUUID)
Line 1323: 
Line 1324:                 break
Line 1325:

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9e07d569eec02ac4dcd386a7c576342f640ec242
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming

Ayal Baron has posted comments on this change.

Change subject: image: copying a template is always allowed
......................................................................

Patch Set 2: (3 inline comments)

....................................................
File vdsm/storage/hsm.py
Line 1298: 
Line 1299:         Copying an image based on a template to a data domain is allowed only
Line 1300:         if the template already exists on the target domain.
Line 1301:         """
Line 1302:         dstAllVols = dstDom.getAllVolumes()
it seems odd that you'd pass srcAllVols but not dstAllVols?
Line 1303: 
Line 1304:         for volName, volImages in self.__imageVolumes(srcAllVols, imgUUID):
Line 1305:             if (len(volImages) > 1 and imgUUID != volImages[0]
Line 1306:                     and volName not in dstAllVols.keys()):

Line 1301:         """
Line 1302:         dstAllVols = dstDom.getAllVolumes()
Line 1303: 
Line 1304:         for volName, volImages in self.__imageVolumes(srcAllVols, imgUUID):
Line 1305:             if (len(volImages) > 1 and imgUUID != volImages[0]
I'm not sure that this is readable enough without at least a comment stating that if volImages is greater then one then first element of volImages is the template image.
And that this test is to make sure that target domain has the template image to make sure image would be complete on destination
Line 1306:                     and volName not in dstAllVols.keys()):
Line 1307:                  self.log.error("Cannot move image %s to domain %s because "
Line 1308:                             "the template %s is missing on the destination",
Line 1309:                             imgUUID, dstDom.sdUUID, volName)

Line 1339: 
Line 1340:         # Validates that the pool is connected. WHY?
Line 1341:         pool = self.getPool(spUUID)
Line 1342: 
Line 1343:         if op != image.COPY_OP:
why negative test and not op == image.MOVE_OP ?
Line 1344:             self.validateImageDelete(srcDom, srcAllVols, imgUUID)
Line 1345: 
Line 1346:         self.validateImageCopy(srcAllVols, dstDom, imgUUID)
Line 1347:

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9e07d569eec02ac4dcd386a7c576342f640ec242
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming

Eduardo has posted comments on this change.

Change subject: image: copying a template is always allowed
......................................................................

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

Why use moveImage(op="copy") for copying a template?
Use copyImage().
In this way we have only one path for copying the template.
Therefore is no need to change the validation.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9e07d569eec02ac4dcd386a7c576342f640ec242
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: image: copying a template is always allowed
......................................................................

Patch Set 2: (3 inline comments)
> Why use moveImage(op="copy") for copying a template? Use copyImage().

I'm not interested in moveImage (I'm just fixing it for consistency), I'm interested in reusing your validations in syncImage.

....................................................
File vdsm/storage/hsm.py
Line 1298: 
Line 1299:         Copying an image based on a template to a data domain is allowed only
Line 1300:         if the template already exists on the target domain.
Line 1301:         """
Line 1302:         dstAllVols = dstDom.getAllVolumes()
You need dstDom to get the sdUUID to maintain the error message at line 1302.
Line 1303: 
Line 1304:         for volName, volImages in self.__imageVolumes(srcAllVols, imgUUID):
Line 1305:             if (len(volImages) > 1 and imgUUID != volImages[0]
Line 1306:                     and volName not in dstAllVols.keys()):

Line 1301:         """
Line 1302:         dstAllVols = dstDom.getAllVolumes()
Line 1303: 
Line 1304:         for volName, volImages in self.__imageVolumes(srcAllVols, imgUUID):
Line 1305:             if (len(volImages) > 1 and imgUUID != volImages[0]
Done
Line 1306:                     and volName not in dstAllVols.keys()):
Line 1307:                  self.log.error("Cannot move image %s to domain %s because "
Line 1308:                             "the template %s is missing on the destination",
Line 1309:                             imgUUID, dstDom.sdUUID, volName)

Line 1339: 
Line 1340:         # Validates that the pool is connected. WHY?
Line 1341:         pool = self.getPool(spUUID)
Line 1342: 
Line 1343:         if op != image.COPY_OP:
Given that op is not a boolean and it could contain any value (even future operations if any), op != image.COPY_OP is stricter than op == image.MOVE_OP.
Line 1344:             self.validateImageDelete(srcDom, srcAllVols, imgUUID)
Line 1345: 
Line 1346:         self.validateImageCopy(srcAllVols, dstDom, imgUUID)
Line 1347:

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9e07d569eec02ac4dcd386a7c576342f640ec242
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming

Shu Ming has posted comments on this change.

Change subject: image: copying a template is always allowed
......................................................................

Patch Set 2: (1 inline comment)

....................................................
File vdsm/storage/hsm.py
Line 1339: 
Line 1340:         # Validates that the pool is connected. WHY?
Line 1341:         pool = self.getPool(spUUID)
Line 1342: 
Line 1343:         if op != image.COPY_OP:
But validateImageDelete() was called in the if condition block, it is implicitly saying that if the op is not COPY_OP, it should be a MOVE_OP.
Line 1344:             self.validateImageDelete(srcDom, srcAllVols, imgUUID)
Line 1345: 
Line 1346:         self.validateImageCopy(srcAllVols, dstDom, imgUUID)
Line 1347:

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9e07d569eec02ac4dcd386a7c576342f640ec242
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Ayal Baron 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Shu Ming

Itamar Heim has posted comments on this change.

Change subject: image: copying a template is always allowed
......................................................................

Patch Set 2:

ping

Itamar Heim has abandoned this change.

Change subject: image: copying a template is always allowed
......................................................................

Abandoned

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