From b5bf270d2246525b920df5795b215cf9033c8cc0 Mon Sep 17 00:00:00 2001 From: Mike Degatano Date: Mon, 12 Feb 2024 11:32:54 -0500 Subject: [PATCH] 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 | 3 + supervisor/dbus/systemd.py | 7 + supervisor/mounts/manager.py | 13 +- supervisor/mounts/mount.py | 114 +++++++++------ tests/api/test_backups.py | 26 ++-- tests/api/test_mounts.py | 15 +- tests/backups/test_manager.py | 44 ++++-- tests/conftest.py | 7 + tests/mounts/test_manager.py | 10 +- tests/mounts/test_mount.py | 135 ++++++++++++++---- .../fixup/test_mount_execute_reload.py | 46 ++++++ .../fixup/test_mount_execute_remove.py | 1 + 12 files changed, 319 insertions(+), 102 deletions(-) diff --git a/supervisor/dbus/const.py b/supervisor/dbus/const.py index 444493119..cd8dc97d2 100644 --- a/supervisor/dbus/const.py +++ b/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" diff --git a/supervisor/dbus/systemd.py b/supervisor/dbus/systemd.py index 60a7ae092..e5abf6dd5 100644 --- a/supervisor/dbus/systemd.py +++ b/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. diff --git a/supervisor/mounts/manager.py b/supervisor/mounts/manager.py index d4118d3cf..140569002 100644 --- a/supervisor/mounts/manager.py +++ b/supervisor/mounts/manager.py @@ -145,16 +145,17 @@ class MountManager(FileConfiguration, CoreSysAttributes): if not self.mounts: return - await asyncio.wait( - [self.sys_create_task(mount.update()) for mount in self.mounts] + mounts = self.mounts.copy() + 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 - for mount in self.mounts - if mount.state != UnitActiveState.ACTIVE - and mount.failed_issue not in self.sys_resolution.issues + mounts[i] + for i in range(len(mounts)) + if results[i] is not True + 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] diff --git a/supervisor/mounts/mount.py b/supervisor/mounts/mount.py index 95861db28..3ed4ec250 100644 --- a/supervisor/mounts/mount.py +++ b/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 - elif self.state != UnitActiveState.ACTIVE: + await self._update_state_await(not_state=UnitActiveState.ACTIVATING) + + # 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: - """Update info about mount from dbus.""" + async def _update_unit(self) -> SystemdUnit | None: + """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 - and self.failed_issue in self.sys_resolution.issues - ): + mounted := await self.is_mounted() + ) 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: - """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) + return mounted - _LOGGER.warning( - "Mount %s still in state %s after waiting for 30 seconods to complete", - self.name, - str(self.state).lower(), - ) + async def _update_state_await( + self, + expected_states: list[UnitActiveState] | None = None, + 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): - """Update info about mount from dbus. If 'activating' wait up to 30 seconds.""" - await self.update() + try: + async with asyncio.timeout(30), self.unit.properties_changed() as signal: + 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 - if self.state == UnitActiveState.ACTIVATING: - _LOGGER.info( - "Mount %s still activating, waiting up to 30 seconds to complete", + except asyncio.TimeoutError: + _LOGGER.warning( + "Mount %s still in state %s after waiting for 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.""" diff --git a/tests/api/test_backups.py b/tests/api/test_backups.py index a6e50953e..77badeb45 100644 --- a/tests/api/test_backups.py +++ b/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( - "/backups/new/full", - json={ - "name": "Mount test", - "location": location, - }, - ) + resp = await api_client.post( + "/backups/new/full", + json={ + "name": "Mount test", + "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( - "/backups/new/full", - json={"name": "Mount test"}, - ) + resp = await api_client.post( + "/backups/new/full", + json={"name": "Mount test"}, + ) result = await resp.json() assert result["result"] == "ok" slug = result["data"]["slug"] diff --git a/tests/api/test_mounts.py b/tests/api/test_mounts.py index b9360b764..e69b459d8 100644 --- a/tests/api/test_mounts.py +++ b/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( diff --git a/tests/backups/test_manager.py b/tests/backups/test_manager.py index 013133be4..ff9b6e7d2 100644 --- a/tests/backups/test_manager.py +++ b/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_partial(folders=["media"], location=mount) + await coresys.backups.do_backup_full(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=[] ): diff --git a/tests/conftest.py b/tests/conftest.py index 01e633416..6fdd2f74f 100644 --- a/tests/conftest.py +++ b/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 diff --git a/tests/mounts/test_manager.py b/tests/mounts/test_manager.py index d4b4c7e72..fe5b0e39e 100644 --- a/tests/mounts/test_manager.py +++ b/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"] diff --git a/tests/mounts/test_mount.py b/tests/mounts/test_mount.py index 896c6cf90..48b4a601d 100644 --- a/tests/mounts/test_mount.py +++ b/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(*_): - assert mount.state == UnitActiveState.ACTIVATING - assert systemd_service.ReloadOrRestartUnit.calls == [] - systemd_unit_service.active_state = ["failed", "active"] - - with patch("supervisor.mounts.mount.asyncio.sleep", new=mock_activation_finished): - await mount.load() + load_task = asyncio.create_task(mount.load()) + await asyncio.sleep(0.1) + systemd_unit_service.emit_properties_changed({"ActiveState": "failed"}) + await asyncio.sleep(0.1) + systemd_unit_service.emit_properties_changed({"ActiveState": "active"}) + await load_task 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(*_): - assert mount.state == UnitActiveState.ACTIVATING - systemd_unit_service.active_state = "failed" - - with patch( - "supervisor.mounts.mount.asyncio.sleep", new=mock_activation_finished - ), pytest.raises(MountError): - await mount.mount() + load_task = asyncio.create_task(mount.mount()) + await asyncio.sleep(0.1) + systemd_unit_service.emit_properties_changed({"ActiveState": "failed"}) + with pytest.raises(MountError): + await load_task 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")], + ], + [], + ) + ] diff --git a/tests/resolution/fixup/test_mount_execute_reload.py b/tests/resolution/fixup/test_mount_execute_reload.py index 7aa80a86f..e43e654c0 100644 --- a/tests/resolution/fixup/test_mount_execute_reload.py +++ b/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 + ) diff --git a/tests/resolution/fixup/test_mount_execute_remove.py b/tests/resolution/fixup/test_mount_execute_remove.py index 727564b07..1e1ed1dbc 100644 --- a/tests/resolution/fixup/test_mount_execute_remove.py +++ b/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"]