Merge "Eliminate race conditions in floating association"

5 files changed, 83 insertions, 38 deletions

diff --git a/nova/db/api.py b/nova/db/api.py
index 4acff8a99..3e350fc75 100644
--- a/nova/db/api.py
+++ b/nova/db/api.py
@@ -290,7 +290,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)
@@ -298,7 +299,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 8d087f28d..a1cca73be 100644
--- a/nova/db/sqlalchemy/api.py
+++ b/nova/db/sqlalchemy/api.py
@@ -761,9 +761,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 0e8530d14..3b1c2abb1 100644
--- a/nova/network/manager.py
+++ b/nova/network/manager.py
@@ -582,29 +582,35 @@ class FloatingIP(object):
     def _associate_floating_ip(self, context, floating_address, fixed_address,
                                interface, instance_uuid):
         """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,
-                       instance_id=instance_uuid,
-                       floating_ip=floating_address)
-        notifier.notify(context,
-                        notifier.publisher_id("network"),
-                        'network.floating_ip.associate',
-                        notifier.INFO, payload=payload)
+
+        @lockutils.synchronized(unicode(floating_address), 'nova-')
+        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,
+                           instance_id=instance_uuid,
+                           floating_ip=floating_address)
+            notifier.notify(context,
+                            notifier.publisher_id("network"),
+                            'network.floating_ip.associate',
+                            notifier.INFO, payload=payload)
+        do_associate()
 
     @rpc_common.client_exceptions(exception.FloatingIpNotFoundForAddress)
     @wrap_check_policy
@@ -664,18 +670,33 @@ class FloatingIP(object):
                                   instance_uuid):
         """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,
-                       instance_id=instance_uuid,
-                       floating_ip=address)
-        notifier.notify(context,
-                        notifier.publisher_id("network"),
-                        'network.floating_ip.disassociate',
-                        notifier.INFO, payload=payload)
+
+        @lockutils.synchronized(unicode(address), 'nova-')
+        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,
+                           instance_id=instance_uuid,
+                           floating_ip=address)
+            notifier.notify(context,
+                            notifier.publisher_id("network"),
+                            'network.floating_ip.disassociate',
+                            notifier.INFO, payload=payload)
+        do_disassociate()
 
     @rpc_common.client_exceptions(exception.FloatingIpNotFound)
     @wrap_check_policy
diff --git a/nova/tests/network/test_manager.py b/nova/tests/network/test_manager.py
index e80ea3936..eb3dcf14a 100644
--- a/nova/tests/network/test_manager.py
+++ b/nova/tests/network/test_manager.py
@@ -669,7 +669,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 ea6e9aea5..af329daf6 100644
--- a/nova/tests/test_db_api.py
+++ b/nova/tests/test_db_api.py
@@ -246,6 +246,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'}