Change in vdsm[ovirt-3.6]: sp: Allow attaching of monitored domain

nsoffer at redhat.com nsoffer at redhat.com
Wed Dec 2 17:10:00 UTC 2015


Hello Adam Litke, Freddy Rolland,

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

    https://gerrit.ovirt.org/49597

to review the following change.

Change subject: sp: Allow attaching of monitored domain
......................................................................

sp: Allow attaching of monitored domain

When attaching a storage domain to the pool, the storage domain may be
an external domain being monitored via startMonitoringDomain, such as the
hosted engine storage domain.

In this case, the monitor may have already acquired the host id, or will
acquire it during the attach operation. We cannot tell since monitors
are using asynchronous acquire.

We must not release the host id when the domain is attached, since this
will cause sanlock to kill any process holding a resource on this
storage domain, such as the qemu process running the hosted engine vm.

We had a magical, sneaky, and unclear code for attaching a monitored
domain when starting a monitor twice. This code is now simplified and
documented, and logging is improved. This will be replaced with explicit
attach later.

Change-Id: Ia2fc1b19a6e70d4001a8f6d8c14bae7f65d0c192
Bug-Url: https://bugzilla.redhat.com/1269768
Signed-off-by: Nir Soffer <nsoffer at redhat.com>
Reviewed-on: https://gerrit.ovirt.org/48217
Continuous-Integration: Jenkins CI
Reviewed-by: Freddy Rolland <frolland at redhat.com>
Reviewed-by: Adam Litke <alitke at redhat.com>
---
M vdsm/storage/monitor.py
M vdsm/storage/sp.py
2 files changed, 35 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/97/49597/1

diff --git a/vdsm/storage/monitor.py b/vdsm/storage/monitor.py
index 0ec4766..25067d2 100644
--- a/vdsm/storage/monitor.py
+++ b/vdsm/storage/monitor.py
@@ -97,8 +97,23 @@
     def startMonitoring(self, sdUUID, hostId, poolDomain=True):
         monitor = self._monitors.get(sdUUID)
 
+        # TODO: Replace with explicit attach.
         if monitor is not None:
-            monitor.poolDomain |= poolDomain
+            if not poolDomain:
+                # Expected when hosted engine agent is restarting.
+                self.log.debug("Monitor for %s is already running", sdUUID)
+                return
+
+            if monitor.poolDomain:
+                self.log.warning("Monitor for %s is already attached to pool",
+                                 sdUUID)
+                return
+
+            # An external storage domain attached to the pool. From this point,
+            # the storage domain is managed by Vdsm.  Expected during Vdsm
+            # startup when using hosted engine.
+            self.log.info("Attaching monitor for %s to the pool", sdUUID)
+            monitor.poolDomain = True
             return
 
         self.log.info("Start monitoring %s", sdUUID)
@@ -115,6 +130,9 @@
                     if monitor.sdUUID in sdUUIDs]
         self._stopMonitors(monitors)
 
+    def isMonitoring(self, sdUUID):
+        return sdUUID in self._monitors
+
     def getDomainsStatus(self):
         for sdUUID, monitor in self._monitors.items():
             yield sdUUID, monitor.getStatus()
diff --git a/vdsm/storage/sp.py b/vdsm/storage/sp.py
index b8fd8f3..9318bf9 100644
--- a/vdsm/storage/sp.py
+++ b/vdsm/storage/sp.py
@@ -864,6 +864,10 @@
         Attach a storage domain to the storage pool.
         This marks the storage domain as "attached" and links it
         to the storage pool
+
+        The storage domain may be the hosted engine storage domain, which is
+        being monitored but does not belong to the pool yet.
+
          'sdUUID' - storage domain UUID
         """
         self.log.info("sdUUID=%s spUUID=%s", sdUUID, self.spUUID)
@@ -887,6 +891,8 @@
                              sdUUID, self.spUUID)
             return
 
+        # We must always acquire a host id, since even if this domain is being
+        # monitored, it may not have a host id yet.
         dom.acquireHostId(self.id)
 
         try:
@@ -907,12 +913,20 @@
                 domains[sdUUID] = sd.DOM_ATTACHED_STATUS
                 self._backend.setDomainsMap(domains)
                 self._refreshDomainLinks(dom)
-
             finally:
                 dom.releaseClusterLock()
 
         finally:
-            dom.releaseHostId(self.id)
+            # If we are monitoring this domain, we must not release the host
+            # id, as it will kill any process holding a resource on this
+            # domain, such as the qemu process running the hosted engine vm.
+            # TODO: Remove this check when the cluster lock supports reference
+            # counting.
+            if self.domainMonitor.isMonitoring(sdUUID):
+                self.log.debug("Domain %s is being monitored, leaving the "
+                               "host id acquired", sdUUID)
+            else:
+                dom.releaseHostId(self.id)
 
         self.updateMonitoringThreads()
 


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia2fc1b19a6e70d4001a8f6d8c14bae7f65d0c192
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.6
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland at redhat.com>


More information about the vdsm-patches mailing list