Merge lp:~soren/nova/cross-binary-sync into lp:~hudson-openstack/nova/trunk

[Soren Hansen]

Add a decorator that lets you synchronise actions across multiple binaries. Like, say, ensuring that only one worker manipulates iptables at a time.

[Jay Pipes (jaypipes)]

How would this work if the binaries are on multiple hosts?

review: Needs Information

[Vish Ishaya (vishvananda)]

If they are on multiple hosts they don't need to be synced. It is to avoid
two processes changing iptables rules on the same host.
On Feb 28, 2011 9:26 PM, "Jay Pipes" <email address hidden> wrote:

[Jay Pipes (jaypipes)]

> If they are on multiple hosts they don't need to be synced. It is to avoid
> two processes changing iptables rules on the same host.

OK, thx for the answer.

review: Approve

[Rick Harris (rconradharris)]

lgtm, though, I think it would ease debugging if we copied the inner-function's metadata out to the wrapper-function.

We can use

   @functools.wraps(f)
   def inner(*args, **kwargs):

to do that.

[Soren Hansen (soren)]

> lgtm, though, I think it would ease debugging if we copied the inner-
> function's metadata out to the wrapper-function.
>
> We can use
>
> @functools.wraps(f)
> def inner(*args, **kwargs):
>
> to do that.

Good suggestion, thanks. Doing so.

[Vish Ishaya (vishvananda)]

Hey Soren. The syncronization test passes just fine normally, but seems to fail when run as part of a build. Any idea why that might be happening?

review: Needs Information

[Vish Ishaya (vishvananda)]

Hmm Ignore that. It started working. I don't know what was causing the issue. The only change i made was fixing the rename of nova-api.conf

review: Approve

[Rick Harris (rconradharris)]

I'm getting sporadic failures. I wrote a little test-harness to make 10 attempts, had 70% fail.

Success: 3
Failed: 7
Total Attempts 10
rick@maverick:~/openstack/nova/cross-binary-sync$ cat /etc/lsb-release
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=10.10
DISTRIB_CODENAME=maverick
DISTRIB_DESCRIPTION="Ubuntu 10.10"

This is the (unhelpful) traceback: http://paste.openstack.org/show/833/

Haven't really dug in yet.

review: Needs Information

[Soren Hansen (soren)]

Ironically, the tests were racy. They depended on one of the processes to be executed first. For some reason they only failed for me roughly 1 in 10-12 times. Thanks for pointing this out.

I refactored the tests to make it work regardless of the order of execution.

[Josh Kearney (jk0)]

Tests are working as expected.

review: Approve

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~soren/nova/cross-binary-sync into lp:nova failed. Below is the output from the failed tests.

Traceback (most recent call last):
  File "run_tests.py", line 68, in <module>
    from nova.tests import fake_flags
  File "/tmp/tmpHVsZF0/nova/tests/fake_flags.py", line 23, in <module>
    flags.DECLARE('volume_driver', 'nova.volume.manager')
  File "/tmp/tmpHVsZF0/nova/flags.py", line 236, in DECLARE
    __import__(module_string, globals(), locals())
  File "/tmp/tmpHVsZF0/nova/volume/__init__.py", line 19, in <module>
    from nova.volume.api import API
  File "/tmp/tmpHVsZF0/nova/volume/api.py", line 25, in <module>
    from nova import db
  File "/tmp/tmpHVsZF0/nova/db/__init__.py", line 23, in <module>
    from nova.db.api import *
  File "/tmp/tmpHVsZF0/nova/db/api.py", line 37, in <module>
    from nova import utils
  File "/tmp/tmpHVsZF0/nova/utils.py", line 29, in <module>
    import lockfile
ImportError: No module named lockfile

[Soren Hansen (soren)]

Installed lockfile on the Hudson box. I'm surprised it wasn't there already. We used to use lockfile back in the very old days (bzr revno 1 has references to it). Meh.

[Jay Pipes (jaypipes)]

It was only used in the server daemons. And, guess what, none of those
are tested in the unit tests.

On Wed, Mar 9, 2011 at 3:05 PM, Soren Hansen <email address hidden> wrote:
> Installed lockfile on the Hudson box. I'm surprised it wasn't there already. We used to use lockfile back in the very old days (bzr revno 1 has references to it). Meh.
> --
> https://code.launchpad.net/~soren/nova/cross-binary-sync/+merge/51533
> You are reviewing the proposed merge of lp:~soren/nova/cross-binary-sync into lp:nova.
>

=== modified file 'nova/flags.py'
--- nova/flags.py	2011-02-25 01:04:25 +0000
+++ nova/flags.py	2011-03-09 09:38:12 +0000
@@ -321,6 +321,8 @@
 
 DEFINE_string('state_path', os.path.join(os.path.dirname(__file__), '../'),
               "Top-level directory for maintaining nova's state")
+DEFINE_string('lock_path', os.path.join(os.path.dirname(__file__), '../'),
+              "Directory for lock files")
 DEFINE_string('logdir', None, 'output to a per-service log file in named '
                               'directory')
 
 
=== modified file 'nova/tests/test_misc.py'
--- nova/tests/test_misc.py	2011-02-21 18:55:25 +0000
+++ nova/tests/test_misc.py	2011-03-09 09:38:12 +0000
@@ -14,10 +14,12 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import errno
 import os
+import select
 
 from nova import test
-from nova.utils import parse_mailmap, str_dict_replace
+from nova.utils import parse_mailmap, str_dict_replace, synchronized
 
 
 class ProjectTestCase(test.TestCase):
@@ -55,3 +57,47 @@
                                 '%r not listed in Authors' % missing)
             finally:
                 tree.unlock()
+
+
+class LockTestCase(test.TestCase):
+    def test_synchronized_wrapped_function_metadata(self):
+        @synchronized('whatever')
+        def foo():
+            """Bar"""
+            pass
+        self.assertEquals(foo.__doc__, 'Bar', "Wrapped function's docstring "
+                                              "got lost")
+        self.assertEquals(foo.__name__, 'foo', "Wrapped function's name "
+                                               "got mangled")
+
+    def test_synchronized(self):
+        rpipe1, wpipe1 = os.pipe()
+        rpipe2, wpipe2 = os.pipe()
+
+        @synchronized('testlock')
+        def f(rpipe, wpipe):
+            try:
+                os.write(wpipe, "foo")
+            except OSError, e:
+                self.assertEquals(e.errno, errno.EPIPE)
+                return
+
+            rfds, _, __ = select.select([rpipe], [], [], 1)
+            self.assertEquals(len(rfds), 0, "The other process, which was"
+                                            " supposed to be locked, "
+                                            "wrote on its end of the "
+                                            "pipe")
+            os.close(rpipe)
+
+        pid = os.fork()
+        if pid > 0:
+            os.close(wpipe1)
+            os.close(rpipe2)
+
+            f(rpipe1, wpipe2)
+        else:
+            os.close(rpipe1)
+            os.close(wpipe2)
+
+            f(rpipe2, wpipe1)
+            os._exit(0)
 
=== modified file 'nova/utils.py'
--- nova/utils.py	2011-02-23 22:50:33 +0000
+++ nova/utils.py	2011-03-09 09:38:12 +0000
@@ -23,10 +23,14 @@
 
 import base64
 import datetime
+import functools
 import inspect
 import json
+import lockfile
+import netaddr
 import os
 import random
+import re
 import socket
 import string
 import struct
@@ -34,8 +38,6 @@
 import time
 import types
 from xml.sax import saxutils
-import re
-import netaddr
 
 from eventlet import event
 from eventlet import greenthread
@@ -43,11 +45,13 @@
 
 from nova import exception
 from nova.exception import ProcessExecutionError
+from nova import flags
 from nova import log as logging
 
 
 LOG = logging.getLogger("nova.utils")
 TIME_FORMAT = "%Y-%m-%dT%H:%M:%SZ"
+FLAGS = flags.FLAGS
 
 
 def import_class(import_str):
@@ -491,6 +495,18 @@
     return json.loads(s)
 
 
+def synchronized(name):
+    def wrap(f):
+        @functools.wraps(f)
+        def inner(*args, **kwargs):
+            lock = lockfile.FileLock(os.path.join(FLAGS.lock_path,
+                                                  'nova-%s.lock' % name))
+            with lock:
+                return f(*args, **kwargs)
+        return inner
+    return wrap
+
+
 def ensure_b64_encoding(val):
     """Safety method to ensure that values expected to be base64-encoded
     actually are. If they are, the value is returned unchanged. Otherwise,