Change in vdsm[master]: avoid redundant domain produce() in createStoragePool

Royce Lv has uploaded a new change for review.

Change subject: avoid redundant domain produce() in createStoragePool
......................................................................

avoid redundant domain produce() in createStoragePool

Change-Id: If1c236aa3043068ca8f3f376cb340e986cd484bb
Signed-off-by: Royce Lv
---
M vdsm/storage/hsm.py
M vdsm/storage/sp.py
2 files changed, 4 insertions(+), 13 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/47/7347/1

diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index 2227c03..b2c20c0 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -765,19 +765,6 @@
         if msdType in sd.BLOCK_DOMAIN_TYPES and msdVersion in blockSD.VERS_METADATA_LV and len(domList) > sp.MAX_DOMAINS:
             raise se.TooManyDomainsInStoragePoolError()

-        for sdUUID in domList:
-            try:
-                dom = sdCache.produce(sdUUID=sdUUID)
-                # TODO: consider removing validate() from here, as the domains
-                # are going to be accessed much later, and may loose validity
-                # until then.
-                dom.validate()
-            except:
-                raise se.StorageDomainAccessError(sdUUID)
-            #If you remove this condition, remove it from StoragePool.attachSD() too.
-            if dom.isData() and (dom.getVersion() != msdVersion):
-                raise se.MixedSDVersionError(dom.sdUUID, dom.getVersion(), msd.sdUUID, msdVersion)
-
         vars.task.getExclusiveLock(STORAGE, spUUID)
         for dom in sorted(domList):
             vars.task.getExclusiveLock(STORAGE, dom)
diff --git a/vdsm/storage/sp.py b/vdsm/storage/sp.py
index ee45151..a5b65c7 100644
--- a/vdsm/storage/sp.py
+++ b/vdsm/storage/sp.py
@@ -549,6 +549,7 @@
                 domain.validate()
                 if sdUUID == msdUUID:
                     msd = domain
+                    msdVersion = msd.getVersion()
             except se.StorageException:
                 self.log.error("Unexpected error", exc_info=True)
                 raise se.StorageDomainAccessError(sdUUID)
@@ -560,6 +561,9 @@
                 # Non ISO domains have only 1 pool
                 if len(spUUIDs) > 0:
                     raise se.StorageDomainAlreadyAttached(spUUIDs[0], sdUUID)
+                
+            if domain.isData() and (domain.getVersion() != msdVersion):
+                raise se.MixedSDVersionError(domain.sdUUID, domain.getVersion(), msd.sdUUID, msdVersion)

fileUtils.createdir(self.poolPath)
         self._acquireTemporaryClusterLock(msdUUID, safeLease)

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: If1c236aa3043068ca8f3f376cb340e986cd484bb
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: avoid redundant domain produce() in createStoragePool
......................................................................

Patch Set 1:

Build Successful

http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/538/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If1c236aa3043068ca8f3f376cb340e986cd484bb
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: oVirt Jenkins CI Server

Shu Ming has posted comments on this change.

Change subject: avoid redundant domain produce() in createStoragePool
......................................................................

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

I don't think this patch is in right direction.  The reasons are:

I) The second domain produce here will be look-uped in the cache, so the time is trivial.

II) The first domain produce will protect function to enter into sp.create() if any exception is raised. So it do shorten the faulty path here.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If1c236aa3043068ca8f3f376cb340e986cd484bb
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Royce Lv has posted comments on this change.

Change subject: avoid redundant domain produce() in createStoragePool
......................................................................

Patch Set 1:

Ming, my consideration is not the case dom can be find and validate case. 
The first lookup and validate is very early from real use of the domain, domain will not be validate till then. So we just check it when we want to use it in case its validity changed and the first check become wasted.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If1c236aa3043068ca8f3f376cb340e986cd484bb
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: avoid redundant domain produce() in createStoragePool
......................................................................

Patch Set 2:

Build Successful

http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/555/ : SUCCESS

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If1c236aa3043068ca8f3f376cb340e986cd484bb
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Royce Lv 
Gerrit-Reviewer: Eduardo 
Gerrit-Reviewer: Royce Lv 
Gerrit-Reviewer: Shu Ming 
Gerrit-Reviewer: oVirt Jenkins CI Server

Itamar Heim has posted comments on this change.

Change subject: avoid redundant domain produce() in createStoragePool
......................................................................

Patch Set 2:

still relevant or should be abandoned?

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: avoid redundant domain produce() in createStoragePool
......................................................................

Patch Set 2: Code-Review-1

(1 comment)

I like the idea but the implementation has a problem that should be addressed.

....................................................
File vdsm/storage/sp.py
Line 561:                 # Non ISO domains have only 1 pool
Line 562:                 if len(spUUIDs) > 0:
Line 563:                     raise se.StorageDomainAlreadyAttached(spUUIDs[0], sdUUID)
Line 564: 
Line 565:             if domain.isData() and (domain.getVersion() != msdVersion):
This works only if the master domain is the first domain in domList, otherwise it will break on msdVersion not being defined.
Line 566:                 raise se.MixedSDVersionError(domain.sdUUID, domain.getVersion(), msd.sdUUID, msdVersion)
Line 567: 
Line 568:         fileUtils.createdir(self.poolPath)
Line 569:         self._acquireTemporaryClusterLock(msdUUID, safeLease)

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: avoid redundant domain produce() in createStoragePool
......................................................................

Patch Set 3:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6646/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5753/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6559/ : SUCCESS

Adam Litke has posted comments on this change.

Change subject: avoid redundant domain produce() in createStoragePool
......................................................................

Patch Set 3: Verified+1

Updated this patch to address Federico's concerns.  Also, I've added a new functional test to exercise the verification checks that have been affected.

Itamar Heim has posted comments on this change.

Change subject: avoid redundant domain produce() in createStoragePool
......................................................................

Patch Set 3:

ping?

Itamar Heim has posted comments on this change.

Change subject: avoid redundant domain produce() in createStoragePool
......................................................................

Patch Set 3:

ping

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: avoid redundant domain produce() in createStoragePool
......................................................................

Patch Set 3: Code-Review+2

(1 comment)

http://gerrit.ovirt.org/#/c/7347/3/vdsm/storage/sp.py
File vdsm/storage/sp.py:

Line 619:                 spUUIDs = domain.getPools()
Line 620:                 # Non ISO domains have only 1 pool
Line 621:                 if len(spUUIDs) > 0:
Line 622:                     raise se.StorageDomainAlreadyAttached(spUUIDs[0], sdUUID)
Line 623:             domains.append(domain)
minor: you can consider to cache only the data domains eventually:

dataDomains.append(domain)
 ...
 for domain in dataDomains:
     if domain.getVersion() != msdVersion:
         ...

anyway I don't want think we should block on this.
Line 624: 
Line 625:         for domain in domains:
Line 626:             if domain.isData() and (domain.getVersion() != msdVersion):
Line 627:                 raise se.MixedSDVersionError(domain.sdUUID,

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: avoid redundant domain produce() in createStoragePool
......................................................................

avoid redundant domain produce() in createStoragePool

As the domains in hsm.createStoragePool are going to
be accessed much later, and may loose validity till then.
Just produce from cache and check the validity when actully use it.

Change-Id: If1c236aa3043068ca8f3f376cb340e986cd484bb
Signed-off-by: Adam Litke 
Reviewed-on: http://gerrit.ovirt.org/7347
Reviewed-by: Federico Simoncelli 
---
M tests/functional/storageTests.py
M vdsm/storage/hsm.py
M vdsm/storage/sp.py
3 files changed, 55 insertions(+), 15 deletions(-)

Approvals:
  Adam Litke: Verified
  Federico Simoncelli: Looks good to me, approved