Change in vdsm[master]: [WIP] Add a releaseHostId option to stop the DomainMonitorTh...

[Federico Simoncelli]

Federico Simoncelli has uploaded a new change for review.

Change subject: [WIP] Add a releaseHostId option to stop the DomainMonitorThread
......................................................................

[WIP] Add a releaseHostId option to stop the DomainMonitorThread

Bug-Id: https://bugzilla.redhat.com/show_bug.cgi?id=851151
Change-Id: I83458fb4146de7e402606916615533da305bd867
Signed-off-by: Federico Simoncelli 
---
M vdsm/storage/domainMonitor.py
M vdsm/storage/sp.py
2 files changed, 19 insertions(+), 9 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/81/7581/1

diff --git a/vdsm/storage/domainMonitor.py b/vdsm/storage/domainMonitor.py
index 95e2f7b..c1ec5a9 100644
--- a/vdsm/storage/domainMonitor.py
+++ b/vdsm/storage/domainMonitor.py
@@ -84,14 +84,14 @@
         # The domain should be added only after it succesfully started
         self._domains[sdUUID] = domainThread

-    def stopMonitoring(self, sdUUID):
+    def stopMonitoring(self, sdUUID, releaseHostId=False):
         # The domain monitor issues events that might become raceful if
         # stopMonitoring doesn't stop until the thread exits.
         # Eg: when a domain is detached the domain monitor is stopped and
         # the host id is released. If the monitor didn't actually exit it
         # might respawn a new acquire host id.
         try:
-            self._domains[sdUUID].stop()
+            self._domains[sdUUID].stop(releaseHostId=releaseHostId)
         except KeyError:
             return

@@ -100,13 +100,15 @@
     def getStatus(self, sdUUID):
         return self._domains[sdUUID].getStatus()

-    def close(self):
+    def close(self, releaseHostId=False):
         for sdUUID in self._domains.keys():
-            self.stopMonitoring(sdUUID)
+            self.stopMonitoring(sdUUID, releaseHostId=releaseHostId)

class DomainMonitorThread(object):
     log = logging.getLogger('Storage.DomainMonitorThread')
+
+    RELEASE_HOSTID_DEFAULT = False

def __init__(self, sdUUID, hostId, interval):
         self.thread = Thread(target=self._monitorLoop)
@@ -121,16 +123,20 @@
         self.nextStatus = DomainMonitorStatus()
         self.isIsoDomain = None
         self.lastRefresh = time()
+        self.releaseHostId = self.RELEASE_HOSTID_DEFAULT
         self.refreshTime = \
                     config.getint("irs", "repo_stats_cache_refresh_timeout")

def start(self):
         self.thread.start()

-    def stop(self, wait=True):
+    def stop(self, wait=True, releaseHostId):
+        self.releaseHostId = releaseHostId
+
         self.stopEvent.set()
         if wait:
             self.thread.join()
+
         self.domain = None

def getStatus(self):
@@ -151,13 +157,17 @@

# If this is an ISO domain we didn't acquire the host id and releasing
         # it is superfluous.
-        if not self.isIsoDomain:
+        if not self.isIsoDomain and self.releaseHostId:
             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)

+        # Resetting the releaseHostId value to its default just to be sure in
+        # case in the future we want to recycle the DomainMonitor objects.
+        self.releaseHostId = self.RELEASE_HOSTID_DEFAULT
+
     def _monitorDomain(self):
         self.nextStatus.clear()

diff --git a/vdsm/storage/sp.py b/vdsm/storage/sp.py
index 40de20a..e8c59f3 100644
--- a/vdsm/storage/sp.py
+++ b/vdsm/storage/sp.py
@@ -647,8 +647,8 @@

@unsecured
-    def stopMonitoringDomains(self):
-        self.domainMonitor.close()
+    def stopMonitoringDomains(self, releaseHostId=False):
+        self.domainMonitor.close(releaseHostId=releaseHostId)
         return True

@@ -675,7 +675,7 @@
         if os.path.exists(self.poolPath):
             fileUtils.cleanupdir(self.poolPath)

-        self.stopMonitoringDomains()
+        self.stopMonitoringDomains(releaseHostId=True)
         return True

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

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

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: [WIP] Add a releaseHostId option to stop the DomainMonitorThread
......................................................................

Patch Set 1:

Build Failed

http://jenkins.ovirt.info/job/patch_vdsm_unit_tests/752/ : FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I83458fb4146de7e402606916615533da305bd867
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: oVirt Jenkins CI Server

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: [WIP] Add a releaseHostId option to stop the DomainMonitorThread
......................................................................

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

(1 inline comment)

....................................................
File vdsm/storage/sp.py
Line 598:         finally:
Line 599:             self._setUnsafe()
Line 600: 
Line 601:             self._releaseTemporaryClusterLock(msdUUID)
Line 602:             self.stopMonitoringDomains()
Testing this patch I discovered that it introduces a race around here (I didn't have time to dig more into details).
Line 603: 
Line 604:         return True
Line 605: 
Line 606:     @unsecured

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I83458fb4146de7e402606916615533da305bd867
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli 
Gerrit-Reviewer: Federico Simoncelli 
Gerrit-Reviewer: Yeela Kaplan 
Gerrit-Reviewer: oVirt Jenkins CI Server

Itamar Heim has posted comments on this change.

Change subject: [WIP] Add a releaseHostId option to stop the DomainMonitorThread
......................................................................

Patch Set 1:

ping

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: [WIP] Add a releaseHostId option to stop the DomainMonitorThread
......................................................................

Patch Set 1:

(3 comments)

Looks ok after fixing stop parameters - but not clear why we need this. What was wrong by always releasing the host id?

....................................................
Commit Message
Line 3: AuthorDate: 2012-08-29 10:28:42 -0400
Line 4: Commit:     Federico Simoncelli 
Line 5: CommitDate: 2012-08-29 10:32:45 -0400
Line 6: 
Line 7: [WIP] Add a releaseHostId option to stop the DomainMonitorThread
Why?
Line 8: 
Line 9: Bug-Id: https://bugzilla.redhat.com/show_bug.cgi?id=851151
Line 10: Change-Id: I83458fb4146de7e402606916615533da305bd867

....................................................
File vdsm/storage/domainMonitor.py
Line 129: 
Line 130:     def start(self):
Line 131:         self.thread.start()
Line 132: 
Line 133:     def stop(self, wait=True, releaseHostId):
Should be:

def stop(self, wait=True, releaseHostId=RELEASE_HOSTID_DEFAULT):
        ...
Line 134:         self.releaseHostId = releaseHostId
Line 135: 
Line 136:         self.stopEvent.set()
Line 137:         if wait:

Line 165:                                "%s", self.hostId, self.sdUUID, exc_info=True)
Line 166: 
Line 167:         # Resetting the releaseHostId value to its default just to be sure in
Line 168:         # case in the future we want to recycle the DomainMonitor objects.
Line 169:         self.releaseHostId = self.RELEASE_HOSTID_DEFAULT
Should use try-finally:

try:
    loop...
finally:
    self.releaseHostId = self.RELEASE_HOSTID_DEFAULT
Line 170: 
Line 171:     def _monitorDomain(self):
Line 172:         self.nextStatus.clear()
Line 173:

Itamar Heim has posted comments on this change.

Change subject: [WIP] Add a releaseHostId option to stop the DomainMonitorThread
......................................................................

Patch Set 1:

ping

Itamar Heim has abandoned this change.

Change subject: [WIP] Add a releaseHostId option to stop the DomainMonitorThread
......................................................................

Abandoned

abandoning per no reply. please restore if still relevant.