Change in vdsm[ovirt-3.3]: iscsi: Iscsi rescan cleanup

smizrahi at redhat.com smizrahi at redhat.com
Mon Nov 11 13:53:56 UTC 2013


Saggi Mizrahi has uploaded a new change for review.

Change subject: iscsi: Iscsi rescan cleanup
......................................................................

iscsi: Iscsi rescan cleanup

* Remove forceIscsiScan(): iscsiadm already does that when refreshing
  sessions
* Cleanup iscsi.rescan(): Replace complex logic with a simpler
  implementation since new iscsiadm takes care of all of this itself
* Make iscsi.rescan() more testable
* Test confusing timeout semantics of iscsi.rescan()

Bug-Url: https://bugzilla.redhat.com/964595
Change-Id: I842eb37cea3bd5e8cd357a310dd966081b242abd
Signed-off-by: Saggi Mizrahi <smizrahi at redhat.com>
---
M tests/Makefile.am
A tests/iscsiTests.py
M vdsm.spec.in
M vdsm/storage/iscsi.py
M vdsm/storage/iscsiadm.py
M vdsm/storage/multipath.py
M vdsm/supervdsmServer
7 files changed, 111 insertions(+), 62 deletions(-)


  git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/36/21136/1

diff --git a/tests/Makefile.am b/tests/Makefile.am
index f87eddd..5efae38 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -37,6 +37,7 @@
 	guestIFTests.py \
 	hooksTests.py \
 	ipwrapperTests.py \
+	iscsiTests.py \
 	jsonRpcTests.py \
 	jsonRpcUtils.py \
 	libvirtconnectionTests.py \
diff --git a/tests/iscsiTests.py b/tests/iscsiTests.py
new file mode 100644
index 0000000..44f6962
--- /dev/null
+++ b/tests/iscsiTests.py
@@ -0,0 +1,80 @@
+import threading
+import time
+from contextlib import contextmanager
+
+from testrunner import VdsmTestCase as TestCaseBase
+
+from storage import iscsi
+
+
+class AsyncStubOperation(object):
+    def __init__(self, timeout):
+        self._evt = threading.Event()
+        if timeout == 0:
+            self._evt.set()
+        else:
+            threading.Timer(timeout, self._evt.set)
+
+    def wait(self, timeout=None, cond=None):
+        if cond is not None:
+            raise Exception("TODO!!!")
+
+        self._evt.wait(timeout)
+
+    def stop(self):
+        self._evt.set()
+
+    def result(self):
+        if self._evt.is_set():
+            return (None, None)
+        else:
+            return None
+
+
+class RescanTimeoutTests(TestCaseBase):
+    def setUp(self):
+        self._iscsiadm_rescan_async = \
+            iscsi.iscsiadm.session_rescan_async
+        iscsi.iscsiadm.session_rescan_async = self._iscsi_stub
+        self._timeout = 0
+
+    def tearDown(self):
+        iscsi.iscsiadm.session_rescan_async = \
+            self._iscsiadm_rescan_async
+
+    def _iscsi_stub(self):
+        return AsyncStubOperation(self._timeout)
+
+    @contextmanager
+    def assertMaxDuration(self, maxtime):
+        start = time.time()
+        try:
+            yield
+        finally:
+            end = time.time()
+            elapsed = end - start
+            if maxtime < elapsed:
+                self.fail("Operation was too slow %fs > %fs" %
+                          (elapsed, maxtime))
+
+    @contextmanager
+    def assertMinDuration(self, mintime):
+        start = time.time()
+        try:
+            yield
+        finally:
+            end = time.time()
+            elapsed = end - start
+            if mintime > elapsed:
+                self.fail("Operation was too fast %fs > %fs" %
+                          (elapsed, mintime))
+
+    def testFast(self):
+        self._timeout = 0
+        with self.assertMinDuration(2):
+            iscsi.rescan(2, 4)
+
+    def testSlow(self):
+        self._timeout = 60
+        with self.assertMaxDuration(3):
+            iscsi.rescan(1, 2)
diff --git a/vdsm.spec.in b/vdsm.spec.in
index b0b89a1..cae9cab 100644
--- a/vdsm.spec.in
+++ b/vdsm.spec.in
@@ -146,7 +146,7 @@
 # Update the qemu-kvm requires when block_stream will be included
 Requires: qemu-kvm >= 2:0.12.1.2-2.295.el6_3.4
 Requires: qemu-img >= 2:0.12.1.2-2.295.el6_3.4
-Requires: iscsi-initiator-utils >= 6.2.0.872-15
+Requires: iscsi-initiator-utils >= 6.2.0.873-3
 Requires: device-mapper-multipath >= 0.4.9-52
 Requires: e2fsprogs >= 1.41.12-11
 Requires: fence-agents
diff --git a/vdsm/storage/iscsi.py b/vdsm/storage/iscsi.py
index 7da94ab..f4fdceb 100644
--- a/vdsm/storage/iscsi.py
+++ b/vdsm/storage/iscsi.py
@@ -30,7 +30,6 @@
 import time
 from collections import namedtuple
 
-from vdsm import constants
 import misc
 from vdsm.config import config
 import devicemapper
@@ -373,38 +372,15 @@
 
 
 @misc.samplingmethod
-def rescan():
-    try:
-        iscsiadm.session_rescan()
-    except iscsiadm.IscsiError:
-        pass
+def rescan(minTimeout=None, maxTimeout=None):
+    # FIXME: This whole thing is wrong from the core. We need to make rescan
+    #        completely async and have methods timeout on their own if they
+    #        can't find the devices they are looking for
+    if minTimeout is None:
+        minTimeout = config.getint('irs', 'scsi_rescan_minimal_timeout')
+    if maxTimeout is None:
+        maxTimeout = config.getint('irs', 'scsi_rescan_maximal_timeout')
 
-
- at misc.samplingmethod
-def forceScsiScan():
-    processes = []
-    minTimeout = config.getint('irs', 'scsi_rescan_minimal_timeout')
-    maxTimeout = config.getint('irs', 'scsi_rescan_maximal_timeout')
-    for hba in glob.glob(SCAN_PATTERN):
-        cmd = [constants.EXT_DD, 'of=' + hba]
-        p = misc.execCmd(cmd, sudo=False, sync=False)
-        try:
-            p.stdin.write("- - -")
-            p.stdin.flush()
-            p.stdin.close()
-        except OSError as e:
-            if p.wait(0) is False:
-                log.error("pid %s still running", p.pid)
-            log.warning("Error in rescan of hba:%s with returncode:%s and "
-                        "error message: %s", hba, p.returncode,
-                        p.stderr.read(1000))
-            if e.errno != errno.EPIPE:
-                raise
-            else:
-                log.warning("Ignoring error in rescan of hba %s: ",
-                            hba, exc_info=True)
-                continue
-        processes.append((hba, p))
     if (minTimeout > maxTimeout or minTimeout < 0):
         minTimeout = 2
         maxTimeout = 30
@@ -412,24 +388,13 @@
                     "illegal value: scsi_rescan_minimal_timeout or "
                     "scsi_rescan_maximal_timeout. Set to %s and %s seconds "
                     "respectively.", minTimeout, maxTimeout)
+
     log.debug("Performing SCSI scan, this will take up to %s seconds",
               maxTimeout)
+
+    rescanOp = iscsiadm.session_rescan_async()
     time.sleep(minTimeout)
-    for i in xrange(maxTimeout - minTimeout):
-        for p in processes[:]:
-            (hba, proc) = p
-            if proc.wait(0):
-                if proc.returncode != 0:
-                    log.warning('returncode for: %s is: %s', hba,
-                                proc.returncode)
-                processes.remove(p)
-        if not processes:
-            break
-        else:
-            time.sleep(1)
-    else:
-        log.warning("Still waiting for scsi scan of hbas: %s",
-                    tuple(hba for p in processes))
+    rescanOp.wait(timeout=(maxTimeout - minTimeout))
 
 
 def devIsiSCSI(dev):
diff --git a/vdsm/storage/iscsiadm.py b/vdsm/storage/iscsiadm.py
index cd28db8..9bb27ea 100644
--- a/vdsm/storage/iscsiadm.py
+++ b/vdsm/storage/iscsiadm.py
@@ -1,6 +1,7 @@
 from threading import Lock
 import misc
 from vdsm import constants
+from vdsm.utils import AsyncProcessOperation
 
 # iscsiadm exit statuses
 ISCSI_ERR_SESS_EXISTS = 15
@@ -71,7 +72,7 @@
 _iscsiadmLock = Lock()
 
 
-def _runCmd(args, hideValue=False):
+def _runCmd(args, hideValue=False, sync=True):
     # FIXME: I don't use supervdsm because this entire module has to just be
     # run as root and there is no such feature yet in supervdsm. When such
     # feature exists please change this.
@@ -88,7 +89,7 @@
                 if i < (len(printCmd) - 1):
                     printCmd[i + 1] = "****"
 
-        return misc.execCmd(cmd, printable=printCmd, sudo=True)
+        return misc.execCmd(cmd, printable=printCmd, sudo=True, sync=sync)
 
 
 def iface_exists(interfaceName):
@@ -294,12 +295,21 @@
     raise IscsiNodeError(rc, out, err)
 
 
-def session_rescan():
-    rc, out, err = _runCmd(["-m", "session", "-R"])
-    if rc == 0:
-        return
+def session_rescan_async():
+    proc = _runCmd(["-m", "session", "-R"], sync=False)
 
-    raise IscsiSessionError(rc, out, err)
+    def parse_result(rc, out, err):
+        if rc == 0:
+            return
+
+        raise IscsiSessionError(rc, out, err)
+
+    return AsyncProcessOperation(proc, parse_result)
+
+
+def session_rescan():
+    aop = session_rescan_async()
+    return aop.result()
 
 
 def session_logout(sessionId):
diff --git a/vdsm/storage/multipath.py b/vdsm/storage/multipath.py
index 1f9773c..29851f3 100644
--- a/vdsm/storage/multipath.py
+++ b/vdsm/storage/multipath.py
@@ -105,8 +105,6 @@
     # First ask iSCSI to rescan all its sessions
     iscsi.rescan()
 
-    supervdsm.getProxy().forceScsiScan()
-
     # Now let multipath daemon pick up new devices
     cmd = [constants.EXT_MULTIPATH, "-r"]
     misc.execCmd(cmd, sudo=True)
diff --git a/vdsm/supervdsmServer b/vdsm/supervdsmServer
index 40ec9df..6c2d3f5 100755
--- a/vdsm/supervdsmServer
+++ b/vdsm/supervdsmServer
@@ -51,7 +51,6 @@
 
 from lsblk import getLsBlk as _getLsBlk
 from storage.multipath import getScsiSerial as _getScsiSerial
-from storage.iscsi import forceScsiScan as _forceScsiScan
 from storage.iscsi import getDevIscsiInfo as _getdeviSCSIinfo
 from storage.iscsi import readSessionInfo as _readSessionInfo
 from supervdsm import _SuperVdsmManager
@@ -142,10 +141,6 @@
     @logDecorator
     def getScsiSerial(self, *args, **kwargs):
         return _getScsiSerial(*args, **kwargs)
-
-    @logDecorator
-    def forceScsiScan(self, *args, **kwargs):
-        return _forceScsiScan(*args, **kwargs)
 
     @logDecorator
     def removeDeviceMapping(self, devName):


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I842eb37cea3bd5e8cd357a310dd966081b242abd
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.3
Gerrit-Owner: Saggi Mizrahi <smizrahi at redhat.com>


More information about the vdsm-patches mailing list