Update DNS plug-in on network change #5331
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NetworkManager
participant DNSPlugin
User->>NetworkManager: Request to update network
NetworkManager->>NetworkManager: Handle update logic
NetworkManager->>DNSPlugin: Restart DNS
DNSPlugin-->>NetworkManager: DNS restarted
NetworkManager-->>User: Update complete
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
supervisor/host/network.py (1)
176-178: LGTM! Consider adding error handling for DNS plugin restart.The addition of restarting the DNS plugin after updating network information is a good improvement. It ensures that the DNS configuration remains up-to-date with any network changes.
Consider adding error handling for the DNS plugin restart to ensure the method continues execution even if the restart fails:
try: await self.sys_plugins.dns.restart() except Exception as err: _LOGGER.warning("Failed to restart DNS plugin: %s", err)This will prevent any potential issues with the DNS plugin from blocking the rest of the update process.
agners
force-pushed
the
update-dns-plugin-on-network-change
branch
from
f0d32b0
to
38affe8
Compare
supervisor/host/network.py
Outdated
| @@ -282,6 +282,10 @@ async def apply_changes( | |||
| state = msg[0] | |||
| _LOGGER.debug("Active connection state changed to %s", state) | |||
|
|
|||
There was a problem hiding this comment.
It would be great if there was a way for supervisor to tell core that the network has changed so we can solve home-assistant/core#59553
There was a problem hiding this comment.
Agree, but that is a good challenge to solve after we've finished replacing/abstracting the communication layer in Core<>Supervisor (currently WIP).
There was a problem hiding this comment.
We do actually fire a message to core currently when system connectivity has changed:
supervisor/supervisor/host/network.py
Lines 65 to 67 in ee5ded2
It doesn't have all the details of the connection if we need that. But if connectivity of the system goes from False to True or vice versa we do let core know. I'm not sure anything is listening for that event currently though.
agners
force-pushed
the
update-dns-plugin-on-network-change
branch
4 times, most recently
from
9e4efb5
to
ba618fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- supervisor/host/network.py (2 hunks)
🧰 Additional context used
🪛 Ruff
supervisor/host/network.py
147-147: Logging statement uses f-string
(G004)
🪛 GitHub Check: Check ruff
supervisor/host/network.py
[failure] 147-147: Ruff (G004)
supervisor/host/network.py:147:22: G004 Logging statement uses f-string
🔇 Additional comments (4)
supervisor/host/network.py (4)
13-15: LGTM: New constants added for D-Bus attributesThe addition of
DBUS_ATTR_PRIMARY_CONNECTIONandDBUS_OBJECT_BASEconstants is appropriate for the new functionality. These constants will help improve code readability and maintainability when working with D-Bus attributes.
154-162: LGTM: Restart DNS plugin on primary connection changeThe new logic to restart the DNS plugin when the primary connection changes aligns well with the PR objective. This ensures that the DNS settings are updated promptly when network changes occur, improving the system's responsiveness to network configuration changes.
Line range hint
1-1: Summary of changesThe changes in this file implement the functionality to restart the DNS plugin when there's a change in the network's primary connection. This aligns well with the PR objective of ensuring the DNS plug-in restarts on network changes.
Key points:
- New constants added for D-Bus attributes.
- Logic implemented to check for primary connection changes and restart the DNS plugin.
- Minor improvement suggested for the logging statement.
- Verification requested for changes mentioned in the AI summary but not visible in the diff.
Overall, the changes look good and should improve the system's responsiveness to network configuration changes.
Line range hint
182-182: Verify changes to theupdatemethodThe AI-generated summary mentioned changes to the
updatemethod to restart the DNS plugin after handling network updates. However, these changes are not visible in the provided code diff. Could you please verify if these changes were made or if they were removed in a subsequent update?To check for any changes in the
updatemethod, you can run the following command:This will show the entire
updatemethod, allowing us to verify if any changes were made.
agners
force-pushed
the
update-dns-plugin-on-network-change
branch
2 times, most recently
from
26d3e1d
to
042107a
Compare
Restart the DNS plug-in when the primary network changes network changes. This makes sure any potential host OS DNS configuration changes get picked up by the DNS plug-in as well.
agners
force-pushed
the
update-dns-plugin-on-network-change
branch
from
042107a
to
f652875
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/host/test_connectivity.py (1)
90-104: Good addition of test, but consider enhancing it further.The new test function
test_dns_restart_on_connection_changeis a valuable addition that aligns well with the PR objectives. It effectively verifies that the DNS plugin is restarted when the primary network connection changes. However, consider the following suggestions to enhance the test:
- Add an assertion to verify the initial state of the network before simulating changes.
- Include more specific assertions after each network change to ensure the behavior is correct.
- Consider adding a test case for when the primary connection doesn't change to ensure no unnecessary restarts occur.
Here's a suggested improvement to the test function:
async def test_dns_restart_on_connection_change( coresys: CoreSys, network_manager_service: NetworkManagerService ): """Test dns plugin is restarted when primary connection changes.""" await coresys.host.network.load() # Verify initial state assert coresys.host.network.primary_connection == "/" with patch.object(PluginDns, "restart") as restart: # Test: No change in primary connection network_manager_service.emit_properties_changed({"PrimaryConnection": "/"}) await network_manager_service.ping() restart.assert_not_called() assert coresys.host.network.primary_connection == "/" # Test: Change in primary connection new_connection = "/org/freedesktop/NetworkManager/ActiveConnection/2" network_manager_service.emit_properties_changed( {"PrimaryConnection": new_connection} ) await network_manager_service.ping() restart.assert_called_once() assert coresys.host.network.primary_connection == new_connection # Test: No additional restarts for subsequent checks await network_manager_service.ping() restart.assert_called_once()This improved version adds more assertions and an additional test case to ensure the behavior is correct in various scenarios.
Also, please ensure that the indentation of the function is consistent with the rest of the file (it seems to be off by one space).
Proposed change
Restart the DNS plug-in on network change. This makes sure any potential host OS DNS configuration changes get picked up by the DNS plug-in as well.
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
New Features
Bug Fixes