NEC plugin: Honor Retry-After response from OFC

A backend OpenFlow controller nec plugin talks to can return
503 response with retry-after header when it is busy.
It is better to honor retry-after header to avoid unnecessary
user-visible errors due to temporary busy condition.

Change-Id: I2ff1c3ac8402a2207bd955e9a9bb61e147950c5c
Closes-Bug: #1294527

diff --git a/etc/neutron/plugins/nec/nec.ini b/etc/neutron/plugins/nec/nec.ini
index 9cbdbe409872176b4434d8cff10d027fce5ac5ea..878f9e17a1c23cae8c4598007f7d2e2117e6b434 100644 (file)
--- a/etc/neutron/plugins/nec/nec.ini
+++ b/etc/neutron/plugins/nec/nec.ini
@@ -37,6 +37,11 @@ firewall_driver = neutron.agent.linux.iptables_firewall.OVSHybridIptablesFirewal
 # Certificate file
 # cert_file =
 
+# Maximum attempts per OFC API request. NEC plugin retries
+# API request to OFC when OFC returns ServiceUnavailable (503).
+# The value must be greater than 0.
+# api_max_attempts = 3
+
 [provider]
 # Default router provider to use.
 # default_router_provider = l3-agent

diff --git a/neutron/plugins/nec/common/config.py b/neutron/plugins/nec/common/config.py
index bcd4cc8d28608678d5437a3982334e8bdbe84aec..3717468a2fbd0f23ad1511f94cd07492c179ae00 100644 (file)
--- a/neutron/plugins/nec/common/config.py
+++ b/neutron/plugins/nec/common/config.py
@@ -48,6 +48,11 @@ ofc_opts = [
                help=_("Key file")),
     cfg.StrOpt('cert_file', default=None,
                help=_("Certificate file")),
+    cfg.IntOpt('api_max_attempts', default=3,
+               help=_("Maximum attempts per OFC API request."
+                      "NEC plugin retries API request to OFC "
+                      "when OFC returns ServiceUnavailable (503)."
+                      "The value must be greater than 0.")),
 ]
 
 provider_opts = [

diff --git a/neutron/plugins/nec/common/exceptions.py b/neutron/plugins/nec/common/exceptions.py
index 0045b54607e28b2cc0c01c4559af44d8e4ae33b5..05a0e885a4337d3546d7463a5b16baf2c391eb0f 100644 (file)
--- a/neutron/plugins/nec/common/exceptions.py
+++ b/neutron/plugins/nec/common/exceptions.py
@@ -42,6 +42,15 @@ class OFCMappingNotFound(qexc.NotFound):
                 "It may be deleted during processing.")
 
 
+class OFCServiceUnavailable(OFCException):
+    message = _("OFC returns Server Unavailable (503) "
+                "(Retry-After=%(retry_after)s)")
+
+    def __init__(self, **kwargs):
+        super(OFCServiceUnavailable, self).__init__(**kwargs)
+        self.retry_after = kwargs.get('retry_after')
+
+
 class PortInfoNotFound(qexc.NotFound):
     message = _("PortInfo %(id)s could not be found")
 

diff --git a/neutron/plugins/nec/common/ofc_client.py b/neutron/plugins/nec/common/ofc_client.py
index 7fbfe9ec459304e589629ff4dd048718e7d91aeb..6456cece85fa1c47eaf395ede14ac45022eb7df4 100644 (file)
--- a/neutron/plugins/nec/common/ofc_client.py
+++ b/neutron/plugins/nec/common/ofc_client.py
@@ -18,8 +18,10 @@
 import httplib
 import json
 import socket
+import time
 
 from neutron.openstack.common import log as logging
+from neutron.plugins.nec.common import config
 from neutron.plugins.nec.common import exceptions as nexc
 
 
@@ -67,7 +69,7 @@ class OFCClient(object):
         return (_("Operation on OFC failed: %(status)s%(msg)s") %
                 {'status': status, 'msg': detail})
 
-    def do_request(self, method, action, body=None):
+    def do_single_request(self, method, action, body=None):
         LOG.debug(_("Client request: %(host)s:%(port)s "
                     "%(method)s %(action)s [%(body)s]"),
                   {'host': self.host, 'port': self.port,
@@ -95,6 +97,11 @@ class OFCClient(object):
                               httplib.ACCEPTED,
                               httplib.NO_CONTENT):
                 return data
+            elif res.status == httplib.SERVICE_UNAVAILABLE:
+                retry_after = res.getheader('retry-after')
+                LOG.warning(_("OFC returns ServiceUnavailable "
+                              "(retry-after=%s)"), retry_after)
+                raise nexc.OFCServiceUnavailable(retry_after=retry_after)
             elif res.status == httplib.NOT_FOUND:
                 LOG.info(_("Specified resource %s does not exist on OFC "),
                          action)
@@ -116,6 +123,23 @@ class OFCClient(object):
             LOG.error(reason)
             raise nexc.OFCException(reason=reason)
 
+    def do_request(self, method, action, body=None):
+        max_attempts = config.OFC.api_max_attempts
+        for i in range(max_attempts, 0, -1):
+            try:
+                return self.do_single_request(method, action, body)
+            except nexc.OFCServiceUnavailable as e:
+                try:
+                    wait_time = int(e.retry_after)
+                except (ValueError, TypeError):
+                    wait_time = None
+                if i > 1 and wait_time:
+                    LOG.info(_("Waiting for %s seconds due to "
+                               "OFC Service_Unavailable."), wait_time)
+                    time.sleep(wait_time)
+                    continue
+                raise
+
     def get(self, action):
         return self.do_request("GET", action)
 

diff --git a/neutron/tests/unit/nec/test_ofc_client.py b/neutron/tests/unit/nec/test_ofc_client.py
index aba037edfa26e59aa7979b91e5427baaa14b56d8..101cca841734032a4dff21608d8456b3e68692c7 100644 (file)
--- a/neutron/tests/unit/nec/test_ofc_client.py
+++ b/neutron/tests/unit/nec/test_ofc_client.py
@@ -20,6 +20,7 @@ import json
 import socket
 
 import mock
+from oslo.config import cfg
 
 from neutron.plugins.nec.common import exceptions as nexc
 from neutron.plugins.nec.common import ofc_client
@@ -120,3 +121,59 @@ class OFCClientTest(base.BaseTestCase):
                 mock.call.request('GET', '/somewhere', '{}', headers),
             ]
             conn.assert_has_calls(expected)
+
+    def test_do_request_retry_fail_after_one_attempts(self):
+        self._test_do_request_retry_after(1, api_max_attempts=1)
+
+    def test_do_request_retry_fail_with_max_attempts(self):
+        self._test_do_request_retry_after(3)
+
+    def test_do_request_retry_succeed_with_2nd_attempt(self):
+        self._test_do_request_retry_after(2, succeed_final=True)
+
+    def test_do_request_retry_succeed_with_1st_attempt(self):
+        self._test_do_request_retry_after(1, succeed_final=True)
+
+    def _test_do_request_retry_after(self, exp_request_count,
+                                     api_max_attempts=None,
+                                     succeed_final=False):
+        if api_max_attempts is not None:
+            cfg.CONF.set_override('api_max_attempts', api_max_attempts,
+                                  group='OFC')
+
+        res_unavail = mock.Mock()
+        res_unavail.status = 503
+        res_unavail.read.return_value = None
+        res_unavail.getheader.return_value = '10'
+
+        res_ok = mock.Mock()
+        res_ok.status = 200
+        res_ok.read.return_value = None
+
+        conn = mock.Mock()
+        if succeed_final:
+            side_effect = [res_unavail] * (exp_request_count - 1) + [res_ok]
+        else:
+            side_effect = [res_unavail] * exp_request_count
+        conn.getresponse.side_effect = side_effect
+
+        with mock.patch.object(ofc_client.OFCClient, 'get_connection',
+                               return_value=conn):
+            with mock.patch('time.sleep') as sleep:
+                client = ofc_client.OFCClient()
+                if succeed_final:
+                    ret = client.do_request('GET', '/somewhere')
+                    self.assertIsNone(ret)
+                else:
+                    e = self.assertRaises(nexc.OFCServiceUnavailable,
+                                          client.do_request,
+                                          'GET', '/somewhere')
+                    self.assertEqual('10', e.retry_after)
+        headers = {"Content-Type": "application/json"}
+        expected = [
+            mock.call.request('GET', '/somewhere', None, headers),
+            mock.call.getresponse(),
+        ] * exp_request_count
+        conn.assert_has_calls(expected)
+        self.assertEqual(exp_request_count, conn.request.call_count)
+        self.assertEqual(exp_request_count - 1, sleep.call_count)