From f8eef18297ec2dba4abf45f8ca57c40c2380cad9 Mon Sep 17 00:00:00 2001 From: Yuriy Nesenenko Date: Wed, 22 Jun 2016 17:36:49 +0300 Subject: [PATCH] Cinder client should retry with Retry-After value If a request fails but the response contains a "Retry-After", the cinder client should wait the amount of time and then retry. Cinder client should report a warning to user and continue with retry, so that user can cancel the operation if not interested in retry. The value in "Retry-After" header will be in seconds or GMT value, client should handle both the cases. How many times client should retry will be controlled by user through "--retries" argument to cinder api example, $ cinder --retries 3 availability-zone-list If request was not sucessful within the retries, client should raise the exception. Change-Id: I99af957bfbbe3a202b148dc2fcafdd20b5d7cda0 Partial-Bug: #1263069 --- cinderclient/client.py | 22 ++++++++++- cinderclient/exceptions.py | 31 ++++++++++++++-- cinderclient/tests/unit/test_client.py | 25 +++++++++++++ cinderclient/tests/unit/test_exceptions.py | 32 ++++++++++++++++ cinderclient/tests/unit/test_http.py | 43 ++++++++++++++++++++++ 5 files changed, 149 insertions(+), 4 deletions(-) diff --git a/cinderclient/client.py b/cinderclient/client.py index 6f9463a80..557a0a15a 100644 --- a/cinderclient/client.py +++ b/cinderclient/client.py @@ -100,6 +100,8 @@ class SessionClient(adapter.LegacyJsonAdapter): def __init__(self, *args, **kwargs): self.api_version = kwargs.pop('api_version', None) self.api_version = self.api_version or api_versions.APIVersion() + self.retries = kwargs.pop('retries', 0) + self._logger = logging.getLogger(__name__) super(SessionClient, self).__init__(*args, **kwargs) def request(self, *args, **kwargs): @@ -125,7 +127,17 @@ class SessionClient(adapter.LegacyJsonAdapter): def _cs_request(self, url, method, **kwargs): # this function is mostly redundant but makes compatibility easier kwargs.setdefault('authenticated', True) - return self.request(url, method, **kwargs) + attempts = 0 + while True: + attempts += 1 + try: + return self.request(url, method, **kwargs) + except exceptions.OverLimit as overlim: + if attempts > self.retries or overlim.retry_after < 1: + raise + msg = "Retrying after %s seconds." % overlim.retry_after + self._logger.debug(msg) + sleep(overlim.retry_after) def get(self, url, **kwargs): return self._cs_request(url, 'GET', **kwargs) @@ -334,6 +346,13 @@ class HTTPClient(object): attempts -= 1 auth_attempts += 1 continue + except exceptions.OverLimit as overlim: + if attempts > self.retries or overlim.retry_after < 1: + raise + msg = "Retrying after %s seconds." % overlim.retry_after + self._logger.debug(msg) + sleep(overlim.retry_after) + continue except exceptions.ClientException as e: if attempts > self.retries: raise @@ -576,6 +595,7 @@ def _construct_http_client(username=None, password=None, project_id=None, service_type=service_type, service_name=service_name, region_name=region_name, + retries=retries, api_version=api_version, **kwargs) else: diff --git a/cinderclient/exceptions.py b/cinderclient/exceptions.py index cab74c64a..03a50e797 100644 --- a/cinderclient/exceptions.py +++ b/cinderclient/exceptions.py @@ -16,6 +16,9 @@ """ Exception definitions. """ +from datetime import datetime + +from oslo_utils import timeutils class UnsupportedVersion(Exception): @@ -80,7 +83,8 @@ class ClientException(Exception): """ The base exception class for all exceptions this library raises. """ - def __init__(self, code, message=None, details=None, request_id=None): + def __init__(self, code, message=None, details=None, + request_id=None, response=None): self.code = code # NOTE(mriedem): Use getattr on self.__class__.message since # BaseException.message was dropped in python 3, see PEP 0352. @@ -147,6 +151,27 @@ class OverLimit(ClientException): http_status = 413 message = "Over limit" + def __init__(self, code, message=None, details=None, + request_id=None, response=None): + super(OverLimit, self).__init__(code, message=message, + details=details, request_id=request_id, + response=response) + self.retry_after = 0 + self._get_rate_limit(response) + + def _get_rate_limit(self, resp): + if resp.headers: + utc_now = timeutils.utcnow() + value = resp.headers.get('Retry-After', '0') + try: + value = datetime.strptime(value, '%a, %d %b %Y %H:%M:%S %Z') + if value > utc_now: + self.retry_after = ((value - utc_now).seconds) + else: + self.retry_after = 0 + except ValueError: + self.retry_after = int(value) + # NotImplemented is a python keyword. class HTTPNotImplemented(ClientException): @@ -193,10 +218,10 @@ def from_response(response, body): message = error.get('message', message) details = error.get('details', details) return cls(code=response.status_code, message=message, details=details, - request_id=request_id) + request_id=request_id, response=response) else: return cls(code=response.status_code, request_id=request_id, - message=response.reason) + message=response.reason, response=response) class VersionNotFoundForAPIMethod(Exception): diff --git a/cinderclient/tests/unit/test_client.py b/cinderclient/tests/unit/test_client.py index 756d2787a..253b9d2d5 100644 --- a/cinderclient/tests/unit/test_client.py +++ b/cinderclient/tests/unit/test_client.py @@ -162,6 +162,31 @@ class ClientTest(utils.TestCase): mock.sentinel.url, 'POST', **kwargs) self.assertEqual(1, mock_log.call_count) + @mock.patch.object(cinderclient.client, '_log_request_id') + @mock.patch.object(adapter.Adapter, 'request') + def test_sessionclient_request_method_raises_overlimit( + self, mock_request, mock_log): + resp = { + "overLimitFault": { + "message": "This request was rate-limited.", + "code": 413 + } + } + + mock_response = utils.TestResponse({ + "status_code": 413, + "text": json.dumps(resp), + }) + + # 'request' method of Adaptor will return 413 response + mock_request.return_value = mock_response + session_client = cinderclient.client.SessionClient( + session=mock.Mock()) + + self.assertRaises(exceptions.OverLimit, session_client.request, + mock.sentinel.url, 'GET') + self.assertEqual(1, mock_log.call_count) + @mock.patch.object(exceptions, 'from_response') def test_keystone_request_raises_auth_failure_exception( self, mock_from_resp): diff --git a/cinderclient/tests/unit/test_exceptions.py b/cinderclient/tests/unit/test_exceptions.py index 0559705c2..2504f6e90 100644 --- a/cinderclient/tests/unit/test_exceptions.py +++ b/cinderclient/tests/unit/test_exceptions.py @@ -14,6 +14,8 @@ """Tests the cinderclient.exceptions module.""" +import datetime +import mock import requests from cinderclient import exceptions @@ -30,3 +32,33 @@ class ExceptionsTest(utils.TestCase): ex = exceptions.from_response(response, body) self.assertIs(exceptions.ClientException, type(ex)) self.assertEqual('n/a', ex.message) + + def test_from_response_overlimit(self): + response = requests.Response() + response.status_code = 413 + response.headers = {"Retry-After": '10'} + body = {'keys': ({})} + ex = exceptions.from_response(response, body) + self.assertEqual(10, ex.retry_after) + self.assertIs(exceptions.OverLimit, type(ex)) + + @mock.patch('oslo_utils.timeutils.utcnow', + return_value=datetime.datetime(2016, 6, 30, 12, 41, 55)) + def test_from_response_overlimit_gmt(self, mock_utcnow): + response = requests.Response() + response.status_code = 413 + response.headers = {"Retry-After": "Thu, 30 Jun 2016 12:43:20 GMT"} + body = {'keys': ({})} + ex = exceptions.from_response(response, body) + self.assertEqual(85, ex.retry_after) + self.assertIs(exceptions.OverLimit, type(ex)) + self.assertTrue(mock_utcnow.called) + + def test_from_response_overlimit_without_header(self): + response = requests.Response() + response.status_code = 413 + response.headers = {} + body = {'keys': ({})} + ex = exceptions.from_response(response, body) + self.assertEqual(0, ex.retry_after) + self.assertIs(exceptions.OverLimit, type(ex)) diff --git a/cinderclient/tests/unit/test_http.py b/cinderclient/tests/unit/test_http.py index b425fd817..99ed1b21c 100644 --- a/cinderclient/tests/unit/test_http.py +++ b/cinderclient/tests/unit/test_http.py @@ -45,6 +45,12 @@ bad_401_response = utils.TestResponse({ }) bad_401_request = mock.Mock(return_value=(bad_401_response)) +bad_413_response = utils.TestResponse({ + "status_code": 413, + "headers": {"Retry-After": "1", "x-compute-request-id": "1234"}, +}) +bad_413_request = mock.Mock(return_value=(bad_413_response)) + bad_500_response = utils.TestResponse({ "status_code": 500, "text": '{"error": {"message": "FAILED!", "details": "DETAILS!"}}', @@ -158,6 +164,43 @@ class ClientTest(utils.TestCase): test_get_call() self.assertEqual([], self.requests) + def test_rate_limit_overlimit_exception(self): + cl = get_authed_client(retries=1) + + self.requests = [bad_413_request, + bad_413_request, + mock_request] + + def request(*args, **kwargs): + next_request = self.requests.pop(0) + return next_request(*args, **kwargs) + + @mock.patch.object(requests, "request", request) + @mock.patch('time.time', mock.Mock(return_value=1234)) + def test_get_call(): + resp, body = cl.get("/hi") + self.assertRaises(exceptions.OverLimit, test_get_call) + self.assertEqual([mock_request], self.requests) + + def test_rate_limit(self): + cl = get_authed_client(retries=1) + + self.requests = [bad_413_request, mock_request] + + def request(*args, **kwargs): + next_request = self.requests.pop(0) + return next_request(*args, **kwargs) + + @mock.patch.object(requests, "request", request) + @mock.patch('time.time', mock.Mock(return_value=1234)) + def test_get_call(): + resp, body = cl.get("/hi") + return resp, body + + resp, body = test_get_call() + self.assertEqual(200, resp.status_code) + self.assertEqual([], self.requests) + def test_retry_limit(self): cl = get_authed_client(retries=1)