Improve DNS plug-in restart (#5999)

* Improve DNS plug-in restart

Instead of simply go by PrimaryConnectioon change, use the DnsManager
Configuration property. This property is ultimately used to write the
DNS plug-in configuration, so it is really the relevant information
we pass on to the plug-in.

* Check for changes and restart DNS plugin

* Check for changes in plug-in DNS

Cache last local (NetworkManager) provided DNS servers. Check against
this DNS server list when deciding when to restart the DNS plug-in.

* Check connectivity unthrottled in certain situations

* Fix pytest

* Fix pytest

* Improve test coverage for DNS plugins restart functionality

* Apply suggestions from code review

Co-authored-by: Mike Degatano <[email protected]>

* Debounce local DNS changes and event based connectivity checks

* Remove connection check logic

* Remove unthrottled connectivity check

* Fix delayed call

* Store restart task and cancel in case a restart is running

* Improve DNS configuration change tests

* Remove stale code

* Improve DNS plug-in tests, less mocking

* Cover multiple private functions at once

Improve tests around notify_locals_changed() to cover multiple
functions at once.

---------

Co-authored-by: Mike Degatano <[email protected]>

Author: agners

supervisor/host/network.py

@@ -8,11 +8,11 @@
 from ..const import ATTR_HOST_INTERNET
 from ..coresys import CoreSys, CoreSysAttributes
 from ..dbus.const import (
+    DBUS_ATTR_CONFIGURATION,
     DBUS_ATTR_CONNECTION_ENABLED,
     DBUS_ATTR_CONNECTIVITY,
-    DBUS_ATTR_PRIMARY_CONNECTION,
+    DBUS_IFACE_DNS,
     DBUS_IFACE_NM,
-    DBUS_OBJECT_BASE,
     DBUS_SIGNAL_NM_CONNECTION_ACTIVE_CHANGED,
     ConnectionStateType,
     ConnectivityState,
@@ -46,6 +46,8 @@ def __init__(self, coresys: CoreSys):
         """Initialize system center handling."""
         self.coresys: CoreSys = coresys
         self._connectivity: bool | None = None
+        # No event need on initial change (NetworkManager initializes with empty list)
+        self._dns_configuration: list = []
 
     @property
     def connectivity(self) -> bool | None:
@@ -142,6 +144,10 @@ async def load(self):
             "properties_changed", self._check_connectivity_changed
         )
 
+        self.sys_dbus.network.dns.dbus.properties.on(
+            "properties_changed", self._check_dns_changed
+        )
+
     async def _check_connectivity_changed(
         self, interface: str, changed: dict[str, Any], invalidated: list[str]
     ):
@@ -152,16 +158,6 @@ async def _check_connectivity_changed(
         connectivity_check: bool | None = changed.get(DBUS_ATTR_CONNECTION_ENABLED)
         connectivity: int | None = changed.get(DBUS_ATTR_CONNECTIVITY)
 
-        # This potentially updated the DNS configuration. Make sure the DNS plug-in
-        # picks up the latest settings.
-        if (
-            DBUS_ATTR_PRIMARY_CONNECTION in changed
-            and changed[DBUS_ATTR_PRIMARY_CONNECTION]
-            and changed[DBUS_ATTR_PRIMARY_CONNECTION] != DBUS_OBJECT_BASE
-            and await self.sys_plugins.dns.is_running()
-        ):
-            await self.sys_plugins.dns.restart()
-
         if (
             connectivity_check is True
             or DBUS_ATTR_CONNECTION_ENABLED in invalidated
@@ -175,6 +171,20 @@ async def _check_connectivity_changed(
         elif connectivity is not None:
             self.connectivity = connectivity == ConnectivityState.CONNECTIVITY_FULL
 
+    async def _check_dns_changed(
+        self, interface: str, changed: dict[str, Any], invalidated: list[str]
+    ):
+        """Check if DNS properties have changed."""
+        if interface != DBUS_IFACE_DNS:
+            return
+
+        if (
+            DBUS_ATTR_CONFIGURATION in changed
+            and self._dns_configuration != changed[DBUS_ATTR_CONFIGURATION]
+        ):
+            self._dns_configuration = changed[DBUS_ATTR_CONFIGURATION]
+            self.sys_plugins.dns.notify_locals_changed()
+
     async def update(self, *, force_connectivity_check: bool = False):
         """Update properties over dbus."""
         _LOGGER.info("Updating local network information")

supervisor/plugins/dns.py

@@ -77,6 +77,11 @@ def __init__(self, coresys: CoreSys):
 
         self._hosts: list[HostEntry] = []
         self._loop: bool = False
+        self._cached_locals: list[str] | None = None
+
+        # Debouncing system for rapid local changes
+        self._locals_changed_handle: asyncio.TimerHandle | None = None
+        self._restart_after_locals_change_handle: asyncio.Task | None = None
 
     @property
     def hosts(self) -> Path:
@@ -91,6 +96,12 @@ def coredns_config(self) -> Path:
     @property
     def locals(self) -> list[str]:
         """Return list of local system DNS servers."""
+        if self._cached_locals is None:
+            self._cached_locals = self._compute_locals()
+        return self._cached_locals
+
+    def _compute_locals(self) -> list[str]:
+        """Compute list of local system DNS servers."""
         servers: list[str] = []
         for server in [
             f"dns://{server!s}" for server in self.sys_host.network.dns_servers
@@ -100,6 +111,43 @@ def locals(self) -> list[str]:
 
         return servers
 
+    async def _restart_dns_after_locals_change(self) -> None:
+        """Restart DNS after a debounced delay for local changes."""
+        old_locals = self._cached_locals
+        new_locals = self._compute_locals()
+        if old_locals == new_locals:
+            return
+
+        _LOGGER.debug("DNS locals changed from %s to %s", old_locals, new_locals)
+        self._cached_locals = new_locals
+        if not await self.instance.is_running():
+            return
+
+        await self.restart()
+        self._restart_after_locals_change_handle = None
+
+    def _trigger_restart_dns_after_locals_change(self) -> None:
+        """Trigger a restart of DNS after local changes."""
+        # Cancel existing restart task if any
+        if self._restart_after_locals_change_handle:
+            self._restart_after_locals_change_handle.cancel()
+
+        self._restart_after_locals_change_handle = self.sys_create_task(
+            self._restart_dns_after_locals_change()
+        )
+        self._locals_changed_handle = None
+
+    def notify_locals_changed(self) -> None:
+        """Schedule a debounced DNS restart for local changes."""
+        # Cancel existing timer if any
+        if self._locals_changed_handle:
+            self._locals_changed_handle.cancel()
+
+        # Schedule new timer with 1 second delay
+        self._locals_changed_handle = self.sys_call_later(
+            1.0, self._trigger_restart_dns_after_locals_change
+        )
+
     @property
     def servers(self) -> list[str]:
         """Return list of DNS servers."""
@@ -243,6 +291,16 @@ async def start(self) -> None:
 
     async def stop(self) -> None:
         """Stop CoreDNS."""
+        # Cancel any pending locals change timer
+        if self._locals_changed_handle:
+            self._locals_changed_handle.cancel()
+            self._locals_changed_handle = None
+
+        # Wait for any pending restart before stopping
+        if self._restart_after_locals_change_handle:
+            self._restart_after_locals_change_handle.cancel()
+            self._restart_after_locals_change_handle = None
+
         _LOGGER.info("Stopping CoreDNS plugin")
         try:
             await self.instance.stop()

supervisor/supervisor.py

@@ -291,14 +291,16 @@ async def repair(self):
         limit=JobExecutionLimit.THROTTLE,
         throttle_period=_check_connectivity_throttle_period,
     )
-    async def check_connectivity(self):
-        """Check the connection."""
+    async def check_connectivity(self) -> None:
+        """Check the Internet connectivity from Supervisor's point of view."""
         timeout = aiohttp.ClientTimeout(total=10)
         try:
             await self.sys_websession.head(
                 "https://checkonline.home-assistant.io/online.txt", timeout=timeout
             )
-        except (ClientError, TimeoutError):
+        except (ClientError, TimeoutError) as err:
+            _LOGGER.debug("Supervisor Connectivity check failed: %s", err)
             self.connectivity = False
         else:
+            _LOGGER.debug("Supervisor Connectivity check succeeded")
             self.connectivity = True

tests/conftest.py

@@ -66,6 +66,7 @@
 from .dbus_service_mocks.network_connection_settings import (
     ConnectionSettings as ConnectionSettingsService,
 )
+from .dbus_service_mocks.network_dns_manager import DnsManager as DnsManagerService
 from .dbus_service_mocks.network_manager import NetworkManager as NetworkManagerService
 
 # pylint: disable=redefined-outer-name, protected-access
@@ -220,6 +221,14 @@ async def network_manager_service(
     yield network_manager_services["network_manager"]
 
 
+@pytest.fixture
+async def dns_manager_service(
+    network_manager_services: dict[str, DBusServiceMock | dict[str, DBusServiceMock]],
+) -> AsyncGenerator[DnsManagerService]:
+    """Return DNS Manager service mock."""
+    yield network_manager_services["network_dns_manager"]
+
+
 @pytest.fixture(name="connection_settings_service")
 async def fixture_connection_settings_service(
     network_manager_services: dict[str, DBusServiceMock | dict[str, DBusServiceMock]],

tests/host/test_connectivity.py

@@ -2,8 +2,9 @@
 
 # pylint: disable=protected-access
 import asyncio
-from unittest.mock import AsyncMock, PropertyMock, patch
+from unittest.mock import PropertyMock, patch
 
+from dbus_fast import Variant
 import pytest
 
 from supervisor.coresys import CoreSys
@@ -87,23 +88,47 @@ async def test_connectivity_events(coresys: CoreSys, force: bool):
             )
 
 
-async def test_dns_restart_on_connection_change(
-    coresys: CoreSys, network_manager_service: NetworkManagerService
+async def test_dns_configuration_change_triggers_notify_locals_changed(
+    coresys: CoreSys, dns_manager_service
 ):
-    """Test dns plugin is restarted when primary connection changes."""
+    """Test that DNS configuration changes trigger notify_locals_changed."""
     await coresys.host.network.load()
-    with (
-        patch.object(PluginDns, "restart") as restart,
-        patch.object(
-            PluginDns, "is_running", new_callable=AsyncMock, return_value=True
-        ),
-    ):
-        network_manager_service.emit_properties_changed({"PrimaryConnection": "/"})
-        await network_manager_service.ping()
-        restart.assert_not_called()
-
-        network_manager_service.emit_properties_changed(
-            {"PrimaryConnection": "/org/freedesktop/NetworkManager/ActiveConnection/2"}
+
+    with patch.object(PluginDns, "notify_locals_changed") as notify_locals_changed:
+        # Test that non-Configuration changes don't trigger notify_locals_changed
+        dns_manager_service.emit_properties_changed({"Mode": "default"})
+        await dns_manager_service.ping()
+        notify_locals_changed.assert_not_called()
+
+        # Test that Configuration changes trigger notify_locals_changed
+        configuration = [
+            {
+                "nameservers": Variant("as", ["192.168.2.2"]),
+                "domains": Variant("as", ["lan"]),
+                "interface": Variant("s", "eth0"),
+                "priority": Variant("i", 100),
+                "vpn": Variant("b", False),
+            }
+        ]
+
+        dns_manager_service.emit_properties_changed({"Configuration": configuration})
+        await dns_manager_service.ping()
+        notify_locals_changed.assert_called_once()
+
+        notify_locals_changed.reset_mock()
+        # Test that subsequent Configuration changes also trigger notify_locals_changed
+        different_configuration = [
+            {
+                "nameservers": Variant("as", ["8.8.8.8"]),
+                "domains": Variant("as", ["example.com"]),
+                "interface": Variant("s", "wlan0"),
+                "priority": Variant("i", 200),
+                "vpn": Variant("b", True),
+            }
+        ]
+
+        dns_manager_service.emit_properties_changed(
+            {"Configuration": different_configuration}
         )
-        await network_manager_service.ping()
-        restart.assert_called_once()
+        await dns_manager_service.ping()
+        notify_locals_changed.assert_called_once()