Change in vdsm[master]: sp: [wip] receive domains map in connectStoragePool

[Federico Simoncelli]

Federico Simoncelli has uploaded a new change for review.

Change subject: sp: [wip] receive domains map in connectStoragePool
......................................................................

sp: [wip] receive domains map in connectStoragePool

Change-Id: I393677f643a62e3711af2a3cfb8b4b9a5ce11c2d
Signed-off-by: Federico Simoncelli 
---
M vdsm/API.py
M vdsm/BindingXMLRPC.py
M vdsm/storage/hsm.py
M vdsm/storage/sp.py
4 files changed, 243 insertions(+), 135 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/27/21427/1

diff --git a/vdsm/API.py b/vdsm/API.py
index f91910c..90ba002 100644
--- a/vdsm/API.py
+++ b/vdsm/API.py
@@ -958,9 +958,12 @@
         APIBase.__init__(self)
         self._UUID = UUID

-    def connect(self, hostID, scsiKey, masterSdUUID, masterVersion):
-        return self._irs.connectStoragePool(self._UUID, hostID, scsiKey,
-                                            masterSdUUID, masterVersion)
+    # FIXME: WIP patch, requires backward compatibility considerations
+    def connect(self, hostID, scsiKey, masterSdUUID, masterVersion,
+                domainsMap):
+        return self._irs.connectStoragePool(
+            self._UUID, hostID, scsiKey, masterSdUUID, masterVersion,
+            domainsMap)

def connectStorageServer(self, domainType, connectionParams):
         return self._irs.connectStorageServer(domainType, self._UUID,
diff --git a/vdsm/BindingXMLRPC.py b/vdsm/BindingXMLRPC.py
index dd82b05..a1c8817 100644
--- a/vdsm/BindingXMLRPC.py
+++ b/vdsm/BindingXMLRPC.py
@@ -542,10 +542,12 @@
         image = API.Image(imgUUID, spUUID, sdUUID)
         return image.download(methodArgs, volUUID)

+    # FIXME: WIP patch, requires backward compatibility considerations
     def poolConnect(self, spUUID, hostID, scsiKey, msdUUID, masterVersion,
-                    options=None):
+                    domainsMap=None, options=None):
         pool = API.StoragePool(spUUID)
-        return pool.connect(hostID, scsiKey, msdUUID, masterVersion)
+        return pool.connect(hostID, scsiKey, msdUUID, masterVersion,
+                            domainsMap)

def poolConnectStorageServer(self, domType, spUUID, conList, options=None):
         pool = API.StoragePool(spUUID)
diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index 3707528..56b24b4 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -981,8 +981,8 @@
             poolName, masterDom, domList, masterVersion, leaseParams)

@public
-    def connectStoragePool(self, spUUID, hostID, scsiKey,
-                           msdUUID, masterVersion, options=None):
+    def connectStoragePool(self, spUUID, hostID, scsiKey, msdUUID,
+                           masterVersion, domainsMap=None, options=None):
         """
         Connect a Host to a specific storage pool.

@@ -1010,11 +1010,12 @@
                                 hostID, scsiKey)))
         with rmanager.acquireResource(STORAGE, HSM_DOM_MON_LOCK,
                                       rm.LockType.exclusive):
-            return self._connectStoragePool(spUUID, hostID, scsiKey, msdUUID,
-                                            masterVersion, options)
+            return self._connectStoragePool(
+                spUUID, hostID, scsiKey, msdUUID, masterVersion,
+                domainsMap, options)

def _connectStoragePool(self, spUUID, hostID, scsiKey, msdUUID,
-                            masterVersion, options=None):
+                            masterVersion, domainsMap=None, options=None):
         misc.validateUUID(spUUID, 'spUUID')

# TBD: To support multiple pool connection on single host,
@@ -1055,7 +1056,13 @@
                                      masterVersion=masterVersion)
                 return

-            pool = sp.StoragePool(spUUID, self.domainMonitor, self.taskMng)
+            if domainsMap is not None:
+                pool = sp.StoragePool(spUUID, self.domainMonitor,
+                                      self.taskMng, domainsMap, masterVersion)
+            else:
+                pool = sp.StoragePoolLegacy(
+                    spUUID, self.domainMonitor, self.taskMng)
+
             res = pool.connect(hostID, scsiKey, msdUUID, masterVersion)

if res:
diff --git a/vdsm/storage/sp.py b/vdsm/storage/sp.py
index 46e8224..638891f 100644
--- a/vdsm/storage/sp.py
+++ b/vdsm/storage/sp.py
@@ -38,6 +38,7 @@
 import fileSD
 import sd
 import misc
+from misc import deprecated
 import fileUtils
 from vdsm.config import config
 from sdc import sdCache
@@ -101,7 +102,7 @@
 MAX_DOMAINS /= 48

-class StoragePool(Securable):
+class StoragePoolBase(Securable):
     '''
     StoragePool object should be relatively cheap to construct. It should defer
     any heavy lifting activities until the time it is really needed.
@@ -113,7 +114,7 @@
     lvExtendPolicy = config.get('irs', 'vol_extend_policy')

def __init__(self, spUUID, domainMonitor, taskManager):
-        Securable.__init__(self)
+        super(StoragePoolBase, self).__init__()

self._formatConverter = DefaultFormatConverter()
         self._domainsToUpgrade = []
@@ -132,10 +133,6 @@
         self.domainMonitor = domainMonitor
         self._upgradeCallback = partial(StoragePool._upgradePoolDomain,
                                         proxy(self))
-
-    @unsecured
-    def getSpmLver(self):
-        return self.getMetaParam(PMDK_LVER)

@unsecured
     def getSpmStatus(self):
@@ -279,9 +276,8 @@
                 self.lver = int(oldlver) + 1

self.invalidateMetadata()
-                self.setMetaParams({
-                    PMDK_LVER: self.lver,
-                    PMDK_SPM_ID: self.id}, __securityOverride=True)
+                self.updateSpmInfo(self.lver, self.id)
+
                 self._maxHostID = maxHostID

# Upgrade the master domain now if needed
@@ -487,10 +483,6 @@
                                sdUUID, msdUUID, masterVersion, exc_info=True)
         # Cleanup links to domains under /rhev/datacenter/poolName
         self.refresh(msdUUID, masterVersion)
-
-    @unsecured
-    def getMasterVersion(self):
-        return self.getMetaParam(PMDK_MASTER_VER)

# TODO: Remove or rename this function.
     @unsecured
@@ -1361,21 +1353,6 @@
         if os.path.exists(os.path.join(vms, vmUUID)):
             fileUtils.cleanupdir(os.path.join(vms, vmUUID))

-    def setDescription(self, descr):
-        """
-        Set storage pool description.
-         'descr' - pool description
-        """
-        if len(descr) > MAX_POOL_DESCRIPTION_SIZE:
-            raise se.StoragePoolDescriptionTooLongError()
-
-        self.log.info("spUUID=%s descr=%s", self.spUUID, descr)
-
-        if not misc.isAscii(descr) and not self.masterDomain.supportsUnicode():
-            raise se.UnicodeArgumentException()
-
-        self.setMetaParam(PMDK_POOL_DESCRIPTION, descr)
-
     def extendVolume(self, sdUUID, volumeUUID, size, isShuttingDown=None):
         # This method is not exposed through the remote API but it's called
         # directly from the mailbox to implement the thin provisioning on
@@ -1391,45 +1368,14 @@
             return sdCache.produce(sdUUID) \
                 .produceVolume(imgUUID, volUUID).extendSize(int(newSize))

-    @classmethod
-    def _getPoolMD(cls, domain):
-        # This might look disgusting but this makes it so that
-        # This is the only intrusion needed to satisfy the
-        # unholy union between pool and SD metadata
-        return DictValidator(domain._metadata._dict, SP_MD_FIELDS)
-
-    @property
-    def _metadata(self):
-        return self._getPoolMD(self.masterDomain)
-
-    @unsecured
-    def getDescription(self):
-        try:
-            return self.getMetaParam(PMDK_POOL_DESCRIPTION)
-            # There was a bug that cause pool description to
-            # disappear. Returning "" might be ugly but it keeps
-            # everyone happy.
-        except KeyError:
-            return ""
-
     @unsecured
     def getVersion(self):
         return self.masterDomain.getVersion()

@unsecured
-    def getSpmId(self):
-        spmid = self.getMetaParam(PMDK_SPM_ID)
-        if spmid != self.id or self.spmRole != SPM_FREE:
-            return spmid
-
-        # If we claim to be the SPM we have to be really sure we are
-        self.invalidateMetadata()
-        return self.getMetaParam(PMDK_SPM_ID)
-
-    @unsecured
     def getInfo(self):
         """
-        Get storage pool info.
+        Get storage group info
         """
         try:
             msdInfo = self.masterDomain.getInfo()
@@ -1438,26 +1384,22 @@
             raise se.StoragePoolMasterNotFound(self.spUUID,
                                                self.masterDomain.sdUUID)

-        try:
-            pmd = self._getPoolMD(self.masterDomain)
-        except Exception:
-            self.log.error("Pool metadata error", exc_info=True)
-            raise se.StoragePoolActionError(self.spUUID)
-
-        poolInfo = {
-            'type': msdInfo['type'],
-            'name': pmd[PMDK_POOL_DESCRIPTION],
-            'domains': domainListEncoder(pmd[PMDK_DOMAINS]),
-            'master_uuid': self.masterDomain.sdUUID,
-            'master_ver': pmd[PMDK_MASTER_VER],
-            'lver': pmd[PMDK_LVER],
-            'spm_id': pmd[PMDK_SPM_ID],
-            'pool_status': 'uninitialized',
-            'version': str(msdInfo['version']),
-            'isoprefix': '',
-            'pool_status': 'connected',
+        groupBaseInfo = {
+            "type": msdInfo["type"],
+            "name": "",
+            "domains": "",
+            "master_uuid": self.masterDomain.sdUUID,
+            "master_ver": 0,
+            "lver": LVER_INVALID,
+            "spm_id": SPM_ID_FREE,
+            "pool_status": "uninitialized",
+            "version": str(msdInfo["version"]),
+            "isoprefix": "",
+            "pool_status": "connected",
         }
-        return poolInfo
+
+        groupBaseInfo.update(self.getGroupInfo())
+        return groupBaseInfo

@unsecured
     def getIsoDomain(self):
@@ -1476,22 +1418,6 @@
                 return dom
         return None

-    def setMetaParams(self, params):
-        self._metadata.update(params)
-
-    def setMetaParam(self, key, value):
-        """
-        Set key:value in pool metadata file
-        """
-        self._metadata[key] = value
-
-    @unsecured
-    def getMetaParam(self, key):
-        """
-        Get parameter from pool metadata file
-        """
-        return self._metadata[key]
-
     @unsecured
     def getMasterDomain(self, msdUUID, masterVersion):
         """
@@ -1506,32 +1432,8 @@
             #Manager should start reconstructMaster if SPM.
             raise se.StoragePoolMasterNotFound(self.spUUID, msdUUID)

-        if not domain.isMaster():
-            self.log.error("Requested master domain %s is not a master domain "
-                           "at all", msdUUID)
-            raise se.StoragePoolWrongMaster(self.spUUID, msdUUID)
-
-        pools = domain.getPools()
-        if (self.spUUID not in pools):
-            self.log.error("Requested master domain %s does not belong to pool"
-                           " %s", msdUUID, self.spUUID)
-            raise se.StoragePoolWrongMaster(self.spUUID, msdUUID)
-
-        version = self._getPoolMD(domain)[PMDK_MASTER_VER]
-        if version != int(masterVersion):
-            self.log.error("Requested master domain %s does not have expected "
-                           "version %s it is version %s",
-                           msdUUID, masterVersion, version)
-            raise se.StoragePoolWrongMaster(self.spUUID, msdUUID)
-
-        self.log.debug("Master domain %s verified, version %s", msdUUID,
-                       masterVersion)
+        self.checkMasterDomain(domain, masterVersion)
         return domain
-
-    @unsecured
-    def invalidateMetadata(self):
-        if not self.spmRole == SPM_ACQUIRED:
-            self._metadata.invalidate()

@unsecured
     @misc.samplingmethod
@@ -1565,10 +1467,9 @@

@unsecured
     def getDomains(self, activeOnly=False):
-        return dict(
-            (sdUUID, status) for sdUUID, status in
-            self.getMetaParam(PMDK_DOMAINS).iteritems()
-            if not activeOnly or status == sd.DOM_ACTIVE_STATUS)
+        return dict((sdUUID, status) for sdUUID, status
+                    in self.getDomainsMap().iteritems()
+                    if not activeOnly or status == sd.DOM_ACTIVE_STATUS)

def checkBackupDomain(self):
         domDict = self.getDomains(activeOnly=True)
@@ -2012,3 +1913,198 @@

def getAllTasksInfo(self):
         return self.taskMng.getAllTasksInfo("spm")
+
+
+class StoragePool(StoragePoolBase):
+
+    def __init__(self, spUUID, domainMonitor, taskManager, domainsMap,
+                 masterVersion):
+        super(StorageGroup, self) \
+            .__init__(spUUID, domainMonitor, taskManager)
+        self.domainsMap = domainsMap
+        self.masterVersion = masterVersion
+
+    @unsecured
+    def getSpmId(self):
+        spmid = self.masterDomain.getClusterLockOwnerId()
+        return SPM_ID_FREE if spmid is None else spmid
+
+    @unsecured
+    def getGroupInfo(self):
+        return {
+            'spm_id': self.getSpmId(),
+            'master_ver': self.masterVersion,
+        }
+
+    @unsecured
+    def getDomainsMap(self):
+        if self.domainsMap is None:
+            raise AttributeError("")
+        return self.domainsMap
+
+    ### Deprecated Methods Section Begin ###
+
+    @deprecated
+    @unsecured
+    def getSpmLver(self):
+        return LVER_INVALID
+
+    @deprecated
+    @unsecured
+    def getMasterVersion(self):
+        return 0
+
+    @deprecated
+    @unsecured
+    def getDescription(self):
+        return ""
+
+    @deprecated
+    def setDescription(self, description):
+        pass
+
+    @deprecated
+    @unsecured
+    def checkMasterDomain(self, domain, masterVersion):
+        pass
+
+    @deprecated
+    @unsecured
+    def invalidateMetadata(self):
+        pass
+
+    @deprecated
+    @unsecured
+    def updateSpmInfo(self, lver, id):
+        pass
+
+    ### Deprecated Methods Section End ###
+
+
+class StoragePoolLegacy(StoragePoolBase):
+
+    def __init__(self, spUUID, domainMonitor, taskManager):
+        super(StoragePool, self) \
+            .__init__(spUUID, domainMonitor, taskManager)
+
+    @classmethod
+    def _getPoolMD(cls, domain):
+        # This might look disgusting but this makes it so that
+        # This is the only intrusion needed to satisfy the
+        # unholy union between pool and SD metadata
+        return DictValidator(domain._metadata._dict, SP_MD_FIELDS)
+
+    @property
+    def _metadata(self):
+        return self._getPoolMD(self.masterDomain)
+
+    @unsecured
+    def getMetaParam(self, key):
+        """
+        Get parameter from pool metadata file
+        """
+        return self._metadata[key]
+
+    def setMetaParams(self, params):
+        self._metadata.update(params)
+
+    def setMetaParam(self, key, value):
+        """
+        Set key:value in pool metadata file
+        """
+        self._metadata[key] = value
+
+    @unsecured
+    def getSpmLver(self):
+        return self.getMetaParam(PMDK_LVER)
+
+    @unsecured
+    def getMasterVersion(self):
+        return self.getMetaParam(PMDK_MASTER_VER)
+
+    @unsecured
+    def getDescription(self):
+        try:
+            return self.getMetaParam(PMDK_POOL_DESCRIPTION)
+            # There was a bug that cause pool description to
+            # disappear. Returning "" might be ugly but it keeps
+            # everyone happy.
+        except KeyError:
+            return ""
+
+    def setDescription(self, descr):
+        """
+        Set storage pool description.
+         'descr' - pool description
+        """
+        if len(descr) > MAX_POOL_DESCRIPTION_SIZE:
+            raise se.StoragePoolDescriptionTooLongError()
+
+        self.log.info("spUUID=%s descr=%s", self.spUUID, descr)
+
+        if not misc.isAscii(descr) and not self.masterDomain.supportsUnicode():
+            raise se.UnicodeArgumentException()
+
+        self.setMetaParam(PMDK_POOL_DESCRIPTION, descr)
+
+    @unsecured
+    def checkMasterDomain(self, domain, masterVersion):
+        if not domain.isMaster():
+            self.log.error("Requested master domain %s is not a master domain "
+                           "at all", domain.sdUUID)
+            raise se.StoragePoolWrongMaster(self.spUUID, domain.sdUUID)
+
+        pools = domain.getPools()
+        if (self.spUUID not in pools):
+            self.log.error("Requested master domain %s does not belong to pool"
+                           " %s", domain.sdUUID, self.spUUID)
+            raise se.StoragePoolWrongMaster(self.spUUID, domain.sdUUID)
+
+        version = self.getMasterVersion()
+        if version != int(masterVersion):
+            self.log.error("Requested master domain %s does not have expected "
+                           "version %s it is version %s",
+                           domain.sdUUID, masterVersion, version)
+            raise se.StoragePoolWrongMaster(self.spUUID, domain.sdUUID)
+
+        self.log.debug("Master domain %s verified, version %s", domain.sdUUID,
+                       masterVersion)
+
+    @unsecured
+    def invalidateMetadata(self):
+        if not self.spmRole == SPM_ACQUIRED:
+            self._metadata.invalidate()
+
+    @unsecured
+    def getSpmId(self):
+        spmid = self.getMetaParam(PMDK_SPM_ID)
+        if spmid != self.id or self.spmRole != SPM_FREE:
+            return spmid
+
+        # If we claim to be the SPM we have to be really sure we are
+        self.invalidateMetadata()
+        return self.getMetaParam(PMDK_SPM_ID)
+
+    @unsecured
+    def updateSpmInfo(self, lver, id):
+        self.setMetaParams({PMDK_LVER: lver, PMDK_SPM_ID: id})
+
+    @unsecured
+    def getGroupInfo(self):
+        try:
+            pmd = self._getPoolMD(self.masterDomain)
+        except Exception:
+            self.log.error("Pool metadata error", exc_info=True)
+            raise se.StoragePoolActionError(self.spUUID)
+
+        return {
+            'name': pmd[PMDK_POOL_DESCRIPTION],
+            'domains': domainListEncoder(pmd[PMDK_DOMAINS]),
+            'master_ver': pmd[PMDK_MASTER_VER],
+            'lver': pmd[PMDK_LVER],
+            'spm_id': pmd[PMDK_SPM_ID],
+        }
+
+    @unsecured
+    def getDomainsMap(self):
+        return self.getMetaParam(PMDK_DOMAINS)

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: sp: [wip] receive domains map in connectStoragePool
......................................................................

Patch Set 1: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4723/ : FAILURE

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

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5603/ : FAILURE

Ayal Baron has posted comments on this change.

Change subject: sp: [wip] receive domains map in connectStoragePool
......................................................................

Patch Set 1:

(3 comments)

....................................................
Commit Message
Line 3: AuthorDate: 2013-11-18 10:54:41 -0500
Line 4: Commit:     Federico Simoncelli 
Line 5: CommitDate: 2013-11-19 10:40:26 -0500
Line 6: 
Line 7: sp: [wip] receive domains map in connectStoragePool
why?
Line 8: 
Line 9: Change-Id: I393677f643a62e3711af2a3cfb8b4b9a5ce11c2d

....................................................
File vdsm/API.py
Line 962:     def connect(self, hostID, scsiKey, masterSdUUID, masterVersion,
Line 963:                 domainsMap):
Line 964:         return self._irs.connectStoragePool(
Line 965:             self._UUID, hostID, scsiKey, masterSdUUID, masterVersion,
Line 966:             domainsMap)
why is this a map and not a list?
I can't think of any reason for engine to pass anything but active domains. In that case, passing 'domUUID:active' is redundant, just pass the list of domains.
Internally in the pool class, if for bc reasons and avoiding changing too much you want to have the map format then you can convert the list to map
Line 967: 
Line 968:     def connectStorageServer(self, domainType, connectionParams):
Line 969:         return self._irs.connectStorageServer(domainType, self._UUID,
Line 970:                                               connectionParams)

....................................................
File vdsm/storage/sp.py
Line 1980: 
Line 1981:     ### Deprecated Methods Section End ###
Line 1982: 
Line 1983: 
Line 1984: class StoragePoolLegacy(StoragePoolBase):
We might end up having several of these since we're making these changes iteratively. So I'm not sure StoragePool and StoragePoolLegacy are the proper terms. what do you think?
Line 1985: 
Line 1986:     def __init__(self, spUUID, domainMonitor, taskManager):
Line 1987:         super(StoragePool, self) \
Line 1988:             .__init__(spUUID, domainMonitor, taskManager)

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: sp: [wip] receive domains map in connectStoragePool
......................................................................

Patch Set 1:

(2 comments)

....................................................
File vdsm/API.py
Line 962:     def connect(self, hostID, scsiKey, masterSdUUID, masterVersion,
Line 963:                 domainsMap):
Line 964:         return self._irs.connectStoragePool(
Line 965:             self._UUID, hostID, scsiKey, masterSdUUID, masterVersion,
Line 966:             domainsMap)
This was indeed a list in my first POC but then I realized that we can't change all at once. As we want incremental changes I wanted connectStoragePool to work as it is now without having to deal with missing "attached" (only) domains. I am sure there is some code on the engine side expecting the domain to be reported as "attached". So for now it's a map relying on the reconstructMaster code (both on vdsm and engine side) for creating and parsing the domains map (really simple and a noop for me).
Line 967: 
Line 968:     def connectStorageServer(self, domainType, connectionParams):
Line 969:         return self._irs.connectStorageServer(domainType, self._UUID,
Line 970:                                               connectionParams)

....................................................
File vdsm/storage/sp.py
Line 1980: 
Line 1981:     ### Deprecated Methods Section End ###
Line 1982: 
Line 1983: 
Line 1984: class StoragePoolLegacy(StoragePoolBase):
You're right. I couldn't find a good name though. But as a general class design is this ok with you?
Line 1985: 
Line 1986:     def __init__(self, spUUID, domainMonitor, taskManager):
Line 1987:         super(StoragePool, self) \
Line 1988:             .__init__(spUUID, domainMonitor, taskManager)

Ayal Baron has posted comments on this change.

Change subject: sp: [wip] receive domains map in connectStoragePool
......................................................................

Patch Set 1:

(2 comments)

....................................................
File vdsm/API.py
Line 962:     def connect(self, hostID, scsiKey, masterSdUUID, masterVersion,
Line 963:                 domainsMap):
Line 964:         return self._irs.connectStoragePool(
Line 965:             self._UUID, hostID, scsiKey, masterSdUUID, masterVersion,
Line 966:             domainsMap)
hmmm.
Engine updates status of domains in case of conflict by running getStoragePoolInfo.
This is mostly relevant in cases where engine calls activateStorageDomain or deactivateStorageDomain and for some reason misses the reply.
However, passing the list of domains makes that flow irrelevant since there are 2 meanings for active domain:
1. engine determines health according to it
2. vdsm monitors it
Both of these only require passing a list.

Attached domain has only 1 meaning: it is marked as belonging to the pool (on the domain itself and in the pool md).

Since the pool MD is now actually determined by the engine, getting it back in getStoragePoolInfo and updating according to it is a bit silly.
This raises 2 questions:
1. what happens if engine calls detachStorageDomain and looses connection with vdsm - engine doesn't know whether the command succeeded or failed (today I believe it would determine that it failed and then update using getStoragePoolInfo.  Using a map this behaviour would remain the same, but if the reason of the disconnect is that vdsm restarted right before it notified engine of the result, either success or failure, then engine has no way of determining what current state is.
2. same thing with attachStorageDomain
Sounds like we'd need to use getStorageDomainInfo to see what is persisted on the domain to be able to determine that (check end state).

So unless I'm missing something, we need to update engine logic (simplify mostly but use getStorageDomainInfo to figure out what happened).

Also sounds like these should really be async operations in engine (so that we don't finish until we determine whether sd is attached or not)
Line 967: 
Line 968:     def connectStorageServer(self, domainType, connectionParams):
Line 969:         return self._irs.connectStorageServer(domainType, self._UUID,
Line 970:                                               connectionParams)

....................................................
File vdsm/storage/sp.py
Line 1980: 
Line 1981:     ### Deprecated Methods Section End ###
Line 1982: 
Line 1983: 
Line 1984: class StoragePoolLegacy(StoragePoolBase):
well, what you actually separated out here is the pool md methods.
you could use composition instead of inheritance here.
Then you'd have a single pool class, and 2 md classes and when you come to make additional changes (since you want to change verbs as well) then the changes would be orthogonal
Line 1985: 
Line 1986:     def __init__(self, spUUID, domainMonitor, taskManager):
Line 1987:         super(StoragePool, self) \
Line 1988:             .__init__(spUUID, domainMonitor, taskManager)

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: sp: [wip] receive domains map in connectStoragePool
......................................................................

Patch Set 1:

(1 comment)

....................................................
File vdsm/API.py
Line 962:     def connect(self, hostID, scsiKey, masterSdUUID, masterVersion,
Line 963:                 domainsMap):
Line 964:         return self._irs.connectStoragePool(
Line 965:             self._UUID, hostID, scsiKey, masterSdUUID, masterVersion,
Line 966:             domainsMap)
What I thought was that in the end each action attach/detach (if they'll survive) activate/deactivate they'll all result in the same call (connectStoragePool) on all the hosts.

Now some hosts might not receive this call but it's not a reason to make the action async (hard to deal with it). When the hosts are available and they report a vds domain status that is different from what is expected then we should trigger a proper connectStoragePool to fix that.

Is this answering all your questions or is there a scenario that I didn't cover?
Line 967: 
Line 968:     def connectStorageServer(self, domainType, connectionParams):
Line 969:         return self._irs.connectStorageServer(domainType, self._UUID,
Line 970:                                               connectionParams)

Ayal Baron has posted comments on this change.

Change subject: sp: [wip] receive domains map in connectStoragePool
......................................................................

Patch Set 1:

(1 comment)

....................................................
File vdsm/API.py
Line 962:     def connect(self, hostID, scsiKey, masterSdUUID, masterVersion,
Line 963:                 domainsMap):
Line 964:         return self._irs.connectStoragePool(
Line 965:             self._UUID, hostID, scsiKey, masterSdUUID, masterVersion,
Line 966:             domainsMap)
"Is this answering all your questions or is there a scenario that I didn't cover?"

No it does not.
It is fine for activate/deactivate, but attach/detach do more than that (spm side).
Note that HSMs don't really care about pool, they can run VMs regardless of being connected to the pool and they don't require the domain lock even, just sanlock on images (once utilised), so the connectSP on the other hosts is not really interesting in this case (and engine should only pass domains that the host should access in order to run VMs.
Going forward, once we are able to run "spm" ops from any host on any domain then the host will acquire the domain lock so no issue there either.

Currently however, detach and attach are special.  Because we want to prevent a case where 2 different SPMs can make changes to a domain then as long as we don't lock the domain for the duration of the operation we need to 'own' it.  This is done by actually writing something on it.

So, engine has to somehow guarantee whether this 'owning' process or 'deowning' process succeeded.

This is relevant on the spm only.

activateStorageDomain and deactivateStorageDomain can be decommissioned the moment you start passing the active domain list in connectSP.

attachStorageDomain and detachStorageDomain can remain the same on vdsm side, but have to undergo some changes in engine

Did this answer the question? am I missing something obvious which could simplify things?
Line 967: 
Line 968:     def connectStorageServer(self, domainType, connectionParams):
Line 969:         return self._irs.connectStorageServer(domainType, self._UUID,
Line 970:                                               connectionParams)

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: sp: [wip] receive domains map in connectStoragePool
......................................................................

Patch Set 1:

(1 comment)

....................................................
File vdsm/API.py
Line 962:     def connect(self, hostID, scsiKey, masterSdUUID, masterVersion,
Line 963:                 domainsMap):
Line 964:         return self._irs.connectStoragePool(
Line 965:             self._UUID, hostID, scsiKey, masterSdUUID, masterVersion,
Line 966:             domainsMap)
> No it does not. It is fine for activate/deactivate, but attach/detach do more than that (spm side).

Ok this is an entirely different matter, it seems to me that it's not related to comment 1 and 2 where we were discussing the domain map (vs a list) and when/how to sync it (e.g. async vs on a mismatch detected in the result of getVdsStats).

If this is a new discussion, then:
> Note that HSMs don't really care about pool

And therefore also the SPM *host* (as it's an HSM to begin with). Now, about attach/detach we have two ways to go:

* force connectStoragePool to validate that all the domains passed in the list/map are marked with the proper pool uuid (more similar to what we do now)
* on connectStoragePool we allow any domain to be part of the list/map (which is what I thought, as it gives us more flexibility in terms of what domains we can see, monitor and eventually consume) and if the host is the SPM validate that the SPM actions will run only on the domains that are marked with the proper pool uuid.
> activateStorageDomain and deactivateStorageDomain can be decommissioned the moment you start passing the active domain list in connectSP.
> attachStorageDomain and detachStorageDomain can remain the same on vdsm side, but have to undergo some changes in engine
> Did this answer the question? am I missing something obvious which could simplify things?

Looks fine.
Line 967: 
Line 968:     def connectStorageServer(self, domainType, connectionParams):
Line 969:         return self._irs.connectStorageServer(domainType, self._UUID,
Line 970:                                               connectionParams)

Change in vdsm[master]: sp: receive domains map in connectStoragePool

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: sp: receive domains map in connectStoragePool
......................................................................

Patch Set 2:

Build Successful

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

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

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

Change in vdsm[master]: sp: receive domains map in connectStoragePool

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: sp: receive domains map in connectStoragePool
......................................................................

Patch Set 2:

(2 comments)

....................................................
Commit Message
Line 4: Commit:     Federico Simoncelli 
Line 5: CommitDate: 2014-01-08 08:52:01 -0500
Line 6: 
Line 7: sp: receive domains map in connectStoragePool
Line 8: 
I know.
Line 9: Change-Id: I393677f643a62e3711af2a3cfb8b4b9a5ce11c2d

....................................................
File vdsm/storage/spbackends.py
Line 326:                 futureMaster.releaseClusterLock()
Line 327: 
Line 328: 
Line 329: @secured
Line 330: class StoragePoolNew(object):
Ah I forgot to come up with a better name for this. Please suggest.
Line 331: 
Line 332:     __slots__ = ('pool', 'masterVersion', 'domainsMap')
Line 333: 
Line 334:     log = logging.getLogger('Storage.StoragePoolNew')

Change in vdsm[master]: sp: receive domains map in connectStoragePool

Ayal Baron has posted comments on this change.

Change subject: sp: receive domains map in connectStoragePool
......................................................................

Patch Set 2:

(4 comments)

http://gerrit.ovirt.org/#/c/21427/2/vdsm/storage/hsm.py
File vdsm/storage/hsm.py:

Line 1006: 
Line 1007:         if domainsMap is None:
Line 1008:             if not isinstance(pool.getBackend(),
Line 1009:                               spbackends.StoragePoolMetaBackend):
Line 1010:                 raise se.StoragePoolConnected('Cannot downgrade pool backend')
1. why is this hsm logic and not pool logic?
2. shouldn't the backend say whether it requires domainsMap or not?
e.g.

if domainsMap is None and pool.getBackend.requiresDomainsMap()
   raise...
else if domainsMap and not pool.getBackend.requiresDomainsMap():
   upgrade

pool.refresh()
Line 1011:         else:
Line 1012:             if isinstance(pool.getBackend(), spbackends.StoragePoolNew):
Line 1013:                 pool.getBackend().setDomainsMap(domainsMap)
Line 1014:             else:

Line 1054: 
Line 1055:             pool = sp.StoragePool(spUUID, self.domainMonitor, self.taskMng)
Line 1056: 
Line 1057:             if domainsMap is None:
Line 1058:                 pool.setBackend(spbackends.StoragePoolMetaBackend(pool))
why is this not in pool __init__?
Line 1059:             else:
Line 1060:                 pool.setBackend(
Line 1061:                     spbackends.StoragePoolNew(pool, masterVersion, domainsMap))
Line 1062:

http://gerrit.ovirt.org/#/c/21427/2/vdsm/storage/spbackends.py
File vdsm/storage/spbackends.py:

Line 326:                 futureMaster.releaseClusterLock()
Line 327: 
Line 328: 
Line 329: @secured
Line 330: class StoragePoolNew(object):
StoragePoolMemoryBackend ?
And rename StoragePoolMetaBackend to StoragePoolDiskBackend ?
Line 331: 
Line 332:     __slots__ = ('pool', 'masterVersion', 'domainsMap')
Line 333: 
Line 334:     log = logging.getLogger('Storage.StoragePoolNew')

Line 332:     __slots__ = ('pool', 'masterVersion', 'domainsMap')
Line 333: 
Line 334:     log = logging.getLogger('Storage.StoragePoolNew')
Line 335: 
Line 336:     def __init__(self, pool, masterVersion, domainsMap):
what is the APi that a backend needs to implement?
e.g. is reconstructMaster required, and if so, why isn't it here?
Line 337:         self.pool = weakref.proxy(pool)
Line 338:         self.masterVersion = masterVersion
Line 339:         self.setDomainsMap(domainsMap)
Line 340:

Change in vdsm[master]: sp: receive domains map in connectStoragePool

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: sp: receive domains map in connectStoragePool
......................................................................

Patch Set 3: Verified-1

Build Failed

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

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6838/ : FAILURE

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

http://jenkins.ovirt.org/job/vdsm_storage_functional_tests/21/ : FAILURE

Change in vdsm[master]: sp: receive domains map in connectStoragePool

Ayal Baron has posted comments on this change.

Change subject: sp: receive domains map in connectStoragePool
......................................................................

Patch Set 3: Code-Review+2

Change in vdsm[master]: sp: receive domains map in connectStoragePool

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: sp: receive domains map in connectStoragePool
......................................................................

Patch Set 3: Verified+1

Verified: createStoragePool, several attach/detachStorageDomain, destroyStoragePool, reconstructMaster and masterMigrate.

Change in vdsm[master]: sp: receive domains map in connectStoragePool

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: sp: receive domains map in connectStoragePool
......................................................................

sp: receive domains map in connectStoragePool

This patch adds the support for the StoragePoolMemoryBackend and
the required support in the HSM class to instantiate it and to
live upgrade from older backends.

The connectStroagePool API has been updated to receive the extra
domainsMap dictionary that triggers the use of the new backend.

Change-Id: I393677f643a62e3711af2a3cfb8b4b9a5ce11c2d
Signed-off-by: Federico Simoncelli 
Reviewed-on: http://gerrit.ovirt.org/21427
Reviewed-by: Ayal Baron 
---
M vdsm/API.py
M vdsm/BindingXMLRPC.py
M vdsm/storage/hsm.py
M vdsm/storage/spbackends.py
M vdsm_api/vdsmapi-schema.json
5 files changed, 151 insertions(+), 17 deletions(-)

Approvals:
  Ayal Baron: Looks good to me, approved
  Federico Simoncelli: Verified