Fix bind mounting and remove on create failure (#4274)

* Fix bind mounting and remove on create failure

* Fix test and make update fully fail on dbus error

supervisor/exceptions.py

@@ -579,6 +579,10 @@ class MountError(HassioError):
     """Raise on an error related to mounting/unmounting."""
 
 
+class MountActivationError(MountError):
+    """Raise on mount not reaching active state after mount/reload."""
+
+
 class MountInvalidError(MountError):
     """Raise on invalid mount attempt."""
 

supervisor/mounts/manager.py

@@ -8,7 +8,7 @@ from pathlib import PurePath
 from ..const import ATTR_NAME
 from ..coresys import CoreSys, CoreSysAttributes
 from ..dbus.const import UnitActiveState
-from ..exceptions import MountError, MountNotFound
+from ..exceptions import MountActivationError, MountError, MountNotFound
 from ..resolution.const import ContextType, IssueType, SuggestionType
 from ..utils.common import FileConfiguration
 from ..utils.sentry import capture_exception
@@ -111,16 +111,20 @@ class MountManager(FileConfiguration, CoreSysAttributes):
         """Add/update a mount."""
         if mount.name in self._mounts:
             _LOGGER.debug("Mount '%s' exists, unmounting then mounting from new config")
-            await self.remove_mount(mount.name)
+            await self.remove_mount(mount.name, retain_entry=True)
 
         _LOGGER.info("Creating or updating mount: %s", mount.name)
-        self._mounts[mount.name] = mount
+        try:
             await mount.load()
+        except MountActivationError as err:
+            await mount.unmount()
+            raise err
 
+        self._mounts[mount.name] = mount
         if mount.usage == MountUsage.MEDIA:
             await self._bind_media(mount)
 
-    async def remove_mount(self, name: str) -> None:
+    async def remove_mount(self, name: str, *, retain_entry: bool = False) -> None:
         """Remove a mount."""
         if name not in self._mounts:
             raise MountNotFound(
@@ -133,6 +137,7 @@ class MountManager(FileConfiguration, CoreSysAttributes):
             del self._bound_mounts[name]
 
         await self._mounts[name].unmount()
+        if not retain_entry:
             del self._mounts[name]
 
     async def reload_mount(self, name: str) -> None:

supervisor/mounts/mount.py

@@ -20,7 +20,13 @@ from ..dbus.const import (
     UnitActiveState,
 )
 from ..dbus.systemd import SystemdUnit
-from ..exceptions import DBusError, DBusSystemdNoSuchUnit, MountError, MountInvalidError
+from ..exceptions import (
+    DBusError,
+    DBusSystemdNoSuchUnit,
+    MountActivationError,
+    MountError,
+    MountInvalidError,
+)
 from ..utils.sentry import capture_exception
 from .const import ATTR_PATH, ATTR_SERVER, ATTR_SHARE, ATTR_USAGE, MountType, MountUsage
 from .validate import MountData
@@ -200,6 +206,9 @@ class Mount(CoreSysAttributes, ABC):
                 if self.options
                 else []
             )
+            if self.type != MountType.BIND:
+                options += [(DBUS_ATTR_TYPE, Variant("s", self.type.value))]
+
             await self.sys_dbus.systemd.start_transient_unit(
                 self.unit_name,
                 StartUnitMode.FAIL,
@@ -207,7 +216,6 @@ class Mount(CoreSysAttributes, ABC):
                 + [
                     (DBUS_ATTR_DESCRIPTION, Variant("s", self.description)),
                     (DBUS_ATTR_WHAT, Variant("s", self.what)),
-                    (DBUS_ATTR_TYPE, Variant("s", self.type.value)),
                 ],
             )
         except DBusError as err:
@@ -218,14 +226,19 @@ class Mount(CoreSysAttributes, ABC):
         await self._update_await_activating()
 
         if self.state != UnitActiveState.ACTIVE:
-            raise MountError(
+            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,
             )
 
     async def unmount(self) -> None:
         """Unmount using systemd."""
+        await self.update()
+
         try:
+            if self.state == UnitActiveState.FAILED:
+                await self.sys_dbus.systemd.reset_failed_unit(self.unit_name)
+            else:
                 await self.sys_dbus.systemd.stop_unit(self.unit_name, StopUnitMode.FAIL)
         except DBusSystemdNoSuchUnit:
             _LOGGER.info("Mount %s is not mounted, skipping unmount", self.name)
@@ -255,7 +268,7 @@ class Mount(CoreSysAttributes, ABC):
         await self._update_await_activating()
 
         if self.state != UnitActiveState.ACTIVE:
-            raise MountError(
+            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,
             )
@@ -407,4 +420,4 @@ class BindMount(Mount):
     @property
     def options(self) -> list[str]:
         """List of options to use to mount."""
-        return []
+        return ["bind"]

tests/api/test_mounts.py

@@ -1,6 +1,7 @@
 """Test mounts API."""
 
 from aiohttp.test_utils import TestClient
+from dbus_fast import DBusError, ErrorType
 import pytest
 
 from supervisor.coresys import CoreSys
@@ -8,6 +9,7 @@ from supervisor.mounts.mount import Mount
 
 from tests.dbus_service_mocks.base import DBusServiceMock
 from tests.dbus_service_mocks.systemd import Systemd as SystemdService
+from tests.dbus_service_mocks.systemd_unit import SystemdUnit as SystemdUnitService
 
 
 @pytest.fixture(name="mount")
@@ -87,6 +89,70 @@ async def test_api_create_error_mount_exists(api_client: TestClient, mount):
     assert result["message"] == "A mount already exists with name backup_test"
 
 
+async def test_api_create_dbus_error_mount_not_added(
+    api_client: TestClient,
+    all_dbus_services: dict[str, DBusServiceMock],
+    tmp_supervisor_data,
+    path_extern,
+):
+    """Test mount not added to list of mounts if a dbus error occurs."""
+    systemd_service: SystemdService = all_dbus_services["systemd"]
+    systemd_unit_service: SystemdUnitService = all_dbus_services["systemd_unit"]
+    systemd_service.response_get_unit = DBusError(
+        "org.freedesktop.systemd1.NoSuchUnit", "error"
+    )
+    systemd_service.response_start_transient_unit = DBusError(ErrorType.FAILED, "fail")
+
+    resp = await api_client.post(
+        "/mounts",
+        json={
+            "name": "backup_test",
+            "type": "cifs",
+            "usage": "backup",
+            "server": "backup.local",
+            "share": "backups",
+        },
+    )
+    assert resp.status == 400
+    result = await resp.json()
+    assert result["result"] == "error"
+    assert result["message"] == "Could not mount backup_test due to: fail"
+
+    resp = await api_client.get("/mounts")
+    result = await resp.json()
+    assert result["data"]["mounts"] == []
+
+    systemd_service.response_get_unit = [
+        DBusError("org.freedesktop.systemd1.NoSuchUnit", "error"),
+        "/org/freedesktop/systemd1/unit/tmp_2dyellow_2emount",
+        "/org/freedesktop/systemd1/unit/tmp_2dyellow_2emount",
+    ]
+    systemd_service.response_start_transient_unit = "/org/freedesktop/systemd1/job/7623"
+    systemd_unit_service.active_state = "failed"
+
+    resp = await api_client.post(
+        "/mounts",
+        json={
+            "name": "backup_test",
+            "type": "cifs",
+            "usage": "backup",
+            "server": "backup.local",
+            "share": "backups",
+        },
+    )
+    assert resp.status == 400
+    result = await resp.json()
+    assert result["result"] == "error"
+    assert (
+        result["message"]
+        == "Mounting backup_test did not succeed. Check host logs for errors from mount or systemd unit mnt-data-supervisor-mounts-backup_test.mount for details."
+    )
+
+    resp = await api_client.get("/mounts")
+    result = await resp.json()
+    assert result["data"]["mounts"] == []
+
+
 async def test_api_update_mount(api_client: TestClient, coresys: CoreSys, mount):
     """Test updating a mount via API."""
     resp = await api_client.put(
@@ -134,6 +200,89 @@ async def test_api_update_error_mount_missing(api_client: TestClient):
     assert result["message"] == "No mount exists with name backup_test"
 
 
+async def test_api_update_dbus_error_mount_remains(
+    api_client: TestClient,
+    all_dbus_services: dict[str, DBusServiceMock],
+    mount,
+    tmp_supervisor_data,
+    path_extern,
+):
+    """Test mount remains in list with unsuccessful state if dbus error occurs during update."""
+    systemd_service: SystemdService = all_dbus_services["systemd"]
+    systemd_unit_service: SystemdUnitService = all_dbus_services["systemd_unit"]
+    systemd_service.response_get_unit = [
+        "/org/freedesktop/systemd1/unit/tmp_2dyellow_2emount",
+        DBusError("org.freedesktop.systemd1.NoSuchUnit", "error"),
+    ]
+    systemd_service.response_start_transient_unit = DBusError(ErrorType.FAILED, "fail")
+
+    resp = await api_client.put(
+        "/mounts/backup_test",
+        json={
+            "type": "cifs",
+            "usage": "backup",
+            "server": "backup.local",
+            "share": "backups1",
+        },
+    )
+    assert resp.status == 400
+    result = await resp.json()
+    assert result["result"] == "error"
+    assert result["message"] == "Could not mount backup_test due to: fail"
+
+    resp = await api_client.get("/mounts")
+    result = await resp.json()
+    assert result["data"]["mounts"] == [
+        {
+            "name": "backup_test",
+            "type": "cifs",
+            "usage": "backup",
+            "server": "backup.local",
+            "share": "backups",
+            "state": None,
+        }
+    ]
+
+    systemd_service.response_get_unit = [
+        "/org/freedesktop/systemd1/unit/tmp_2dyellow_2emount",
+        DBusError("org.freedesktop.systemd1.NoSuchUnit", "error"),
+        "/org/freedesktop/systemd1/unit/tmp_2dyellow_2emount",
+        "/org/freedesktop/systemd1/unit/tmp_2dyellow_2emount",
+    ]
+    systemd_service.response_start_transient_unit = "/org/freedesktop/systemd1/job/7623"
+    systemd_unit_service.active_state = "failed"
+
+    resp = await api_client.put(
+        "/mounts/backup_test",
+        json={
+            "type": "cifs",
+            "usage": "backup",
+            "server": "backup.local",
+            "share": "backups2",
+        },
+    )
+    assert resp.status == 400
+    result = await resp.json()
+    assert result["result"] == "error"
+    assert (
+        result["message"]
+        == "Mounting backup_test did not succeed. Check host logs for errors from mount or systemd unit mnt-data-supervisor-mounts-backup_test.mount for details."
+    )
+
+    resp = await api_client.get("/mounts")
+    result = await resp.json()
+    assert result["data"]["mounts"] == [
+        {
+            "name": "backup_test",
+            "type": "cifs",
+            "usage": "backup",
+            "server": "backup.local",
+            "share": "backups",
+            "state": None,
+        }
+    ]
+
+
 async def test_api_reload_mount(
     api_client: TestClient, all_dbus_services: dict[str, DBusServiceMock], mount
 ):

tests/backups/test_manager.py

@@ -377,6 +377,8 @@ async def test_backup_media_with_mounts(
         "/org/freedesktop/systemd1/unit/tmp_2dyellow_2emount",
         DBusError("org.freedesktop.systemd1.NoSuchUnit", "error"),
         "/org/freedesktop/systemd1/unit/tmp_2dyellow_2emount",
+        "/org/freedesktop/systemd1/unit/tmp_2dyellow_2emount",
+        "/org/freedesktop/systemd1/unit/tmp_2dyellow_2emount",
     ]
 
     # Make some normal test files

tests/mounts/test_manager.py

@@ -4,13 +4,13 @@ import json
 import os
 from pathlib import Path
 
-from dbus_fast import DBusError, Variant
+from dbus_fast import DBusError, ErrorType, Variant
 from dbus_fast.aio.message_bus import MessageBus
 import pytest
 
 from supervisor.coresys import CoreSys
 from supervisor.dbus.const import UnitActiveState
-from supervisor.exceptions import MountNotFound
+from supervisor.exceptions import MountActivationError, MountError, MountNotFound
 from supervisor.mounts.manager import MountManager
 from supervisor.mounts.mount import Mount
 from supervisor.resolution.const import ContextType, IssueType, SuggestionType
@@ -101,9 +101,9 @@ async def test_load(
             "mnt-data-supervisor-mounts-backup_test.mount",
             "fail",
             [
+                ["Type", Variant("s", "cifs")],
                 ["Description", Variant("s", "Supervisor cifs mount: backup_test")],
                 ["What", Variant("s", "//backup.local/backups")],
-                ["Type", Variant("s", "cifs")],
             ],
             [],
         ),
@@ -111,9 +111,9 @@ async def test_load(
             "mnt-data-supervisor-mounts-media_test.mount",
             "fail",
             [
+                ["Type", Variant("s", "nfs")],
                 ["Description", Variant("s", "Supervisor nfs mount: media_test")],
                 ["What", Variant("s", "media.local:/media")],
-                ["Type", Variant("s", "nfs")],
             ],
             [],
         ),
@@ -121,9 +121,9 @@ async def test_load(
             "mnt-data-supervisor-media-media_test.mount",
             "fail",
             [
+                ["Options", Variant("s", "bind")],
                 ["Description", Variant("s", "Supervisor bind mount: bind_media_test")],
                 ["What", Variant("s", "/mnt/data/supervisor/mounts/media_test")],
-                ["Type", Variant("s", "bind")],
             ],
             [],
         ),
@@ -229,12 +229,12 @@ async def test_mount_failed_during_load(
         "mnt-data-supervisor-media-media_test.mount",
         "fail",
         [
+            ["Options", Variant("s", "bind")],
             [
                 "Description",
                 Variant("s", "Supervisor bind mount: emergency_media_test"),
             ],
             ["What", Variant("s", "/mnt/data/supervisor/emergency/media_test")],
-            ["Type", Variant("s", "bind")],
         ],
         [],
     )
@@ -295,6 +295,8 @@ async def test_update_mount(
     assert mount_new.state is None
 
     systemd_service.response_get_unit = [
+        "/org/freedesktop/systemd1/unit/tmp_2dyellow_2emount",
+        "/org/freedesktop/systemd1/unit/tmp_2dyellow_2emount",
         ERROR_NO_UNIT,
         "/org/freedesktop/systemd1/unit/tmp_2dyellow_2emount",
         ERROR_NO_UNIT,
@@ -404,3 +406,69 @@ async def test_save_data(coresys: CoreSys, tmp_supervisor_data: Path, path_exter
                 "password": "password",
             }
         ]
+
+
+async def test_create_mount_start_unit_failure(
+    coresys: CoreSys,
+    all_dbus_services: dict[str, DBusServiceMock],
+    tmp_supervisor_data,
+    path_extern,
+):
+    """Test failure to start mount unit does not add mount to the list."""
+    systemd_service: SystemdService = all_dbus_services["systemd"]
+    systemd_service.StartTransientUnit.calls.clear()
+    systemd_service.ResetFailedUnit.calls.clear()
+    systemd_service.StopUnit.calls.clear()
+
+    systemd_service.response_get_unit = ERROR_NO_UNIT
+    systemd_service.response_start_transient_unit = DBusError(ErrorType.FAILED, "fail")
+
+    await coresys.mounts.load()
+
+    mount = Mount.from_dict(coresys, BACKUP_TEST_DATA)
+
+    with pytest.raises(MountError):
+        await coresys.mounts.create_mount(mount)
+
+    assert mount.state is None
+    assert mount not in coresys.mounts
+
+    assert len(systemd_service.StartTransientUnit.calls) == 1
+    assert not systemd_service.ResetFailedUnit.calls
+    assert not systemd_service.StopUnit.calls
+
+
+async def test_create_mount_activation_failure(
+    coresys: CoreSys,
+    all_dbus_services: dict[str, DBusServiceMock],
+    tmp_supervisor_data,
+    path_extern,
+):
+    """Test activation failure during create mount does not add mount to the list and unmounts new mount."""
+    systemd_service: SystemdService = all_dbus_services["systemd"]
+    systemd_unit_service: SystemdUnitService = all_dbus_services["systemd_unit"]
+
+    systemd_service.StartTransientUnit.calls.clear()
+    systemd_service.ResetFailedUnit.calls.clear()
+    systemd_service.StopUnit.calls.clear()
+
+    systemd_service.response_get_unit = [
+        ERROR_NO_UNIT,
+        "/org/freedesktop/systemd1/unit/tmp_2dyellow_2emount",
+        "/org/freedesktop/systemd1/unit/tmp_2dyellow_2emount",
+    ]
+    systemd_unit_service.active_state = "failed"
+
+    await coresys.mounts.load()
+
+    mount = Mount.from_dict(coresys, BACKUP_TEST_DATA)
+
+    with pytest.raises(MountActivationError):
+        await coresys.mounts.create_mount(mount)
+
+    assert mount.state is None
+    assert mount not in coresys.mounts
+
+    assert len(systemd_service.StartTransientUnit.calls) == 1
+    assert len(systemd_service.ResetFailedUnit.calls) == 1
+    assert not systemd_service.StopUnit.calls

tests/mounts/test_mount.py

@@ -73,9 +73,9 @@ async def test_cifs_mount(
             "fail",
             [
                 ["Options", Variant("s", "username=admin,password=password")],
+                ["Type", Variant("s", "cifs")],
                 ["Description", Variant("s", "Supervisor cifs mount: test")],
                 ["What", Variant("s", "//test.local/camera")],
-                ["Type", Variant("s", "cifs")],
             ],
             [],
         )
@@ -130,9 +130,9 @@ async def test_nfs_mount(
             "fail",
             [
                 ["Options", Variant("s", "port=1234")],
+                ["Type", Variant("s", "nfs")],
                 ["Description", Variant("s", "Supervisor nfs mount: test")],
                 ["What", Variant("s", "test.local:/media/camera")],
-                ["Type", Variant("s", "nfs")],
             ],
             [],
         )
@@ -176,9 +176,9 @@ async def test_load(
             "mnt-data-supervisor-mounts-test.mount",
             "fail",
             [
+                ["Type", Variant("s", "cifs")],
                 ["Description", Variant("s", "Supervisor cifs mount: test")],
                 ["What", Variant("s", "//test.local/share")],
-                ["Type", Variant("s", "cifs")],
             ],
             [],
         )