Merge "Eliminate race conditions in floating association" into stable/folsom

5 files changed, 79 insertions, 33 deletions

diff --git a/nova/db/api.py b/nova/db/api.py
index cb651793e9..bb69558378 100644
--- a/nova/db/api.py
+++ b/nova/db/api.py
@@ -303,7 +303,8 @@ def floating_ip_destroy(context, address):
 def floating_ip_disassociate(context, address):
     """Disassociate a floating ip from a fixed ip by address.
 
-    :returns: the address of the existing fixed ip.
+    :returns: the address of the previous fixed ip or None
+              if the ip was not associated to an ip.
 
     """
     return IMPL.floating_ip_disassociate(context, address)
@@ -311,7 +312,12 @@ def floating_ip_disassociate(context, address):
 
 def floating_ip_fixed_ip_associate(context, floating_address,
                                    fixed_address, host):
-    """Associate a floating ip to a fixed_ip by address."""
+    """Associate a floating ip to a fixed_ip by address.
+
+    :returns: the address of the new fixed ip (fixed_address) or None
+              if the ip was already associated to the fixed ip.
+    """
+
     return IMPL.floating_ip_fixed_ip_associate(context,
                                                floating_address,
                                                fixed_address,
diff --git a/nova/db/sqlalchemy/api.py b/nova/db/sqlalchemy/api.py
index f363dc764f..4bdab492b4 100644
--- a/nova/db/sqlalchemy/api.py
+++ b/nova/db/sqlalchemy/api.py
@@ -812,9 +812,12 @@ def floating_ip_fixed_ip_associate(context, floating_address,
         fixed_ip_ref = fixed_ip_get_by_address(context,
                                                fixed_address,
                                                session=session)
+        if floating_ip_ref.fixed_ip_id == fixed_ip_ref["id"]:
+            return None
         floating_ip_ref.fixed_ip_id = fixed_ip_ref["id"]
         floating_ip_ref.host = host
         floating_ip_ref.save(session=session)
+        return fixed_address
 
 
 @require_context
diff --git a/nova/network/manager.py b/nova/network/manager.py
index 2306e0ca68..00a6e58c43 100644
--- a/nova/network/manager.py
+++ b/nova/network/manager.py
@@ -555,27 +555,34 @@ class FloatingIP(object):
     def _associate_floating_ip(self, context, floating_address, fixed_address,
                                interface):
         """Performs db and driver calls to associate floating ip & fixed ip"""
-        # associate floating ip
-        self.db.floating_ip_fixed_ip_associate(context,
-                                               floating_address,
-                                               fixed_address,
-                                               self.host)
-        try:
-            # gogo driver time
-            self.l3driver.add_floating_ip(floating_address, fixed_address,
-                    interface)
-        except exception.ProcessExecutionError as e:
-            fixed_address = self.db.floating_ip_disassociate(context,
-                                                             floating_address)
-            if "Cannot find device" in str(e):
-                LOG.error(_('Interface %(interface)s not found'), locals())
-                raise exception.NoFloatingIpInterface(interface=interface)
-        payload = dict(project_id=context.project_id,
-                       floating_ip=floating_address)
-        notifier.notify(context,
-                        notifier.publisher_id("network"),
-                        'network.floating_ip.associate',
-                        notifier.INFO, payload=payload)
+
+        @utils.synchronized(unicode(floating_address))
+        def do_associate():
+            # associate floating ip
+            res = self.db.floating_ip_fixed_ip_associate(context,
+                                                         floating_address,
+                                                         fixed_address,
+                                                         self.host)
+            if not res:
+                # NOTE(vish): ip was already associated
+                return
+            try:
+                # gogo driver time
+                self.l3driver.add_floating_ip(floating_address, fixed_address,
+                        interface)
+            except exception.ProcessExecutionError as e:
+                self.db.floating_ip_disassociate(context, floating_address)
+                if "Cannot find device" in str(e):
+                    LOG.error(_('Interface %(interface)s not found'), locals())
+                    raise exception.NoFloatingIpInterface(interface=interface)
+
+            payload = dict(project_id=context.project_id,
+                           floating_ip=floating_address)
+            notifier.notify(context,
+                            notifier.publisher_id("network"),
+                            'network.floating_ip.associate',
+                            notifier.INFO, payload=payload)
+        do_associate()
 
     @wrap_check_policy
     def disassociate_floating_ip(self, context, address,
@@ -635,16 +642,31 @@ class FloatingIP(object):
     def _disassociate_floating_ip(self, context, address, interface):
         """Performs db and driver calls to disassociate floating ip"""
         # disassociate floating ip
-        fixed_address = self.db.floating_ip_disassociate(context, address)
 
-        if interface:
-            # go go driver time
-            self.l3driver.remove_floating_ip(address, fixed_address, interface)
-        payload = dict(project_id=context.project_id, floating_ip=address)
-        notifier.notify(context,
-                        notifier.publisher_id("network"),
-                        'network.floating_ip.disassociate',
-                        notifier.INFO, payload=payload)
+        @utils.synchronized(unicode(address))
+        def do_disassociate():
+            # NOTE(vish): Note that we are disassociating in the db before we
+            #             actually remove the ip address on the host. We are
+            #             safe from races on this host due to the decorator,
+            #             but another host might grab the ip right away. We
+            #             don't worry about this case because the miniscule
+            #             window where the ip is on both hosts shouldn't cause
+            #             any problems.
+            fixed_address = self.db.floating_ip_disassociate(context, address)
+
+            if not fixed_address:
+                # NOTE(vish): ip was already disassociated
+                return
+            if interface:
+                # go go driver time
+                self.l3driver.remove_floating_ip(address, fixed_address,
+                                                 interface)
+            payload = dict(project_id=context.project_id, floating_ip=address)
+            notifier.notify(context,
+                            notifier.publisher_id("network"),
+                            'network.floating_ip.disassociate',
+                            notifier.INFO, payload=payload)
+        do_disassociate()
 
     @wrap_check_policy
     def get_floating_ip(self, context, id):
diff --git a/nova/tests/network/test_manager.py b/nova/tests/network/test_manager.py
index 75a7cb5958..e983ad6a5d 100644
--- a/nova/tests/network/test_manager.py
+++ b/nova/tests/network/test_manager.py
@@ -639,7 +639,7 @@ class VlanNetworkTestCase(test.TestCase):
                                       is_admin=False)
 
         def fake1(*args, **kwargs):
-            pass
+            return '10.0.0.1'
 
         # floating ip that's already associated
         def fake2(*args, **kwargs):
diff --git a/nova/tests/test_db_api.py b/nova/tests/test_db_api.py
index 641c7c8cd9..82063db4cc 100644
--- a/nova/tests/test_db_api.py
+++ b/nova/tests/test_db_api.py
@@ -207,6 +207,21 @@ class DbApiTestCase(test.TestCase):
         self.assertEqual(0, len(results))
         db.instance_update(ctxt, instance['uuid'], {"task_state": None})
 
+    def test_multi_associate_disassociate(self):
+        ctxt = context.get_admin_context()
+        values = {'address': 'floating'}
+        floating = db.floating_ip_create(ctxt, values)
+        values = {'address': 'fixed'}
+        fixed = db.fixed_ip_create(ctxt, values)
+        res = db.floating_ip_fixed_ip_associate(ctxt, floating, fixed, 'foo')
+        self.assertEqual(res, fixed)
+        res = db.floating_ip_fixed_ip_associate(ctxt, floating, fixed, 'foo')
+        self.assertEqual(res, None)
+        res = db.floating_ip_disassociate(ctxt, floating)
+        self.assertEqual(res, fixed)
+        res = db.floating_ip_disassociate(ctxt, floating)
+        self.assertEqual(res, None)
+
     def test_network_create_safe(self):
         ctxt = context.get_admin_context()
         values = {'host': 'localhost', 'project_id': 'project1'}