Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add retries to requests sessions #14740

Merged
merged 1 commit into from
Jan 24, 2024
Merged

feat: Add retries to requests sessions #14740

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions awx/main/credential_plugins/hashivault.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ def method_auth(**kwargs):
cacert = kwargs.get('cacert', None)

sess = requests.Session()
sess.mount(url, requests.adapters.HTTPAdapter(max_retries=5))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does HTTPAdapter have backoff between retries? or does it wait a fixed amount of time between posts/gets? is this configurable?

what http code triggers the retry?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTTPAdapter passes the max_retries argument to urlopen from urllib3.poolmanager.PoolManager.
Based on the description at https://urllib3.readthedocs.io/en/stable/reference/urllib3.connectionpool.html#urllib3.HTTPConnectionPool.urlopen there is no builtin back-off behaviour, unless we use a Retry object in which case all of this is configurable.

max_retries is only used for things like DNS lookup errors, connection time outs, ... as soon as the request hit the server this doesn't apply anymore.
See https://docs.python-requests.org/en/latest/api/#requests.adapters.HTTPAdapter


# Namespace support
if kwargs.get('namespace'):
Expand Down Expand Up @@ -263,6 +264,7 @@ def kv_backend(**kwargs):
}

sess = requests.Session()
sess.mount(url, requests.adapters.HTTPAdapter(max_retries=5))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that the sess.post below are already in a retry loop, so I'm wondering if this mount is needed, or how it will work with the retry logic below

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The max_retries for HTTPAdapter are only applicable for connection level errors while the retry loop is only relevant in case of an HTTP 412 (returned in case a resource in Vault isn't replicated yet).

sess.headers['Authorization'] = 'Bearer {}'.format(token)
# Compatibility header for older installs of Hashicorp Vault
sess.headers['X-Vault-Token'] = token
Expand Down Expand Up @@ -333,6 +335,7 @@ def ssh_backend(**kwargs):
request_kwargs['json']['valid_principals'] = kwargs['valid_principals']

sess = requests.Session()
sess.mount(url, requests.adapters.HTTPAdapter(max_retries=5))
sess.headers['Authorization'] = 'Bearer {}'.format(token)
if kwargs.get('namespace'):
sess.headers['X-Vault-Namespace'] = kwargs['namespace']
Expand Down
Loading