Fix logging and requests sync calls#64
Fix logging and requests sync calls#64carbonin wants to merge 1 commit intoopenshift-assisted:masterfrom
Conversation
The blockbuster tool revealed that both logging and the use of the requests library for getting the token and pull secret were blocking the async nature of the mcp server. This commit handles the logging by splitting the log into a producer and consumer through a queue and using async.to_thread for the requests calls.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carbonin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughAsync refactor converts token and pull-secret retrieval to awaitable functions using asyncio.to_thread for blocking HTTP calls. Logger switches to a queue-backed architecture with QueueHandler/QueueListener. Exception logging now uses log.exception. Tests are migrated to async style aligning with new interfaces. Public APIs updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant SSO as SSO Endpoint
Client->>Server: await get_access_token()
activate Server
Server->>Server: to_thread(blocking HTTP)
Server->>SSO: POST /token
SSO-->>Server: 200 OK (access_token)
deactivate Server
Server-->>Client: access_token
sequenceDiagram
participant Caller
participant IC as InventoryClient
participant PS as Pull-Secret Service
Caller->>IC: await get_pull_secret()
alt cache miss
IC->>IC: to_thread(blocking HTTP)
IC->>PS: POST PULL_SECRET_URL (Bearer token)
PS-->>IC: 200 OK (pull_secret)
IC->>IC: cache pull_secret
else cache hit
IC->>IC: return cached pull_secret
end
IC-->>Caller: pull_secret
sequenceDiagram
participant AppLogger
participant QH as QueueHandler
participant Q as Queue
participant QL as QueueListener
participant SH as StreamHandler
participant FH as FileHandler
AppLogger->>QH: log(record)
QH->>Q: enqueue(record)
Q->>QL: record
QL->>SH: handle(record)
QL->>FH: handle(record)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the "Integrations" page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
openshift-ci
Bot
added
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/test_assisted_service_api.py (2)
414-420: Repeated pull secret mocking can be simplified.This is another instance of redundant pull secret mocking that could be simplified by using the existing client fixture.
446-452: Repeated pull secret mocking in infra env creation test.This is another instance where the pull secret mocking could be removed in favor of using the pre-configured client fixture.
🧹 Nitpick comments (3)
service_client/assisted_service_api.py (1)
61-62: Consider storing the pull secret more securely.While the current implementation caches the pull secret in memory as a plain string, consider implementing additional security measures for sensitive data:
- Use a secure string wrapper that clears memory on deletion
- Implement automatic expiration/refresh of cached secrets
- Consider using a dedicated secrets management solution for production environments
tests/test_assisted_service_api.py (2)
35-39: Extract pull secret mocking to a fixture for better test maintainability.The pull secret mocking pattern is repeated across multiple tests. Consider extracting it to a shared fixture to reduce code duplication and improve test maintainability.
Add a new fixture at the class level:
@pytest.fixture def mock_pull_secret(self) -> Mock: """Mock pull secret response.""" with patch("requests.post") as mock_post: mock_response = Mock() mock_response.text = "mock-pull-secret" mock_post.return_value = mock_response yield mock_postThen simplify the client fixture:
@pytest.fixture -def client(self, mock_access_token: str) -> InventoryClient: +def client(self, mock_access_token: str, mock_pull_secret: Mock) -> InventoryClient: """Create a test client instance.""" - with patch("requests.post") as mock_post: - mock_response = Mock() - mock_response.text = "mock-pull-secret" - mock_post.return_value = mock_response - return InventoryClient(mock_access_token) + return InventoryClient(mock_access_token)
376-382: Remove redundant pull secret mocking in tests that don't use it.Several test methods mock the pull secret HTTP response but never actually verify or use it. This adds unnecessary complexity to the tests.
For tests like
test_create_cluster_success,test_create_cluster_single_node, andtest_create_infra_env_success, consider using the existingclientfixture which already handles pull secret mocking, instead of re-mocking it in each test.- with ( - patch.object(client, "_installer_api") as mock_installer_api, - patch("requests.post") as mock_post, - ): - # Mock pull secret HTTP response - mock_response = Mock() - mock_response.text = "mock-pull-secret" - mock_post.return_value = mock_response - + with patch.object(client, "_installer_api") as mock_installer_api:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
server.py(16 hunks)service_client/assisted_service_api.py(4 hunks)service_client/exceptions.py(1 hunks)service_client/logger.py(3 hunks)tests/test_assisted_service_api.py(10 hunks)tests/test_server.py(9 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
server.py (2)
tests/test_assisted_service_api.py (1)
client(33-39)service_client/assisted_service_api.py (1)
InventoryClient(22-518)
tests/test_server.py (1)
server.py (1)
get_access_token(91-130)
tests/test_assisted_service_api.py (1)
service_client/assisted_service_api.py (1)
get_pull_secret(43-65)
🔇 Additional comments (15)
service_client/exceptions.py (1)
49-49: LGTM! Good choice using log.exception() for better debugging.The change from
log.error()tolog.exception()is a good improvement that will include the full stack trace in the logs, making debugging easier without changing the exception propagation behavior.server.py (3)
91-91: LGTM! Function correctly converted to async.The function signature change aligns with the PR's goal to prevent blocking the async event loop.
127-127: Good use of asyncio.to_thread for the blocking HTTP call.Wrapping the
requests.postcall withasyncio.to_threadcorrectly prevents blocking the event loop while maintaining the existing error handling behavior.
154-154: LGTM! All call sites correctly updated to await the async token function.All instances of
get_access_token()have been properly updated to useawait, maintaining consistency throughout the codebase.Also applies to: 179-179, 212-212, 236-236, 262-262, 356-356, 419-419, 451-451, 472-472, 492-492, 518-518, 560-560, 594-594, 620-620
tests/test_server.py (2)
103-111: Test methods correctly converted to async.The test methods have been properly converted to async and correctly await the
get_access_token()function.
115-129: LGTM! All test methods properly handle async behavior.All test methods have been correctly converted to async and properly await the async
get_access_token()function while maintaining the same test coverage and scenarios.Also applies to: 131-145, 148-164, 177-195, 208-219, 221-236
service_client/logger.py (4)
93-97: Well-structured factory function for file handlers.The new private factory function cleanly encapsulates handler creation with the sensitive formatter.
100-104: Good separation of concerns with stream handler factory.The stream handler factory follows the same pattern as the file handler, maintaining consistency.
120-143: Excellent implementation of non-blocking logging architecture!The queue-based logging implementation effectively prevents blocking I/O operations from affecting the async event loop. Key benefits:
- QueueHandler ensures logging calls return immediately
- QueueListener processes logs on a background thread
- Proper handling of multiple loggers with propagation disabled to avoid duplicates
145-152: Good practice with graceful shutdown handler.The atexit handler ensures the queue listener is properly stopped when the application exits, with appropriate exception handling for best-effort cleanup.
service_client/assisted_service_api.py (3)
43-46: Well-implemented lazy loading pattern for pull secret.The caching mechanism avoids redundant API calls while maintaining the async nature of the operation.
56-58: Good use of asyncio.to_thread for the blocking HTTP call.Properly prevents the
requests.postcall from blocking the event loop while maintaining error handling.
245-245: LGTM! Consistent async/await usage for pull secret retrieval.Both
create_clusterandcreate_infra_envmethods correctly await the asyncget_pull_secret()method.Also applies to: 275-275
tests/test_assisted_service_api.py (2)
393-398: Verify the pull secret is correctly passed in cluster creation.Good verification that the mock pull secret value is correctly passed to the API call.
465-465: Good verification of pull secret in infra env params.The test correctly verifies that the mocked pull secret value is passed through to the infra environment creation parameters.
| with patch("requests.post") as mock_post: | ||
| mock_response = Mock() | ||
| mock_response.text = "test-pull-secret" | ||
| mock_post.return_value = mock_response | ||
|
|
||
| client = InventoryClient(mock_access_token) | ||
| assert client.access_token == mock_access_token | ||
| assert client.pull_secret == "test-pull-secret" | ||
| assert await client.get_pull_secret() == "test-pull-secret" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test should verify HTTP call parameters.
The test creates a mock but doesn't verify that the HTTP request was made with the correct parameters (URL, headers, timeout). This verification is important to ensure the implementation correctly fetches the pull secret.
with patch("requests.post") as mock_post:
mock_response = Mock()
mock_response.text = "test-pull-secret"
mock_post.return_value = mock_response
client = InventoryClient(mock_access_token)
assert client.access_token == mock_access_token
assert await client.get_pull_secret() == "test-pull-secret"
+
+ # Verify the HTTP call was made correctly
+ mock_post.assert_called_once_with(
+ "https://api.openshift.com/api/accounts_mgmt/v1/access_token",
+ headers={"Authorization": f"Bearer {mock_access_token}"},
+ timeout=30
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with patch("requests.post") as mock_post: | |
| mock_response = Mock() | |
| mock_response.text = "test-pull-secret" | |
| mock_post.return_value = mock_response | |
| client = InventoryClient(mock_access_token) | |
| assert client.access_token == mock_access_token | |
| assert client.pull_secret == "test-pull-secret" | |
| assert await client.get_pull_secret() == "test-pull-secret" | |
| with patch("requests.post") as mock_post: | |
| mock_response = Mock() | |
| mock_response.text = "test-pull-secret" | |
| mock_post.return_value = mock_response | |
| client = InventoryClient(mock_access_token) | |
| assert client.access_token == mock_access_token | |
| assert await client.get_pull_secret() == "test-pull-secret" | |
| # Verify the HTTP call was made correctly | |
| mock_post.assert_called_once_with( | |
| "https://api.openshift.com/api/accounts_mgmt/v1/access_token", | |
| headers={"Authorization": f"Bearer {mock_access_token}"}, | |
| timeout=30 | |
| ) |
|
Ideally we should do away with requests completely and use aiohttp |
|
/hold Going to switch the http client and test this a bit more thoroughly |
openshift-ci
Bot
added
the
do-not-merge/hold
|
Let's cloes this? |
|
Yup |
3 participants
The blockbuster tool revealed that both logging and the use of the requests library for getting the token and pull secret were blocking the async nature of the mcp server.
This commit handles the logging by splitting the log into a producer and consumer through a queue and using async.to_thread for the requests calls.
cc @eranco74 @omertuc
Maybe this was causing some of the slowness in your scale test
If you were only testing with "list_clusters" though, you wouldn't have been hitting the pull secret issue.
I'm not sure how you were passing the token, but I guess you were passing the access token in the request so the token fetching requests call was probably also not an issue.
This only leaves the logging which I'm not sure would be an issue with it disabled with
LOG_TO_FILE=falsein the container, but 🤷 nowblockbusterdoesn't complain so I suppose that's good.Summary by CodeRabbit
New Features
Refactor
Tests