Prevent possible server list damage in BigSwitch plugin

The old failover logic for the BigSwitch plugin modified a
list of controllers as it tested them. If the code for one
thread unexpectedly raised an exception or died, the global
server list could lose a server permanently.
This patch addresses that by flagging servers as failed instead
so the global server list is never modified.

Fixes: bug #1200022
Change-Id: Id2dcb820ef9f62fd03e3215bff3345e56c78afe2

diff --git a/neutron/plugins/bigswitch/plugin.py b/neutron/plugins/bigswitch/plugin.py
index 01c29bc91cb387e395b5b818d3afde2e209aa832..31add796ebe95b9645f787f04134f145ccf8b682 100644 (file)
--- a/neutron/plugins/bigswitch/plugin.py
+++ b/neutron/plugins/bigswitch/plugin.py
@@ -184,10 +184,10 @@ class ServerProxy(object):
         self.success_codes = SUCCESS_CODES
         self.auth = None
         self.neutron_id = neutron_id
+        self.failed = False
         if auth:
             self.auth = 'Basic ' + base64.encodestring(auth).strip()
 
-    @utils.synchronized('bsn-rest-call', external=True)
     def rest_call(self, action, resource, data, headers):
         uri = self.base_uri + resource
         body = json.dumps(data)
@@ -283,13 +283,13 @@ class ServerPool(object):
         """
         return resp[0] in SUCCESS_CODES
 
+    @utils.synchronized('bsn-rest-call', external=True)
     def rest_call(self, action, resource, data, headers, ignore_codes):
-        failed_servers = []
-        while self.servers:
-            active_server = self.servers[0]
+        good_first = sorted(self.servers, key=lambda x: x.failed)
+        for active_server in good_first:
             ret = active_server.rest_call(action, resource, data, headers)
             if not self.server_failure(ret, ignore_codes):
-                self.servers.extend(failed_servers)
+                active_server.failed = False
                 return ret
             else:
                 LOG.error(_('ServerProxy: %(action)s failure for servers: '
@@ -297,15 +297,14 @@ class ServerPool(object):
                           {'action': action,
                            'server': (active_server.server,
                                       active_server.port)})
-                failed_servers.append(self.servers.pop(0))
+                active_server.failed = True
 
         # All servers failed, reset server list and try again next time
         LOG.error(_('ServerProxy: %(action)s failure for all servers: '
                     '%(server)r'),
                   {'action': action,
                    'server': tuple((s.server,
-                                    s.port) for s in failed_servers)})
-        self.servers.extend(failed_servers)
+                                    s.port) for s in self.servers)})
         return (0, None, None, None)
 
     def get(self, resource, data='', headers=None, ignore_codes=[]):

diff --git a/neutron/tests/unit/bigswitch/etc/restproxy.ini.test b/neutron/tests/unit/bigswitch/etc/restproxy.ini.test
index 3c1c17f944a8951728283d85a13b4fd5c9cc2aee..8df78a6eb0ce88d42b9095e7d4611665f1761937 100644 (file)
--- a/neutron/tests/unit/bigswitch/etc/restproxy.ini.test
+++ b/neutron/tests/unit/bigswitch/etc/restproxy.ini.test
@@ -21,7 +21,7 @@ retry_interval = 2
 #   serverauth  :   <username:password>         (default: no auth)
 #   serverssl   :   True | False                (default: False)
 #
-servers=localhost:8899
+servers=localhost:9000,localhost:8899
 serverssl=False
 #serverauth=username:password
 

diff --git a/neutron/tests/unit/bigswitch/test_restproxy_plugin.py b/neutron/tests/unit/bigswitch/test_restproxy_plugin.py
index b6d743191507f9cd7d3ba5231b8003d6352530b4..13caebb6d96ebbb5ca8242396987443dccf30861 100644 (file)
--- a/neutron/tests/unit/bigswitch/test_restproxy_plugin.py
+++ b/neutron/tests/unit/bigswitch/test_restproxy_plugin.py
@@ -55,13 +55,33 @@ class HTTPResponseMock404():
         return "{'status': '404 Not Found'}"
 
 
+class HTTPResponseMock500():
+    status = 500
+    reason = 'Internal Server Error'
+
+    def __init__(self, sock, debuglevel=0, strict=0, method=None,
+                 buffering=False):
+        pass
+
+    def read(self):
+        return "{'status': '500 Internal Server Error'}"
+
+
 class HTTPConnectionMock():
 
     def __init__(self, server, port, timeout):
-        self.response = None
-        pass
+        if port == 9000:
+            self.response = HTTPResponseMock500(None)
+            self.broken = True
+        else:
+            self.response = HTTPResponseMock(None)
+            self.broken = False
 
     def request(self, action, uri, body, headers):
+        if self.broken:
+            if "ExceptOnBadServer" in uri:
+                raise Exception("Broken server got an unexpected request")
+            return
         if uri.endswith('attachment') and action == 'DELETE':
             self.response = HTTPResponseMock404(None)
         else:
@@ -101,7 +121,14 @@ class TestBigSwitchProxyBasicGet(test_plugin.TestBasicGet,
 class TestBigSwitchProxyV2HTTPResponse(test_plugin.TestV2HTTPResponse,
                                        BigSwitchProxyPluginV2TestCase):
 
-    pass
+    def test_failover_memory(self):
+        # first request causes failover so next shouldn't hit bad server
+        with self.network() as net:
+            kwargs = {'tenant_id': 'ExceptOnBadServer'}
+            with self.network(**kwargs) as net:
+                req = self.new_show_request('networks', net['network']['id'])
+                res = req.get_response(self.api)
+                self.assertEqual(res.status_int, 200)
 
 
 class TestBigSwitchProxyPortsV2(test_plugin.TestPortsV2,