Change in vdsm[master]: domainMonitor: Extract domain monitoring methods

nsoffer at redhat.com nsoffer at redhat.com
Thu May 15 06:44:56 UTC 2014


Nir Soffer has uploaded a new change for review.

Change subject: domainMonitor: Extract domain monitoring methods
......................................................................

domainMonitor: Extract domain monitoring methods

The _domainMonitor method was extremely long and was doing too much.
This patch breaks it to multiple small methods to make it easier to
understand and modify.

One of the planned changes is to allow _domainMonitor to abort in the
middle if a monitor is stopped. Implementing this in the original method
would only make it worse.

This patch (hopefully) does change the behavior of the monitor.

Change-Id: I87ac6a82e560bc4360a3bc3f6f6fd94623678cc2
Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
M vdsm/storage/domainMonitor.py
1 file changed, 111 insertions(+), 73 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/14/27714/1

diff --git a/vdsm/storage/domainMonitor.py b/vdsm/storage/domainMonitor.py
index 8a50b4f..8057910 100644
--- a/vdsm/storage/domainMonitor.py
+++ b/vdsm/storage/domainMonitor.py
@@ -174,66 +174,20 @@
 
         self.log.debug("Stopping domain monitor for %s", self.sdUUID)
 
-        # If this is an ISO domain we didn't acquire the host id and releasing
-        # it is superfluous.
-        if self.domain and not self.isIsoDomain:
-            try:
-                self.domain.releaseHostId(self.hostId, unused=True)
-            except:
-                self.log.debug("Unable to release the host id %s for domain "
-                               "%s", self.hostId, self.sdUUID, exc_info=True)
+        if self._shouldReleaseHostId():
+            self._releaseHostId()
 
     def _monitorDomain(self):
         self.nextStatus.clear()
 
-        if time() - self.lastRefresh > self.refreshTime:
-            # Refreshing the domain object in order to pick up changes as,
-            # for example, the domain upgrade.
-            self.log.debug("Refreshing domain %s", self.sdUUID)
-            sdCache.manuallyRemoveDomain(self.sdUUID)
-            self.lastRefresh = time()
+        if self._shouldRefreshDomain():
+            self._refreshDomain()
 
         try:
-            # We should produce the domain inside the monitoring loop because
-            # it might take some time and we don't want to slow down the thread
-            # start (and anything else that relies on that as for example
-            # updateMonitoringThreads). It also needs to be inside the loop
-            # since it might fail and we want keep trying until we succeed or
-            # the domain is deactivated.
-            if self.domain is None:
-                self.domain = sdCache.produce(self.sdUUID)
-
-            if self.isIsoDomain is None:
-                # The isIsoDomain assignment is delayed because the isoPrefix
-                # discovery might fail (if the domain suddenly disappears) and
-                # we could risk to never try to set it again.
-                isIsoDomain = self.domain.isISO()
-                if isIsoDomain:
-                    self.isoPrefix = self.domain.getIsoDomainImagesDir()
-                self.isIsoDomain = isIsoDomain
-
-            self.domain.selftest()
-
-            self.nextStatus.readDelay = self.domain.getReadDelay()
-
-            stats = self.domain.getStats()
-            self.nextStatus.diskUtilization = (stats["disktotal"],
-                                               stats["diskfree"])
-
-            self.nextStatus.vgMdUtilization = (stats["mdasize"],
-                                               stats["mdafree"])
-
-            self.nextStatus.vgMdHasEnoughFreeSpace = stats["mdavalid"]
-            self.nextStatus.vgMdFreeBelowThreashold = stats["mdathreshold"]
-
-            masterStats = self.domain.validateMaster()
-            self.nextStatus.masterValid = masterStats['valid']
-            self.nextStatus.masterMounted = masterStats['mount']
-
-            self.nextStatus.hasHostId = self.domain.hasHostId(self.hostId)
-            self.nextStatus.isoPrefix = self.isoPrefix
-            self.nextStatus.version = self.domain.getVersion()
-
+            self._initializeDomain()
+            self._checkDomain()
+            self._checkReadDelay()
+            self._collectStats()
         except Exception as e:
             self.log.error("Error while collecting domain %s monitoring "
                            "information", self.sdUUID, exc_info=True)
@@ -243,29 +197,113 @@
         self.nextStatus.valid = (self.nextStatus.error is None)
 
         if self._statusDidChange():
-            self.log.debug("Domain %s changed its status to %s", self.sdUUID,
-                           "Valid" if self.nextStatus.valid else "Invalid")
-
-            try:
-                self.domainMonitor.onDomainStateChange.emit(
-                    self.sdUUID, self.nextStatus.valid)
-            except:
-                self.log.warn("Could not emit domain state change event",
-                              exc_info=True)
-
+            self._notifyStatusChanges()
         self.firstChange = False
 
-        # An ISO domain can be shared by multiple pools
-        if (not self.isIsoDomain and self.nextStatus.valid
-                and self.nextStatus.hasHostId is False):
-            try:
-                self.domain.acquireHostId(self.hostId, async=True)
-            except:
-                self.log.debug("Unable to issue the acquire host id %s "
-                               "request for domain %s", self.hostId,
-                               self.sdUUID, exc_info=True)
+        if self._shouldAcquireHostId():
+            self._acquireHostId()
 
         self.status.update(self.nextStatus)
 
+    # Notifiying status changes
+
     def _statusDidChange(self):
         return self.firstChange or self.status.valid != self.nextStatus.valid
+
+    def _notifyStatusChanges(self):
+        self.log.debug("Domain %s changed its status to %s", self.sdUUID,
+                       "Valid" if self.nextStatus.valid else "Invalid")
+        try:
+            self.domainMonitor.onDomainStateChange.emit(
+                self.sdUUID, self.nextStatus.valid)
+        except:
+            self.log.warn("Could not emit domain state change event",
+                          exc_info=True)
+
+    # Refreshing domain
+
+    def _shouldRefreshDomain(self):
+        return time() - self.lastRefresh > self.refreshTime
+
+    def _refreshDomain(self):
+        # Refreshing the domain object in order to pick up changes as,
+        # for example, the domain upgrade.
+        self.log.debug("Refreshing domain %s", self.sdUUID)
+        sdCache.manuallyRemoveDomain(self.sdUUID)
+        self.lastRefresh = time()
+
+    # Deferred initialization
+
+    def _initializeDomain(self):
+        # We should produce the domain inside the monitoring loop because
+        # it might take some time and we don't want to slow down the thread
+        # start (and anything else that relies on that as for example
+        # updateMonitoringThreads). It also needs to be inside the loop
+        # since it might fail and we want keep trying until we succeed or
+        # the domain is deactivated.
+        if self.domain is None:
+            self.domain = sdCache.produce(self.sdUUID)
+
+        if self.isIsoDomain is None:
+            # The isIsoDomain assignment is delayed because the isoPrefix
+            # discovery might fail (if the domain suddenly disappears) and
+            # we could risk to never try to set it again.
+            isIsoDomain = self.domain.isISO()
+            if isIsoDomain:
+                self.isoPrefix = self.domain.getIsoDomainImagesDir()
+            self.isIsoDomain = isIsoDomain
+
+    # Collecting monitoring info
+
+    def _checkDomain(self):
+        self.domain.selftest()
+
+    def _checkReadDelay(self):
+        self.nextStatus.readDelay = self.domain.getReadDelay()
+
+    def _collectStats(self):
+        stats = self.domain.getStats()
+        self.nextStatus.diskUtilization = (stats["disktotal"],
+                                           stats["diskfree"])
+
+        self.nextStatus.vgMdUtilization = (stats["mdasize"],
+                                           stats["mdafree"])
+
+        self.nextStatus.vgMdHasEnoughFreeSpace = stats["mdavalid"]
+        self.nextStatus.vgMdFreeBelowThreashold = stats["mdathreshold"]
+
+        masterStats = self.domain.validateMaster()
+        self.nextStatus.masterValid = masterStats['valid']
+        self.nextStatus.masterMounted = masterStats['mount']
+
+        self.nextStatus.hasHostId = self.domain.hasHostId(self.hostId)
+        self.nextStatus.isoPrefix = self.isoPrefix
+        self.nextStatus.version = self.domain.getVersion()
+
+    # Managing host id
+
+    def _shouldAcquireHostId(self):
+        # An ISO domain can be shared by multiple pools
+        return (not self.isIsoDomain and
+                self.nextStatus.valid and
+                self.nextStatus.hasHostId is False)
+
+    def _shouldReleaseHostId(self):
+        # If this is an ISO domain we didn't acquire the host id and releasing
+        # it is superfluous.
+        return self.domain and not self.isIsoDomain
+
+    def _acquireHostId(self):
+        try:
+            self.domain.acquireHostId(self.hostId, async=True)
+        except:
+            self.log.debug("Unable to issue the acquire host id %s "
+                           "request for domain %s", self.hostId,
+                           self.sdUUID, exc_info=True)
+
+    def _releaseHostId(self):
+        try:
+            self.domain.releaseHostId(self.hostId, unused=True)
+        except:
+            self.log.debug("Unable to release the host id %s for domain "
+                           "%s", self.hostId, self.sdUUID, exc_info=True)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I87ac6a82e560bc4360a3bc3f6f6fd94623678cc2
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>


More information about the vdsm-patches mailing list