From 373cbdbda8ab37ee48671258bad4c7645cd8ceae Mon Sep 17 00:00:00 2001 From: Rabi Mishra Date: Thu, 15 Aug 2019 12:35:18 +0530 Subject: [PATCH] Allow initializing session with connection retries Though we can now set ``connect_retires`` while creating an adapter object, that would allow retries in case of connection timeout (ex. with session clients derived from Adapater/LegacyJsonAdapater), it can't be used in certain scenarios like endpoint discovery with auth plugin get_discovery() or getting AccessInfo with get_access()/get_auth_ref(). Having ``connect_retries`` in Session constructor would allow users with option of setting it when creating session objects (if they want) and can be overridden per service with the adapter interface. This commit also changes the default value of ``connect_retries`` from 0 to None to allow for adapter's to override retries on the session object. Depends-On: https://review.opendev.org/#/c/680497/ Change-Id: Iffb671fefae23926b1f09017d9db438341eae238 Partial-Bug: #1840235 --- keystoneauth1/session.py | 13 +++-- keystoneauth1/tests/unit/test_session.py | 54 ++++++++++++++++--- .../notes/bug-1840235-ef2946d149ac329c.yaml | 11 ++++ 3 files changed, 69 insertions(+), 9 deletions(-) create mode 100644 releasenotes/notes/bug-1840235-ef2946d149ac329c.yaml diff --git a/keystoneauth1/session.py b/keystoneauth1/session.py index 8486643c..bfc5149e 100644 --- a/keystoneauth1/session.py +++ b/keystoneauth1/session.py @@ -330,6 +330,9 @@ class Session(object): :param rate_semaphore: Semaphore to be used to control concurrency and rate limiting of requests. (optional, defaults to no concurrency or rate control) + :param int connect_retries: the maximum number of retries that should + be attempted for connection errors. + (optional, defaults to 0 - never retry). """ user_agent = None @@ -343,7 +346,8 @@ class Session(object): redirect=_DEFAULT_REDIRECT_LIMIT, additional_headers=None, app_name=None, app_version=None, additional_user_agent=None, discovery_cache=None, split_loggers=None, - collect_timing=False, rate_semaphore=None): + collect_timing=False, rate_semaphore=None, + connect_retries=0): self.auth = auth self.session = _construct_session(session) @@ -364,6 +368,7 @@ class Session(object): # so we can distinguish between the value being set or not. self._split_loggers = split_loggers self._collect_timing = collect_timing + self._connect_retries = connect_retries self._api_times = [] self._rate_semaphore = rate_semaphore or NoOpSemaphore() @@ -603,7 +608,7 @@ class Session(object): user_agent=None, redirect=None, authenticated=None, endpoint_filter=None, auth=None, requests_auth=None, raise_exc=True, allow_reauth=True, log=True, - endpoint_override=None, connect_retries=0, logger=None, + endpoint_override=None, connect_retries=None, logger=None, allow=None, client_name=None, client_version=None, microversion=None, microversion_service_type=None, status_code_retries=0, retriable_status_codes=None, @@ -636,7 +641,7 @@ class Session(object): for forever/never. (optional) :param int connect_retries: the maximum number of retries that should be attempted for connection errors. - (optional, defaults to 0 - never retry). + (optional, defaults to None - never retry). :param bool authenticated: True if a token should be attached to this request, False if not or None for attach if an auth_plugin is available. @@ -730,6 +735,8 @@ class Session(object): else: split_loggers = None logger = logger or utils.get_logger(__name__) + if connect_retries is None: + connect_retries = self._connect_retries # HTTP 503 - Service Unavailable retriable_status_codes = retriable_status_codes or [503] rate_semaphore = rate_semaphore or self._rate_semaphore diff --git a/keystoneauth1/tests/unit/test_session.py b/keystoneauth1/tests/unit/test_session.py index 1a169f76..e79baf35 100644 --- a/keystoneauth1/tests/unit/test_session.py +++ b/keystoneauth1/tests/unit/test_session.py @@ -421,24 +421,66 @@ class SessionTests(utils.TestCase): self.assertIn('--cacert', self.logger.output) self.assertIn(path_to_certs, self.logger.output) - def test_connect_retries(self): + def _connect_retries_check(self, session, expected_retries=0, + call_args=None): + call_args = call_args or {} + self.stub_url('GET', exc=requests.exceptions.Timeout()) - session = client_session.Session() - retries = 3 + call_args['url'] = self.TEST_URL with mock.patch('time.sleep') as m: self.assertRaises(exceptions.ConnectTimeout, session.get, - self.TEST_URL, connect_retries=retries) + **call_args) - self.assertEqual(retries, m.call_count) + self.assertEqual(expected_retries, m.call_count) # 3 retries finishing with 2.0 means 0.5, 1.0 and 2.0 m.assert_called_with(2.0) # we count retries so there will be one initial request + 3 retries self.assertThat(self.requests_mock.request_history, - matchers.HasLength(retries + 1)) + matchers.HasLength(expected_retries + 1)) + + def test_session_connect_retries(self): + retries = 3 + session = client_session.Session(connect_retries=retries) + self._connect_retries_check(session=session, expected_retries=retries) + + def test_call_args_connect_retries_session_init(self): + session = client_session.Session() + retries = 3 + call_args = {'connect_retries': retries} + self._connect_retries_check(session=session, + expected_retries=retries, + call_args=call_args) + + def test_call_args_connect_retries_overrides_session_retries(self): + session_retries = 6 + call_arg_retries = 3 + call_args = {'connect_retries': call_arg_retries} + session = client_session.Session(connect_retries=session_retries) + self._connect_retries_check(session=session, + expected_retries=call_arg_retries, + call_args=call_args) + + def test_override_session_connect_retries_for_request(self): + session_retries = 1 + session = client_session.Session(connect_retries=session_retries) + + self.stub_url('GET', exc=requests.exceptions.Timeout()) + call_args = {'connect_retries': 0} + + with mock.patch('time.sleep') as m: + self.assertRaises( + exceptions.ConnectTimeout, + session.request, + self.TEST_URL, + 'GET', + **call_args + ) + + self.assertEqual(0, m.call_count) def test_connect_retries_interval_limit(self): self.stub_url('GET', exc=requests.exceptions.Timeout()) diff --git a/releasenotes/notes/bug-1840235-ef2946d149ac329c.yaml b/releasenotes/notes/bug-1840235-ef2946d149ac329c.yaml new file mode 100644 index 00000000..e45d00ce --- /dev/null +++ b/releasenotes/notes/bug-1840235-ef2946d149ac329c.yaml @@ -0,0 +1,11 @@ +--- +features: + - | + [`feature bug 1840235 `_] + Adds ``connect_retries`` to Session.__init__(), that can then be used + by projects when creating session objects, to set the required number of + retries for new connection requests. This would specifically help avoid + a scalability issue that results in number of ConnectTimeout errors when + doing endpoint discovery and fetching roles using an auth plugin under + heavy load. This still allows for it to be overridden per service with + the adapter interface.