Recreate aiohttp ClientSession after DNS plug-in load#5862
Conversation
Create a temporary ClientSession early in case we need to load version information from the internet. This doesn't use the final DNS setup and hence might fail to load in certain situations since we don't have the fallback mechanims in place yet. But if the DNS container image is present, we'll continue the setup and load the DNS plug-in. We then can recreate the ClientSession such that it uses the DNS plug-in. This works around an issue with aiodns, which today doesn't reload `resolv.conf` automatically when it changes. This lead to Supervisor using the initial `resolv.conf` as created by Docker. It meant that we did not use the DNS plug-in (and its fallback capabilities) in Supervisor. Also it meant that changes to the DNS setup at runtime did not propagate to the aiohttp ClientSession (as observed in #5332).
Currently in several places pytest actually uses the aiohttp ClientSession and reaches out to the internet. This is not ideal for unit tests and should be avoided. This creates several new fixtures to aid this effort: The `websession` fixture simply returns a mocked aiohttp.ClientSession, which can be used whenever a function is tested which needs the global websession. A separate new fixture to mock the connectivity check named `supervisor_internet` since this is often used through the Job decorator which require INTERNET_SYSTEM. And the `mock_update_data` uses the already existing update json test data from the fixture directory instead of loading the data from the internet.
When recreating the aiohttp ClientSession, log information what nameservers exactly are going to be used.
📝 Walkthrough""" WalkthroughThe changes refactor how the aiohttp ClientSession is initialized and managed within the Supervisor codebase. The ClientSession is no longer created in the constructor but is instead initialized asynchronously through a new Changes
Sequence Diagram(s)sequenceDiagram
participant Supervisor
participant CoreSys
participant DNSPlugin
participant aiohttp.ClientSession
Supervisor->>CoreSys: await init_websession()
CoreSys->>aiohttp.ClientSession: Create session with current /etc/resolv.conf
Note right of CoreSys: Session uses Docker DNS (127.0.0.11) initially
Supervisor->>DNSPlugin: Setup DNS plugin
DNSPlugin->>CoreSys: await init_websession()
CoreSys->>aiohttp.ClientSession: Re-create session with updated /etc/resolv.conf
Note right of CoreSys: Session now uses Supervisor-managed DNS
Supervisor->>CoreSys: Use websession for HTTP requests
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ Finishing Touches
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. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/common.py (1)
108-134: Well-structured MockResponse implementation for aiohttp testing.The new MockResponse class provides a clean implementation for mocking aiohttp responses in tests, with appropriate async methods for text/read operations and context manager support.
One small suggestion:
async def __aexit__(self, exc_type, exc, tb): """Exit the context manager.""" + pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
supervisor/core.py(1 hunks)supervisor/coresys.py(3 hunks)supervisor/plugins/dns.py(1 hunks)tests/addons/test_manager.py(1 hunks)tests/api/test_auth.py(3 hunks)tests/api/test_backups.py(8 hunks)tests/api/test_discovery.py(1 hunks)tests/api/test_security.py(2 hunks)tests/api/test_store.py(2 hunks)tests/api/test_supervisor.py(3 hunks)tests/common.py(1 hunks)tests/conftest.py(3 hunks)tests/misc/test_tasks.py(3 hunks)tests/os/test_manager.py(3 hunks)tests/plugins/test_plugin_manager.py(3 hunks)tests/resolution/fixup/test_system_execute_integrity.py(2 hunks)tests/test_core.py(3 hunks)tests/test_coresys.py(2 hunks)tests/test_supervisor.py(5 hunks)tests/test_updater.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
`*/**(html|markdown|md)`: - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure t...
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
tests/common.pytests/test_coresys.pysupervisor/core.pytests/test_core.pysupervisor/coresys.pytests/conftest.pytests/test_updater.pytests/test_supervisor.py
`*/**(html|markdown|md)`: - Use bold to mark UI strings. - If "" are used to mark UI strings, replace them by bold.
*/**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
tests/common.pytests/test_coresys.pysupervisor/core.pytests/test_core.pysupervisor/coresys.pytests/conftest.pytests/test_updater.pytests/test_supervisor.py
`*/**(html|markdown|md)`: - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
tests/common.pytests/test_coresys.pysupervisor/core.pytests/test_core.pysupervisor/coresys.pytests/conftest.pytests/test_updater.pytests/test_supervisor.py
`*/**(html|markdown|md)`: - Use sentence-style capitalization also in headings.
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
tests/common.pytests/test_coresys.pysupervisor/core.pytests/test_core.pysupervisor/coresys.pytests/conftest.pytests/test_updater.pytests/test_supervisor.py
`*/**(html|markdown|md)`: do not comment on HTML used for icons
*/**(html|markdown|md): do not comment on HTML used for icons
tests/common.pytests/test_coresys.pysupervisor/core.pytests/test_core.pysupervisor/coresys.pytests/conftest.pytests/test_updater.pytests/test_supervisor.py
`*/**(html|markdown|md)`: Avoid flagging inline HTML for embedding videos in future reviews for this repository.
*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
tests/common.pytests/test_coresys.pysupervisor/core.pytests/test_core.pysupervisor/coresys.pytests/conftest.pytests/test_updater.pytests/test_supervisor.py
🧬 Code Graph Analysis (11)
tests/api/test_store.py (1)
tests/conftest.py (3)
api_client(494-517)coresys(331-410)supervisor_internet(521-525)
tests/misc/test_tasks.py (2)
tests/common.py (3)
MockResponse(108-133)text(124-126)update_text(116-118)tests/conftest.py (3)
coresys(331-410)mock_update_data(536-542)supervisor_internet(521-525)
tests/plugins/test_plugin_manager.py (2)
tests/common.py (1)
MockResponse(108-133)tests/conftest.py (3)
coresys(331-410)mock_update_data(536-542)supervisor_internet(521-525)
tests/api/test_discovery.py (3)
tests/conftest.py (4)
api_client(494-517)coresys(331-410)install_addon_ssh(607-615)websession(529-532)supervisor/coresys.py (2)
CoreSys(60-642)websession(182-186)supervisor/addons/addon.py (1)
Addon(132-1608)
tests/api/test_supervisor.py (1)
tests/conftest.py (4)
api_client(494-517)coresys(331-410)supervisor_internet(521-525)websession(529-532)
tests/addons/test_manager.py (2)
tests/conftest.py (3)
coresys(331-410)install_addon_ssh(607-615)websession(529-532)supervisor/coresys.py (2)
CoreSys(60-642)websession(182-186)
tests/api/test_auth.py (2)
tests/conftest.py (3)
coresys(331-410)api_client(494-517)websession(529-532)tests/common.py (2)
MockResponse(108-133)text(124-126)
tests/os/test_manager.py (3)
tests/common.py (1)
MockResponse(108-133)tests/dbus_service_mocks/rauc.py (1)
Rauc(143-212)tests/conftest.py (3)
coresys(331-410)mock_update_data(536-542)supervisor_internet(521-525)
tests/test_core.py (1)
tests/conftest.py (2)
coresys(331-410)websession(529-532)
tests/conftest.py (3)
tests/common.py (2)
MockResponse(108-133)text(124-126)supervisor/coresys.py (3)
supervisor(292-296)supervisor(299-303)websession(182-186)supervisor/supervisor.py (1)
check_connectivity(287-297)
tests/test_supervisor.py (4)
tests/common.py (2)
MockResponse(108-133)reset_last_call(103-105)tests/conftest.py (3)
websession(529-532)coresys(331-410)tmp_supervisor_data(425-444)supervisor/supervisor.py (3)
connectivity(72-74)connectivity(77-85)update_apparmor(123-191)supervisor/const.py (1)
UpdateChannel(443-448)
⏰ 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.3
🔇 Additional comments (54)
tests/api/test_security.py (2)
3-4: Added AsyncMock import for test parameter support.This import is required to support the new
supervisor_internetparameter in the test function.
41-43: Added supervisor_internet mock parameter to tests.The test now accepts the
supervisor_internetfixture from the test configuration to mock the Supervisor's internet connectivity check. This aligns with the new websession initialization pattern introduced in this PR.tests/plugins/test_plugin_manager.py (3)
3-4: Added AsyncMock import to support mocking.The import statement has been updated to include AsyncMock for testing asynchronous connectivity checks.
13-14: Added MockResponse import for HTTP response mocking.This import allows tests to use the MockResponse class that simulates aiohttp response objects.
42-44: Added fixture parameters for web requests and internet connectivity.The test function now accepts two important fixtures:
mock_update_data- Provides mocked updater JSON data responsessupervisor_internet- Simulates the Supervisor's internet connectivity checkThis change supports the new websession initialization pattern implemented in this PR.
tests/api/test_store.py (2)
5-6: Updated import statement to include AsyncMock.The import has been expanded to include AsyncMock for the new parameter in test functions.
95-97: Added supervisor_internet parameter and return type annotation.The function signature has been updated to:
- Accept the
supervisor_internetfixture of type AsyncMock to mock internet connectivity checks- Add an explicit return type annotation of
-> NoneThis change aligns with the broader refactoring of websession initialization in the Supervisor.
tests/test_coresys.py (1)
40-49: Appropriate refactoring to test async websession initialization.The test correctly verifies that the custom user agent is set when initializing the ClientSession asynchronously. This change aligns with the PR objective of allowing ClientSession recreation after DNS plugin loading.
tests/api/test_auth.py (3)
3-4: Update to imports to accommodate new mocking approach.Added support for MockResponse to replace direct aiohttp patching.
12-12: Good addition of MockResponse import.This supports the shift to using the centralized mock response class.
81-99: Well-implemented transition to injected websession mock.The test has been correctly updated to use the websession fixture with the new MockResponse class, replacing direct patching of aiohttp. This is consistent with the mocking approach used across the test suite.
tests/resolution/fixup/test_system_execute_integrity.py (2)
19-19: Appropriate addition of supervisor_internet parameter.Adding the AsyncMock parameter ensures the test works with the updated fixture system that mocks internet connectivity.
45-45: Consistent addition of supervisor_internet parameter.Similar to the previous test function, this ensures compatibility with the updated fixture system.
tests/api/test_discovery.py (1)
86-91: Properly updated test signature to use injected websession mockThe addition of the websession parameter aligns with the broader pattern of injecting mocks rather than patching or assigning them within test functions. This approach improves test consistency and reduces boilerplate code.
tests/addons/test_manager.py (1)
192-194: Good refactor to use the websession fixtureThis change correctly updates the function signature to use the injected websession mock instead of manually creating it inside the test. This is consistent with the pattern used throughout the test suite and improves maintainability.
tests/misc/test_tasks.py (3)
20-20: Updated imports to include MockResponseGood addition of the MockResponse import to support the refactored mocking approach.
174-179: Improved test setup with specialized fixturesExcellent refactoring to use the dedicated mock_update_data and supervisor_internet fixtures instead of direct patching. This approach:
- Improves test readability by clearly indicating dependencies
- Makes test behavior more consistent across the test suite
- Centralizes mocking logic to reduce duplication
202-203: Updated version data manipulation to use MockResponse methodsGood adaptation of the version updating logic to work with the new MockResponse approach, properly using text() and update_text() methods.
supervisor/core.py (1)
127-139: Well-documented early websession initializationExcellent implementation and documentation of the early websession initialization. The detailed comments:
- Clearly explain the purpose (getting initial version information)
- Document the limitations (Docker DNS without fallbacks might fail)
- Explain the different behavior in various installation scenarios
- Document the plan to reinitialize the session after DNS plugin setup
This change directly addresses the core issue described in PR #5857 where the supervisor couldn't use the DNS plugin's fallback mechanisms because the ClientSession was created too early.
tests/test_core.py (3)
37-37: Parameter addition follows test pattern refactoring.The addition of the
websession: MagicMockparameter aligns with the PR's goal to refactor how aiohttp ClientSession is initialized and managed. This ensures the test correctly handles the websession dependency that's now externally injected.
55-57: Parameter addition maintains consistent test patterns.Adding the
websession: MagicMockparameter to this test function ensures consistency with the refactored approach for managing aiohttp ClientSession. The parameter signature is properly formatted according to the project's style.
72-74: Parameter addition correctly implements dependency injection.The addition of the
websession: MagicMockparameter here completes the refactoring pattern across all related test functions, ensuring consistent dependency injection throughout the test suite.tests/api/test_supervisor.py (3)
5-5: Import addition supports new parameter types.The updated import correctly adds
AsyncMockalongside the existingMagicMockimport to support the new parameter types in the test functions.
36-38: Parameter addition ensures proper internet connectivity mocking.Adding the
supervisor_internet: AsyncMockparameter allows the test to properly mock supervisor internet connectivity, which is essential for the aiohttp ClientSession initialization changes in the PR.
234-236: Parameter additions maintain consistent testing approach.The addition of both
supervisor_internet: AsyncMockandwebsession: MagicMockparameters ensures this test function has all the dependencies needed after the ClientSession initialization refactoring. This creates a consistent approach to dependency injection across the test suite.tests/os/test_manager.py (4)
3-3: Import update adds necessary mock types.The expanded import now includes
AsyncMockalongside existing mock types, properly supporting the new parameter types used in test functions.
13-13: Common test utilities import improves test infrastructure.Adding the
MockResponseimport fromtests.commonpromotes code reuse by utilizing a shared test utility for HTTP response mocking, which is needed for the websession tests.
21-23: Parameter additions follow consistent dependency injection pattern.Adding
mock_update_data: MockResponseandsupervisor_internet: AsyncMockas parameters ensures this test has all the necessary mocked dependencies after the ClientSession initialization refactoring.
71-73: Parameter addition ensures consistent test approach.Adding the
supervisor_internet: AsyncMockparameter to this test function aligns with the pattern established across the test suite, providing consistent mocking of internet connectivity.tests/api/test_backups.py (7)
279-280: Parameter addition follows established test pattern.Adding the
supervisor_internet: AsyncMockparameter ensures this test has access to the mocked internet connectivity, following the consistent pattern established throughout the test suite.
476-477: Parameter addition maintains consistent mocking approach.The
supervisor_internet: AsyncMockparameter addition ensures this test properly mocks the supervisor's internet connectivity, which is necessary after the ClientSession initialization refactoring.
1015-1016: Parameter addition ensures proper dependency injection.The
supervisor_internet: AsyncMockparameter addition follows the consistent pattern of dependency injection for mocked internet connectivity established throughout the test suite.
1065-1066: Parameter addition maintains testing pattern consistency.Adding the
supervisor_internet: AsyncMockparameter ensures consistent mocking of supervisor internet connectivity across all backup restoration tests.
1336-1337: Parameter addition follows test refactoring pattern.The
supervisor_internet: AsyncMockparameter addition aligns with the established pattern for mocking supervisor internet connectivity in tests involving backup operations.
1396-1397: Parameter addition maintains consistent test approach.Adding the
supervisor_internet: AsyncMockparameter ensures this test follows the same pattern of dependency injection established across the test suite.
1422-1425: Parameter additions align with test suite refactoring.The additions of
api_client: TestClient,coresys: CoreSys, andsupervisor_internet: AsyncMockparameters follow the consistent pattern established across the test suite for dependency injection.supervisor/coresys.py (3)
102-102: Change to websession initialization looks good!This change initializes the
_websessionattribute asNoneinstead of creating it immediately in the constructor, which is crucial for the DNS resolution fix. This supports the PR objective of recreating the session after DNS plugin load.
112-132: Strong implementation of async websession initializationThis new asynchronous method
init_websessionproperly:
- Closes any existing session before creating a new one
- Creates an AsyncResolver for DNS resolution
- Logs the nameservers being used (helpful for debugging)
- Sets up a TCPConnector with the resolver
- Initializes the ClientSession with proper headers
This implementation addresses the core issue by allowing recreation of the ClientSession after DNS plugin changes, ensuring the updated DNS configuration is used.
184-185: Good defensive programmingAdding this runtime check ensures that code attempting to use the websession before it's properly initialized will fail fast with a clear error message, rather than experiencing a NoneType error later.
tests/conftest.py (4)
56-57: Good import updateAdding the
MockResponseimport supports the new testing approach for HTTP responses.
520-525: Well-designed supervisor_internet fixtureThis fixture cleanly mocks the Supervisor's internet connectivity check, returning
Trueby default. This allows tests to simulate internet connectivity without making actual network requests.
528-532: Well-implemented websession fixtureThis fixture replaces the previous approach of patching ClientSession methods directly. Using
spec_set=ClientSessionensures type compatibility with the real ClientSession class.
535-542: Good mock_update_data fixture implementationThis fixture creates a consistent mock response for update data, simplifying tests that need to verify version data processing. Using the MockResponse class ensures compatibility with async context managers used in the codebase.
tests/test_supervisor.py (6)
5-5: Import update aligns with new testing approachExplicitly importing AsyncMock supports the new testing pattern using mock fixtures.
26-26: Good addition of testing utilitiesAdding MockResponse and reset_last_call imports supports the updated testing approach.
32-36: Test updated correctly to use websession fixtureThe test has been properly updated to use the injected websession mock instead of patching ClientSession directly, which aligns with the new approach to session management.
Also applies to: 42-43
57-60: Proper test adaptation for throttling checksThe connectivity check throttling test has been correctly updated to use the websession fixture, maintaining test functionality with the new session management approach.
Also applies to: 66-67
97-102: Good update to apparmor testThe test for updating apparmor has been properly modified to use the websession fixture and MockResponse, which aligns with the new approach while maintaining test coverage.
Also applies to: 108-111
115-120: Properly updated error test caseThe test for apparmor update errors has been correctly adapted to use the websession fixture and MockResponse pattern, maintaining error testing coverage.
tests/test_updater.py (5)
4-4: Good addition of json moduleAdding the json module import supports parsing the mock response data correctly.
15-15: Proper testing utilities importIncluding MockResponse from common supports the updated testing approach.
24-26: Well-adapted test_fetch_versions functionThe test has been properly updated to:
- Accept the new mock fixtures
- Parse JSON data from the mock response instead of a direct web request
- Maintain the same validation logic
This ensures the test continues to work correctly with the new session management approach.
Also applies to: 32-33
77-83: Properly updated test_os_update_path signatureAdding the mock fixtures to the function signature ensures this test works with the new session management approach, while maintaining the same test logic.
94-98: Good update to connectivity testThe test for delayed fetching has been properly adapted to use the websession fixture, ensuring it works with the new session management approach while maintaining test coverage.
|
|
||
| # Reinitializing aiohttp.ClientSession after DNS setup makes sure that | ||
| # aiodns is using the right DNS servers (see #5857). | ||
| # At this point it should be fairly safe to replace the session since | ||
| # we only use the session synchronously during setup and not thorugh the | ||
| # API which previously caused issues (see #5851). | ||
| await self.coresys.init_websession() |
There was a problem hiding this comment.
Reinitialize websession after DNS setup to fix resolver issues.
This is a key change that addresses the core issue of this PR. Instead of just checking connectivity, the code now explicitly reinitializes the aiohttp ClientSession to ensure that aiodns uses the correct DNS servers after the DNS plugin is loaded.
The detailed comments explain the reasoning: replacing the session at this point is safe because it's only used synchronously during setup and not through the API, which prevents issues that were previously encountered.
This fix addresses issue #5857 by ensuring that DNS resolution works correctly after the DNS plugin is loaded, allowing for proper use of fallback mechanisms.
Previous attempts to reinitialize the ClientSession have shown use of the ClientSession after it was closed due to API requets being handled in parallel to the reinitialization (see #5851). Make sure this is not possible by refusing to reinitialize the ClientSession when the API is available.
ab88778 to
8a6e041
Compare
Also sure we don't create aiohttp ClientSession objects unnecessarily.
8a6e041 to
ddb8590
Compare
|
aio-libs/aiodns#145 should hopefully solve this problem upstream and avoid the need to do this. |
|
Hopefully #5864 will take care of this 🤞 |
Unfortunately, it seems not 😢 I've removed the re-init and instead read the nameservers currently reported by the I've double checked, the relevant Python package versions look good: |
Can you share how you are testing this? |
|
Ok, I've dig into what is going on here, and it seems c-ares rely on inotify to monitor for changes: So we monitor
So unfortunately, the current implementation is not really reliable in detecting Ideally we just resolve through a statically configured resolver which is reliably gets DNS changes through other mechanisms. The plug-in DNS actually does that already, its just that Supervisor is a special situation since it sets up the plug-in first. Ideally we'd make the local resolver available on OS level, such that it is there from the very beginning. |
Is this something we can get |
For 1, I guess we could follow the symlink and monitor that parent directory. For 2, which is actual the problem here, we could monitor |
|
Since this works right now, and is really the only re-initialization we need (DNS changes afterwards are handled by the resoolver running in the DNS plug-in, the |
sairon
left a comment
There was a problem hiding this comment.
Looks good, if the tests somehow catch how things are initialized, should be fine.
FWIW, I was thinking about another approach at this - and that's using short-lived ClientSession where relevant. So subclass ClientSession, move the code that's currently in init_websession to the class initializer, and use it as a context manager instead of sys_websession where it makes sense (namely for requests going out of the docker networks). The websession property then could initialize the session (along with an INFO logging entry maybe) instead of raising an error. Anyway with 9d869e1 implemented we might be fine with the single one without the risk of races. Probably just a food for thought now.
Co-authored-by: Jan Čermák <[email protected]>
Yeah I assume that as long as there are no calls through the API, all code using the web session (during startup) is sequential, and hence we should be free to safely initialize. I guess we'll see if that is indeed the case on Sentry rather quickly. Creating |
Proposed change
Create a temporary ClientSession early in case we need to load version information from the internet. This doesn't use the final DNS setup and hence might fail to load in certain situations since we don't have the fallback mechanims in place yet. But if the DNS container image is present, we'll continue the setup and load the DNS plug-in. We then can recreate the ClientSession such that it uses the DNS plug-in.
This works around an issue with aiodns, which today doesn't reload
resolv.confautomatically when it changes. This lead to Supervisor using the initialresolv.confas created by Docker. It meant that we did not use the DNS plug-in (and its fallback capabilities) in Supervisor. Also it meant that changes to the DNS setup at runtime did not propagate to the aiohttp ClientSession (as observed in #5332).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