Fix bind mounting and remove on create failure

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 ..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
@@ -112,11 +112,17 @@ async def create_mount(self, mount: Mount) -> None:
         if mount.name in self._mounts:
             _LOGGER.debug("Mount '%s' exists, unmounting then mounting from new config")
             await self.remove_mount(mount.name)
+            # For updates, add to mounts immediately so mount failure doesn't delete it
+            self._mounts[mount.name] = mount
 
         _LOGGER.info("Creating or updating mount: %s", mount.name)
-        self._mounts[mount.name] = mount
-        await mount.load()
+        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)
 

supervisor/mounts/mount.py

@@ -20,7 +20,13 @@
     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,14 +206,16 @@ async def mount(self) -> None:
                 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,
                 options
                 + [
                     (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,15 +226,20 @@ async def mount(self) -> None:
         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:
-            await self.sys_dbus.systemd.stop_unit(self.unit_name, StopUnitMode.FAIL)
+            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)
         except DBusError as err:
@@ -255,7 +268,7 @@ async def reload(self) -> None:
         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 @@ def where(self) -> PurePath:
     @property
     def options(self) -> list[str]:
         """List of options to use to mount."""
-        return []
+        return ["bind"]

tests/api/test_mounts.py

@@ -1,13 +1,15 @@
 """Test mounts API."""
 
 from aiohttp.test_utils import TestClient
+from dbus_fast import DBusError, ErrorType
 import pytest
 
 from supervisor.coresys import CoreSys
 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": "backups1",
+            "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": "backups2",
+            "state": None,
+        }
+    ]
+
+
 async def test_api_reload_mount(
     api_client: TestClient, all_dbus_services: dict[str, DBusServiceMock], mount
 ):