Improve error handling when mounts fail

supervisor/backups/manager.py

@@ -15,7 +15,12 @@
     CoreState,
 )
 from ..dbus.const import UnitActiveState
-from ..exceptions import BackupError, BackupInvalidError, BackupJobError
+from ..exceptions import (
+    BackupError,
+    BackupInvalidError,
+    BackupJobError,
+    BackupMountDownError,
+)
 from ..jobs.const import JOB_GROUP_BACKUP_MANAGER, JobCondition, JobExecutionLimit
 from ..jobs.decorator import Job
 from ..jobs.job_group import JobGroup
@@ -74,12 +79,16 @@ def get(self, slug: str) -> Backup:
 
     def _get_base_path(self, location: Mount | type[DEFAULT] | None = DEFAULT) -> Path:
         """Get base path for backup using location or default location."""
+        if location == DEFAULT and self.sys_mounts.default_backup_mount:
+            location = self.sys_mounts.default_backup_mount
+
         if location:
+            if not location.local_where.is_mount():
+                raise BackupMountDownError(
+                    f"{location.name} is down, cannot back-up to it", _LOGGER.error
+                )
             return location.local_where
 
-        if location == DEFAULT and self.sys_mounts.default_backup_mount:
-            return self.sys_mounts.default_backup_mount.local_where
-
         return self.sys_config.path_backup
 
     def _change_stage(

supervisor/exceptions.py

@@ -607,6 +607,10 @@ class BackupInvalidError(BackupError):
     """Raise if backup or password provided is invalid."""
 
 
+class BackupMountDownError(BackupError):
+    """Raise if mount specified for backup is down."""
+
+
 class BackupJobError(BackupError, JobException):
     """Raise on Backup job error."""
 

supervisor/mounts/const.py

@@ -8,6 +8,7 @@
 ATTR_DEFAULT_BACKUP_MOUNT = "default_backup_mount"
 ATTR_MOUNTS = "mounts"
 ATTR_PATH = "path"
+ATTR_READ_ONLY = "read_only"
 ATTR_SERVER = "server"
 ATTR_SHARE = "share"
 ATTR_USAGE = "usage"

supervisor/mounts/manager.py

@@ -297,6 +297,7 @@ async def _bind_mount(self, mount: Mount, where: PurePath) -> None:
                 name=f"{'emergency' if emergency else 'bind'}_{mount.name}",
                 path=path,
                 where=where,
+                read_only=emergency,
             ),
             emergency=emergency,
         )

supervisor/mounts/mount.py

@@ -39,6 +39,7 @@
 from ..utils.sentry import capture_exception
 from .const import (
     ATTR_PATH,
+    ATTR_READ_ONLY,
     ATTR_SERVER,
     ATTR_SHARE,
     ATTR_USAGE,
@@ -81,7 +82,9 @@ def from_dict(cls, coresys: CoreSys, data: MountData) -> "Mount":
 
     def to_dict(self, *, skip_secrets: bool = True) -> MountData:
         """Return dictionary representation."""
-        return MountData(name=self.name, type=self.type, usage=self.usage)
+        return MountData(
+            name=self.name, type=self.type, usage=self.usage, read_only=self.read_only
+        )
 
     @property
     def name(self) -> str:
@@ -102,6 +105,11 @@ def usage(self) -> MountUsage | None:
             else None
         )
 
+    @property
+    def read_only(self) -> bool:
+        """Is mount read-only."""
+        return self._data.get(ATTR_READ_ONLY, False)
+
     @property
     @abstractmethod
     def what(self) -> str:
@@ -113,9 +121,9 @@ def where(self) -> PurePath:
         """Where to mount (on host)."""
 
     @property
-    @abstractmethod
     def options(self) -> list[str]:
         """List of options to use to mount."""
+        return ["ro"] if self.read_only else []
 
     @property
     def description(self) -> str:
@@ -358,7 +366,10 @@ def where(self) -> PurePath:
     @property
     def options(self) -> list[str]:
         """Options to use to mount."""
-        return [f"port={self.port}"] if self.port else []
+        options = super().options
+        if self.port:
+            options.append(f"port={self.port}")
+        return options
 
 
 class CIFSMount(NetworkMount):
@@ -484,6 +495,7 @@ def create(
         path: Path,
         usage: MountUsage | None = None,
         where: PurePath | None = None,
+        read_only: bool = False,
     ) -> "BindMount":
         """Create a new bind mount instance."""
         return BindMount(
@@ -493,6 +505,7 @@ def create(
                 type=MountType.BIND,
                 path=path.as_posix(),
                 usage=usage and usage,
+                read_only=read_only,
             ),
             where=where,
         )
@@ -523,4 +536,4 @@ def where(self) -> PurePath:
     @property
     def options(self) -> list[str]:
         """List of options to use to mount."""
-        return ["bind"]
+        return super().options + ["bind"]

supervisor/mounts/validate.py

@@ -1,7 +1,7 @@
 """Validation for mount manager."""
 
 import re
-from typing import NotRequired, TypedDict
+from typing import Any, NotRequired, TypedDict
 
 import voluptuous as vol
 
@@ -18,6 +18,7 @@
     ATTR_DEFAULT_BACKUP_MOUNT,
     ATTR_MOUNTS,
     ATTR_PATH,
+    ATTR_READ_ONLY,
     ATTR_SERVER,
     ATTR_SHARE,
     ATTR_USAGE,
@@ -26,6 +27,18 @@
     MountUsage,
 )
 
+
+def usage_specific_validation(config: dict[str, Any]) -> dict[str, Any]:
+    """Validate config based on usage."""
+    # Backup mounts cannot be read only
+    if config[ATTR_USAGE] == MountUsage.BACKUP and config[ATTR_READ_ONLY]:
+        raise vol.Invalid("Backup mounts cannot be read only")
+
+    return config
+
+
+# pylint: disable=no-value-for-parameter
+
 RE_MOUNT_NAME = re.compile(r"^[A-Za-z0-9_]+$")
 RE_PATH_PART = re.compile(r"^[^\\\/]+")
 RE_MOUNT_OPTION = re.compile(r"^[^,=]+$")
@@ -41,6 +54,7 @@
             vol.In([MountType.CIFS.value, MountType.NFS.value]), vol.Coerce(MountType)
         ),
         vol.Required(ATTR_USAGE): vol.Coerce(MountUsage),
+        vol.Optional(ATTR_READ_ONLY, default=False): vol.Boolean(),
     },
     extra=vol.REMOVE_EXTRA,
 )
@@ -71,7 +85,9 @@
     }
 )
 
-SCHEMA_MOUNT_CONFIG = vol.Any(SCHEMA_MOUNT_CIFS, SCHEMA_MOUNT_NFS)
+SCHEMA_MOUNT_CONFIG = vol.All(
+    vol.Any(SCHEMA_MOUNT_CIFS, SCHEMA_MOUNT_NFS), usage_specific_validation
+)
 
 SCHEMA_MOUNTS_CONFIG = vol.Schema(
     {
@@ -86,6 +102,7 @@ class MountData(TypedDict):
 
     name: str
     type: str
+    read_only: bool
     usage: NotRequired[str]
 
     # CIFS and NFS fields

tests/api/test_backups.py

@@ -90,13 +90,14 @@ async def test_backup_to_location(
 
     coresys.core.state = CoreState.RUNNING
     coresys.hardware.disk.get_disk_free_space = lambda x: 5000
-    resp = await api_client.post(
-        "/backups/new/full",
-        json={
-            "name": "Mount test",
-            "location": location,
-        },
-    )
+    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,
+            },
+        )
     result = await resp.json()
     assert result["result"] == "ok"
     slug = result["data"]["slug"]
@@ -134,10 +135,11 @@ async def test_backup_to_default(
 
     coresys.core.state = CoreState.RUNNING
     coresys.hardware.disk.get_disk_free_space = lambda x: 5000
-    resp = await api_client.post(
-        "/backups/new/full",
-        json={"name": "Mount test"},
-    )
+    with patch("supervisor.mounts.mount.Path.is_mount", return_value=True):
+        resp = await api_client.post(
+            "/backups/new/full",
+            json={"name": "Mount test"},
+        )
     result = await resp.json()
     assert result["result"] == "ok"
     slug = result["data"]["slug"]

tests/api/test_mounts.py

@@ -79,6 +79,7 @@ async def test_api_create_mount(
             "server": "backup.local",
             "share": "backups",
             "state": "active",
+            "read_only": False,
         }
     ]
     coresys.mounts.save_data.assert_called_once()
@@ -253,6 +254,7 @@ async def test_api_update_mount(
             "server": "backup.local",
             "share": "new_backups",
             "state": "active",
+            "read_only": False,
         }
     ]
     coresys.mounts.save_data.assert_called_once()
@@ -320,6 +322,7 @@ async def test_api_update_dbus_error_mount_remains(
             "server": "backup.local",
             "share": "backups",
             "state": None,
+            "read_only": False,
         }
     ]
 
@@ -366,6 +369,7 @@ async def test_api_update_dbus_error_mount_remains(
             "server": "backup.local",
             "share": "backups",
             "state": None,
+            "read_only": False,
         }
     ]
 
@@ -776,3 +780,109 @@ async def test_api_create_mount_fails_special_chars(
     result = await resp.json()
     assert result["result"] == "error"
     assert "does not match regular expression" in result["message"]
+
+
+async def test_api_create_read_only_cifs_mount(
+    api_client: TestClient,
+    coresys: CoreSys,
+    tmp_supervisor_data,
+    path_extern,
+    mount_propagation,
+):
+    """Test creating a read-only cifs mount via API."""
+    resp = await api_client.post(
+        "/mounts",
+        json={
+            "name": "media_test",
+            "type": "cifs",
+            "usage": "media",
+            "server": "media.local",
+            "share": "media",
+            "version": "2.0",
+            "read_only": True,
+        },
+    )
+    result = await resp.json()
+    assert result["result"] == "ok"
+
+    resp = await api_client.get("/mounts")
+    result = await resp.json()
+
+    assert result["data"]["mounts"] == [
+        {
+            "version": "2.0",
+            "name": "media_test",
+            "type": "cifs",
+            "usage": "media",
+            "server": "media.local",
+            "share": "media",
+            "state": "active",
+            "read_only": True,
+        }
+    ]
+    coresys.mounts.save_data.assert_called_once()
+
+
+async def test_api_create_read_only_nfs_mount(
+    api_client: TestClient,
+    coresys: CoreSys,
+    tmp_supervisor_data,
+    path_extern,
+    mount_propagation,
+):
+    """Test creating a read-only nfs mount via API."""
+    resp = await api_client.post(
+        "/mounts",
+        json={
+            "name": "media_test",
+            "type": "nfs",
+            "usage": "media",
+            "server": "media.local",
+            "path": "/media/camera",
+            "read_only": True,
+        },
+    )
+    result = await resp.json()
+    assert result["result"] == "ok"
+
+    resp = await api_client.get("/mounts")
+    result = await resp.json()
+
+    assert result["data"]["mounts"] == [
+        {
+            "name": "media_test",
+            "type": "nfs",
+            "usage": "media",
+            "server": "media.local",
+            "path": "/media/camera",
+            "state": "active",
+            "read_only": True,
+        }
+    ]
+    coresys.mounts.save_data.assert_called_once()
+
+
+async def test_api_read_only_backup_mount_invalid(
+    api_client: TestClient,
+    coresys: CoreSys,
+    tmp_supervisor_data,
+    path_extern,
+    mount_propagation,
+):
+    """Test cannot create a read-only backup mount."""
+    resp = await api_client.post(
+        "/mounts",
+        json={
+            "name": "backup_test",
+            "type": "cifs",
+            "usage": "backup",
+            "server": "backup.local",
+            "share": "backup",
+            "version": "2.0",
+            "read_only": True,
+        },
+    )
+    assert resp.status == 400
+    result = await resp.json()
+    assert result["result"] == "error"
+    assert "Backup mounts cannot be read only" in result["message"]