Workaround issue with greenthreads and lockfiles

 * Adds a GreenLockFile that always works in greenthreads
 * Adds test to verify that regular Lockfile is broken
 * Adds test to verify that GreenLockfile works
 * Adds note about limitation of external locks
 * Adds test showing limitation of nested locks
 * Fixes bug 956313

Change-Id: I11cd1206611aa4862dadd2fcc077c4c2e0f798f6

3 files changed, 98 insertions, 1 deletions

diff --git a/nova/tests/test_misc.py b/nova/tests/test_misc.py
index 0baf38236..63c2773bf 100644
--- a/nova/tests/test_misc.py
+++ b/nova/tests/test_misc.py
@@ -22,6 +22,7 @@ import select
 
 from eventlet import greenpool
 from eventlet import greenthread
+import lockfile
 
 from nova import exception
 from nova import test
@@ -134,6 +135,21 @@ class LockTestCase(test.TestCase):
         self.assertEqual(saved_sem_num, len(utils._semaphores),
                          "Semaphore leak detected")
 
+    def test_nested_external_fails(self):
+        """We can not nest external syncs"""
+
+        @utils.synchronized('testlock1', external=True)
+        def outer_lock():
+
+            @utils.synchronized('testlock2', external=True)
+            def inner_lock():
+                pass
+            inner_lock()
+        try:
+            self.assertRaises(lockfile.NotMyLock, outer_lock)
+        finally:
+            utils.cleanup_file_locks()
+
     def test_synchronized_externally(self):
         """We can lock across multiple processes"""
         rpipe1, wpipe1 = os.pipe()
diff --git a/nova/tests/test_utils.py b/nova/tests/test_utils.py
index f29da5466..501ff91d0 100644
--- a/nova/tests/test_utils.py
+++ b/nova/tests/test_utils.py
@@ -24,7 +24,10 @@ import shutil
 import StringIO
 import tempfile
 
+import eventlet
+from eventlet import greenpool
 import iso8601
+import lockfile
 import mox
 
 import nova
@@ -832,6 +835,42 @@ class Iso8601TimeTest(test.TestCase):
         self._instaneous(normed, 2012, 2, 13, 23, 53, 07, 0)
 
 
+class TestGreenLocks(test.TestCase):
+    def test_concurrent_green_lock_succeeds(self):
+        """Verify spawn_n greenthreads with two locks run concurrently.
+
+        This succeeds with spawn but fails with spawn_n because lockfile
+        gets the same thread id for both spawn_n threads. Our workaround
+        of using the GreenLockFile will work even if the issue is fixed.
+        """
+        self.completed = False
+        with utils.tempdir() as tmpdir:
+
+            def locka(wait):
+                a = utils.GreenLockFile(os.path.join(tmpdir, 'a'))
+                a.acquire()
+                wait.wait()
+                a.release()
+                self.completed = True
+
+            def lockb(wait):
+                b = utils.GreenLockFile(os.path.join(tmpdir, 'b'))
+                b.acquire()
+                wait.wait()
+                b.release()
+
+            wait1 = eventlet.event.Event()
+            wait2 = eventlet.event.Event()
+            pool = greenpool.GreenPool()
+            pool.spawn_n(locka, wait1)
+            pool.spawn_n(lockb, wait2)
+            wait2.send()
+            eventlet.sleep(0)
+            wait1.send()
+            pool.waitall()
+        self.assertTrue(self.completed)
+
+
 class TestLockCleanup(test.TestCase):
     """unit tests for utils.cleanup_file_locks()"""
 
diff --git a/nova/utils.py b/nova/utils.py
index e375f11e5..a23feea73 100644
--- a/nova/utils.py
+++ b/nova/utils.py
@@ -36,12 +36,14 @@ import socket
 import struct
 import sys
 import tempfile
+import threading
 import time
 import types
 import uuid
 import warnings
 from xml.sax import saxutils
 
+from eventlet import corolocal
 from eventlet import event
 from eventlet import greenthread
 from eventlet import semaphore
@@ -838,6 +840,33 @@ else:
     anyjson.force_implementation("nova.utils")
 
 
+class GreenLockFile(lockfile.FileLock):
+    """Implementation of lockfile that allows for a lock per greenthread.
+
+    Simply implements lockfile:LockBase init with an addiontall suffix
+    on the unique name of the greenthread identifier
+    """
+    def __init__(self, path, threaded=True):
+        self.path = path
+        self.lock_file = os.path.abspath(path) + ".lock"
+        self.hostname = socket.gethostname()
+        self.pid = os.getpid()
+        if threaded:
+            t = threading.current_thread()
+            # Thread objects in Python 2.4 and earlier do not have ident
+            # attrs.  Worm around that.
+            ident = getattr(t, "ident", hash(t))
+            gident = corolocal.get_ident()
+            self.tname = "-%x-%x" % (ident & 0xffffffff, gident & 0xffffffff)
+        else:
+            self.tname = ""
+        dirname = os.path.dirname(self.lock_file)
+        self.unique_name = os.path.join(dirname,
+                                        "%s%s.%s" % (self.hostname,
+                                                     self.tname,
+                                                     self.pid))
+
+
 _semaphores = {}
 
 
@@ -869,6 +898,19 @@ def synchronized(name, external=False):
     a method decorated with @synchronized('mylock', external=True), only one
     of them will execute at a time.
 
+    Important limitation: you can only have one external lock running per
+    thread at a time. For example the following will fail:
+
+        @utils.synchronized('testlock1', external=True)
+        def outer_lock():
+
+            @utils.synchronized('testlock2', external=True)
+            def inner_lock():
+                pass
+            inner_lock()
+
+        outer_lock()
+
     """
 
     def wrap(f):
@@ -893,7 +935,7 @@ def synchronized(name, external=False):
                               {'lock': name, 'method': f.__name__})
                     lock_file_path = os.path.join(FLAGS.lock_path,
                                                   'nova-%s' % name)
-                    lock = lockfile.FileLock(lock_file_path)
+                    lock = GreenLockFile(lock_file_path)
                     with lock:
                         LOG.debug(_('Got file lock "%(lock)s" for '
                                     'method "%(method)s"...') %