Finish out effort of adding and enabling blockbuster in tests #5735
Conversation
mdegat01
changed the title
📝 WalkthroughWalkthroughThe changes refactor several synchronous file system and I/O operations into asynchronous calls using executor helpers. In the supervisor modules, methods now delegate blocking operations (such as property access, file existence checks, and directory creation) to helper functions that run in an executor. Similarly, many test suites have been updated to wrap setup operations, validations, and fixture loading within asynchronous executor calls. Additionally, some test function signatures have been modified from asynchronous to synchronous to align with the new execution context. Changes
Sequence Diagram(s)Asynchronous Operation in Supervisor Module (_addon_save / _addon_restore)sequenceDiagram
participant Client
participant Backup
participant Executor
participant File
Client->>Backup: Call _addon_save or _addon_restore
Backup->>Executor: await sys_run_in_executor(getattr, addon_file, "size") / await sys_run_in_executor(addon_file.path.exists)
Executor->>File: Execute file size or path existence check
File-->>Executor: Return requested data
Executor-->>Backup: Return result asynchronously
Backup->>Client: Return final result
Asynchronous Profile Loading in AppArmorControlsequenceDiagram
participant Client
participant AppArmorControl
participant Executor
participant FS as Filesystem
Client->>AppArmorControl: Call load()
AppArmorControl->>Executor: Run inner function find_profiles
Executor->>FS: Iterate over profile files in path_apparmor
FS-->>Executor: Return valid profile names
Executor-->>AppArmorControl: Return profile set
AppArmorControl->>Client: Log and provide loaded profiles
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (6)`*/**(html|markdown|md)`: - For instructional content in doc...
`*/**(html|markdown|md)`: - Use bold to mark UI strings. - I...
`*/**(html|markdown|md)`: - Be brief in your replies and don...
`*/**(html|markdown|md)`: - Use sentence-style capitalizatio...
`*/**(html|markdown|md)`: do not comment on HTML used for ic...
`*/**(html|markdown|md)`: Avoid flagging inline HTML for emb...
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (6)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
agners
added
the
refactor
mdegat01
force-pushed
the
add-blockbuster-cont
branch
from
39d9b25 to
57164d4
Compare
| from supervisor.bootstrap import initialize_coresys | ||
|
|
||
|
|
||
| async def test_sentry_disabled_by_default(supervisor_name): | ||
| @pytest.mark.usefixtures("supervisor_name", "docker") |
There was a problem hiding this comment.
This is unrelated to the PR, this test is just broken in devcontainer right now. Mocking all docker connections with the docker fixture keeps it working there.
However on the bright side, it did find more blocking I/O. Turns out initializing the docker APIClient object does blocking I/O. Normally this part of setup is all mocked because we can't do that in devcontainer but running this test un-mocked found it 😆
![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
🧹 Nitpick comments (2)
supervisor/docker/manager.py (2)
108-112: Ensure consistent lazy initialization usage.Assigning
Noneto_docker,_network, and_infofacilitates your lazy initialization pattern. However, consider making it clear in the class docstring or method docstrings thatload_config()must be called prior to using these properties. This helps prevent runtime errors when developers forget to initialize them.
132-137: Raise a more specific Docker-related error.Using
RuntimeErroris functionally correct but might be less consistent with your existing Docker exception types (e.g.,DockerAPIError). Consider switching to a custom or existing Docker-specific error to help calling code distinguish uninitialized Docker setups from generic runtime failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
supervisor/docker/manager.py(3 hunks)tests/utils/test_sentry.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
- GitHub Check: Run tests Python 3.13.2
🔇 Additional comments (5)
tests/utils/test_sentry.py (2)
5-6: Appropriate addition of pytest import.Adding the pytest import is necessary to use the pytest.mark.usefixtures decorator.
10-11: Good refactoring to use fixtures and async pattern.This change properly refactors the test to:
- Use pytest fixtures instead of direct parameters
- Make the test function explicitly async to match its behavior
This aligns with the PR objective of enabling blockbuster library and eliminating I/O operations in the event loop.
supervisor/docker/manager.py (3)
3-5: Use of asyncio and partial is well-structured.Importing
asyncioand usingpartialis a sensible approach for offloading blocking Docker operations into a separate executor, keeping the event loop responsive. No issues found here.
139-144: Same concern about RuntimeError for the Docker network property.As with the
dockerproperty, returning a Docker-specific exception type (instead ofRuntimeError) would maintain consistency and clarity across your codebase.
164-165: Use DockerError to match existing error patterns.When
_infois not initialized, consider raising aDockerErroror a similar specialized exception rather thanRuntimeError. This ensures uniform error handling across all Docker initialization checks.
supervisor/docker/manager.py
Outdated
| async def load_config(self) -> Self: | ||
| """Load config in executor.""" | ||
| self._docker = await asyncio.get_running_loop().run_in_executor( | ||
| None, | ||
| partial( | ||
| DockerClient, | ||
| base_url=f"unix:/{str(SOCKET_DOCKER)}", | ||
| version="auto", | ||
| timeout=900, | ||
| ), | ||
| ) | ||
| self._network = DockerNetwork(self._docker) |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Wrap Docker client creation in a try-except block.
Asynchronously creating the DockerClient inside run_in_executor is a good step to avoid blocking the event loop. However, you may want to wrap it in a try-except block for DockerException or requests.RequestException to raise a more descriptive error (e.g., DockerAPIError) if Docker is unavailable or the socket is misconfigured. This aligns with your handling of Docker-related exceptions elsewhere in the file.
Wrap Docker client creation with exception handling.
In supervisor/docker/manager.py (lines 116–127), please wrap the Docker client instantiation in a try-except block. This will catch potential DockerException or requests.RequestException errors—raising a more descriptive error (e.g., a custom DockerAPIError) if Docker is unavailable or the socket is misconfigured. This change will also bring the exception handling in line with similar parts of the file.
- Action Required:
- Surround the
asyncio.get_running_loop().run_in_executor(...)call with a try-except block. - Catch exceptions like
DockerExceptionandrequests.RequestExceptionand re-raise them with a more descriptive error.
- Surround the
Proposed change
Follow-up to #5731 - complete the addition of blockbuster library to Supervisor and enabling it for all unit tests. This removes all remaining I/O in the event loop in Supervisor production code discovered by it while running unit tests.
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
Refactor
Tests