BigSwitch: Widen range of HTTPExceptions caught

In order to trigger a reconnect, the Big Switch
plugin was only checking for httplib.ImproperConnectionState
which doesn't include relevant exceptions that
indicate a reconnect is necessary. This patch
adjusts it to reconnect on any HTTPException.

Closes-Bug: #1289236
Change-Id: I4b389172dc4a53b0381e40005d8421f71b7d8522

diff --git a/neutron/plugins/bigswitch/servermanager.py b/neutron/plugins/bigswitch/servermanager.py
index 5efafed7e5f4f208269d286d59b57b6100b91d0d..ea59e8e4e5b815c5fb23270ba26ccdd042772ad5 100644 (file)
--- a/neutron/plugins/bigswitch/servermanager.py
+++ b/neutron/plugins/bigswitch/servermanager.py
@@ -191,17 +191,22 @@ class ServerProxy(object):
                     # response was not JSON, ignore the exception
                     pass
             ret = (response.status, response.reason, respstr, respdata)
-        except httplib.ImproperConnectionState:
+        except httplib.HTTPException:
             # If we were using a cached connection, try again with a new one.
             with excutils.save_and_reraise_exception() as ctxt:
-                if not reconnect:
-                    ctxt.reraise = False
-
-            if self.currentconn:
                 self.currentconn.close()
+                if reconnect:
+                    # if reconnect is true, this was on a fresh connection so
+                    # reraise since this server seems to be broken
+                    ctxt.reraise = True
+                else:
+                    # if reconnect is false, it was a cached connection so
+                    # try one more time before re-raising
+                    ctxt.reraise = False
             return self.rest_call(action, resource, data, headers,
                                   timeout=timeout, reconnect=True)
         except (socket.timeout, socket.error) as e:
+            self.currentconn.close()
             LOG.error(_('ServerProxy: %(action)s failure, %(e)r'),
                       {'action': action, 'e': e})
             ret = 0, None, None, None

diff --git a/neutron/tests/unit/bigswitch/test_servermanager.py b/neutron/tests/unit/bigswitch/test_servermanager.py
index e09858cdc2e29c81fbe48faab19e5371ff44377c..20f2fb6fe28966ff9ea1f1c35d13d64286d5d678 100644 (file)
--- a/neutron/tests/unit/bigswitch/test_servermanager.py
+++ b/neutron/tests/unit/bigswitch/test_servermanager.py
@@ -14,6 +14,9 @@
 #
 # @author: Kevin Benton, kevin.benton@bigswitch.com
 #
+import httplib
+import socket
+
 from contextlib import nested
 import mock
 from oslo.config import cfg
@@ -22,6 +25,7 @@ from neutron.manager import NeutronManager
 from neutron.plugins.bigswitch import servermanager
 from neutron.tests.unit.bigswitch import test_restproxy_plugin as test_rp
 
+HTTPCON = 'httplib.HTTPConnection'
 SERVERMANAGER = 'neutron.plugins.bigswitch.servermanager'
 
 
@@ -97,3 +101,49 @@ class ServerManagerTests(test_rp.BigSwitchProxyPluginV2TestCase):
                 mock.call.read(),
                 mock.call.write('certdata')
             ])
+
+    def test_reconnect_cached_connection(self):
+        sp = servermanager.ServerPool()
+        with mock.patch(HTTPCON) as conmock:
+            rv = conmock.return_value
+            rv.getresponse.return_value.getheader.return_value = 'HASH'
+            sp.servers[0].capabilities = ['keep-alive']
+            sp.servers[0].rest_call('GET', '/first')
+            # raise an error on re-use to verify reconnect
+            # return okay the second time so the reconnect works
+            rv.request.side_effect = [httplib.ImproperConnectionState(),
+                                      mock.MagicMock()]
+            sp.servers[0].rest_call('GET', '/second')
+        uris = [c[1][1] for c in rv.request.mock_calls]
+        expected = [
+            sp.base_uri + '/first',
+            sp.base_uri + '/second',
+            sp.base_uri + '/second',
+        ]
+        self.assertEqual(uris, expected)
+
+    def test_no_reconnect_recurse_to_infinity(self):
+        # retry uses recursion when a reconnect is necessary
+        # this test makes sure it stops after 1 recursive call
+        sp = servermanager.ServerPool()
+        with mock.patch(HTTPCON) as conmock:
+            rv = conmock.return_value
+            # hash header must be string instead of mock object
+            rv.getresponse.return_value.getheader.return_value = 'HASH'
+            sp.servers[0].capabilities = ['keep-alive']
+            sp.servers[0].rest_call('GET', '/first')
+            # after retrying once, the rest call should raise the
+            # exception up
+            rv.request.side_effect = httplib.ImproperConnectionState()
+            self.assertRaises(httplib.ImproperConnectionState,
+                              sp.servers[0].rest_call,
+                              *('GET', '/second'))
+            # 1 for the first call, 2 for the second with retry
+            self.assertEqual(rv.request.call_count, 3)
+
+    def test_socket_error(self):
+        sp = servermanager.ServerPool()
+        with mock.patch(HTTPCON) as conmock:
+            conmock.return_value.request.side_effect = socket.timeout()
+            resp = sp.servers[0].rest_call('GET', '/')
+            self.assertEqual(resp, (0, None, None, None))