From ee9fe2458f813b5d367bc7263c7a30a3af46aa2b Mon Sep 17 00:00:00 2001
From: Kevin Benton <blak111@gmail.com>
Date: Tue, 1 Jul 2014 06:03:19 -0700
Subject: [PATCH] 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
---
 .../plugins/bigswitch/db/consistency_db.py    | 12 +++++------
 neutron/plugins/bigswitch/plugin.py           |  3 +++
 neutron/plugins/bigswitch/servermanager.py    | 21 ++++++++++++-------
 3 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/neutron/plugins/bigswitch/db/consistency_db.py b/neutron/plugins/bigswitch/db/consistency_db.py
index 4d1a1db79..e5f1cc915 100644
--- 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 3cc6e00d1..306016a8f 100644
--- 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 b2e56271b..f02748248 100644
--- 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:
-- 
2.45.2