From 73d795e05efdebd6c49bfbd22384a9686b6919b8 Mon Sep 17 00:00:00 2001
From: Mike Degatano <michael.degatano@gmail.com>
Date: Tue, 30 May 2023 17:29:51 -0400
Subject: [PATCH] Improve handling of NFS mounts and backup manager errors
 (#4323)

---
 supervisor/backups/manager.py | 12 +++++++++++-
 supervisor/mounts/mount.py    |  5 +++++
 tests/backups/test_manager.py | 34 ++++++++++++++++++++++++++++++++++
 tests/mounts/test_manager.py  |  2 ++
 tests/mounts/test_mount.py    |  4 ++--
 5 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/supervisor/backups/manager.py b/supervisor/backups/manager.py
index 20358e303..a3e14cc49 100644
--- a/supervisor/backups/manager.py
+++ b/supervisor/backups/manager.py
@@ -2,6 +2,7 @@
 from __future__ import annotations
 
 import asyncio
+from collections.abc import Iterable
 import logging
 from pathlib import Path
 
@@ -29,6 +30,15 @@ from .validate import ALL_FOLDERS, SCHEMA_BACKUPS_CONFIG
 _LOGGER: logging.Logger = logging.getLogger(__name__)
 
 
+def _list_backup_files(path: Path) -> Iterable[Path]:
+    """Return iterable of backup files, suppress and log OSError for network mounts."""
+    try:
+        return path.glob("*.tar")
+    except OSError as err:
+        _LOGGER.error("Could not list backups from %s: %s", path.as_posix(), err)
+        return []
+
+
 class BackupManager(FileConfiguration, CoreSysAttributes):
     """Manage backups."""
 
@@ -119,7 +129,7 @@ class BackupManager(FileConfiguration, CoreSysAttributes):
         tasks = [
             self.sys_create_task(_load_backup(tar_file))
             for path in self.backup_locations
-            for tar_file in path.glob("*.tar")
+            for tar_file in _list_backup_files(path)
         ]
 
         _LOGGER.info("Found %d backup files", len(tasks))
diff --git a/supervisor/mounts/mount.py b/supervisor/mounts/mount.py
index a032129d5..e08a00e4b 100644
--- a/supervisor/mounts/mount.py
+++ b/supervisor/mounts/mount.py
@@ -381,6 +381,11 @@ class NFSMount(NetworkMount):
         """What to mount."""
         return f"{self.server}:{self.path.as_posix()}"
 
+    @property
+    def options(self) -> list[str]:
+        """Options to use to mount."""
+        return super().options + ["soft", "timeo=200"]
+
 
 class BindMount(Mount):
     """A bind type mount."""
diff --git a/tests/backups/test_manager.py b/tests/backups/test_manager.py
index c9f559669..55b74ccc6 100644
--- a/tests/backups/test_manager.py
+++ b/tests/backups/test_manager.py
@@ -5,6 +5,7 @@ from unittest.mock import AsyncMock, MagicMock, Mock, PropertyMock, patch
 
 from awesomeversion import AwesomeVersion
 from dbus_fast import DBusError
+import pytest
 
 from supervisor.addons.addon import Addon
 from supervisor.backups.backup import Backup
@@ -662,3 +663,36 @@ async def test_backup_to_default(
         )
 
     assert (mount_dir / f"{backup.slug}.tar").exists()
+
+
+async def test_load_network_error(
+    coresys: CoreSys,
+    caplog: pytest.LogCaptureFixture,
+    tmp_supervisor_data,
+    path_extern,
+    mount_propagation,
+):
+    """Test load of backup manager when there is a network error."""
+    (coresys.config.path_mounts / "backup_test").mkdir()
+    await coresys.mounts.load()
+    mount = Mount.from_dict(
+        coresys,
+        {
+            "name": "backup_test",
+            "usage": "backup",
+            "type": "cifs",
+            "server": "test.local",
+            "share": "test",
+        },
+    )
+    await coresys.mounts.create_mount(mount)
+    caplog.clear()
+
+    # This should not raise, manager should just ignore backup locations with errors
+    mock_path = MagicMock()
+    mock_path.glob.side_effect = OSError("Host is down")
+    mock_path.as_posix.return_value = "/data/backup_test"
+    with patch.object(Mount, "local_where", new=PropertyMock(return_value=mock_path)):
+        await coresys.backups.load()
+
+    assert "Could not list backups from /data/backup_test" in caplog.text
diff --git a/tests/mounts/test_manager.py b/tests/mounts/test_manager.py
index a4e951b9d..839a04ae3 100644
--- a/tests/mounts/test_manager.py
+++ b/tests/mounts/test_manager.py
@@ -126,6 +126,7 @@ async def test_load(
             "mnt-data-supervisor-mounts-media_test.mount",
             "fail",
             [
+                ["Options", Variant("s", "soft,timeo=200")],
                 ["Type", Variant("s", "nfs")],
                 ["Description", Variant("s", "Supervisor nfs mount: media_test")],
                 ["What", Variant("s", "media.local:/media")],
@@ -189,6 +190,7 @@ async def test_load_share_mount(
             "mnt-data-supervisor-mounts-share_test.mount",
             "fail",
             [
+                ["Options", Variant("s", "soft,timeo=200")],
                 ["Type", Variant("s", "nfs")],
                 ["Description", Variant("s", "Supervisor nfs mount: share_test")],
                 ["What", Variant("s", "share.local:/share")],
diff --git a/tests/mounts/test_mount.py b/tests/mounts/test_mount.py
index d9aa08c75..24183686a 100644
--- a/tests/mounts/test_mount.py
+++ b/tests/mounts/test_mount.py
@@ -114,7 +114,7 @@ async def test_nfs_mount(
     assert mount.what == "test.local:/media/camera"
     assert mount.where == Path("/mnt/data/supervisor/mounts/test")
     assert mount.local_where == tmp_supervisor_data / "mounts" / "test"
-    assert mount.options == ["port=1234"]
+    assert mount.options == ["port=1234", "soft", "timeo=200"]
 
     assert not mount.local_where.exists()
     assert mount.to_dict() == mount_data
@@ -130,7 +130,7 @@ async def test_nfs_mount(
             "mnt-data-supervisor-mounts-test.mount",
             "fail",
             [
-                ["Options", Variant("s", "port=1234")],
+                ["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")],