Azure: Add default timeout of 5s for all requests#188
Merged
Conversation
This commit changes the `PartnerPortalSession` to always request a connection using a default timeout of 5 seconds. The default timeout is defined via `AZURE_SESSION_TIMEOUT` which can be overridden by an envvar with the same name. With the timeout in place it prevents issues of hanging connections whenever the server doesn't properly respond. Refers to SPSTRAT-708 Signed-off-by: Jonathan Gangi <jgangi@redhat.com>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds a configurable default 5-second timeout for all Azure PartnerPortalSession HTTP requests, replacing hardcoded timeouts and ensuring all login and generic requests respect the shared AZURE_SESSION_TIMEOUT constant. Sequence diagram for PartnerPortalSession request with default timeoutsequenceDiagram
actor Caller
participant PartnerPortalSession
participant requests_Session as requests_Session
Caller->>PartnerPortalSession: _request(method, path, params, headers)
PartnerPortalSession->>PartnerPortalSession: timeout = AZURE_SESSION_TIMEOUT
PartnerPortalSession->>requests_Session: request(method, url, params, headers, timeout)
requests_Session-->>PartnerPortalSession: Response
PartnerPortalSession-->>Caller: Response
Sequence diagram for PartnerPortalSession login with shared timeoutsequenceDiagram
actor Caller
participant PartnerPortalSession
participant requests_Session as requests_Session
Caller->>PartnerPortalSession: _login()
PartnerPortalSession->>requests_Session: post(url, headers, data, timeout=AZURE_SESSION_TIMEOUT)
requests_Session-->>PartnerPortalSession: Response
PartnerPortalSession->>PartnerPortalSession: AccessToken(Response.json())
PartnerPortalSession-->>Caller: AccessToken
Class diagram for updated Azure PartnerPortalSession timeout handlingclassDiagram
class SessionModule {
+float AZURE_SESSION_TIMEOUT
}
class PartnerPortalSession {
-session requests_Session
-_prefix_url str
-auth_keys dict
+_login() AccessToken
+_request(method str, path str, params dict, headers dict, kwargs dict) requests_Response
+get(path str, kwargs dict) requests_Response
+post(path str, kwargs dict) requests_Response
}
class AccessToken {
-token_data dict
}
SessionModule <.. PartnerPortalSession : uses
PartnerPortalSession o-- AccessToken
PartnerPortalSession o-- requests_Session
class requests_Session {
+post(url str, headers dict, data dict, timeout float) requests_Response
+request(method str, url str, params dict, headers dict, timeout float, kwargs dict) requests_Response
}
class requests_Response {
+raise_for_status() void
+json() dict
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Parsing
AZURE_SESSION_TIMEOUTwithfloat(os.environ.get(...))will raise aValueErrorif the env var is set to a non-numeric value; consider handling invalid input (e.g., with a try/except and falling back to the default) to avoid import-time failures. - Because
AZURE_SESSION_TIMEOUTis evaluated at import time, changing the environment variable after startup has no effect; if you expect runtime configurability, consider moving the env lookup into session initialization or a small helper function that is called when creating aPartnerPortalSession.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Parsing `AZURE_SESSION_TIMEOUT` with `float(os.environ.get(...))` will raise a `ValueError` if the env var is set to a non-numeric value; consider handling invalid input (e.g., with a try/except and falling back to the default) to avoid import-time failures.
- Because `AZURE_SESSION_TIMEOUT` is evaluated at import time, changing the environment variable after startup has no effect; if you expect runtime configurability, consider moving the env lookup into session initialization or a small helper function that is called when creating a `PartnerPortalSession`.
## Individual Comments
### Comment 1
<location path="cloudpub/ms_azure/session.py" line_range="14" />
<code_context>
log = logging.getLogger(__name__)
+AZURE_SESSION_TIMEOUT: float = float(os.environ.get("AZURE_SESSION_TIMEOUT", 5.0))
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Parsing the timeout env var at import time can raise on invalid values and crash the module import.
If `AZURE_SESSION_TIMEOUT` is non-numeric, `float(...)` will raise `ValueError` at import and can crash the process. Please parse this defensively (e.g., `try/except` with a sane default) so bad config values don’t prevent startup or at least fail in a controlled way.
</issue_to_address>
### Comment 2
<location path="tests/ms_azure/test_session.py" line_range="75" />
<code_context>
session_mock.return_value.request.assert_called_once()
session_mock.return_value.post.assert_called_once_with(
- login_url, headers=login_header, data=login_data, timeout=30
+ login_url, headers=login_header, data=login_data, timeout=AZURE_SESSION_TIMEOUT
)
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests to verify the default timeout value and env-var override for `AZURE_SESSION_TIMEOUT`.
This assertion now checks that `AZURE_SESSION_TIMEOUT` is passed, but there’s no test confirming its behavior. Please add tests that (1) verify the default timeout is 5.0 when the env var is unset, and (2) override `AZURE_SESSION_TIMEOUT` via `monkeypatch`/`os.environ` and confirm the login request uses that value. This will more robustly cover the timeout configuration and prevent regressions.
Suggested implementation:
```python
import importlib
import pytest
from cloudpub.ms_azure.session import AZURE_SESSION_TIMEOUT, AccessToken, PartnerPortalSession
from cloudpub.utils import join_url
```
```python
session_mock.return_value.request.assert_called_once()
session_mock.return_value.post.assert_called_once_with(
login_url, headers=login_header, data=login_data, timeout=AZURE_SESSION_TIMEOUT
)
def test_azure_session_timeout_default(monkeypatch):
"""Default timeout should be 5.0s when AZURE_SESSION_TIMEOUT is not set."""
# Ensure the env var is not set before reloading the module
monkeypatch.delenv("AZURE_SESSION_TIMEOUT", raising=False)
import cloudpub.ms_azure.session as session_module
session_module = importlib.reload(session_module)
assert session_module.AZURE_SESSION_TIMEOUT == 5.0
def test_azure_session_timeout_env_override_is_used_in_login(
monkeypatch,
session_mock,
login_url,
login_header,
login_data,
):
"""AZURE_SESSION_TIMEOUT env var should override the default and be used for the login call."""
monkeypatch.setenv("AZURE_SESSION_TIMEOUT", "12.5")
import cloudpub.ms_azure.session as session_module
session_module = importlib.reload(session_module)
# Re-create PartnerPortalSession from the reloaded module so it uses the new timeout value.
partner_portal_session = session_module.PartnerPortalSession()
# Trigger the login flow; adjust arguments to match the real method signature.
partner_portal_session.login(login_url, login_header, login_data)
# The login request should use the overridden timeout value from the module.
session_mock.return_value.post.assert_called_once_with(
login_url,
headers=login_header,
data=login_data,
timeout=session_module.AZURE_SESSION_TIMEOUT,
)
```
These edits assume a few details that you may need to align with the existing test suite:
1. **`PartnerPortalSession.login` signature**
- Update the `partner_portal_session.login(...)` call to match the actual method signature.
- If the login flow is invoked differently (e.g. `partner_portal_session.get_access_token(...)` or similar), call that instead.
2. **`session_mock` fixture wiring**
- The new test assumes `session_mock` is already correctly patching the underlying HTTP session used by `PartnerPortalSession` (e.g. `requests.Session` in `cloudpub.ms_azure.session`).
- If `PartnerPortalSession` in the reloaded module isn’t using the existing `session_mock` fixture, ensure your fixture patches the correct symbol in `cloudpub.ms_azure.session` and that the patch still applies after `importlib.reload`.
3. **Existing fixtures (`login_url`, `login_header`, `login_data`)**
- The new test reuses these fixtures; if they are named differently or constructed via a helper, adjust the parameter list and call site accordingly.
With those adjustments, the two tests will (1) validate the default timeout of `5.0` when the env var is unset and (2) confirm that overriding `AZURE_SESSION_TIMEOUT` via the environment is honored by the login HTTP request.
</issue_to_address>
### Comment 3
<location path="tests/ms_azure/test_session.py" line_range="113-118" />
<code_context>
getattr(session, method)(path, json)
mock_session.return_value.request.assert_called_once_with(
- method, url=url, params=put_param, headers=put_headers, json={"foo": "bar"}
+ method,
+ url=url,
+ params=put_param,
+ headers=put_headers,
+ json={"foo": "bar"},
+ timeout=AZURE_SESSION_TIMEOUT,
)
else:
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test case to ensure an explicitly provided timeout overrides `AZURE_SESSION_TIMEOUT` in `_request`.
Since `_request` now accepts an explicit `timeout` via `**kwargs`, please add coverage for the override behavior—for example, calling `session.put(path, json, timeout=10)` and asserting that `request` is invoked with `timeout=10` instead of `AZURE_SESSION_TIMEOUT` to verify precedence and avoid regressions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Patches `AZURE_SESSION_TIMEOUT` in the environment, `importlib.reload`s `cloudpub.ms_azure.session` (because `AZURE_SESSION_TIMEOUT` is set at import time in `14:14:cloudpub/ms_azure/session.py`).
- Asserts the reloaded module’s constant and checks that both `_login`’s post and the API request use that timeout.
- Uses `try / finally` with a final reload so the module is reset even when a check fails, and so later tests see the normal timeout again.
- Parametrizes `get`, `post`, and `put` with `timeout=88.0`.
- Asserts `session.request` is called with `timeout=88.0`, matching `_request`’s `kwargs.pop("timeout", AZURE_SESSION_TIMEOUT)` in `165:168:cloudpub/ms_azure/session.py`.
- Imports added: `importlib`, `os`.
Refers to SPSTRAT-708
Signed-off-by: Jonathan Gangi <jgangi@redhat.com>
Assisted-by: Cursor/Gemini
lslebodn
reviewed
Apr 7, 2026
Collaborator
lslebodn
left a comment
lslebodn
left a comment
There was a problem hiding this comment.
BTW based on logs in RHELDST-34940. We hit timeout mostly in Sending a get request to ....
Maybe we should have automatic retry for get/put requests similar as in starmap-client. I am not sure about automatic retry for Post requests. But that can be done in different MR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit changes the
PartnerPortalSessionto always request a connection using a default timeout of 5 seconds.The default timeout is defined via
AZURE_SESSION_TIMEOUTwhich can be overridden by an envvar with the same name.With the timeout in place it prevents issues of hanging connections whenever the server doesn't properly respond.
Refers to SPSTRAT-708
Summary by Sourcery
Introduce a configurable default timeout for Azure partner portal HTTP requests to avoid hanging connections.
Enhancements:
Tests: