BSN: Remove db lock and add missing contexts

Adds context tracking decorators that were missing
from router interface methods. Without them, new
sessions were being created instead of using the
existing context which was causing transaction
issues.

Modifies the servermanager to store context references
as weakrefs so if multiple functions are called before
the rest functions are called, the first one doesn't steal
the only context reference with a pop() call.

Removes a DB lock for update in the server manager that occured
during rest calls that was triggering deadlocks due to the
file lock synchronization for the rest calls.

Closes-Bug: #1336251
Change-Id: Iad3d61e2c23832b3ad760a999fbab7feaa13f805

diff --git a/neutron/plugins/bigswitch/db/consistency_db.py b/neutron/plugins/bigswitch/db/consistency_db.py
index 4d1a1db792431c2873c65e42f558f417a6124f3e..e5f1cc9150c8343069cec012dc235c02fe444b0e 100644 (file)
--- a/neutron/plugins/bigswitch/db/consistency_db.py
+++ b/neutron/plugins/bigswitch/db/consistency_db.py
@@ -55,13 +55,13 @@ class HashHandler(object):
         if self.transaction:
             raise MultipleReadForUpdateCalls()
         self.transaction = self.session.begin(subtransactions=True)
-        # Lock for update here to prevent another server from reading the hash
-        # while this one is in the middle of a transaction.
-        # This may not lock the SQL table in MySQL Galera deployments
-        # but that's okay because the worst case is a double-sync
+        # REVISIT(kevinbenton): locking here with the DB is prone to deadlocks
+        # in various multi-REST-call scenarios (router intfs, flips, etc).
+        # Since it doesn't work in Galera deployments anyway, another sync
+        # mechanism will have to be introduced to prevent inefficient double
+        # syncs in HA deployments.
         res = (self.session.query(ConsistencyHash).
-               filter_by(hash_id=self.hash_id).
-               with_lockmode('update').first())
+               filter_by(hash_id=self.hash_id).first())
         if not res:
             return ''
         self.hash_db_obj = res

diff --git a/neutron/plugins/bigswitch/plugin.py b/neutron/plugins/bigswitch/plugin.py
index 3cc6e00d1040dd42a6da3516853f1d3ff065491b..306016a8feab559171589628b54e10a8469dcaec 100644 (file)
--- a/neutron/plugins/bigswitch/plugin.py
+++ b/neutron/plugins/bigswitch/plugin.py
@@ -968,6 +968,7 @@ class NeutronRestProxyV2(NeutronRestProxyV2Base,
             self.servers.rest_delete_router(tenant_id, router_id)
             return ret_val
 
+    @put_context_in_serverpool
     def add_router_interface(self, context, router_id, interface_info):
 
         LOG.debug(_("NeutronRestProxyV2: add_router_interface() called"))
@@ -996,6 +997,7 @@ class NeutronRestProxyV2(NeutronRestProxyV2Base,
                                                    intf_details)
             return new_intf_info
 
+    @put_context_in_serverpool
     def remove_router_interface(self, context, router_id, interface_info):
 
         LOG.debug(_("NeutronRestProxyV2: remove_router_interface() called"))
@@ -1087,6 +1089,7 @@ class NeutronRestProxyV2(NeutronRestProxyV2Base,
             else:
                 self._send_floatingip_update(context)
 
+    @put_context_in_serverpool
     def disassociate_floatingips(self, context, port_id):
         LOG.debug(_("NeutronRestProxyV2: diassociate_floatingips() called"))
         super(NeutronRestProxyV2, self).disassociate_floatingips(context,

diff --git a/neutron/plugins/bigswitch/servermanager.py b/neutron/plugins/bigswitch/servermanager.py
index b2e56271bbda60defbe2bfb10a6b73cce98d2a40..f02748248ac9c4baa16d0a2914a648f027e1be3a 100644 (file)
--- a/neutron/plugins/bigswitch/servermanager.py
+++ b/neutron/plugins/bigswitch/servermanager.py
@@ -35,6 +35,7 @@ import httplib
 import os
 import socket
 import ssl
+import weakref
 
 import eventlet
 import eventlet.corolocal
@@ -266,14 +267,18 @@ class ServerPool(object):
 
     def set_context(self, context):
         # this context needs to be local to the greenthread
-        # so concurrent requests don't use the wrong context
-        self.contexts[eventlet.corolocal.get_ident()] = context
-
-    def pop_context(self):
-        # Don't store these contexts after use. They should only
-        # last for one request.
+        # so concurrent requests don't use the wrong context.
+        # Use a weakref so the context is garbage collected
+        # after the plugin is done with it.
+        ref = weakref.ref(context)
+        self.contexts[eventlet.corolocal.get_ident()] = ref
+
+    def get_context_ref(self):
+        # Try to get the context cached for this thread. If one
+        # doesn't exist or if it's been garbage collected, this will
+        # just return None.
         try:
-            return self.contexts.pop(eventlet.corolocal.get_ident())
+            return self.contexts[eventlet.corolocal.get_ident()]()
         except KeyError:
             return None
 
@@ -403,7 +408,7 @@ class ServerPool(object):
     @utils.synchronized('bsn-rest-call')
     def rest_call(self, action, resource, data, headers, ignore_codes,
                   timeout=False):
-        hash_handler = cdb.HashHandler(context=self.pop_context())
+        hash_handler = cdb.HashHandler(context=self.get_context_ref())
         good_first = sorted(self.servers, key=lambda x: x.failed)
         first_response = None
         for active_server in good_first: