Mount status checks look at connection (#4882)

* Mount status checks look at connection

* Fix tests and refactor to fixture

* Fix test

supervisor/dbus/const.py

@@ -36,12 +36,14 @@ DBUS_IFACE_RAUC_INSTALLER = "de.pengutronix.rauc.Installer"
 DBUS_IFACE_RESOLVED_MANAGER = "org.freedesktop.resolve1.Manager"
 DBUS_IFACE_SETTINGS_CONNECTION = "org.freedesktop.NetworkManager.Settings.Connection"
 DBUS_IFACE_SYSTEMD_MANAGER = "org.freedesktop.systemd1.Manager"
+DBUS_IFACE_SYSTEMD_UNIT = "org.freedesktop.systemd1.Unit"
 DBUS_IFACE_TIMEDATE = "org.freedesktop.timedate1"
 DBUS_IFACE_UDISKS2_MANAGER = "org.freedesktop.UDisks2.Manager"
 
 DBUS_SIGNAL_NM_CONNECTION_ACTIVE_CHANGED = (
     "org.freedesktop.NetworkManager.Connection.Active.StateChanged"
 )
+DBUS_SIGNAL_PROPERTIES_CHANGED = "org.freedesktop.DBus.Properties.PropertiesChanged"
 DBUS_SIGNAL_RAUC_INSTALLER_COMPLETED = "de.pengutronix.rauc.Installer.Completed"
 
 DBUS_OBJECT_BASE = "/"
@@ -64,6 +66,7 @@ DBUS_OBJECT_UDISKS2 = "/org/freedesktop/UDisks2/Manager"
 DBUS_ATTR_ACTIVE_ACCESSPOINT = "ActiveAccessPoint"
 DBUS_ATTR_ACTIVE_CONNECTION = "ActiveConnection"
 DBUS_ATTR_ACTIVE_CONNECTIONS = "ActiveConnections"
+DBUS_ATTR_ACTIVE_STATE = "ActiveState"
 DBUS_ATTR_ACTIVITY_LED = "ActivityLED"
 DBUS_ATTR_ADDRESS_DATA = "AddressData"
 DBUS_ATTR_BITRATE = "Bitrate"

supervisor/dbus/systemd.py

@@ -13,6 +13,7 @@ from ..exceptions import (
     DBusServiceUnkownError,
     DBusSystemdNoSuchUnit,
 )
+from ..utils.dbus import DBusSignalWrapper
 from .const import (
     DBUS_ATTR_FINISH_TIMESTAMP,
     DBUS_ATTR_FIRMWARE_TIMESTAMP_MONOTONIC,
@@ -23,6 +24,7 @@ from .const import (
     DBUS_IFACE_SYSTEMD_MANAGER,
     DBUS_NAME_SYSTEMD,
     DBUS_OBJECT_SYSTEMD,
+    DBUS_SIGNAL_PROPERTIES_CHANGED,
     StartUnitMode,
     StopUnitMode,
     UnitActiveState,
@@ -64,6 +66,11 @@ class SystemdUnit(DBusInterface):
         """Get active state of the unit."""
         return await self.dbus.Unit.get_active_state()
 
+    @dbus_connected
+    def properties_changed(self) -> DBusSignalWrapper:
+        """Return signal wrapper for properties changed."""
+        return self.dbus.signal(DBUS_SIGNAL_PROPERTIES_CHANGED)
+
 
 class Systemd(DBusInterfaceProxy):
     """Systemd function handler.

supervisor/mounts/manager.py

@@ -145,16 +145,17 @@ class MountManager(FileConfiguration, CoreSysAttributes):
         if not self.mounts:
             return
 
-        await asyncio.wait(
+        mounts = self.mounts.copy()
-            [self.sys_create_task(mount.update()) for mount in self.mounts]
+        results = await asyncio.gather(
+            *[mount.update() for mount in mounts], return_exceptions=True
         )
 
         # Try to reload any newly failed mounts and report issues if failure persists
         new_failures = [
-            mount
+            mounts[i]
-            for mount in self.mounts
+            for i in range(len(mounts))
-            if mount.state != UnitActiveState.ACTIVE
+            if results[i] is not True
-            and mount.failed_issue not in self.sys_resolution.issues
+            and mounts[i].failed_issue not in self.sys_resolution.issues
         ]
         await self._mount_errors_to_issues(
             new_failures, [mount.reload() for mount in new_failures]

supervisor/mounts/mount.py

@@ -18,10 +18,12 @@ from ..const import (
 )
 from ..coresys import CoreSys, CoreSysAttributes
 from ..dbus.const import (
+    DBUS_ATTR_ACTIVE_STATE,
     DBUS_ATTR_DESCRIPTION,
     DBUS_ATTR_OPTIONS,
     DBUS_ATTR_TYPE,
     DBUS_ATTR_WHAT,
+    DBUS_IFACE_SYSTEMD_UNIT,
     StartUnitMode,
     StopUnitMode,
     UnitActiveState,
@@ -162,23 +164,28 @@ class Mount(CoreSysAttributes, ABC):
         """Get issue used if this mount has failed."""
         return Issue(IssueType.MOUNT_FAILED, ContextType.MOUNT, reference=self.name)
 
+    async def is_mounted(self) -> bool:
+        """Return true if successfully mounted and available."""
+        return self.state == UnitActiveState.ACTIVE
+
     def __eq__(self, other):
         """Return true if mounts are the same."""
         return isinstance(other, Mount) and self.name == other.name
 
     async def load(self) -> None:
         """Initialize object."""
-        await self._update_await_activating()
-
         # If there's no mount unit, mount it to make one
-        if not self.unit:
+        if not await self._update_unit():
             await self.mount()
+            return
 
-        # At this point any state besides active is treated as a failed mount, try to reload it
+        await self._update_state_await(not_state=UnitActiveState.ACTIVATING)
-        elif self.state != UnitActiveState.ACTIVE:
+
+        # If mount is not available, try to reload it
+        if not await self.is_mounted():
             await self.reload()
 
-    async def update_state(self) -> None:
+    async def _update_state(self) -> UnitActiveState | None:
         """Update mount unit state."""
         try:
             self._state = await self.unit.get_active_state()
@@ -188,56 +195,66 @@ class Mount(CoreSysAttributes, ABC):
                 f"Could not get active state of mount due to: {err!s}"
             ) from err
 
-    async def update(self) -> None:
+    async def _update_unit(self) -> SystemdUnit | None:
-        """Update info about mount from dbus."""
+        """Get systemd unit from dbus."""
         try:
             self._unit = await self.sys_dbus.systemd.get_unit(self.unit_name)
         except DBusSystemdNoSuchUnit:
             self._unit = None
             self._state = None
-            return
         except DBusError as err:
             capture_exception(err)
             raise MountError(f"Could not get mount unit due to: {err!s}") from err
+        return self.unit
 
-        await self.update_state()
+    async def update(self) -> bool:
+        """Update info about mount from dbus. Return true if it is mounted and available."""
+        if not await self._update_unit():
+            return False
+
+        await self._update_state()
 
         # If active, dismiss corresponding failed mount issue if found
         if (
-            self.state == UnitActiveState.ACTIVE
+            mounted := await self.is_mounted()
-            and self.failed_issue in self.sys_resolution.issues
+        ) and self.failed_issue in self.sys_resolution.issues:
-        ):
             self.sys_resolution.dismiss_issue(self.failed_issue)
 
-    async def _update_state_await(self, expected_states: list[UnitActiveState]) -> None:
+        return mounted
-        """Update state info about mount from dbus. Wait up to 30 seconds for the state to appear."""
-        for i in range(5):
-            await self.update_state()
-            if self.state in expected_states:
-                return
-            await asyncio.sleep(i**2)
 
-        _LOGGER.warning(
+    async def _update_state_await(
-            "Mount %s still in state %s after waiting for 30 seconods to complete",
+        self,
-            self.name,
+        expected_states: list[UnitActiveState] | None = None,
-            str(self.state).lower(),
+        not_state: UnitActiveState = UnitActiveState.ACTIVATING,
-        )
+    ) -> None:
+        """Update state info about mount from dbus. Wait for one of expected_states to appear or state to change from not_state."""
+        if not self.unit:
+            return
 
-    async def _update_await_activating(self):
+        try:
-        """Update info about mount from dbus. If 'activating' wait up to 30 seconds."""
+            async with asyncio.timeout(30), self.unit.properties_changed() as signal:
-        await self.update()
+                await self._update_state()
+                while (
+                    expected_states
+                    and self.state not in expected_states
+                    or not expected_states
+                    and self.state == not_state
+                ):
+                    prop_change_signal = await signal.wait_for_signal()
+                    if (
+                        prop_change_signal[0] == DBUS_IFACE_SYSTEMD_UNIT
+                        and DBUS_ATTR_ACTIVE_STATE in prop_change_signal[1]
+                    ):
+                        self._state = prop_change_signal[1][
+                            DBUS_ATTR_ACTIVE_STATE
+                        ].value
 
-        # If we're still activating, give it up to 30 seconds to finish
+        except asyncio.TimeoutError:
-        if self.state == UnitActiveState.ACTIVATING:
+            _LOGGER.warning(
-            _LOGGER.info(
+                "Mount %s still in state %s after waiting for 30 seconds to complete",
-                "Mount %s still activating, waiting up to 30 seconds to complete",
                 self.name,
+                str(self.state).lower(),
             )
-            for _ in range(3):
-                await asyncio.sleep(10)
-                await self.update()
-                if self.state != UnitActiveState.ACTIVATING:
-                    break
 
     async def mount(self) -> None:
         """Mount using systemd."""
@@ -282,9 +299,10 @@ class Mount(CoreSysAttributes, ABC):
                 f"Could not mount {self.name} due to: {err!s}", _LOGGER.error
             ) from err
 
-        await self._update_await_activating()
+        if await self._update_unit():
+            await self._update_state_await(not_state=UnitActiveState.ACTIVATING)
 
-        if self.state != UnitActiveState.ACTIVE:
+        if not await self.is_mounted():
             raise MountActivationError(
                 f"Mounting {self.name} did not succeed. Check host logs for errors from mount or systemd unit {self.unit_name} for details.",
                 _LOGGER.error,
@@ -292,8 +310,11 @@ class Mount(CoreSysAttributes, ABC):
 
     async def unmount(self) -> None:
         """Unmount using systemd."""
-        await self.update()
+        if not await self._update_unit():
+            _LOGGER.info("Mount %s is not mounted, skipping unmount", self.name)
+            return
 
+        await self._update_state()
         try:
             if self.state != UnitActiveState.FAILED:
                 await self.sys_dbus.systemd.stop_unit(self.unit_name, StopUnitMode.FAIL)
@@ -304,8 +325,6 @@ class Mount(CoreSysAttributes, ABC):
 
             if self.state == UnitActiveState.FAILED:
                 await self.sys_dbus.systemd.reset_failed_unit(self.unit_name)
-        except DBusSystemdNoSuchUnit:
-            _LOGGER.info("Mount %s is not mounted, skipping unmount", self.name)
         except DBusError as err:
             raise MountError(
                 f"Could not unmount {self.name} due to: {err!s}", _LOGGER.error
@@ -329,9 +348,10 @@ class Mount(CoreSysAttributes, ABC):
                 f"Could not reload mount {self.name} due to: {err!s}", _LOGGER.error
             ) from err
 
-        await self._update_await_activating()
+        if await self._update_unit():
+            await self._update_state_await(not_state=UnitActiveState.ACTIVATING)
 
-        if self.state != UnitActiveState.ACTIVE:
+        if not await self.is_mounted():
             raise MountActivationError(
                 f"Reloading {self.name} did not succeed. Check host logs for errors from mount or systemd unit {self.unit_name} for details.",
                 _LOGGER.error,
@@ -371,6 +391,12 @@ class NetworkMount(Mount, ABC):
             options.append(f"port={self.port}")
         return options
 
+    async def is_mounted(self) -> bool:
+        """Return true if successfully mounted and available."""
+        return self.state == UnitActiveState.ACTIVE and await self.sys_run_in_executor(
+            self.local_where.is_mount
+        )
+
 
 class CIFSMount(NetworkMount):
     """A CIFS type mount."""

tests/api/test_backups.py

@@ -71,6 +71,7 @@ async def test_backup_to_location(
     tmp_supervisor_data: Path,
     path_extern,
     mount_propagation,
+    mock_is_mount,
 ):
     """Test making a backup to a specific location with default mount."""
     await coresys.mounts.load()
@@ -90,14 +91,13 @@ async def test_backup_to_location(
 
     coresys.core.state = CoreState.RUNNING
     coresys.hardware.disk.get_disk_free_space = lambda x: 5000
-    with patch("supervisor.mounts.mount.Path.is_mount", return_value=True):
+    resp = await api_client.post(
-        resp = await api_client.post(
+        "/backups/new/full",
-            "/backups/new/full",
+        json={
-            json={
+            "name": "Mount test",
-                "name": "Mount test",
+            "location": location,
-                "location": location,
+        },
-            },
+    )
-        )
     result = await resp.json()
     assert result["result"] == "ok"
     slug = result["data"]["slug"]
@@ -116,6 +116,7 @@ async def test_backup_to_default(
     tmp_supervisor_data,
     path_extern,
     mount_propagation,
+    mock_is_mount,
 ):
     """Test making backup to default mount."""
     await coresys.mounts.load()
@@ -135,11 +136,10 @@ async def test_backup_to_default(
 
     coresys.core.state = CoreState.RUNNING
     coresys.hardware.disk.get_disk_free_space = lambda x: 5000
-    with patch("supervisor.mounts.mount.Path.is_mount", return_value=True):
+    resp = await api_client.post(
-        resp = await api_client.post(
+        "/backups/new/full",
-            "/backups/new/full",
+        json={"name": "Mount test"},
-            json={"name": "Mount test"},
+    )
-        )
     result = await resp.json()
     assert result["result"] == "ok"
     slug = result["data"]["slug"]

tests/api/test_mounts.py

@@ -51,6 +51,7 @@ async def test_api_create_mount(
     tmp_supervisor_data,
     path_extern,
     mount_propagation,
+    mock_is_mount,
 ):
     """Test creating a mount via API."""
     resp = await api_client.post(
@@ -225,6 +226,7 @@ async def test_api_update_mount(
     coresys: CoreSys,
     all_dbus_services: dict[str, DBusServiceMock],
     mount,
+    mock_is_mount,
 ):
     """Test updating a mount via API."""
     systemd_service: SystemdService = all_dbus_services["systemd"]
@@ -375,7 +377,10 @@ async def test_api_update_dbus_error_mount_remains(
 
 
 async def test_api_reload_mount(
-    api_client: TestClient, all_dbus_services: dict[str, DBusServiceMock], mount
+    api_client: TestClient,
+    all_dbus_services: dict[str, DBusServiceMock],
+    mount,
+    mock_is_mount,
 ):
     """Test reloading a mount via API."""
     systemd_service: SystemdService = all_dbus_services["systemd"]
@@ -446,6 +451,7 @@ async def test_api_create_backup_mount_sets_default(
     tmp_supervisor_data,
     path_extern,
     mount_propagation,
+    mock_is_mount,
 ):
     """Test creating backup mounts sets default if not set."""
     await coresys.mounts.load()
@@ -486,6 +492,7 @@ async def test_update_backup_mount_changes_default(
     coresys: CoreSys,
     all_dbus_services: dict[str, DBusServiceMock],
     mount,
+    mock_is_mount,
 ):
     """Test updating a backup mount may unset the default."""
     systemd_unit_service: SystemdUnitService = all_dbus_services["systemd_unit"]
@@ -540,6 +547,7 @@ async def test_delete_backup_mount_changes_default(
     coresys: CoreSys,
     all_dbus_services: dict[str, DBusServiceMock],
     mount,
+    mock_is_mount,
 ):
     """Test deleting a backup mount may unset the default."""
     systemd_unit_service: SystemdUnitService = all_dbus_services["systemd_unit"]
@@ -580,6 +588,7 @@ async def test_backup_mounts_reload_backups(
     tmp_supervisor_data,
     path_extern,
     mount_propagation,
+    mock_is_mount,
 ):
     """Test actions on a backup mount reload backups."""
     systemd_unit_service: SystemdUnitService = all_dbus_services["systemd_unit"]
@@ -678,7 +687,7 @@ async def test_backup_mounts_reload_backups(
         reload.assert_called_once()
 
 
-async def test_options(api_client: TestClient, coresys: CoreSys, mount):
+async def test_options(api_client: TestClient, coresys: CoreSys, mount, mock_is_mount):
     """Test changing options."""
     resp = await api_client.post(
         "/mounts",
@@ -788,6 +797,7 @@ async def test_api_create_read_only_cifs_mount(
     tmp_supervisor_data,
     path_extern,
     mount_propagation,
+    mock_is_mount,
 ):
     """Test creating a read-only cifs mount via API."""
     resp = await api_client.post(
@@ -829,6 +839,7 @@ async def test_api_create_read_only_nfs_mount(
     tmp_supervisor_data,
     path_extern,
     mount_propagation,
+    mock_is_mount,
 ):
     """Test creating a read-only nfs mount via API."""
     resp = await api_client.post(

tests/backups/test_manager.py

@@ -412,6 +412,7 @@ async def test_backup_media_with_mounts(
     tmp_supervisor_data,
     path_extern,
     mount_propagation,
+    mock_is_mount,
 ):
     """Test backing up media folder with mounts."""
     systemd_service: SystemdService = all_dbus_services["systemd"]
@@ -473,6 +474,7 @@ async def test_backup_media_with_mounts_retains_files(
     tmp_supervisor_data,
     path_extern,
     mount_propagation,
+    mock_is_mount,
 ):
     """Test backing up media folder with mounts retains mount files."""
     systemd_service: SystemdService = all_dbus_services["systemd"]
@@ -526,6 +528,7 @@ async def test_backup_share_with_mounts(
     tmp_supervisor_data,
     path_extern,
     mount_propagation,
+    mock_is_mount,
 ):
     """Test backing up share folder with mounts."""
     systemd_service: SystemdService = all_dbus_services["systemd"]
@@ -589,7 +592,11 @@ async def test_backup_share_with_mounts(
 
 
 async def test_full_backup_to_mount(
-    coresys: CoreSys, tmp_supervisor_data, path_extern, mount_propagation
+    coresys: CoreSys,
+    tmp_supervisor_data,
+    path_extern,
+    mount_propagation,
+    mock_is_mount,
 ):
     """Test full backup to and restoring from a mount."""
     (marker := coresys.config.path_homeassistant / "test.txt").touch()
@@ -613,8 +620,7 @@ async def test_full_backup_to_mount(
     # Make a backup and add it to mounts. Confirm it exists in the right place
     coresys.core.state = CoreState.RUNNING
     coresys.hardware.disk.get_disk_free_space = lambda x: 5000
-    with patch("supervisor.mounts.mount.Path.is_mount", return_value=True):
+    backup: Backup = await coresys.backups.do_backup_full("test", location=mount)
-        backup: Backup = await coresys.backups.do_backup_full("test", location=mount)
     assert (mount_dir / f"{backup.slug}.tar").exists()
 
     # Reload and check that backups in mounts are listed
@@ -635,6 +641,7 @@ async def test_partial_backup_to_mount(
     tmp_supervisor_data,
     path_extern,
     mount_propagation,
+    mock_is_mount,
 ):
     """Test partial backup to and restoring from a mount."""
     (marker := coresys.config.path_homeassistant / "test.txt").touch()
@@ -663,7 +670,7 @@ async def test_partial_backup_to_mount(
         HomeAssistant,
         "version",
         new=PropertyMock(return_value=AwesomeVersion("2023.1.1")),
-    ), patch("supervisor.mounts.mount.Path.is_mount", return_value=True):
+    ):
         backup: Backup = await coresys.backups.do_backup_partial(
             "test", homeassistant=True, location=mount
         )
@@ -684,7 +691,11 @@ async def test_partial_backup_to_mount(
 
 
 async def test_backup_to_down_mount_error(
-    coresys: CoreSys, tmp_supervisor_data, path_extern, mount_propagation
+    coresys: CoreSys,
+    mock_is_mount: MagicMock,
+    tmp_supervisor_data,
+    path_extern,
+    mount_propagation,
 ):
     """Test backup to mount when down raises error."""
     # Add a backup mount
@@ -704,6 +715,7 @@ async def test_backup_to_down_mount_error(
     assert mount_dir in coresys.backups.backup_locations
 
     # Attempt to make a backup which fails because is_mount on directory is false
+    mock_is_mount.return_value = False
     coresys.core.state = CoreState.RUNNING
     coresys.hardware.disk.get_disk_free_space = lambda x: 5000
     with pytest.raises(BackupMountDownError):
@@ -719,6 +731,7 @@ async def test_backup_to_local_with_default(
     tmp_supervisor_data,
     path_extern,
     mount_propagation,
+    mock_is_mount,
 ):
     """Test making backup to local when a default mount is specified."""
     # Add a default backup mount
@@ -753,7 +766,7 @@ async def test_backup_to_local_with_default(
 
 
 async def test_backup_to_default(
-    coresys: CoreSys, tmp_supervisor_data, path_extern, mount_propagation
+    coresys: CoreSys, tmp_supervisor_data, path_extern, mount_propagation, mock_is_mount
 ):
     """Test making backup to default mount."""
     # Add a default backup mount
@@ -780,7 +793,7 @@ async def test_backup_to_default(
         HomeAssistant,
         "version",
         new=PropertyMock(return_value=AwesomeVersion("2023.1.1")),
-    ), patch("supervisor.mounts.mount.Path.is_mount", return_value=True):
+    ):
         backup: Backup = await coresys.backups.do_backup_partial(
             "test", homeassistant=True
         )
@@ -789,7 +802,11 @@ async def test_backup_to_default(
 
 
 async def test_backup_to_default_mount_down_error(
-    coresys: CoreSys, tmp_supervisor_data, path_extern, mount_propagation
+    coresys: CoreSys,
+    mock_is_mount: MagicMock,
+    tmp_supervisor_data,
+    path_extern,
+    mount_propagation,
 ):
     """Test making backup to default mount when it is down."""
     # Add a default backup mount
@@ -809,6 +826,7 @@ async def test_backup_to_default_mount_down_error(
     coresys.mounts.default_backup_mount = mount
 
     # Attempt to make a backup which fails because is_mount on directory is false
+    mock_is_mount.return_value = False
     coresys.core.state = CoreState.RUNNING
     coresys.hardware.disk.get_disk_free_space = lambda x: 5000
 
@@ -819,6 +837,7 @@ async def test_backup_to_default_mount_down_error(
 async def test_load_network_error(
     coresys: CoreSys,
     caplog: pytest.LogCaptureFixture,
+    mock_is_mount: MagicMock,
     tmp_supervisor_data,
     path_extern,
     mount_propagation,
@@ -840,6 +859,7 @@ async def test_load_network_error(
     caplog.clear()
 
     # This should not raise, manager should just ignore backup locations with errors
+    mock_is_mount.return_value = False
     mock_path = MagicMock()
     mock_path.is_dir.side_effect = OSError("Host is down")
     mock_path.as_posix.return_value = "/data/backup_test"
@@ -1552,6 +1572,7 @@ async def test_backup_to_mount_bypasses_free_space_condition(
     tmp_supervisor_data,
     path_extern,
     mount_propagation,
+    mock_is_mount,
 ):
     """Test backing up to a mount bypasses the check on local free space."""
     coresys.core.state = CoreState.RUNNING
@@ -1590,9 +1611,8 @@ async def test_backup_to_mount_bypasses_free_space_condition(
     mount = coresys.mounts.get("backup_test")
 
     # These succeed because local free space does not matter when using a mount
-    with patch("supervisor.mounts.mount.Path.is_mount", return_value=True):
+    await coresys.backups.do_backup_full(location=mount)
-        await coresys.backups.do_backup_full(location=mount)
+    await coresys.backups.do_backup_partial(folders=["media"], location=mount)
-        await coresys.backups.do_backup_partial(folders=["media"], location=mount)
 
 
 @pytest.mark.parametrize(
@@ -1686,6 +1706,7 @@ async def test_reload_error(
     caplog: pytest.LogCaptureFixture,
     error_path: Path,
     healthy_expected: bool,
+    mock_is_mount: MagicMock,
     path_extern,
     mount_propagation,
 ):
@@ -1713,6 +1734,7 @@ async def test_reload_error(
         )
     )
 
+    mock_is_mount.return_value = False
     with patch("supervisor.backups.manager.Path.is_dir", new=mock_is_dir), patch(
         "supervisor.backups.manager.Path.glob", return_value=[]
     ):

tests/conftest.py

@@ -711,3 +711,10 @@ def mock_aarch64_arch_supported(coresys: CoreSys) -> None:
     """Mock aarch64 arch as supported."""
     with patch.object(coresys.arch, "_supported_set", {"aarch64"}):
         yield
+
+
+@pytest.fixture
+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

tests/mounts/test_manager.py

@@ -52,7 +52,7 @@ SHARE_TEST_DATA = {
 
 @pytest.fixture(name="mount")
 async def fixture_mount(
-    coresys: CoreSys, tmp_supervisor_data, path_extern, mount_propagation
+    coresys: CoreSys, tmp_supervisor_data, path_extern, mount_propagation, mock_is_mount
 ) -> Mount:
     """Add an initial mount and load mounts."""
     mount = Mount.from_dict(coresys, MEDIA_TEST_DATA)
@@ -328,6 +328,7 @@ async def test_create_mount(
     tmp_supervisor_data,
     path_extern,
     mount_propagation,
+    mock_is_mount,
 ):
     """Test creating a mount."""
     systemd_service: SystemdService = all_dbus_services["systemd"]
@@ -459,7 +460,11 @@ async def test_remove_reload_mount_missing(coresys: CoreSys, mount_propagation):
 
 
 async def test_save_data(
-    coresys: CoreSys, tmp_supervisor_data: Path, path_extern, mount_propagation
+    coresys: CoreSys,
+    tmp_supervisor_data: Path,
+    path_extern,
+    mount_propagation,
+    mock_is_mount,
 ):
     """Test saving mount config data."""
     # Replace mount manager with one that doesn't have save_data mocked
@@ -639,6 +644,7 @@ async def test_create_share_mount(
     tmp_supervisor_data,
     path_extern,
     mount_propagation,
+    mock_is_mount,
 ):
     """Test creating a share mount."""
     systemd_service: SystemdService = all_dbus_services["systemd"]

tests/mounts/test_mount.py

@@ -1,18 +1,19 @@
 """Tests for mounts."""
 from __future__ import annotations
 
+import asyncio
 import os
 from pathlib import Path
 import stat
 from typing import Any
-from unittest.mock import patch
+from unittest.mock import MagicMock
 
 from dbus_fast import DBusError, ErrorType, Variant
 import pytest
 
 from supervisor.coresys import CoreSys
 from supervisor.dbus.const import UnitActiveState
-from supervisor.exceptions import MountError, MountInvalidError
+from supervisor.exceptions import MountActivationError, MountError, MountInvalidError
 from supervisor.mounts.const import MountCifsVersion, MountType, MountUsage
 from supervisor.mounts.mount import CIFSMount, Mount, NFSMount
 from supervisor.resolution.const import ContextType, IssueType, SuggestionType
@@ -49,6 +50,7 @@ async def test_cifs_mount(
     path_extern,
     additional_data: dict[str, Any],
     expected_options: list[str],
+    mock_is_mount,
 ):
     """Test CIFS mount."""
     systemd_service: SystemdService = all_dbus_services["systemd"]
@@ -143,6 +145,7 @@ async def test_cifs_mount_read_only(
     all_dbus_services: dict[str, DBusServiceMock],
     tmp_supervisor_data: Path,
     path_extern,
+    mock_is_mount,
 ):
     """Test a read-only cifs mount."""
     systemd_service: SystemdService = all_dbus_services["systemd"]
@@ -188,6 +191,7 @@ async def test_nfs_mount(
     all_dbus_services: dict[str, DBusServiceMock],
     tmp_supervisor_data: Path,
     path_extern,
+    mock_is_mount,
 ):
     """Test NFS mount."""
     systemd_service: SystemdService = all_dbus_services["systemd"]
@@ -247,6 +251,7 @@ async def test_nfs_mount_read_only(
     all_dbus_services: dict[str, DBusServiceMock],
     tmp_supervisor_data: Path,
     path_extern,
+    mock_is_mount,
 ):
     """Test NFS mount."""
     systemd_service: SystemdService = all_dbus_services["systemd"]
@@ -292,6 +297,7 @@ async def test_load(
     all_dbus_services: dict[str, DBusServiceMock],
     tmp_supervisor_data,
     path_extern,
+    mock_is_mount,
 ):
     """Test mount loading."""
     systemd_service: SystemdService = all_dbus_services["systemd"]
@@ -365,13 +371,12 @@ async def test_load(
     systemd_unit_service.active_state = "activating"
     mount = Mount.from_dict(coresys, mount_data)
 
-    async def mock_activation_finished(*_):
+    load_task = asyncio.create_task(mount.load())
-        assert mount.state == UnitActiveState.ACTIVATING
+    await asyncio.sleep(0.1)
-        assert systemd_service.ReloadOrRestartUnit.calls == []
+    systemd_unit_service.emit_properties_changed({"ActiveState": "failed"})
-        systemd_unit_service.active_state = ["failed", "active"]
+    await asyncio.sleep(0.1)
-
+    systemd_unit_service.emit_properties_changed({"ActiveState": "active"})
-    with patch("supervisor.mounts.mount.asyncio.sleep", new=mock_activation_finished):
+    await load_task
-        await mount.load()
 
     assert mount.state == UnitActiveState.ACTIVE
     assert systemd_service.StartTransientUnit.calls == []
@@ -381,7 +386,10 @@ async def test_load(
 
 
 async def test_unmount(
-    coresys: CoreSys, all_dbus_services: dict[str, DBusServiceMock], path_extern
+    coresys: CoreSys,
+    all_dbus_services: dict[str, DBusServiceMock],
+    path_extern,
+    mock_is_mount,
 ):
     """Test unmounting."""
     systemd_service: SystemdService = all_dbus_services["systemd"]
@@ -418,6 +426,7 @@ async def test_mount_failure(
     all_dbus_services: dict[str, DBusServiceMock],
     tmp_supervisor_data,
     path_extern,
+    mock_is_mount,
 ):
     """Test failure to mount."""
     systemd_service: SystemdService = all_dbus_services["systemd"]
@@ -461,18 +470,15 @@ async def test_mount_failure(
     systemd_service.GetUnit.calls.clear()
     systemd_unit_service.active_state = "activating"
 
-    async def mock_activation_finished(*_):
+    load_task = asyncio.create_task(mount.mount())
-        assert mount.state == UnitActiveState.ACTIVATING
+    await asyncio.sleep(0.1)
-        systemd_unit_service.active_state = "failed"
+    systemd_unit_service.emit_properties_changed({"ActiveState": "failed"})
-
+    with pytest.raises(MountError):
-    with patch(
+        await load_task
-        "supervisor.mounts.mount.asyncio.sleep", new=mock_activation_finished
-    ), pytest.raises(MountError):
-        await mount.mount()
 
     assert mount.state == UnitActiveState.FAILED
     assert len(systemd_service.StartTransientUnit.calls) == 1
-    assert len(systemd_service.GetUnit.calls) == 2
+    assert len(systemd_service.GetUnit.calls) == 1
 
 
 async def test_unmount_failure(
@@ -500,11 +506,11 @@ async def test_unmount_failure(
 
     assert len(systemd_service.StopUnit.calls) == 1
 
-    # If error is NoSuchUnit then ignore, it has already been unmounted
+    # If unit is missing we skip unmounting, its already gone
     systemd_service.StopUnit.calls.clear()
-    systemd_service.response_stop_unit = ERROR_NO_UNIT
+    systemd_service.response_get_unit = ERROR_NO_UNIT
     await mount.unmount()
-    assert len(systemd_service.StopUnit.calls) == 1
+    assert systemd_service.StopUnit.calls == []
 
 
 async def test_reload_failure(
@@ -512,6 +518,7 @@ async def test_reload_failure(
     all_dbus_services: dict[str, DBusServiceMock],
     tmp_supervisor_data,
     path_extern,
+    mock_is_mount,
 ):
     """Test failure to reload."""
     systemd_service: SystemdService = all_dbus_services["systemd"]
@@ -610,7 +617,7 @@ async def test_mount_local_where_invalid(
     assert systemd_service.StartTransientUnit.calls == []
 
 
-async def test_update_clears_issue(coresys: CoreSys, path_extern):
+async def test_update_clears_issue(coresys: CoreSys, path_extern, mock_is_mount):
     """Test updating mount data clears corresponding failed mount issue if active."""
     mount = Mount.from_dict(
         coresys,
@@ -635,8 +642,88 @@ async def test_update_clears_issue(coresys: CoreSys, path_extern):
     assert mount.failed_issue in coresys.resolution.issues
     assert len(coresys.resolution.suggestions_for_issue(mount.failed_issue)) == 2
 
-    await mount.update()
+    assert await mount.update() is True
 
     assert mount.state == UnitActiveState.ACTIVE
     assert mount.failed_issue not in coresys.resolution.issues
     assert not coresys.resolution.suggestions_for_issue(mount.failed_issue)
+
+
+async def test_update_leaves_issue_if_down(
+    coresys: CoreSys, mock_is_mount: MagicMock, path_extern
+):
+    """Test issue is left if system is down after update (is_mount is false)."""
+    mount = Mount.from_dict(
+        coresys,
+        {
+            "name": "test",
+            "usage": "media",
+            "type": "cifs",
+            "server": "test.local",
+            "share": "share",
+        },
+    )
+
+    assert mount.failed_issue not in coresys.resolution.issues
+
+    coresys.resolution.create_issue(
+        IssueType.MOUNT_FAILED,
+        ContextType.MOUNT,
+        reference="test",
+        suggestions=[SuggestionType.EXECUTE_RELOAD, SuggestionType.EXECUTE_REMOVE],
+    )
+
+    assert mount.failed_issue in coresys.resolution.issues
+    assert len(coresys.resolution.suggestions_for_issue(mount.failed_issue)) == 2
+
+    mock_is_mount.return_value = False
+    assert (await mount.update()) is False
+
+    assert mount.state == UnitActiveState.ACTIVE
+    assert mount.failed_issue in coresys.resolution.issues
+    assert len(coresys.resolution.suggestions_for_issue(mount.failed_issue)) == 2
+
+
+async def test_mount_fails_if_down(
+    coresys: CoreSys,
+    all_dbus_services: dict[str, DBusServiceMock],
+    tmp_supervisor_data: Path,
+    mock_is_mount: MagicMock,
+    path_extern,
+):
+    """Test mount fails if system is down (is_mount is false)."""
+    systemd_service: SystemdService = all_dbus_services["systemd"]
+    systemd_service.StartTransientUnit.calls.clear()
+
+    mount_data = {
+        "name": "test",
+        "usage": "media",
+        "type": "nfs",
+        "server": "test.local",
+        "path": "/media/camera",
+        "port": 1234,
+        "read_only": False,
+    }
+    mount: NFSMount = Mount.from_dict(coresys, mount_data)
+
+    mock_is_mount.return_value = False
+    with pytest.raises(MountActivationError):
+        await mount.mount()
+
+    assert mount.state == UnitActiveState.ACTIVE
+    assert mount.local_where.exists()
+    assert mount.local_where.is_dir()
+
+    assert systemd_service.StartTransientUnit.calls == [
+        (
+            "mnt-data-supervisor-mounts-test.mount",
+            "fail",
+            [
+                ["Options", Variant("s", "port=1234,soft,timeo=200")],
+                ["Type", Variant("s", "nfs")],
+                ["Description", Variant("s", "Supervisor nfs mount: test")],
+                ["What", Variant("s", "test.local:/media/camera")],
+            ],
+            [],
+        )
+    ]

tests/resolution/fixup/test_mount_execute_reload.py

@@ -1,8 +1,14 @@
 """Test fixup mount reload."""
 
+from unittest.mock import MagicMock
+
+import pytest
+
 from supervisor.coresys import CoreSys
+from supervisor.exceptions import MountActivationError
 from supervisor.mounts.mount import Mount
 from supervisor.resolution.const import ContextType, IssueType, SuggestionType
+from supervisor.resolution.data import Issue
 from supervisor.resolution.fixups.mount_execute_reload import FixupMountExecuteReload
 
 from tests.dbus_service_mocks.base import DBusServiceMock
@@ -14,6 +20,7 @@ async def test_fixup(
     all_dbus_services: dict[str, DBusServiceMock],
     path_extern,
     mount_propagation,
+    mock_is_mount,
 ):
     """Test fixup."""
     systemd_service: SystemdService = all_dbus_services["systemd"]
@@ -50,3 +57,42 @@ async def test_fixup(
     assert systemd_service.ReloadOrRestartUnit.calls == [
         ("mnt-data-supervisor-mounts-test.mount", "fail")
     ]
+
+
+async def test_fixup_error_after_reload(
+    coresys: CoreSys,
+    all_dbus_services: dict[str, DBusServiceMock],
+    mock_is_mount: MagicMock,
+    path_extern,
+    mount_propagation,
+):
+    """Test fixup."""
+    mount_execute_reload = FixupMountExecuteReload(coresys)
+    await coresys.mounts.create_mount(
+        Mount.from_dict(
+            coresys,
+            {
+                "name": "test",
+                "usage": "backup",
+                "type": "cifs",
+                "server": "test.local",
+                "share": "test",
+            },
+        )
+    )
+
+    coresys.resolution.create_issue(
+        IssueType.MOUNT_FAILED,
+        ContextType.MOUNT,
+        reference="test",
+        suggestions=[SuggestionType.EXECUTE_RELOAD, SuggestionType.EXECUTE_REMOVE],
+    )
+    mock_is_mount.return_value = False
+    with pytest.raises(MountActivationError):
+        await mount_execute_reload()
+
+    # Since is_mount is false, issue remains
+    assert (
+        Issue(IssueType.MOUNT_FAILED, ContextType.MOUNT, reference="test")
+        in coresys.resolution.issues
+    )

tests/resolution/fixup/test_mount_execute_remove.py

@@ -15,6 +15,7 @@ async def test_fixup(
     all_dbus_services: dict[str, DBusServiceMock],
     path_extern,
     mount_propagation,
+    mock_is_mount,
 ):
     """Test fixup."""
     systemd_service: SystemdService = all_dbus_services["systemd"]