From 6f2010ca086a1c770017643f631322d4734facd8 Mon Sep 17 00:00:00 2001 From: Johannes Erdfelt Date: Tue, 18 Sep 2012 20:03:53 +0000 Subject: Fix race condition in CacheConcurrencyTestCase Fixes bug 1052641 Recent changes made to fix bug 1035426 ended up creating a race condition in the tests for CacheConcurrencyTestCase. Since two greenthreads are used, the changes to utils.synchronized() can cause a race where one thread is deleting the lock directory out from under the other thread. An unrelated bug caused this race condition to be ignored since the greenthreads were never waited on and as a result, the exceptions never had a chance to be raised in the test case. Change-Id: I86b45414bbacd12bd3f5af323d26e0861c354ba8 --- nova/tests/test_libvirt.py | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/nova/tests/test_libvirt.py b/nova/tests/test_libvirt.py index 19839c56e..8e052a104 100644 --- a/nova/tests/test_libvirt.py +++ b/nova/tests/test_libvirt.py @@ -375,9 +375,17 @@ class CacheConcurrencyTestCase(test.TestCase): super(CacheConcurrencyTestCase, self).setUp() self.flags(instances_path='nova.compute.manager') + # utils.synchronized() will create the lock_path for us if it + # doesn't already exist. It will also delete it when it's done, + # which can cause race conditions with the multiple threads we + # use for tests. So, create the path here so utils.synchronized() + # won't delete it out from under one of the threads. + self.lock_path = os.path.join(FLAGS.instances_path, 'locks') + utils.ensure_tree(self.lock_path) + def fake_exists(fname): basedir = os.path.join(FLAGS.instances_path, FLAGS.base_dir_name) - if fname == basedir: + if fname == basedir or fname == self.lock_path: return True return False @@ -394,6 +402,11 @@ class CacheConcurrencyTestCase(test.TestCase): def tearDown(self): imagebackend.libvirt_utils = libvirt_utils + + # Make sure the lock_path for this test is cleaned up + if os.path.exists(self.lock_path): + shutil.rmtree(self.lock_path) + super(CacheConcurrencyTestCase, self).tearDown() def test_same_fname_concurrency(self): @@ -401,11 +414,11 @@ class CacheConcurrencyTestCase(test.TestCase): backend = imagebackend.Backend(False) wait1 = eventlet.event.Event() done1 = eventlet.event.Event() - eventlet.spawn(backend.image('instance', 'name').cache, + thr1 = eventlet.spawn(backend.image('instance', 'name').cache, _concurrency, 'fname', None, wait=wait1, done=done1) wait2 = eventlet.event.Event() done2 = eventlet.event.Event() - eventlet.spawn(backend.image('instance', 'name').cache, + thr2 = eventlet.spawn(backend.image('instance', 'name').cache, _concurrency, 'fname', None, wait=wait2, done=done2) wait2.send() eventlet.sleep(0) @@ -416,17 +429,21 @@ class CacheConcurrencyTestCase(test.TestCase): done1.wait() eventlet.sleep(0) self.assertTrue(done2.ready()) + # Wait on greenthreads to assert they didn't raise exceptions + # during execution + thr1.wait() + thr2.wait() def test_different_fname_concurrency(self): """Ensures that two different fname caches are concurrent""" backend = imagebackend.Backend(False) wait1 = eventlet.event.Event() done1 = eventlet.event.Event() - eventlet.spawn(backend.image('instance', 'name').cache, + thr1 = eventlet.spawn(backend.image('instance', 'name').cache, _concurrency, 'fname2', None, wait=wait1, done=done1) wait2 = eventlet.event.Event() done2 = eventlet.event.Event() - eventlet.spawn(backend.image('instance', 'name').cache, + thr2 = eventlet.spawn(backend.image('instance', 'name').cache, _concurrency, 'fname1', None, wait=wait2, done=done2) wait2.send() eventlet.sleep(0) @@ -435,6 +452,10 @@ class CacheConcurrencyTestCase(test.TestCase): finally: wait1.send() eventlet.sleep(0) + # Wait on greenthreads to assert they didn't raise exceptions + # during execution + thr1.wait() + thr2.wait() class FakeVolumeDriver(object): -- cgit