Change in vdsm[ovirt-3.3]: Adding [start|stop]MonitoringDomain().

[Federico Simoncelli]

Hello Sandro Bonazzola, Dan Kenigsberg, Eduardo,

I'd like you to do a code review.  Please visit

http://gerrit.ovirt.org/20194

to review the following change.

Change subject: Adding [start|stop]MonitoringDomain().
......................................................................

Adding [start|stop]MonitoringDomain().

startMonitoringDomain() is added for monitoring a storage domain
without being connected to the pool and geting and ID for such
SD lockspace.

stopMonitoringDomain() stops the monitoring and releases the ID.

Monitoring results are gathered with repoStats().

Caveat Emptor:
SDs added to the monitor using startMonitoringDomain() should not
be part of the pool! i.e. the pool domain set and the added SDs set
should be disjoint.

Making repoStats pool independent.

Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1019273
Change-Id: I983d49b0a42cc06428ec75b7795d23abaa6ab84c
Signed-off-by: Eduardo Warszawski 
Reviewed-on: http://gerrit.ovirt.org/19762
Tested-by: Sandro Bonazzola 
Reviewed-by: Dan Kenigsberg 
---
M client/vdsClient.py
M vdsm/API.py
M vdsm/BindingXMLRPC.py
M vdsm/storage/hsm.py
M vdsm/storage/sp.py
M vdsm_api/Bridge.py
M vdsm_api/vdsmapi-schema.json
7 files changed, 123 insertions(+), 32 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/94/20194/1

diff --git a/client/vdsClient.py b/client/vdsClient.py
index 1087f32..e727b49 100644
--- a/client/vdsClient.py
+++ b/client/vdsClient.py
@@ -1692,6 +1692,16 @@
             print 'Domain %s %s' % (d, str(stats[d]))
         return 0, ''

+    def startMonitoringDomain(self, args):
+        sdUUID, hostID = args
+        status = self.s.startMonitoringDomain(sdUUID, hostID)
+        return status['status']['code'], status['status']['message']
+
+    def stopMonitoringDomain(self, args):
+        sdUUID, = args
+        status = self.s.stopMonitoringDomain(sdUUID)
+        return status['status']['code'], status['status']['message']
+
     def snapshot(self, args):
         vmUUID, sdUUID, imgUUID, baseVolUUID, volUUID = args

@@ -2466,8 +2476,16 @@
                       )),
         'repoStats': (serv.repoStats,
                       ('',
-                       'Get the the health status of the active domains'
+                       'Get the health status of the monitored domains'
                        )),
+        'startMonitoringDomain': (serv.startMonitoringDomain,
+                                  ('<sdUUID> <hostID>',
+                                   'Start SD: sdUUID monitoring with hostID'
+                                   )),
+        'stopMonitoringDomain': (serv.stopMonitoringDomain,
+                                 ('<sdUUID>',
+                                  'Stop monitoring SD: sdUUID'
+                                  )),
         'snapshot': (serv.snapshot,
                      ('<vmId> <sdUUID> <imgUUID> <baseVolUUID> <volUUID>',
                       'Take a live snapshot'
diff --git a/vdsm/API.py b/vdsm/API.py
index 37bb908..fa9a3ab 100644
--- a/vdsm/API.py
+++ b/vdsm/API.py
@@ -1396,6 +1396,12 @@
     def getStorageRepoStats(self):
         return self._irs.repoStats()

+    def startMonitoringDomain(self, sdUUID, hostID):
+        return self._irs.startMonitoringDomain(sdUUID, hostID)
+
+    def stopMonitoringDomain(self, sdUUID):
+        return self._irs.stopMonitoringDomain(sdUUID)
+
     def getLVMVolumeGroups(self, storageType=None):
         return self._irs.getVGList(storageType)

diff --git a/vdsm/BindingXMLRPC.py b/vdsm/BindingXMLRPC.py
index 20aaee1..48ced76 100644
--- a/vdsm/BindingXMLRPC.py
+++ b/vdsm/BindingXMLRPC.py
@@ -787,6 +787,14 @@
         api = API.Global()
         return api.getStorageRepoStats()

+    def startMonitoringDomain(self, sdUUID, hostID, options=None):
+        api = API.Global()
+        return api.startMonitoringDomain(sdUUID, hostID)
+
+    def stopMonitoringDomain(self, sdUUID, options=None):
+        api = API.Global()
+        return api.stopMonitoringDomain(sdUUID)
+
     def vgsGetList(self, storageType=None, options=None):
         api = API.Global()
         return api.getLVMVolumeGroups(storageType)
@@ -937,6 +945,8 @@
                 (self.domainsGetList, 'getStorageDomainsList'),
                 (self.poolsGetConnectedList, 'getConnectedStoragePoolsList'),
                 (self.storageRepoGetStats, 'repoStats'),
+                (self.startMonitoringDomain, 'startMonitoringDomain'),
+                (self.stopMonitoringDomain, 'stopMonitoringDomain'),
                 (self.vgsGetList, 'getVGList'),
                 (self.devicesGetList, 'getDeviceList'),
                 (self.devicesGetVisibility, 'getDevicesVisibility'),
diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index d73d0f7..0744ae8 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -40,6 +40,7 @@

from vdsm.config import config
 import sp
+import domainMonitor
 import sd
 import blockSD
 import nfsSD
@@ -93,6 +94,8 @@
 STORAGE_CONNECTION_DIR = os.path.join(constants.P_VDSM_LIB, "connections/")

QEMU_READABLE_TIMEOUT = 30
+
+HSM_DOM_MON_LOCK = "HsmDomainMonitorLock"

def public(f=None, **kwargs):
@@ -398,6 +401,9 @@
                                                 name="storageRefresh")
         storageRefreshThread.daemon = True
         storageRefreshThread.start()
+
+        monitorInterval = config.getint('irs', 'sd_health_check_delay')
+        self.domainMonitor = domainMonitor.DomainMonitor(monitorInterval)

@public
     def registerDomainStateChangeCallback(self, callbackFunc):
@@ -971,9 +977,8 @@
         for dom in sorted(domList):
             vars.task.getExclusiveLock(STORAGE, dom)

-        return sp.StoragePool(
-            spUUID, self.taskMng).create(poolName, masterDom, domList,
-                                         masterVersion, leaseParams)
+        return sp.StoragePool(spUUID, self.domainMonitor, self.taskMng).create(
+            poolName, masterDom, domList, masterVersion, leaseParams)

@public
     def connectStoragePool(self, spUUID, hostID, scsiKey,
@@ -1003,8 +1008,10 @@
                 "spUUID=%s, msdUUID=%s, masterVersion=%s, hostID=%s, "
                 "scsiKey=%s" % (spUUID, msdUUID, masterVersion,
                                 hostID, scsiKey)))
-        return self._connectStoragePool(spUUID, hostID, scsiKey, msdUUID,
-                                        masterVersion, options)
+        with rmanager.acquireResource(STORAGE, HSM_DOM_MON_LOCK,
+                                      rm.LockType.exclusive):
+            return self._connectStoragePool(spUUID, hostID, scsiKey, msdUUID,
+                                            masterVersion, options)

def _connectStoragePool(self, spUUID, hostID, scsiKey, msdUUID,
                             masterVersion, options=None):
@@ -1048,7 +1055,7 @@
                                      masterVersion=masterVersion)
                 return

-            pool = sp.StoragePool(spUUID, self.taskMng)
+            pool = sp.StoragePool(spUUID, self.domainMonitor, self.taskMng)
             if not hostID or not scsiKey or not msdUUID or not masterVersion:
                 hostID, scsiKey, msdUUID, masterVersion = pool.getPoolParams()
             res = pool.connect(hostID, scsiKey, msdUUID, masterVersion)
@@ -1103,8 +1110,10 @@

def _disconnectPool(self, pool, hostID, scsiKey, remove):
         self.validateNotSPM(pool.spUUID)
-        res = pool.disconnect()
-        del self.pools[pool.spUUID]
+        with rmanager.acquireResource(STORAGE, HSM_DOM_MON_LOCK,
+                                      rm.LockType.exclusive):
+            res = pool.disconnect()
+            del self.pools[pool.spUUID]
         return res

@public
@@ -1854,7 +1863,7 @@
         try:
             pool = self.getPool(spUUID)
         except se.StoragePoolUnknown:
-            pool = sp.StoragePool(spUUID, self.taskMng)
+            pool = sp.StoragePool(spUUID, self.domainMonitor, self.taskMng)
         else:
             raise se.StoragePoolConnected(spUUID)

@@ -3482,14 +3491,12 @@
                 if self.pools[spUUID].hsmMailer:
                     self.pools[spUUID].hsmMailer.stop()

-                # Stop repoStat threads
-                for pool in self.pools.values():
-                    try:
-                        pool.stopMonitoringDomains()
-                    except Exception:
-                        self.log.warning("Failed to stop RepoStats thread",
-                                         exc_info=True)
-                        continue
+            # Stop repoStat threads
+            try:
+                self.domainMonitor.close()
+            except Exception:
+                self.log.warning("Failed to stop RepoStats thread",
+                                 exc_info=True)

self.taskMng.prepareForShutdown()
         except:
@@ -3662,10 +3669,23 @@
         """
         result = {}

-        for p in self.pools.values():
-            repo_stats = self._getRepoStats(p.domainMonitor)
+        repo_stats = self._getRepoStats(self.domainMonitor)

-            for d in repo_stats:
-                result[d] = repo_stats[d]['result']
+        for d in repo_stats:
+            result[d] = repo_stats[d]['result']

return result
+
+    @deprecated
+    @public
+    def startMonitoringDomain(self, sdUUID, hostID, options=None):
+        with rmanager.acquireResource(STORAGE, HSM_DOM_MON_LOCK,
+                                      rm.LockType.exclusive):
+            self.domainMonitor.startMonitoring(sdUUID, int(hostID))
+
+    @deprecated
+    @public
+    def stopMonitoringDomain(self, sdUUID, options=None):
+        with rmanager.acquireResource(STORAGE, HSM_DOM_MON_LOCK,
+                                      rm.LockType.exclusive):
+            self.domainMonitor.stopMonitoring(sdUUID)
diff --git a/vdsm/storage/sp.py b/vdsm/storage/sp.py
index e0559bd..cae84b3 100644
--- a/vdsm/storage/sp.py
+++ b/vdsm/storage/sp.py
@@ -51,7 +51,6 @@
 import resourceManager as rm
 import volume
 import mount
-from domainMonitor import DomainMonitor

POOL_MASTER_DOMAIN = 'mastersd'

@@ -112,9 +111,8 @@
     storage_repository = config.get('irs', 'repository')
     _poolsTmpDir = config.get('irs', 'pools_data_dir')
     lvExtendPolicy = config.get('irs', 'vol_extend_policy')
-    monitorInterval = config.getint('irs', 'sd_health_check_delay')

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

self._formatConverter = DefaultFormatConverter()
@@ -131,7 +129,7 @@
         self.spmMailer = None
         self.masterDomain = None
         self.spmRole = SPM_FREE
-        self.domainMonitor = DomainMonitor(self.monitorInterval)
+        self.domainMonitor = domainMonitor
         self._upgradeCallback = partial(StoragePool._upgradePoolDomain,
                                         proxy(self))

@@ -702,7 +700,8 @@

@unsecured
     def stopMonitoringDomains(self):
-        self.domainMonitor.close()
+        for sdUUID in self.getDomains():
+            self.domainMonitor.stopMonitoring(sdUUID)
         return True

@unsecured
@@ -1542,13 +1541,15 @@
     @unsecured
     @misc.samplingmethod
     def updateMonitoringThreads(self):
-        # domain list it's list of sdUUID:status
-        # sdUUID1:status1,sdUUID2:status2,...
+        # getDomains() returns a dict of {sdUUID:status}
+        # {sdUUID1: status1, sdUUID2: status2, ...}
         self.invalidateMetadata()
-        activeDomains = self.getDomains(activeOnly=True)
+        poolDoms = self.getDomains()
+        activeDomains = tuple(sdUUID for sdUUID in poolDoms
+                              if poolDoms[sdUUID] == sd.DOM_ACTIVE_STATUS)
         monitoredDomains = self.domainMonitor.monitoredDomains

-        for sdUUID in monitoredDomains:
+        for sdUUID in poolDoms:
             if sdUUID not in activeDomains:
                 try:
                     self.domainMonitor.stopMonitoring(sdUUID)
diff --git a/vdsm_api/Bridge.py b/vdsm_api/Bridge.py
index 76fd908..b9fdaf8 100644
--- a/vdsm_api/Bridge.py
+++ b/vdsm_api/Bridge.py
@@ -291,6 +291,8 @@
     'Host_getStats': {'ret': 'info'},
     'Host_getStorageDomains': {'ret': 'domlist'},
     'Host_getStorageRepoStats': {'ret': Host_getStorageRepoStats_Ret},
+    'Host_startMonitoringDomain': {},
+    'Host_stopMonitoringDomain': {},
     'Host_getVMList': {'call': Host_getVMList_Call, 'ret': Host_getVMList_Ret},
     'Image_delete': {'ret': 'uuid'},
     'Image_deleteVolumes': {'ret': 'uuid'},
diff --git a/vdsm_api/vdsmapi-schema.json b/vdsm_api/vdsmapi-schema.json
index 898ab5c..e9f23f0 100644
--- a/vdsm_api/vdsmapi-schema.json
+++ b/vdsm_api/vdsmapi-schema.json
@@ -1716,7 +1716,7 @@
 ##
 # @Host.getStorageRepoStats:
 #
-# Get statistics and liveness of currently attached Storage Domains.
+# Get statistics and liveness of currently monitored Storage Domains.
 #
 # Returns:
 # Statistics for all storage domains
@@ -1727,6 +1727,40 @@
  'returns': 'StorageDomainVitalsMap'}

##
+# @Host.startMonitoringDomain:
+#
+# Start SD monitoring with hostID
+#
+# @SdUUID:        The Storage Domain UUID
+#
+# @HostID:        A host ID number in the Storage Domain Lockspace
+#
+# Returns:
+# None
+#
+# Since: 4.12.2
+##
+{'command': {'class': 'Host', 'name': 'startMonitoringDomain'},
+ 'data': {'SdUUID': 'UUID', 'HostID': 'int' }
+ 'returns': ''}
+
+##
+# @Host.stopMonitoringDomain:
+#
+# Stop SD monitoring with hostID
+#
+# @SdUUID:        The Storage Domain UUID
+#
+# Returns:
+# None
+#
+# Since: 4.12.2
+##
+{'command': {'class': 'Host', 'name': 'stopMonitoringDomain'},
+ 'data': {'SdUUID': 'UUID' }
+ 'returns': ''}
+
+##
 # @VmStatus:
 #
 # An enumeration of possible virtual machine statuses.

[Sandro Bonazzola]

Sandro Bonazzola has posted comments on this change.

Change subject: Adding [start|stop]MonitoringDomain().
......................................................................

Patch Set 1:

(1 comment)

....................................................
File vdsm_api/vdsmapi-schema.json
Line 1737: #
Line 1738: # Returns:
Line 1739: # None
Line 1740: #
Line 1741: # Since: 4.12.2
will this require also the fix for 4.14 versioning?
Line 1742: ##
Line 1743: {'command': {'class': 'Host', 'name': 'startMonitoringDomain'},
Line 1744:  'data': {'SdUUID': 'UUID', 'HostID': 'int' }
Line 1745:  'returns': ''}

[Sandro Bonazzola]

Sandro Bonazzola has posted comments on this change.

Change subject: Adding [start|stop]MonitoringDomain().
......................................................................

Patch Set 2:

Any chance to have this in oVirt 3.3.2 ?

[Sandro Bonazzola]

Sandro Bonazzola has posted comments on this change.

Change subject: Adding [start|stop]MonitoringDomain().
......................................................................

Patch Set 3: Code-Review+1

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding [start|stop]MonitoringDomain().
......................................................................

Patch Set 3: Verified+1 Code-Review+2

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding [start|stop]MonitoringDomain().
......................................................................

Patch Set 3:

please rebase and verify as well. thanks

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding [start|stop]MonitoringDomain().
......................................................................

Patch Set 3: -Verified

Saggi Mizrahi has posted comments on this change.

Change subject: Adding [start|stop]MonitoringDomain().
......................................................................

Patch Set 3:

One of SaggiMizrahi's automated scripts discovered this patch might require his approval.
Please wait until he had time to check it out.

Yaniv Bronhaim has posted comments on this change.

Change subject: Adding [start|stop]MonitoringDomain().
......................................................................

Patch Set 3:

please rebase and verify as well. thanks

[Federico Simoncelli]

Federico Simoncelli has abandoned this change.

Change subject: Adding [start|stop]MonitoringDomain().
......................................................................

Abandoned