Change in vdsm[ovirt-3.4]: hsm: move domainStateCallback to HSM

[Federico Simoncelli]

Federico Simoncelli has uploaded a new change for review.

Change subject: hsm: move domainStateCallback to HSM
......................................................................

hsm: move domainStateCallback to HSM

It seems that the garbage collector is not fast enough to clear the
callback per-pool that we added in commit:

7e30615 sp: update domain links on state change

This different approach will allow us to add only one callback (per
HSM) and forward the event to the current connected pools.

Change-Id: I7509aced432dfa77dbec727d57f507d1a9c550b7
Signed-off-by: Federico Simoncelli 
---
M vdsm/storage/hsm.py
M vdsm/storage/sp.py
2 files changed, 11 insertions(+), 5 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/78/27478/1

diff --git a/vdsm/storage/hsm.py b/vdsm/storage/hsm.py
index 7e4dc15..21a0002 100644
--- a/vdsm/storage/hsm.py
+++ b/vdsm/storage/hsm.py
@@ -37,6 +37,7 @@
 import types
 import math
 import stat
+from weakref import proxy

from vdsm.config import config
 import sp
@@ -399,6 +400,11 @@
         monitorInterval = config.getint('irs', 'sd_health_check_delay')
         self.domainMonitor = domainMonitor.DomainMonitor(monitorInterval)

+        self._domainStateCallback = partial(
+            HSM._onDomainStateChange, proxy(self))
+        self.domainMonitor.onDomainStateChange.register(
+            self._domainStateCallback)
+
     @property
     def ready(self):
         return self._ready
@@ -411,6 +417,10 @@
         """
         self.domainStateChangeCallbacks.add(callbackFunc)

+    def _onDomainStateChange(self, sdUUID, isValid):
+        for pool in self.pools.values():
+            pool.domainStateChangeEvent(sdUUID, isValid)
+
     def _hsmSchedule(self, name, func, *args):
         self.taskMng.scheduleJob("hsm", self.tasksDir, vars.task,
                                  name, func, *args)
diff --git a/vdsm/storage/sp.py b/vdsm/storage/sp.py
index 4d6a487..3d7ab55 100644
--- a/vdsm/storage/sp.py
+++ b/vdsm/storage/sp.py
@@ -104,10 +104,6 @@
         self.domainMonitor = domainMonitor
         self._upgradeCallback = partial(StoragePool._upgradePoolDomain,
                                         proxy(self))
-        self._domainStateCallback = partial(
-            StoragePool._domainStateChange, proxy(self))
-        self.domainMonitor.onDomainStateChange.register(
-            self._domainStateCallback)
         self._backend = None

def __is_secure__(self):
@@ -141,7 +137,7 @@
     def getBackend(self):
         return self._backend

-    def _domainStateChange(self, sdUUID, isValid):
+    def domainStateChangeEvent(self, sdUUID, isValid):
         if isValid and sdUUID in self.getDomains():
             self._refreshDomainLinks(sdCache.produce(sdUUID))

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: hsm: move domainStateCallback to HSM
......................................................................

Patch Set 2: Verified+1

It does its own dirty job of re-creating the links... and at least it doesn't need to be garbage collected. Meh.

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: hsm: move domainStateCallback to HSM
......................................................................

Patch Set 2:

(2 comments)

Looks good expect thread safety when accessing HSM.pools dict.

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

Line 418:         self.domainStateChangeCallbacks.add(callbackFunc)
Line 419: 
Line 420:     def _onDomainStateChange(self, sdUUID, isValid):
Line 421:         for pool in self.pools.values():
Line 422:             pool.domainStateChangeEvent(sdUUID, isValid)
This happens from an event thread - I'm sure that this is not thread safe as is.

Don't we use locking when accessing the pools dict?
Line 423: 
Line 424:     def _hsmSchedule(self, name, func, *args):
Line 425:         self.taskMng.scheduleJob("hsm", self.tasksDir, vars.task,
Line 426:                                  name, func, *args)

http://gerrit.ovirt.org/#/c/27478/2/vdsm/storage/sp.py
File vdsm/storage/sp.py:

Line 136:     @unsecured
Line 137:     def getBackend(self):
Line 138:         return self._backend
Line 139: 
Line 140:     def domainStateChangeEvent(self, sdUUID, isValid):
Do we have this pattern - xxxEvent? If we don't I prefer something like "domainStateChanged" or "domainStateDidChange" (if you have time and like this direction).
Line 141:         if isValid and sdUUID in self.getDomains():
Line 142:             self._refreshDomainLinks(sdCache.produce(sdUUID))
Line 143: 
Line 144:     def _upgradePoolDomain(self, sdUUID, isValid):

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: hsm: move domainStateCallback to HSM
......................................................................

Patch Set 2:

(1 comment)

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

Line 418:         self.domainStateChangeCallbacks.add(callbackFunc)
Line 419: 
Line 420:     def _onDomainStateChange(self, sdUUID, isValid):
Line 421:         for pool in self.pools.values():
Line 422:             pool.domainStateChangeEvent(sdUUID, isValid)
> This happens from an event thread - I'm sure that this is not thread safe a

It seems not (getPools, etc.). Anyway values() is going to create another list.
Line 423: 
Line 424:     def _hsmSchedule(self, name, func, *args):
Line 425:         self.taskMng.scheduleJob("hsm", self.tasksDir, vars.task,
Line 426:                                  name, func, *args)

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: hsm: move domainStateCallback to HSM
......................................................................

Patch Set 3: Verified+1

Method name change only.

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: hsm: move domainStateCallback to HSM
......................................................................

Patch Set 3: Code-Review+1

Probably has some races, but good enough.

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: hsm: move domainStateCallback to HSM
......................................................................

Patch Set 3: Code-Review+2

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: hsm: move domainStateCallback to HSM
......................................................................

hsm: move domainStateCallback to HSM

It seems that the garbage collector is not fast enough to clear the
callback per-pool that we added in commit:

7e30615 sp: update domain links on state change

This different approach will allow us to add only one callback (per
HSM) and forward the event to the current connected pools.

Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1086210
Relates-To: https://bugzilla.redhat.com/show_bug.cgi?id=1093924
Change-Id: I7509aced432dfa77dbec727d57f507d1a9c550b7
Signed-off-by: Federico Simoncelli 
Reviewed-on: http://gerrit.ovirt.org/27478
Reviewed-by: Nir Soffer 
Reviewed-by: Dan Kenigsberg 
---
M vdsm/storage/hsm.py
M vdsm/storage/sp.py
2 files changed, 11 insertions(+), 5 deletions(-)

Approvals:
  Nir Soffer: Looks good to me, but someone else must approve
  Federico Simoncelli: Verified
  Dan Kenigsberg: Looks good to me, approved

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: hsm: move domainStateCallback to HSM
......................................................................

Patch Set 4:

Build Successful

http://jenkins.ovirt.org/job/vdsm_3.4_create-rpms_merged/208/ : SUCCESS