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.

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

Add retry for ULN authentication #3892

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGES/3891.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Added a retry mechanism for client authentication in ULN-based downloaders to improve connection stability.
31 changes: 30 additions & 1 deletion pulp_rpm/app/downloaders.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import os
import asyncio

from aiohttp_xmlrpc.client import ServerProxy, _Method
from logging import getLogger
Expand Down Expand Up @@ -174,7 +175,7 @@ async def _run(self, extra_data=None):
SERVER_URL, proxy=self.proxy, proxy_auth=self.proxy_auth, auth=self.auth
)
if not self.session_key:
self.session_key = await client.auth.login(self.username, self.password)
self.session_key = await self._login_and_retry(client)
if len(self.session_key) != 43:
raise UlnCredentialsError()
self.headers = {"X-ULN-API-User-Key": self.session_key}
Expand All @@ -196,6 +197,34 @@ async def _run(self, extra_data=None):
client.close()
return to_return

async def _login_and_retry(self, client, max_attempts=5, delay=1):
"""
Attempts to log in using the provided client. Retries up to `max_attempts`
times with a delay between attempts.

Args:
client: The client object used for authentication.
max_attempts (int): Maximum number of login attempts.
delay (int or float): Delay in seconds between retries.

Returns:
The session key from the login.

Raises:
UlnCredentialsError: If all login attempts fail.
"""
for attempt in range(1, max_attempts + 1):
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW we do have the backoff crate for this which we already use in pulpcore: https://github.com/pulp/pulpcore/blob/139955c975af0c4bbaad2096f25808206b4d67a3/pulpcore/download/http.py#L250

With that said, this code isn't complex, I'm unsure whether we actually care to pull in a new dependency to the RPM plugin that may conflict with pulpcore at some point if we're not careful

Does the connection seem to drop specifically on the login attempt, but not during the downloads themselves? Or I suppose presumably the existing retry logic might be handling that.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ggainey Do you have an opinion. I'm a bit partial to leaving it as-is

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I would prefer to keep the default number of retries consistent with RpmRemote, so 4.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the connection seem to drop specifically on the login attempt, but not during the downloads themselves? Or I suppose presumably the existing retry logic might be handling that.

Yes the connection drops on the login attempt. This happens a lot during a sync and will throw an error. Adding the login retry fixes the issue.

Also I would prefer to keep the default number of retries consistent with RpmRemote, so 4.

I can change this.

try:
return await client.auth.login(self.username, self.password)
except Exception as exc:
if attempt < max_attempts:
log.info(f"ULN login attempt {attempt} failed. Retrying...")
await asyncio.sleep(delay)
else:
raise UlnCredentialsError(
f"ULN login failed after {max_attempts} attempts."
) from exc


class AllowProxyServerProxy(ServerProxy):
"""
Expand Down