Delay initial version fetch until there is connectivity (home-assistant#5603)

* Delay inital version fetch until there is connectivity

* Add test

* Only mock get not whole websession object

* drive delayed fetch off of supervisor connectivity not host

* Fix test to not rely on sleep guessing to track tasks

* Use fixture to remove job throttle temporarily

Author: mdegat01

supervisor/const.py

@@ -484,6 +484,7 @@ class BusEvent(StrEnum):
     DOCKER_CONTAINER_STATE_CHANGE = "docker_container_state_change"
     HARDWARE_NEW_DEVICE = "hardware_new_device"
     HARDWARE_REMOVE_DEVICE = "hardware_remove_device"
+    SUPERVISOR_CONNECTIVITY_CHANGE = "supervisor_connectivity_change"
     SUPERVISOR_JOB_END = "supervisor_job_end"
     SUPERVISOR_JOB_START = "supervisor_job_start"
     SUPERVISOR_STATE_CHANGE = "supervisor_state_change"

supervisor/host/network.py

@@ -67,6 +67,8 @@ def connectivity(self, state: bool | None) -> None:
         self.sys_homeassistant.websocket.supervisor_update_event(
             "network", {ATTR_HOST_INTERNET: state}
         )
+        if state and not self.sys_supervisor.connectivity:
+            self.sys_create_task(self.sys_supervisor.check_connectivity())
 
     @property
     def interfaces(self) -> list[Interface]:
@@ -148,7 +150,7 @@ async def _check_connectivity_changed(
             return
 
         connectivity_check: bool | None = changed.get(DBUS_ATTR_CONNECTION_ENABLED)
-        connectivity: bool | None = changed.get(DBUS_ATTR_CONNECTIVITY)
+        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.

supervisor/jobs/__init__.py

@@ -213,9 +213,9 @@ def _notify_on_job_change(
 
         if attribute.name == "done":
             if value is False:
-                self.sys_bus.fire_event(BusEvent.SUPERVISOR_JOB_START, job.uuid)
+                self.sys_bus.fire_event(BusEvent.SUPERVISOR_JOB_START, job)
             if value is True:
-                self.sys_bus.fire_event(BusEvent.SUPERVISOR_JOB_END, job.uuid)
+                self.sys_bus.fire_event(BusEvent.SUPERVISOR_JOB_END, job)
 
     def new_job(
         self,

supervisor/supervisor.py

@@ -13,7 +13,12 @@
 from aiohttp.client_exceptions import ClientError
 from awesomeversion import AwesomeVersion, AwesomeVersionException
 
-from .const import ATTR_SUPERVISOR_INTERNET, SUPERVISOR_VERSION, URL_HASSIO_APPARMOR
+from .const import (
+    ATTR_SUPERVISOR_INTERNET,
+    SUPERVISOR_VERSION,
+    URL_HASSIO_APPARMOR,
+    BusEvent,
+)
 from .coresys import CoreSys, CoreSysAttributes
 from .docker.stats import DockerStats
 from .docker.supervisor import DockerSupervisor
@@ -74,6 +79,7 @@ def connectivity(self, state: bool) -> None:
         if self._connectivity == state:
             return
         self._connectivity = state
+        self.sys_bus.fire_event(BusEvent.SUPERVISOR_CONNECTIVITY_CHANGE, state)
         self.sys_homeassistant.websocket.supervisor_update_event(
             "network", {ATTR_SUPERVISOR_INTERNET: state}
         )

supervisor/updater.py

@@ -8,6 +8,7 @@
 import aiohttp
 from awesomeversion import AwesomeVersion
 
+from .bus import EventListener
 from .const import (
     ATTR_AUDIO,
     ATTR_AUTO_UPDATE,
@@ -23,6 +24,7 @@
     ATTR_SUPERVISOR,
     FILE_HASSIO_UPDATER,
     URL_HASSIO_VERSION,
+    BusEvent,
     UpdateChannel,
 )
 from .coresys import CoreSysAttributes
@@ -47,11 +49,18 @@ def __init__(self, coresys):
         """Initialize updater."""
         super().__init__(FILE_HASSIO_UPDATER, SCHEMA_UPDATER_CONFIG)
         self.coresys = coresys
+        self._connectivity_listener: EventListener | None = None
 
     async def load(self) -> None:
         """Update internal data."""
-        with suppress(UpdaterError):
-            await self.fetch_data()
+        # If there's no connectivity, delay initial version fetch
+        if not self.sys_supervisor.connectivity:
+            self._connectivity_listener = self.sys_bus.register_event(
+                BusEvent.SUPERVISOR_CONNECTIVITY_CHANGE, self._check_connectivity
+            )
+            return
+
+        await self.reload()
 
     async def reload(self) -> None:
         """Update internal data."""
@@ -180,6 +189,11 @@ def auto_update(self, value: bool) -> None:
         """Set Supervisor auto updates enabled."""
         self._data[ATTR_AUTO_UPDATE] = value
 
+    async def _check_connectivity(self, connectivity: bool):
+        """Fetch data once connectivity is true."""
+        if connectivity:
+            await self.reload()
+
     @Job(
         name="updater_fetch_data",
         conditions=[JobCondition.INTERNET_SYSTEM],
@@ -214,6 +228,11 @@ async def fetch_data(self):
                 _LOGGER.warning,
             ) from err
 
+        # Fetch was successful. If there's a connectivity listener, time to remove it
+        if self._connectivity_listener:
+            self.sys_bus.remove_listener(self._connectivity_listener)
+            self._connectivity_listener = None
+
         # Validate
         try:
             await self.sys_security.verify_own_content(calc_checksum(data))

tests/conftest.py

@@ -2,8 +2,7 @@
 
 import asyncio
 from collections.abc import AsyncGenerator, Generator
-from functools import partial
-from inspect import unwrap
+from datetime import datetime
 import os
 from pathlib import Path
 import subprocess
@@ -381,11 +380,6 @@ async def coresys(
         ha_version=AwesomeVersion("2021.2.4")
     )
 
-    # Remove rate limiting decorator from fetch_data
-    coresys_obj.updater.fetch_data = partial(
-        unwrap(coresys_obj.updater.fetch_data), coresys_obj.updater
-    )
-
     # Don't remove files/folders related to addons and stores
     with patch("supervisor.store.git.GitRepo._remove"):
         yield coresys_obj
@@ -765,3 +759,10 @@ def mock_is_mount() -> MagicMock:
     """Mock is_mount in mounts."""
     with patch("supervisor.mounts.mount.Path.is_mount", return_value=True) as is_mount:
         yield is_mount
+
+
+@pytest.fixture
+def no_job_throttle():
+    """Remove job throttle for tests."""
+    with patch("supervisor.jobs.decorator.Job.last_call", return_value=datetime.min):
+        yield

tests/dbus_service_mocks/network_manager.py

@@ -22,6 +22,7 @@ class NetworkManager(DBusServiceMock):
     interface = "org.freedesktop.NetworkManager"
     object_path = "/org/freedesktop/NetworkManager"
     version = "1.22.10"
+    connectivity_check_enabled = True
     connectivity = 4
     devices = [
         "/org/freedesktop/NetworkManager/Devices/1",
@@ -155,7 +156,7 @@ def ConnectivityCheckAvailable(self) -> "b":
     @dbus_property()
     def ConnectivityCheckEnabled(self) -> "b":
         """Get ConnectivityCheckEnabled."""
-        return True
+        return self.connectivity_check_enabled
 
     @ConnectivityCheckEnabled.setter
     def ConnectivityCheckEnabled(self, value: "b"):

tests/jobs/test_job_decorator.py

@@ -1144,13 +1144,13 @@ async def job_task(self) -> None:
     started = False
     ended = False
 
-    async def start_listener(job_id: str):
+    async def start_listener(evt_job: SupervisorJob):
         nonlocal started
-        started = started or job_id == job.uuid
+        started = started or evt_job.uuid == job.uuid
 
-    async def end_listener(job_id: str):
+    async def end_listener(evt_job: SupervisorJob):
         nonlocal ended
-        ended = ended or job_id == job.uuid
+        ended = ended or evt_job.uuid == job.uuid
 
     coresys.bus.register_event(BusEvent.SUPERVISOR_JOB_START, start_listener)
     coresys.bus.register_event(BusEvent.SUPERVISOR_JOB_END, end_listener)
@@ -1196,13 +1196,13 @@ async def job_task(self) -> None:
     started = False
     ended = False
 
-    async def start_listener(job_id: str):
+    async def start_listener(evt_job: SupervisorJob):
         nonlocal started
-        started = started or job_id == job.uuid
+        started = started or evt_job.uuid == job.uuid
 
-    async def end_listener(job_id: str):
+    async def end_listener(evt_job: SupervisorJob):
         nonlocal ended
-        ended = ended or job_id == job.uuid
+        ended = ended or evt_job.uuid == job.uuid
 
     coresys.bus.register_event(BusEvent.SUPERVISOR_JOB_START, start_listener)
     coresys.bus.register_event(BusEvent.SUPERVISOR_JOB_END, end_listener)

tests/misc/test_tasks.py

@@ -171,6 +171,7 @@ async def test_watchdog_homeassistant_api_reanimation_limit(
         rebuild.assert_not_called()
 
 
+@pytest.mark.usefixtures("no_job_throttle")
 async def test_reload_updater_triggers_supervisor_update(
     tasks: Tasks, coresys: CoreSys
 ):

tests/os/test_manager.py

@@ -16,7 +16,7 @@
 # pylint: disable=protected-access
 
 
-@pytest.mark.asyncio
+@pytest.mark.usefixtures("no_job_throttle")
 async def test_ota_url_generic_x86_64_rename(coresys: CoreSys) -> None:
     """Test download URL generated."""
     coresys.os._board = "intel-nuc"

tests/plugins/test_plugin_manager.py

@@ -3,6 +3,7 @@
 from unittest.mock import PropertyMock, patch
 
 from awesomeversion import AwesomeVersion
+import pytest
 
 from supervisor.coresys import CoreSys
 from supervisor.docker.interface import DockerInterface
@@ -35,6 +36,7 @@ async def test_repair(coresys: CoreSys):
         assert install.call_count == len(coresys.plugins.all_plugins)
 
 
+@pytest.mark.usefixtures("no_job_throttle")
 async def test_load(coresys: CoreSys):
     """Test plugin manager load."""
     coresys.hardware.disk.get_disk_free_space = lambda x: 5000

tests/test_supervisor.py

@@ -36,29 +36,23 @@ async def fixture_webession(coresys: CoreSys) -> AsyncMock:
         yield mock_websession
 
 
-@pytest.fixture(name="supervisor_unthrottled")
-async def fixture_supervisor_unthrottled(coresys: CoreSys) -> Supervisor:
-    """Get supervisor object with connectivity check throttle removed."""
-    with patch("supervisor.jobs.decorator.Job.last_call", return_value=datetime.min):
-        yield coresys.supervisor
-
-
 @pytest.mark.parametrize(
     "side_effect,connectivity", [(ClientError(), False), (None, True)]
 )
+@pytest.mark.usefixtures("no_job_throttle")
 async def test_connectivity_check(
-    supervisor_unthrottled: Supervisor,
+    coresys: CoreSys,
     websession: AsyncMock,
     side_effect: Exception | None,
     connectivity: bool,
 ):
     """Test connectivity check."""
-    assert supervisor_unthrottled.connectivity is True
+    assert coresys.supervisor.connectivity is True
 
     websession.head.side_effect = side_effect
-    await supervisor_unthrottled.check_connectivity()
+    await coresys.supervisor.check_connectivity()
 
-    assert supervisor_unthrottled.connectivity is connectivity
+    assert coresys.supervisor.connectivity is connectivity
 
 
 @pytest.mark.parametrize(

tests/test_updater.py

@@ -1,16 +1,25 @@
 """Test updater files."""
 
-from unittest.mock import patch
+import asyncio
+from unittest.mock import AsyncMock, MagicMock, patch
 
 from awesomeversion import AwesomeVersion
 import pytest
 
+from supervisor.const import BusEvent
 from supervisor.coresys import CoreSys
+from supervisor.dbus.const import ConnectivityState
+from supervisor.jobs import SupervisorJob
+
+from tests.common import load_binary_fixture
+from tests.dbus_service_mocks.network_manager import (
+    NetworkManager as NetworkManagerService,
+)
 
 URL_TEST = "https://version.home-assistant.io/stable.json"
 
 
-@pytest.mark.asyncio
+@pytest.mark.usefixtures("no_job_throttle")
 async def test_fetch_versions(coresys: CoreSys) -> None:
     """Test download and sync version."""
 
@@ -53,6 +62,7 @@ async def test_fetch_versions(coresys: CoreSys) -> None:
     )
 
 
+@pytest.mark.usefixtures("no_job_throttle")
 @pytest.mark.parametrize(
     "version, expected",
     [
@@ -71,3 +81,51 @@ async def test_os_update_path(coresys: CoreSys, version: str, expected: str):
         await coresys.updater.fetch_data()
 
         assert coresys.updater.version_hassos == AwesomeVersion(expected)
+
+
+@pytest.mark.usefixtures("no_job_throttle")
+async def test_delayed_fetch_for_connectivity(
+    coresys: CoreSys, network_manager_service: NetworkManagerService
+):
+    """Test initial version fetch waits for connectivity on load."""
+    coresys.websession.get = MagicMock()
+    coresys.websession.get.return_value.__aenter__.return_value.status = 200
+    coresys.websession.get.return_value.__aenter__.return_value.read.return_value = (
+        load_binary_fixture("version_stable.json")
+    )
+    coresys.websession.head = AsyncMock()
+    coresys.security.verify_own_content = AsyncMock()
+
+    # Network connectivity change causes a series of async tasks to eventually do a version fetch
+    # Rather then use some kind of sleep loop, set up listener for start of fetch data job
+    event = asyncio.Event()
+
+    async def find_fetch_data_job_start(job: SupervisorJob):
+        if job.name == "updater_fetch_data":
+            event.set()
+
+    coresys.bus.register_event(BusEvent.SUPERVISOR_JOB_START, find_fetch_data_job_start)
+
+    # Start with no connectivity and confirm there is no version fetch on load
+    coresys.supervisor.connectivity = False
+    network_manager_service.connectivity = ConnectivityState.CONNECTIVITY_NONE.value
+    await coresys.host.network.load()
+    await coresys.host.network.check_connectivity()
+
+    await coresys.updater.load()
+    coresys.websession.get.assert_not_called()
+
+    # Now signal host has connectivity and wait for fetch data to complete to assert
+    network_manager_service.emit_properties_changed(
+        {"Connectivity": ConnectivityState.CONNECTIVITY_FULL}
+    )
+    await network_manager_service.ping()
+    async with asyncio.timeout(5):
+        await event.wait()
+    await asyncio.sleep(0)
+
+    coresys.websession.get.assert_called_once()
+    assert (
+        coresys.websession.get.call_args[0][0]
+        == "https://version.home-assistant.io/stable.json"
+    )