From bca9ee7d3cb4202a529c454a8828890ece141d37 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Fri, 26 Jul 2019 11:38:18 +0200 Subject: [PATCH] Allow requesting fixed retry delay instead of exponential Clients like ironicclient and swiftclient use fixed delay for their build-in retry functionality. To replace it without changing behavior we need a similar feature. Change-Id: I1f9de98dae5719842f03d45e5a9d724199d5718b --- keystoneauth1/adapter.py | 19 ++++- keystoneauth1/loading/adapter.py | 19 +++++ keystoneauth1/session.py | 78 ++++++++++++++----- .../tests/unit/loading/test_adapter.py | 13 +++- keystoneauth1/tests/unit/test_session.py | 39 ++++++++++ .../notes/retry-delay-68d0c0a1dffcf2fd.yaml | 6 ++ 6 files changed, 150 insertions(+), 24 deletions(-) create mode 100644 releasenotes/notes/retry-delay-68d0c0a1dffcf2fd.yaml diff --git a/keystoneauth1/adapter.py b/keystoneauth1/adapter.py index 6bba62f3..8d438f37 100644 --- a/keystoneauth1/adapter.py +++ b/keystoneauth1/adapter.py @@ -103,6 +103,14 @@ class Adapter(object): :param int concurrency: How many simultaneous http requests this Adapter can be used for. (optional, defaults to None, which means no limit). + :param float connect_retry_delay: + Delay (in seconds) between two connect retries (if enabled). + By default exponential retry starting with 0.5 seconds up to + a maximum of 60 seconds is used. + :param float status_code_retry_delay: + Delay (in seconds) between two status code retries (if enabled). + By default exponential retry starting with 0.5 seconds up to + a maximum of 60 seconds is used. """ client_name = None @@ -119,6 +127,7 @@ class Adapter(object): default_microversion=None, status_code_retries=None, retriable_status_codes=None, raise_exc=None, rate_limit=None, concurrency=None, + connect_retry_delay=None, status_code_retry_delay=None, ): if version and (min_version or max_version): raise TypeError( @@ -148,6 +157,8 @@ class Adapter(object): self.default_microversion = default_microversion self.status_code_retries = status_code_retries self.retriable_status_codes = retriable_status_codes + self.connect_retry_delay = connect_retry_delay + self.status_code_retry_delay = status_code_retry_delay self.raise_exc = raise_exc self.global_request_id = global_request_id @@ -195,10 +206,10 @@ class Adapter(object): kwargs.setdefault('auth', self.auth) if self.user_agent: kwargs.setdefault('user_agent', self.user_agent) - if self.connect_retries is not None: - kwargs.setdefault('connect_retries', self.connect_retries) - if self.status_code_retries is not None: - kwargs.setdefault('status_code_retries', self.status_code_retries) + for arg in ('connect_retries', 'status_code_retries', + 'connect_retry_delay', 'status_code_retry_delay'): + if getattr(self, arg) is not None: + kwargs.setdefault(arg, getattr(self, arg)) if self.retriable_status_codes: kwargs.setdefault('retriable_status_codes', self.retriable_status_codes) diff --git a/keystoneauth1/loading/adapter.py b/keystoneauth1/loading/adapter.py index 5fe6b54b..daf7a304 100644 --- a/keystoneauth1/loading/adapter.py +++ b/keystoneauth1/loading/adapter.py @@ -140,11 +140,27 @@ class Adapter(base.BaseLoader): 'connect-retries'), help='The maximum number of retries that should be ' 'attempted for connection errors.'), + cfg.FloatOpt('connect-retry-delay', + deprecated_opts=deprecated_opts.get( + 'connect-retry-delay'), + help='Delay (in seconds) between two retries ' + 'for connection errors. If not set, ' + 'exponential retry starting with 0.5 ' + 'seconds up to a maximum of 60 seconds ' + 'is used.'), cfg.IntOpt('status-code-retries', deprecated_opts=deprecated_opts.get( 'status-code-retries'), help='The maximum number of retries that should be ' 'attempted for retriable HTTP status codes.'), + cfg.FloatOpt('status-code-retry-delay', + deprecated_opts=deprecated_opts.get( + 'status-code-retry-delay'), + help='Delay (in seconds) between two retries ' + 'for retriable status codes. If not set, ' + 'exponential retry starting with 0.5 ' + 'seconds up to a maximum of 60 seconds ' + 'is used.'), ] if include_deprecated: opts += [ @@ -271,7 +287,10 @@ def process_conf_options(confgrp, kwargs): "version is mutually exclusive with min_version and" " max_version") kwargs.setdefault('connect_retries', confgrp.connect_retries) + kwargs.setdefault('connect_retry_delay', confgrp.connect_retry_delay) kwargs.setdefault('status_code_retries', confgrp.status_code_retries) + kwargs.setdefault('status_code_retry_delay', + confgrp.status_code_retry_delay) def register_argparse_arguments(*args, **kwargs): diff --git a/keystoneauth1/session.py b/keystoneauth1/session.py index d5938066..8486643c 100644 --- a/keystoneauth1/session.py +++ b/keystoneauth1/session.py @@ -51,6 +51,7 @@ DEFAULT_USER_AGENT = 'keystoneauth1/%s %s %s/%s' % ( _LOG_CONTENT_TYPES = set(['application/json']) _MAX_RETRY_INTERVAL = 60.0 +_EXPONENTIAL_DELAY_START = 0.5 # NOTE(efried): This is defined in oslo_middleware.request_id.INBOUND_HEADER, # but it didn't seem worth adding oslo_middleware to requirements just for that @@ -239,6 +240,29 @@ class RequestTiming(object): self.elapsed = elapsed +class _Retries(object): + __slots__ = ('_fixed_delay', '_current') + + def __init__(self, fixed_delay=None): + self._fixed_delay = fixed_delay + self.reset() + + def __next__(self): + value = self._current + if not self._fixed_delay: + self._current = min(value * 2, _MAX_RETRY_INTERVAL) + return value + + def reset(self): + if self._fixed_delay: + self._current = self._fixed_delay + else: + self._current = _EXPONENTIAL_DELAY_START + + # Python 2 compatibility + next = __next__ + + class Session(object): """Maintains client communication state and common functionality. @@ -583,7 +607,9 @@ class Session(object): allow=None, client_name=None, client_version=None, microversion=None, microversion_service_type=None, status_code_retries=0, retriable_status_codes=None, - rate_semaphore=None, global_request_id=None, **kwargs): + rate_semaphore=None, global_request_id=None, + connect_retry_delay=None, status_code_retry_delay=None, + **kwargs): """Send an HTTP request with the specified characteristics. Wrapper around `requests.Session.request` to handle tasks such as @@ -673,6 +699,16 @@ class Session(object): and rate limiting of requests. (optional, defaults to no concurrency or rate control) :param global_request_id: Value for the X-Openstack-Request-Id header. + :param float connect_retry_delay: Delay (in seconds) between two + connect retries (if enabled). + By default exponential retry starting + with 0.5 seconds up to a maximum of + 60 seconds is used. + :param float status_code_retry_delay: Delay (in seconds) between two + status code retries (if enabled). + By default exponential retry + starting with 0.5 seconds up to + a maximum of 60 seconds is used. :param kwargs: any other parameter that can be passed to :meth:`requests.Session.request` (such as `headers`). Except: @@ -827,11 +863,15 @@ class Session(object): if redirect is None: redirect = self.redirect + connect_retry_delays = _Retries(connect_retry_delay) + status_code_retry_delays = _Retries(status_code_retry_delay) + send = functools.partial(self._send_request, url, method, redirect, log, logger, split_loggers, connect_retries, status_code_retries, retriable_status_codes, - rate_semaphore) + rate_semaphore, connect_retry_delays, + status_code_retry_delays) try: connection_params = self.get_auth_connection_params(auth=auth) @@ -920,7 +960,7 @@ class Session(object): def _send_request(self, url, method, redirect, log, logger, split_loggers, connect_retries, status_code_retries, retriable_status_codes, rate_semaphore, - connect_retry_delay=0.5, status_code_retry_delay=0.5, + connect_retry_delays, status_code_retry_delays, **kwargs): # NOTE(jamielennox): We handle redirection manually because the # requests lib follows some browser patterns where it will redirect @@ -962,12 +1002,10 @@ class Session(object): if connect_retries <= 0: raise + delay = next(connect_retry_delays) logger.info('Failure: %(e)s. Retrying in %(delay).1fs.', - {'e': e, 'delay': connect_retry_delay}) - time.sleep(connect_retry_delay) - - connect_retry_delay = min(connect_retry_delay * 2, - _MAX_RETRY_INTERVAL) + {'e': e, 'delay': delay}) + time.sleep(delay) return self._send_request( url, method, redirect, log, logger, split_loggers, @@ -975,7 +1013,8 @@ class Session(object): retriable_status_codes=retriable_status_codes, rate_semaphore=rate_semaphore, connect_retries=connect_retries - 1, - connect_retry_delay=connect_retry_delay, + connect_retry_delays=connect_retry_delays, + status_code_retry_delays=status_code_retry_delays, **kwargs) if log: @@ -1000,14 +1039,18 @@ class Session(object): logger.warning("Failed to redirect request to %s as new " "location was not provided.", resp.url) else: - # NOTE(jamielennox): We don't pass through connect_retry_delay. + # NOTE(jamielennox): We don't keep increasing delays. # This request actually worked so we can reset the delay count. + connect_retry_delays.reset() + status_code_retry_delays.reset() new_resp = self._send_request( location, method, redirect, log, logger, split_loggers, rate_semaphore=rate_semaphore, connect_retries=connect_retries, status_code_retries=status_code_retries, retriable_status_codes=retriable_status_codes, + connect_retry_delays=connect_retry_delays, + status_code_retry_delays=status_code_retry_delays, **kwargs) if not isinstance(new_resp.history, list): @@ -1017,24 +1060,23 @@ class Session(object): elif (resp.status_code in retriable_status_codes and status_code_retries > 0): + delay = next(status_code_retry_delays) logger.info('Retriable status code %(code)s. Retrying in ' '%(delay).1fs.', - {'code': resp.status_code, - 'delay': status_code_retry_delay}) - time.sleep(status_code_retry_delay) + {'code': resp.status_code, 'delay': delay}) + time.sleep(delay) - status_code_retry_delay = min(status_code_retry_delay * 2, - _MAX_RETRY_INTERVAL) - - # NOTE(jamielennox): We don't pass through connect_retry_delay. + # NOTE(jamielennox): We don't keep increasing connection delays. # This request actually worked so we can reset the delay count. + connect_retry_delays.reset() return self._send_request( url, method, redirect, log, logger, split_loggers, connect_retries=connect_retries, status_code_retries=status_code_retries - 1, retriable_status_codes=retriable_status_codes, rate_semaphore=rate_semaphore, - status_code_retry_delay=status_code_retry_delay, + connect_retry_delays=connect_retry_delays, + status_code_retry_delays=status_code_retry_delays, **kwargs) return resp diff --git a/keystoneauth1/tests/unit/loading/test_adapter.py b/keystoneauth1/tests/unit/loading/test_adapter.py index f38c98c2..29b80d10 100644 --- a/keystoneauth1/tests/unit/loading/test_adapter.py +++ b/keystoneauth1/tests/unit/loading/test_adapter.py @@ -158,19 +158,24 @@ class ConfLoadingTests(utils.TestCase): self.conf_fx.config( service_type='type', service_name='name', connect_retries=3, status_code_retries=5, + connect_retry_delay=0.5, status_code_retry_delay=2.0, group=self.GROUP) adap = loading.load_adapter_from_conf_options( self.conf_fx.conf, self.GROUP, session='session', auth='auth') self.assertEqual('type', adap.service_type) self.assertEqual('name', adap.service_name) self.assertEqual(3, adap.connect_retries) + self.assertEqual(0.5, adap.connect_retry_delay) self.assertEqual(5, adap.status_code_retries) + self.assertEqual(2.0, adap.status_code_retry_delay) def test_get_conf_options(self): opts = loading.get_adapter_conf_options() for opt in opts: if opt.name.endswith('-retries'): self.assertIsInstance(opt, cfg.IntOpt) + elif opt.name.endswith('-retry-delay'): + self.assertIsInstance(opt, cfg.FloatOpt) elif opt.name != 'valid-interfaces': self.assertIsInstance(opt, cfg.StrOpt) else: @@ -179,7 +184,8 @@ class ConfLoadingTests(utils.TestCase): 'interface', 'valid-interfaces', 'region-name', 'endpoint-override', 'version', 'min-version', 'max-version', 'connect-retries', - 'status-code-retries'}, + 'status-code-retries', 'connect-retry-delay', + 'status-code-retry-delay'}, {opt.name for opt in opts}) def test_get_conf_options_undeprecated(self): @@ -187,6 +193,8 @@ class ConfLoadingTests(utils.TestCase): for opt in opts: if opt.name.endswith('-retries'): self.assertIsInstance(opt, cfg.IntOpt) + elif opt.name.endswith('-retry-delay'): + self.assertIsInstance(opt, cfg.FloatOpt) elif opt.name != 'valid-interfaces': self.assertIsInstance(opt, cfg.StrOpt) else: @@ -194,7 +202,8 @@ class ConfLoadingTests(utils.TestCase): self.assertEqual({'service-type', 'service-name', 'valid-interfaces', 'region-name', 'endpoint-override', 'version', 'min-version', 'max-version', 'connect-retries', - 'status-code-retries'}, + 'status-code-retries', 'connect-retry-delay', + 'status-code-retry-delay'}, {opt.name for opt in opts}) def test_deprecated(self): diff --git a/keystoneauth1/tests/unit/test_session.py b/keystoneauth1/tests/unit/test_session.py index 59846943..1a169f76 100644 --- a/keystoneauth1/tests/unit/test_session.py +++ b/keystoneauth1/tests/unit/test_session.py @@ -458,6 +458,25 @@ class SessionTests(utils.TestCase): self.assertThat(self.requests_mock.request_history, matchers.HasLength(retries + 1)) + def test_connect_retries_fixed_delay(self): + self.stub_url('GET', exc=requests.exceptions.Timeout()) + + session = client_session.Session() + retries = 3 + + with mock.patch('time.sleep') as m: + self.assertRaises(exceptions.ConnectTimeout, + session.get, + self.TEST_URL, connect_retries=retries, + connect_retry_delay=0.5) + + self.assertEqual(retries, m.call_count) + m.assert_has_calls([mock.call(0.5)] * retries) + + # we count retries so there will be one initial request + 3 retries + self.assertThat(self.requests_mock.request_history, + matchers.HasLength(retries + 1)) + def test_http_503_retries(self): self.stub_url('GET', status_code=503) @@ -514,6 +533,26 @@ class SessionTests(utils.TestCase): self.assertThat(self.requests_mock.request_history, matchers.HasLength(1)) + def test_http_status_retries_fixed_delay(self): + self.stub_url('GET', status_code=409) + + session = client_session.Session() + retries = 3 + + with mock.patch('time.sleep') as m: + self.assertRaises(exceptions.Conflict, + session.get, + self.TEST_URL, status_code_retries=retries, + status_code_retry_delay=0.5, + retriable_status_codes=[503, 409]) + + self.assertEqual(retries, m.call_count) + m.assert_has_calls([mock.call(0.5)] * retries) + + # we count retries so there will be one initial request + 3 retries + self.assertThat(self.requests_mock.request_history, + matchers.HasLength(retries + 1)) + def test_http_status_retries_inverval_limit(self): self.stub_url('GET', status_code=409) diff --git a/releasenotes/notes/retry-delay-68d0c0a1dffcf2fd.yaml b/releasenotes/notes/retry-delay-68d0c0a1dffcf2fd.yaml new file mode 100644 index 00000000..d8ec8fc3 --- /dev/null +++ b/releasenotes/notes/retry-delay-68d0c0a1dffcf2fd.yaml @@ -0,0 +1,6 @@ +--- +features: + - | + Allows configuring fixed retry delay for connection and status code retries + via the new parameters ``connect_retry_delay`` and + ``status_code_retry_delay`` accordingly.