From 63d271d4e4a8bab947f90f457ed176e46135d39e Mon Sep 17 00:00:00 2001
From: Kevin Benton <blak111@gmail.com>
Date: Mon, 2 Jun 2014 21:39:51 -0700
Subject: [PATCH] Big Switch: fix capabilities retrieval code

References the raw REST response in the capabilities
parsing code so json.loads doesn't get an object
that may already be decoded and fail.

Closes-Bug: #1326173
Change-Id: Ia3179b7499f35a6ab2e9ce1631ab15ed27d64647
---
 neutron/plugins/bigswitch/servermanager.py    |  6 ++--
 .../tests/unit/bigswitch/test_capabilities.py |  6 ++--
 .../unit/bigswitch/test_servermanager.py      | 31 +++++++++++++++++++
 3 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/neutron/plugins/bigswitch/servermanager.py b/neutron/plugins/bigswitch/servermanager.py
index 2ab629797e4..e33bbece558 100644
--- a/neutron/plugins/bigswitch/servermanager.py
+++ b/neutron/plugins/bigswitch/servermanager.py
@@ -110,11 +110,11 @@ class ServerProxy(object):
 
     def get_capabilities(self):
         try:
-            body = self.rest_call('GET', CAPABILITIES_PATH)[3]
+            body = self.rest_call('GET', CAPABILITIES_PATH)[2]
             self.capabilities = json.loads(body)
         except Exception:
-            LOG.error(_("Couldn't retrieve capabilities. "
-                        "Newer API calls won't be supported."))
+            LOG.exception(_("Couldn't retrieve capabilities. "
+                            "Newer API calls won't be supported."))
         LOG.info(_("The following capabilities were received "
                    "for %(server)s: %(cap)s"), {'server': self.server,
                                                 'cap': self.capabilities})
diff --git a/neutron/tests/unit/bigswitch/test_capabilities.py b/neutron/tests/unit/bigswitch/test_capabilities.py
index cc81b70b0a3..89e4c5b728f 100644
--- a/neutron/tests/unit/bigswitch/test_capabilities.py
+++ b/neutron/tests/unit/bigswitch/test_capabilities.py
@@ -34,7 +34,7 @@ class CapabilitiesTests(test_router_db.RouterDBTestBase):
     def test_floating_ip_capability(self):
         with contextlib.nested(
             mock.patch(SERVERRESTCALL,
-                       return_value=(200, None, None, '["floatingip"]')),
+                       return_value=(200, None, '["floatingip"]', None)),
             mock.patch(SERVERPOOL + '.rest_create_floatingip',
                        return_value=(200, None, None, None)),
             mock.patch(SERVERPOOL + '.rest_delete_floatingip',
@@ -53,7 +53,7 @@ class CapabilitiesTests(test_router_db.RouterDBTestBase):
     def test_floating_ip_capability_neg(self):
         with contextlib.nested(
             mock.patch(SERVERRESTCALL,
-                       return_value=(200, None, None, '[""]')),
+                       return_value=(200, None, '[""]', None)),
             mock.patch(SERVERPOOL + '.rest_update_network',
                        return_value=(200, None, None, None))
         ) as (mock_rest, mock_netupdate):
@@ -67,7 +67,7 @@ class CapabilitiesTests(test_router_db.RouterDBTestBase):
 
     def test_keep_alive_capability(self):
         with mock.patch(
-            SERVERRESTCALL, return_value=(200, None, None, '["keep-alive"]')
+            SERVERRESTCALL, return_value=(200, None, '["keep-alive"]', None)
         ):
             # perform a task to cause capabilities to be retrieved
             with self.floatingip_with_assoc():
diff --git a/neutron/tests/unit/bigswitch/test_servermanager.py b/neutron/tests/unit/bigswitch/test_servermanager.py
index 914a8874431..d37e8504eb1 100644
--- a/neutron/tests/unit/bigswitch/test_servermanager.py
+++ b/neutron/tests/unit/bigswitch/test_servermanager.py
@@ -174,6 +174,37 @@ class ServerManagerTests(test_rp.BigSwitchProxyPluginV2TestCase):
         self.assertIn('EXTRA-HEADER', callheaders)
         self.assertEqual(callheaders['EXTRA-HEADER'], 'HI')
 
+    def test_capabilities_retrieval(self):
+        sp = servermanager.ServerPool()
+        with mock.patch(HTTPCON) as conmock:
+            rv = conmock.return_value.getresponse.return_value
+            rv.getheader.return_value = 'HASHHEADER'
+
+            # each server will get different capabilities
+            rv.read.side_effect = ['["a","b","c"]', '["b","c","d"]']
+            # pool capabilities is intersection between both
+            self.assertEqual(set(['b', 'c']), sp.get_capabilities())
+            self.assertEqual(2, rv.read.call_count)
+
+            # the pool should cache after the first call so no more
+            # HTTP calls should be made
+            rv.read.side_effect = ['["w","x","y"]', '["x","y","z"]']
+            self.assertEqual(set(['b', 'c']), sp.get_capabilities())
+            self.assertEqual(2, rv.read.call_count)
+
+    def test_capabilities_retrieval_failure(self):
+        sp = servermanager.ServerPool()
+        with mock.patch(HTTPCON) as conmock:
+            rv = conmock.return_value.getresponse.return_value
+            rv.getheader.return_value = 'HASHHEADER'
+            # a failure to parse should result in an empty capability set
+            rv.read.return_value = 'XXXXX'
+            self.assertEqual([], sp.servers[0].get_capabilities())
+
+            # One broken server should affect all capabilities
+            rv.read.side_effect = ['{"a": "b"}', '["b","c","d"]']
+            self.assertEqual(set(), sp.get_capabilities())
+
     def test_reconnect_on_timeout_change(self):
         sp = servermanager.ServerPool()
         with mock.patch(HTTPCON) as conmock: