Make use of new fair locking subsystem

Makefile

@@ -100,7 +100,7 @@ precommit: build
 	CHANGED=$$(git status --porcelain $(SM_PY_FILES) | awk '{print $$2}'); \
 	for i in $$CHANGED; do \
 		echo Checking $${i} ...; \
-		PYTHONPATH=./drivers:$$PYTHONPATH $(PYLINT) --rcfile=tests/pylintrc $${i}; \
+		PYTHONPATH=./drivers:./misc/fairlock:$$PYTHONPATH $(PYLINT) --rcfile=tests/pylintrc $${i}; \
 		[ $$? -ne 0 ] && QUIT=1 ; \
 	done; \
 	if [ $$QUIT -ne 0 ]; then \
@@ -111,7 +111,7 @@ precommit: build
 
 .PHONY: precheck
 precheck: build
-	PYTHONPATH=./drivers:$$PYTHONPATH $(PYLINT) --rcfile=tests/pylintrc $(SM_PY_FILES)
+	PYTHONPATH=./drivers:./misc/fairlock:$$PYTHONPATH $(PYLINT) --rcfile=tests/pylintrc $(SM_PY_FILES)
 	echo "Precheck succeeded with no outstanding issues found."
 
 .PHONY: install

drivers/lvutil.py

@@ -22,8 +22,7 @@
 import errno
 import time
 
-import lock
-import lock_queue
+from fairlock import Fairlock
 import util
 import xs_errors
 import xml.dom.minidom
@@ -112,47 +111,6 @@ def extract_vgname(str_in):
 
     return None
 
-class LvmLockContext(lock_queue.LockQueue):
-    """
-    Context manager around the LVM lock.
-
-    To allow for higher level operations, e.g. VDI snapshot to pre-emptively
-    acquire the lock to encapsulte a set of calls and avoid having to reacquire
-    the locks for each LVM call.
-    """
-
-    def __init__(self, cmd=None):
-        self.cmd = cmd
-        self.locked = False
-        try:
-            super().__init__(LVM_LOCK)
-        except Exception as e:
-            util.SMlog(f"===> LvmLockContext __init__ {e} {traceback.format_exc()}")
-            raise
-
-    def __enter__(self):
-        if self.cmd and '--readonly' in self.cmd:
-            return
-
-        while True:
-            try:
-                super().__enter__()
-            except Exception as e:
-                util.SMlog(f"===> LvmLockContext __enter__ {e} {traceback.format_exc()}")
-                continue
-            break
-
-        self.locked = True
-
-    def __exit__(self, exc_type, value, tbck):
-        if self.locked:
-            try:
-                super().__exit__(exc_type, value, tbck)
-            except Exception as e:
-                util.SMlog(f"===> LvmLockContext __exit__ {e} {traceback.format_exc()}")
-                raise
-
-
 LVM_RETRY_ERRORS = [
     "Incorrect checksum in metadata area header"
 ]
@@ -223,7 +181,7 @@ def cmd_lvm(cmd, pread_func=util.pread2, *args):
             util.SMlog("CMD_LVM: Not all lvm arguments are of type 'str'")
             return None
 
-    with LvmLockContext(cmd):
+    with Fairlock("devicemapper"):
         start_time = time.time()
         stdout = pread_func([os.path.join(LVM_BIN, lvm_cmd)] + lvm_args, * args)
         end_time = time.time()
@@ -695,7 +653,8 @@ def activateNoRefcount(path, refresh):
         text = cmd_lvm([CMD_LVCHANGE, "--refresh", path])
         mapperDevice = path[5:].replace("-", "--").replace("/", "-")
         cmd = [CMD_DMSETUP, "table", mapperDevice]
-        ret = util.pread(cmd)
+        with Fairlock("devicemapper"):
+            ret = util.pread(cmd)
         util.SMlog("DM table for %s: %s" % (path, ret.strip()))
         # Restore slave mode lvm.conf
         os.environ['LVM_SYSTEM_DIR'] = DEF_LVM_CONF
@@ -737,7 +696,8 @@ def _checkActive(path):
     mapperDevice = path[5:].replace("-", "--").replace("/", "-")
     cmd = [CMD_DMSETUP, "status", mapperDevice]
     try:
-        ret = util.pread2(cmd)
+        with Fairlock("devicemapper"):
+            ret = util.pread2(cmd)
         mapperDeviceExists = True
         util.SMlog("_checkActive: %s: %s" % (mapperDevice, ret))
     except util.CommandException:
@@ -776,7 +736,8 @@ def _lvmBugCleanup(path):
     cmd_rf = [CMD_DMSETUP, "remove", mapperDevice, "--force"]
 
     try:
-        util.pread(cmd_st, expect_rc=1)
+        with Fairlock("devicemapper"):
+            util.pread(cmd_st, expect_rc=1)
     except util.CommandException as e:
         if e.code == 0:
             nodeExists = True
@@ -791,13 +752,15 @@ def _lvmBugCleanup(path):
         util.SMlog("_lvmBugCleanup: removing dm device %s" % mapperDevice)
         for i in range(LVM_FAIL_RETRIES):
             try:
-                util.pread2(cmd_rm)
+                with Fairlock("devicemapper"):
+                    util.pread2(cmd_rm)
                 break
             except util.CommandException as e:
                 if i < LVM_FAIL_RETRIES - 1:
                     util.SMlog("Failed on try %d, retrying" % i)
                     try:
-                        util.pread(cmd_st, expect_rc=1)
+                        with Fairlock("devicemapper"):
+                            util.pread(cmd_st, expect_rc=1)
                         util.SMlog("_lvmBugCleanup: dm device {}"
                                    " removed".format(mapperDevice)
                                    )
@@ -841,7 +804,8 @@ def removeDevMapperEntry(path, strict=True):
         if not strict:
             cmd = [CMD_DMSETUP, "status", path]
             try:
-                util.pread(cmd, expect_rc=1)
+                with Fairlock("devicemapper"):
+                    util.pread(cmd, expect_rc=1)
                 return True
             except:
                 pass  # Continuining will fail and log the right way

drivers/mpath_cli.py

@@ -18,6 +18,7 @@
 import util
 import re
 import time
+from fairlock import Fairlock
 
 
 class MPathCLIFail(Exception):
@@ -32,7 +33,8 @@ def __str__(self):
 
 def mpexec(cmd):
     util.SMlog("mpath cmd: %s" % cmd)
-    (rc, stdout, stderr) = util.doexec(mpathcmd, cmd)
+    with Fairlock("devicemapper"):
+        (rc, stdout, stderr) = util.doexec(mpathcmd, cmd)
     if stdout != "multipathd> ok\nmultipathd> " \
             and stdout != "multipathd> " + cmd + "\nok\nmultipathd> ":
         raise MPathCLIFail
@@ -77,7 +79,8 @@ def is_working():
 
 def do_get_topology(cmd):
     util.SMlog("mpath cmd: %s" % cmd)
-    (rc, stdout, stderr) = util.doexec(mpathcmd, cmd)
+    with Fairlock("devicemapper"):
+        (rc, stdout, stderr) = util.doexec(mpathcmd, cmd)
     util.SMlog("mpath output: %s" % stdout)
     lines = stdout.split('\n')[:-1]
     if len(lines):
@@ -109,7 +112,8 @@ def list_paths(scsi_id):
 def list_maps():
     cmd = "list maps"
     util.SMlog("mpath cmd: %s" % cmd)
-    (rc, stdout, stderr) = util.doexec(mpathcmd, cmd)
+    with Fairlock("devicemapper"):
+        (rc, stdout, stderr) = util.doexec(mpathcmd, cmd)
     util.SMlog("mpath output: %s" % stdout)
     return [x.split(' ')[0] for x in stdout.split('\n')[2:-1]]
 

drivers/mpath_dmp.py

@@ -23,6 +23,7 @@
 import scsiutil
 import wwid_conf
 import errno
+from fairlock import Fairlock
 
 DMPBIN = "/sbin/multipath"
 DEVMAPPERPATH = "/dev/mapper"
@@ -77,10 +78,11 @@ def _resetDMP(sid, explicit_unmap=False):
 # tables. In that case, list_paths will return [], but remove_map might
 # throw an exception. Catch it and ignore it.
     if explicit_unmap:
-        util.retry(lambda: util.pread2(['/usr/sbin/multipath', '-f', sid]),
-                   maxretry=3, period=4)
-        util.retry(lambda: util.pread2(['/usr/sbin/multipath', '-W']), maxretry=3,
-                   period=4)
+        with Fairlock("devicemapper"):
+            util.retry(lambda: util.pread2(['/usr/sbin/multipath', '-f', sid]),
+                    maxretry=3, period=4)
+            util.retry(lambda: util.pread2(['/usr/sbin/multipath', '-W']), maxretry=3,
+                    period=4)
     else:
         mpath_cli.ensure_map_gone(sid)
 
@@ -109,33 +111,35 @@ def refresh(sid, npaths):
 def _is_valid_multipath_device(sid):
 
     # Check if device is already multipathed
-    (ret, stdout, stderr) = util.doexec(['/usr/sbin/multipath', '-l', sid])
+    with Fairlock("devicemapper"):
+        (ret, stdout, stderr) = util.doexec(['/usr/sbin/multipath', '-l', sid])
     if not stdout + stderr:
-        (ret, stdout, stderr) = util.doexec(['/usr/sbin/multipath', '-ll', sid])
+        with Fairlock("devicemapper"):
+            (ret, stdout, stderr) = util.doexec(['/usr/sbin/multipath', '-ll', sid])
     if not stdout + stderr:
-        (ret, stdout, stderr) = util.doexec(['/usr/sbin/multipath', '-a', sid])
+        with Fairlock("devicemapper"):
+            (ret, stdout, stderr) = util.doexec(['/usr/sbin/multipath', '-a', sid])
         if ret < 0:
             util.SMlog("Failed to add {}: wwid could be explicitly "
-                       "blacklisted\n Continue with multipath disabled for "
-                       "this SR".format(sid))
+                    "blacklisted\n Continue with multipath disabled for "
+                    "this SR".format(sid))
             return False
 
         by_scsid_path = "/dev/disk/by-scsid/" + sid
         if os.path.exists(by_scsid_path):
             devs = os.listdir(by_scsid_path)
         else:
-            util.SMlog("Device {} is not ready yet, skipping multipath check"
-                       .format(by_scsid_path))
+            util.SMlog("Device {} is not ready yet, skipping multipath check".format(by_scsid_path))
             return False
         ret = 1
         # Some paths might be down, check all associated devices
         for dev in devs:
             devpath = os.path.join(by_scsid_path, dev)
             real_path = util.get_real_path(devpath)
-            (ret, stdout, stderr) = util.doexec(['/usr/sbin/multipath', '-c',
-                                                 real_path])
-            if ret == 0:
-                break
+            with Fairlock("devicemapper"):
+                (ret, stdout, stderr) = util.doexec(['/usr/sbin/multipath', '-c', real_path])
+                if ret == 0:
+                    break
 
         if ret == 1:
             # This is very fragile but it is not a good sign to fail without
@@ -146,8 +150,8 @@ def _is_valid_multipath_device(sid):
             if not stdout + stderr:
                 # Attempt to cleanup wwids file before raising
                 try:
-                    (ret, stdout, stderr) = util.doexec(['/usr/sbin/multipath',
-                                                         '-w', sid])
+                    with Fairlock("devicemapper"):
+                        (ret, stdout, stderr) = util.doexec(['/usr/sbin/multipath', '-w', sid])
                 except OSError:
                     util.SMlog("Error removing {} from wwids file".format(sid))
                 raise xs_errors.XenError('MultipathGenericFailure',
@@ -166,10 +170,8 @@ def _refresh_DMP(sid, npaths):
     path = os.path.join(DEVMAPPERPATH, sid)
     # If the mapper path doesn't exist force a reload in multipath
     if not os.path.exists(path):
-        util.retry(lambda: util.pread2(
-            ['/usr/sbin/multipath', '-r', sid]),
-                   maxretry=3,
-                   period=4)
+        with Fairlock("devicemapper"):
+            util.retry(lambda: util.pread2(['/usr/sbin/multipath', '-r', sid]), maxretry=3, period=4)
         util.wait_for_path(path, 30)
     if not os.path.exists(path):
         raise xs_errors.XenError('MultipathMapperPathMissing',

tests/test_LVHDSR.py

@@ -54,7 +54,7 @@ def create_LVHDSR(self, master=False, command='foo', sr_uuid=None):
             sr_uuid = str(uuid.uuid4())
         return LVHDSR.LVHDSR(srcmd, sr_uuid)
 
-    @mock.patch('lvutil.LvmLockContext', autospec=True)
+    @mock.patch('lvutil.Fairlock', autospec=True)
     @mock.patch('lvhdutil.getVDIInfo', autospec=True)
     @mock.patch('LVHDSR.Lock', autospec=True)
     @mock.patch('SR.XenAPI')

tests/test_LVHDoHBASR.py

@@ -87,7 +87,7 @@ def setUp(self):
         self.mock_lvhdsr = lvhdsr_patcher.start()
         util_patcher = mock.patch('LVHDoHBASR.util', autospec=True)
         self.mock_util = util_patcher.start()
-        lc_patcher = mock.patch('LVHDSR.lvmcache.lvutil.LvmLockContext', autospec=True)
+        lc_patcher = mock.patch('LVHDSR.lvmcache.lvutil.Fairlock', autospec=True)
         self.mock_lc = lc_patcher.start()
         xenapi_patcher = mock.patch('SR.XenAPI')
         self.mock_xapi = xenapi_patcher.start()

tests/test_LVHDoISCSISR.py

@@ -162,7 +162,7 @@ def deepcopy(to_copy):
 
         lock_patcher = mock.patch('LVHDSR.Lock')
         self.mock_lock = lock_patcher.start()
-        lvlock_patcher = mock.patch('LVHDSR.lvutil.LvmLockContext')
+        lvlock_patcher = mock.patch('LVHDSR.lvutil.Fairlock')
         self.mock_lvlock = lvlock_patcher.start()
 
         self.addCleanup(mock.patch.stopall)

tests/test_lvutil.py

@@ -26,7 +26,7 @@ def decorated(self, context, *args, **kwargs):
 
 class TestCreate(unittest.TestCase):
     def setUp(self):
-        lock_patcher = mock.patch('lvutil.LvmLockContext', autospec=True)
+        lock_patcher = mock.patch('lvutil.Fairlock', autospec=True)
         self.addCleanup(lock_patcher.stop)
         self.mock_lock = lock_patcher.start()
 
@@ -99,7 +99,7 @@ def test_create_percentage_has_precedence_over_size(self, mock_pread):
 
 class TestRemove(unittest.TestCase):
     def setUp(self):
-        lock_patcher = mock.patch('lvutil.LvmLockContext', autospec=True)
+        lock_patcher = mock.patch('lvutil.Fairlock', autospec=True)
         self.addCleanup(lock_patcher.stop)
         self.mock_lock = lock_patcher.start()
 
@@ -125,7 +125,7 @@ def test_remove_additional_config_param(self, mock_pread, _bugCleanup):
 class TestDeactivate(unittest.TestCase):
 
     def setUp(self):
-        lock_patcher = mock.patch('lvutil.LvmLockContext', autospec=True)
+        lock_patcher = mock.patch('lvutil.Fairlock', autospec=True)
         pathexists_patcher = mock.patch('lvutil.util.pathexists', autospec=True)
         lexists_patcher = mock.patch('lvutil.os.path.lexists', autospec=True)
         unlink_patcher = mock.patch('lvutil.os.unlink', autospec=True)
@@ -211,7 +211,7 @@ class TestActivate(unittest.TestCase):
     def setUp(self):
         self.addCleanup(mock.patch.stopall)
 
-        lock_patcher = mock.patch('lvutil.LvmLockContext', autospec=True)
+        lock_patcher = mock.patch('lvutil.Fairlock', autospec=True)
         self.mock_lock = lock_patcher.start()
         pathexists_patcher = mock.patch('lvutil.util.pathexists', autospec=True)
         self.mock_exists = pathexists_patcher.start()
@@ -310,44 +310,9 @@ def test_activate_noref_not_activated(self, lvsystem):
 
         self.assertIn('LV not activated', ce.exception.reason)
 
-class TestLvmLockContext(unittest.TestCase):
-
-    @mock.patch('lock_queue.LockQueue.__enter__')
-    @mock.patch('lock_queue.LockQueue.__exit__')
-    @mock.patch('lock_queue.LockQueue.__init__')
-    def test_LvmLockContext(self, m_init, m_exit, m_enter):
-        """
-        LvmLockContext should act as a wrapper around LockQueue.
-        """
-        with lvutil.LvmLockContext("pancakes"):
-            self.assertEqual(m_init.call_count, 1)
-            self.assertEqual(m_enter.call_count, 1)
-            self.assertEqual(m_exit.call_count, 0)
-
-        self.assertEqual(m_init.call_count, 1)
-        self.assertEqual(m_enter.call_count, 1)
-        self.assertEqual(m_exit.call_count, 1)
-
-    @mock.patch('lock_queue.LockQueue.__enter__')
-    @mock.patch('lock_queue.LockQueue.__exit__')
-    @mock.patch('lock_queue.LockQueue.__init__')
-    def test_LvmLockContext_readlonly(self, m_init, m_exit, m_enter):
-        """
-        If the LVM command is readonly then LvmLockContext should not do any
-        actual locking.
-        """
-        with lvutil.LvmLockContext("waffles --readonly"):
-            self.assertEqual(m_init.call_count, 1)
-            self.assertEqual(m_enter.call_count, 0)
-            self.assertEqual(m_exit.call_count, 0)
-
-        self.assertEqual(m_init.call_count, 1)
-        self.assertEqual(m_enter.call_count, 0)
-        self.assertEqual(m_exit.call_count, 0)
-
 
 @mock.patch('util.pread', autospec=True) # m_pread
-@mock.patch('lvutil.LvmLockContext', autospec=True) # _1
+@mock.patch('lvutil.Fairlock', autospec=True) # _1
 class Test_cmd_lvm(unittest.TestCase):
 
     def test_refuse_to_run_empty_list(self, _1, m_pread):

tests/test_mpath_dmp.py

@@ -30,6 +30,9 @@ def setUp(self):
         mpath_cli_patcher = mock.patch('mpath_dmp.mpath_cli', autospec=True)
         self.mock_mpath_cli = mpath_cli_patcher.start()
 
+        lock_patcher = mock.patch('mpath_dmp.Fairlock', autospec=True)
+        self.mock_lock = lock_patcher.start()
+
         self.addCleanup(mock.patch.stopall)
 
     @testlib.with_context