Change in vdsm[ovirt-3.6]: sp: update domain links on state change

ishaby at redhat.com ishaby at redhat.com
Sun Jan 31 14:26:55 UTC 2016


Hello Nir Soffer,

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

    https://gerrit.ovirt.org/52902

to review the following change.

Change subject: sp: update domain links on state change
......................................................................

sp: update domain links on state change

In some situations (e.g. after a reconstructMaster command) the domain
links may be missing and it's impossible to recreate them on
connectStoragePool or refreshStoragePool since the storage may be
unreachable. The best we can do is recreate them as soon as the domains
are visible again.

This patch is based on a patch that was merged into ovirt-engine-3.4
but was never merged into master: https://gerrit.ovirt.org/#/c/27466/ .
It also deals with a few issues that patch had:

- It adds the "@unsecured" decoration to _domainStateChange, since
  without it we get a SecureError in case that the host that runs it is
  not the SPM.

- In the other patch, onDomainStateChange was registered in the
  constructor of StoragePool.
  This is a waste of resources since these events can be called only
  after the connect method is called (specifically, after __rebuild is
  called from within connect).
  Also, onDomainStateChange is never unregistered, causing a leak of a
  registration for every pool disconnection or unsuccessful connection.
  This patch moves the registration to onDomainStateChange to be right
  before it can actually be called (in connect), and calls unregister if
  an error has occurred in __rebuild.
  A call to unregister was also added to the disconnect method, so that
  we won't be listening to events that cannot be called anymore.

- This patch locks the pool and the domain id's in _domainStateChange:
  On the pool, a shared lock is taken since we query the pool's state.
  On the domain, we take a new <domain_uuid>_repo exclusive lock to
  prevent concurrent calls to_refreshDomainLinks.

Relates-To: https://bugzilla.redhat.com/show_bug.cgi?id=1091030
Bug-Url: https://bugzilla.redhat.com/1271771
Change-Id: If116100d3ae967f6a5490a2d91bf923e953cb4ee
Signed-off-by: Idan Shaby <ishaby at redhat.com>
Signed-off-by: Federico Simoncelli <fsimonce at redhat.com>
Reviewed-on: https://gerrit.ovirt.org/51393
Reviewed-by: Nir Soffer <nsoffer at redhat.com>
Continuous-Integration: Jenkins CI
---
M vdsm/storage/sp.py
1 file changed, 47 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/02/52902/1

diff --git a/vdsm/storage/sp.py b/vdsm/storage/sp.py
index 9318bf9..f5e69e8 100644
--- a/vdsm/storage/sp.py
+++ b/vdsm/storage/sp.py
@@ -89,6 +89,8 @@
         self.domainMonitor = domainMonitor
         self._upgradeCallback = partial(StoragePool._upgradePoolDomain,
                                         proxy(self))
+        self._domainStateCallback = partial(
+            StoragePool._domainStateChange, proxy(self))
         self._backend = None
 
     def __is_secure__(self):
@@ -131,6 +133,24 @@
     @unsecured
     def getBackend(self):
         return self._backend
+
+    @unsecured
+    def _domainStateChange(self, sdUUID, isValid):
+        if not isValid:
+            return
+
+        domain = sdCache.produce(sdUUID)
+        with rmanager.acquireResource(STORAGE, self.spUUID,
+                                      rm.LockType.shared):
+            if sdUUID not in self.getDomains(activeOnly=True):
+                self.log.debug("Domain %s is not an active pool domain, "
+                               "skipping domain links refresh",
+                               sdUUID)
+                return
+            with rmanager.acquireResource(STORAGE, sdUUID + "_repo",
+                                          rm.LockType.exclusive):
+                self.log.debug("Refreshing domain links for %s", sdUUID)
+                self._refreshDomainLinks(domain)
 
     def _upgradePoolDomain(self, sdUUID, isValid):
         # This method is called everytime the onDomainStateChange
@@ -628,11 +648,34 @@
         # Make sure SDCache doesn't have stale data (it can be in case of FC)
         sdCache.invalidateStorage()
         sdCache.refresh()
-        # Rebuild whole Pool
-        self.__rebuild(msdUUID=msdUUID, masterVersion=masterVersion)
-        self.__createMailboxMonitor()
+        # Since we start the monitor threads during the call to __rebuild,
+        # we should start watching the domains states right before we call
+        # __rebuild.
+        self._startWatchingDomainsState()
+        try:
+            # Rebuild whole Pool
+            self.__rebuild(msdUUID=msdUUID, masterVersion=masterVersion)
+            self.__createMailboxMonitor()
+        except Exception:
+            self._stopWatchingDomainsState()
+            raise
 
         return True
+
+    @unsecured
+    def _startWatchingDomainsState(self):
+        self.log.debug("Start watching domains state")
+        self.domainMonitor.onDomainStateChange.register(
+            self._domainStateCallback)
+
+    @unsecured
+    def _stopWatchingDomainsState(self):
+        self.log.debug("Stop watching domains state")
+        try:
+            self.domainMonitor.onDomainStateChange.unregister(
+                self._domainStateCallback)
+        except KeyError:
+            self.log.warning("Domain state callback is not registered")
 
     @unsecured
     def stopMonitoringDomains(self):
@@ -660,6 +703,7 @@
             fileUtils.cleanupdir(self.poolPath)
 
         self.stopMonitoringDomains()
+        self._stopWatchingDomainsState()
         return True
 
     @unsecured


-- 
To view, visit https://gerrit.ovirt.org/52902
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: If116100d3ae967f6a5490a2d91bf923e953cb4ee
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Idan Shaby <ishaby at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>


More information about the vdsm-patches mailing list