Stop backup if pre backup failed in Core (#5203)

* Stop backup if pre backup failed in Core

* Fix API tests

* Partial backup in ci since there's no Home assistant

* Add ssl folder to partial backup

* Allow backups when Home Assistant is not running

* Undo change to skip db test

supervisor/backups/manager.py

@@ -1,4 +1,5 @@
 """Backup manager."""
+
 from __future__ import annotations
 
 import asyncio
@@ -259,11 +260,6 @@ class BackupManager(FileConfiguration, JobGroup):
             self.sys_core.state = CoreState.FREEZE
 
             async with backup:
-                # Backup add-ons
-                if addon_list:
-                    self._change_stage(BackupJobStage.ADDONS, backup)
-                    addon_start_tasks = await backup.store_addons(addon_list)
-
                 # HomeAssistant Folder is for v1
                 if homeassistant:
                     self._change_stage(BackupJobStage.HOME_ASSISTANT, backup)
@@ -273,6 +269,11 @@ class BackupManager(FileConfiguration, JobGroup):
                         else homeassistant_exclude_database
                     )
 
+                # Backup add-ons
+                if addon_list:
+                    self._change_stage(BackupJobStage.ADDONS, backup)
+                    addon_start_tasks = await backup.store_addons(addon_list)
+
                 # Backup folders
                 if folder_list:
                     self._change_stage(BackupJobStage.FOLDERS, backup)

supervisor/homeassistant/const.py

@@ -7,7 +7,9 @@ from awesomeversion import AwesomeVersion
 
 from ..const import CoreState
 
+ATTR_ERROR = "error"
 ATTR_OVERRIDE_IMAGE = "override_image"
+ATTR_SUCCESS = "success"
 LANDINGPAGE: AwesomeVersion = AwesomeVersion("landingpage")
 WATCHDOG_RETRY_SECONDS = 10
 WATCHDOG_MAX_ATTEMPTS = 5

supervisor/homeassistant/module.py

@@ -1,4 +1,5 @@
 """Home Assistant control object."""
+
 import asyncio
 from datetime import timedelta
 import errno
@@ -22,6 +23,7 @@ from ..const import (
     ATTR_BACKUPS_EXCLUDE_DATABASE,
     ATTR_BOOT,
     ATTR_IMAGE,
+    ATTR_MESSAGE,
     ATTR_PORT,
     ATTR_REFRESH_TOKEN,
     ATTR_SSL,
@@ -48,7 +50,7 @@ from ..utils import remove_folder
 from ..utils.common import FileConfiguration
 from ..utils.json import read_json_file, write_json_file
 from .api import HomeAssistantAPI
-from .const import ATTR_OVERRIDE_IMAGE, LANDINGPAGE, WSType
+from .const import ATTR_ERROR, ATTR_OVERRIDE_IMAGE, ATTR_SUCCESS, LANDINGPAGE, WSType
 from .core import HomeAssistantCore
 from .secrets import HomeAssistantSecrets
 from .validate import SCHEMA_HASS_CONFIG
@@ -345,20 +347,37 @@ class HomeAssistant(FileConfiguration, CoreSysAttributes):
     async def begin_backup(self) -> None:
         """Inform Home Assistant a backup is beginning."""
         try:
-            await self.websocket.async_send_command({ATTR_TYPE: WSType.BACKUP_START})
-        except HomeAssistantWSError:
-            _LOGGER.warning(
-                "Preparing backup of Home Assistant Core failed. Check HA Core logs."
+            resp = await self.websocket.async_send_command(
+                {ATTR_TYPE: WSType.BACKUP_START}
+            )
+        except HomeAssistantWSError as err:
+            raise HomeAssistantBackupError(
+                "Preparing backup of Home Assistant Core failed. Check HA Core logs.",
+                _LOGGER.error,
+            ) from err
+
+        if resp and not resp.get(ATTR_SUCCESS):
+            raise HomeAssistantBackupError(
+                f"Preparing backup of Home Assistant Core failed due to: {resp.get(ATTR_ERROR, {}).get(ATTR_MESSAGE, "")}. Check HA Core logs.",
+                _LOGGER.error,
             )
 
     @Job(name="home_assistant_module_end_backup")
     async def end_backup(self) -> None:
         """Inform Home Assistant the backup is ending."""
         try:
-            await self.websocket.async_send_command({ATTR_TYPE: WSType.BACKUP_END})
+            resp = await self.websocket.async_send_command(
+                {ATTR_TYPE: WSType.BACKUP_END}
+            )
         except HomeAssistantWSError:
             _LOGGER.warning(
-                "Error during Home Assistant Core backup. Check HA Core logs."
+                "Error resuming normal operations after backup of Home Assistant Core. Check HA Core logs."
+            )
+
+        if resp and not resp.get(ATTR_SUCCESS):
+            _LOGGER.warning(
+                "Error resuming normal operations after backup of Home Assistant Core due to: %s. Check HA Core logs.",
+                resp.get(ATTR_ERROR, {}).get(ATTR_MESSAGE, ""),
             )
 
     @Job(name="home_assistant_module_backup")

tests/api/test_backups.py

@@ -348,15 +348,15 @@ async def test_api_backup_errors(
     assert job["done"] is True
     assert job["reference"] == slug
     assert job["errors"] == []
-    assert job["child_jobs"][0]["name"] == "backup_store_addons"
+    assert job["child_jobs"][0]["name"] == "backup_store_homeassistant"
     assert job["child_jobs"][0]["reference"] == slug
-    assert job["child_jobs"][0]["child_jobs"][0]["name"] == "backup_addon_save"
-    assert job["child_jobs"][0]["child_jobs"][0]["reference"] == "local_ssh"
-    assert job["child_jobs"][0]["child_jobs"][0]["errors"] == [
+    assert job["child_jobs"][1]["name"] == "backup_store_addons"
+    assert job["child_jobs"][1]["reference"] == slug
+    assert job["child_jobs"][1]["child_jobs"][0]["name"] == "backup_addon_save"
+    assert job["child_jobs"][1]["child_jobs"][0]["reference"] == "local_ssh"
+    assert job["child_jobs"][1]["child_jobs"][0]["errors"] == [
         {"type": "BackupError", "message": "Can't create backup for local_ssh"}
     ]
-    assert job["child_jobs"][1]["name"] == "backup_store_homeassistant"
-    assert job["child_jobs"][1]["reference"] == slug
     assert job["child_jobs"][2]["name"] == "backup_store_folders"
     assert job["child_jobs"][2]["reference"] == slug
     assert {j["reference"] for j in job["child_jobs"][2]["child_jobs"]} == {
@@ -366,9 +366,14 @@ async def test_api_backup_errors(
         "ssl",
     }
 
-    with patch.object(
-        HomeAssistant, "backup", side_effect=HomeAssistantBackupError("Backup error")
-    ), patch.object(Addon, "backup"):
+    with (
+        patch.object(
+            HomeAssistant,
+            "backup",
+            side_effect=HomeAssistantBackupError("Backup error"),
+        ),
+        patch.object(Addon, "backup"),
+    ):
         resp = await api_client.post(
             f"/backups/new/{backup_type}",
             json={"name": f"{backup_type} backup"} | options,
@@ -384,10 +389,9 @@ async def test_api_backup_errors(
     assert job["errors"] == (
         err := [{"type": "HomeAssistantBackupError", "message": "Backup error"}]
     )
-    assert job["child_jobs"][0]["name"] == "backup_store_addons"
-    assert job["child_jobs"][1]["name"] == "backup_store_homeassistant"
-    assert job["child_jobs"][1]["errors"] == err
-    assert len(job["child_jobs"]) == 2
+    assert job["child_jobs"][0]["name"] == "backup_store_homeassistant"
+    assert job["child_jobs"][0]["errors"] == err
+    assert len(job["child_jobs"]) == 1
 
 
 async def test_backup_immediate_errors(api_client: TestClient, coresys: CoreSys):
@@ -426,14 +430,17 @@ async def test_restore_immediate_errors(
     assert resp.status == 400
     assert "only a partial backup" in (await resp.json())["message"]
 
-    with patch.object(
-        Backup,
-        "supervisor_version",
-        new=PropertyMock(return_value=AwesomeVersion("2024.01.0")),
-    ), patch.object(
-        Supervisor,
-        "version",
-        new=PropertyMock(return_value=AwesomeVersion("2023.12.0")),
+    with (
+        patch.object(
+            Backup,
+            "supervisor_version",
+            new=PropertyMock(return_value=AwesomeVersion("2024.01.0")),
+        ),
+        patch.object(
+            Supervisor,
+            "version",
+            new=PropertyMock(return_value=AwesomeVersion("2023.12.0")),
+        ),
     ):
         resp = await api_client.post(
             f"/backups/{mock_partial_backup.slug}/restore/partial",
@@ -442,9 +449,10 @@ async def test_restore_immediate_errors(
     assert resp.status == 400
     assert "Must update supervisor" in (await resp.json())["message"]
 
-    with patch.object(
-        Backup, "protected", new=PropertyMock(return_value=True)
-    ), patch.object(Backup, "set_password", return_value=False):
+    with (
+        patch.object(Backup, "protected", new=PropertyMock(return_value=True)),
+        patch.object(Backup, "set_password", return_value=False),
+    ):
         resp = await api_client.post(
             f"/backups/{mock_partial_backup.slug}/restore/partial",
             json={"background": True, "homeassistant": True},

tests/backups/test_manager.py

@@ -31,6 +31,7 @@ from supervisor.exceptions import (
     DockerError,
 )
 from supervisor.homeassistant.api import HomeAssistantAPI
+from supervisor.homeassistant.const import WSType
 from supervisor.homeassistant.core import HomeAssistantCore
 from supervisor.homeassistant.module import HomeAssistant
 from supervisor.jobs.const import JobCondition
@@ -335,9 +336,14 @@ async def test_fail_invalid_full_backup(
 
     backup_instance.protected = False
     backup_instance.supervisor_version = "2022.08.4"
-    with patch.object(
-        type(coresys.supervisor), "version", new=PropertyMock(return_value="2022.08.3")
-    ), pytest.raises(BackupInvalidError):
+    with (
+        patch.object(
+            type(coresys.supervisor),
+            "version",
+            new=PropertyMock(return_value="2022.08.3"),
+        ),
+        pytest.raises(BackupInvalidError),
+    ):
         await manager.do_restore_full(backup_instance)
 
 
@@ -364,9 +370,14 @@ async def test_fail_invalid_partial_backup(
         await manager.do_restore_partial(backup_instance, homeassistant=True)
 
     backup_instance.supervisor_version = "2022.08.4"
-    with patch.object(
-        type(coresys.supervisor), "version", new=PropertyMock(return_value="2022.08.3")
-    ), pytest.raises(BackupInvalidError):
+    with (
+        patch.object(
+            type(coresys.supervisor),
+            "version",
+            new=PropertyMock(return_value="2022.08.3"),
+        ),
+        pytest.raises(BackupInvalidError),
+    ):
         await manager.do_restore_partial(backup_instance)
 
 
@@ -766,7 +777,11 @@ async def test_backup_to_local_with_default(
 
 
 async def test_backup_to_default(
-    coresys: CoreSys, tmp_supervisor_data, path_extern, mount_propagation, mock_is_mount
+    coresys: CoreSys,
+    tmp_supervisor_data,
+    path_extern,
+    mount_propagation,
+    mock_is_mount,
 ):
     """Test making backup to default mount."""
     # Add a default backup mount
@@ -926,9 +941,15 @@ async def test_backup_with_healthcheck(
         nonlocal _container_events_task
         _container_events_task = asyncio.create_task(container_events())
 
-    with patch.object(DockerAddon, "run", new=container_events_task), patch.object(
-        AddonModel, "backup_mode", new=PropertyMock(return_value=AddonBackupMode.COLD)
-    ), patch.object(DockerAddon, "is_running", side_effect=[True, False, False]):
+    with (
+        patch.object(DockerAddon, "run", new=container_events_task),
+        patch.object(
+            AddonModel,
+            "backup_mode",
+            new=PropertyMock(return_value=AddonBackupMode.COLD),
+        ),
+        patch.object(DockerAddon, "is_running", side_effect=[True, False, False]),
+    ):
         backup = await coresys.backups.do_backup_partial(
             homeassistant=False, addons=["local_ssh"]
         )
@@ -1000,10 +1021,11 @@ async def test_restore_with_healthcheck(
         nonlocal _container_events_task
         _container_events_task = asyncio.create_task(container_events())
 
-    with patch.object(DockerAddon, "run", new=container_events_task), patch.object(
-        DockerAddon, "is_running", return_value=False
-    ), patch.object(AddonModel, "_validate_availability"), patch.object(
-        Addon, "with_ingress", new=PropertyMock(return_value=False)
+    with (
+        patch.object(DockerAddon, "run", new=container_events_task),
+        patch.object(DockerAddon, "is_running", return_value=False),
+        patch.object(AddonModel, "_validate_availability"),
+        patch.object(Addon, "with_ingress", new=PropertyMock(return_value=False)),
     ):
         await coresys.backups.do_restore_partial(backup, addons=["local_ssh"])
 
@@ -1054,16 +1076,22 @@ async def test_backup_progress(
     coresys.core.state = CoreState.RUNNING
     coresys.hardware.disk.get_disk_free_space = lambda x: 5000
 
-    with patch.object(
-        AddonModel, "backup_mode", new=PropertyMock(return_value=AddonBackupMode.COLD)
-    ), patch("supervisor.addons.addon.asyncio.Event.wait"):
+    with (
+        patch.object(
+            AddonModel,
+            "backup_mode",
+            new=PropertyMock(return_value=AddonBackupMode.COLD),
+        ),
+        patch("supervisor.addons.addon.asyncio.Event.wait"),
+    ):
         full_backup: Backup = await coresys.backups.do_backup_full()
     await asyncio.sleep(0)
 
     messages = [
         call.args[0]
         for call in ha_ws_client.async_send_command.call_args_list
-        if call.args[0]["data"].get("data", {}).get("name")
+        if call.args[0]["type"] == WSType.SUPERVISOR_EVENT
+        and call.args[0]["data"].get("data", {}).get("name")
         == "backup_manager_full_backup"
     ]
     assert messages == [
@@ -1075,10 +1103,10 @@ async def test_backup_progress(
         _make_backup_message_for_assert(
             reference=full_backup.slug, stage="docker_config"
         ),
-        _make_backup_message_for_assert(reference=full_backup.slug, stage="addons"),
         _make_backup_message_for_assert(
             reference=full_backup.slug, stage="home_assistant"
         ),
+        _make_backup_message_for_assert(reference=full_backup.slug, stage="addons"),
         _make_backup_message_for_assert(reference=full_backup.slug, stage="folders"),
         _make_backup_message_for_assert(
             reference=full_backup.slug, stage="finishing_file"
@@ -1100,7 +1128,8 @@ async def test_backup_progress(
     messages = [
         call.args[0]
         for call in ha_ws_client.async_send_command.call_args_list
-        if call.args[0]["data"].get("data", {}).get("name")
+        if call.args[0]["type"] == WSType.SUPERVISOR_EVENT
+        and call.args[0]["data"].get("data", {}).get("name")
         == "backup_manager_partial_backup"
     ]
     assert messages == [
@@ -1162,18 +1191,21 @@ async def test_restore_progress(
 
     # Install another addon to be uninstalled
     request.getfixturevalue("install_addon_example")
-    with patch("supervisor.addons.addon.asyncio.Event.wait"), patch.object(
-        HomeAssistant, "restore"
-    ), patch.object(HomeAssistantCore, "update"), patch.object(
-        AddonModel, "_validate_availability"
-    ), patch.object(AddonModel, "with_ingress", new=PropertyMock(return_value=False)):
+    with (
+        patch("supervisor.addons.addon.asyncio.Event.wait"),
+        patch.object(HomeAssistant, "restore"),
+        patch.object(HomeAssistantCore, "update"),
+        patch.object(AddonModel, "_validate_availability"),
+        patch.object(AddonModel, "with_ingress", new=PropertyMock(return_value=False)),
+    ):
         await coresys.backups.do_restore_full(full_backup)
     await asyncio.sleep(0)
 
     messages = [
         call.args[0]
         for call in ha_ws_client.async_send_command.call_args_list
-        if call.args[0]["data"].get("data", {}).get("name")
+        if call.args[0]["type"] == WSType.SUPERVISOR_EVENT
+        and call.args[0]["data"].get("data", {}).get("name")
         == "backup_manager_full_restore"
     ]
     assert messages == [
@@ -1242,7 +1274,8 @@ async def test_restore_progress(
     messages = [
         call.args[0]
         for call in ha_ws_client.async_send_command.call_args_list
-        if call.args[0]["data"].get("data", {}).get("name")
+        if call.args[0]["type"] == WSType.SUPERVISOR_EVENT
+        and call.args[0]["data"].get("data", {}).get("name")
         == "backup_manager_partial_restore"
     ]
     assert messages == [
@@ -1277,8 +1310,9 @@ async def test_restore_progress(
     addon_backup: Backup = await coresys.backups.do_backup_partial(addons=["local_ssh"])
 
     ha_ws_client.async_send_command.reset_mock()
-    with patch.object(AddonModel, "_validate_availability"), patch.object(
-        HomeAssistantCore, "start"
+    with (
+        patch.object(AddonModel, "_validate_availability"),
+        patch.object(HomeAssistantCore, "start"),
     ):
         await coresys.backups.do_restore_partial(addon_backup, addons=["local_ssh"])
     await asyncio.sleep(0)
@@ -1286,7 +1320,8 @@ async def test_restore_progress(
     messages = [
         call.args[0]
         for call in ha_ws_client.async_send_command.call_args_list
-        if call.args[0]["data"].get("data", {}).get("name")
+        if call.args[0]["type"] == WSType.SUPERVISOR_EVENT
+        and call.args[0]["data"].get("data", {}).get("name")
         == "backup_manager_partial_restore"
     ]
     assert messages == [
@@ -1338,10 +1373,13 @@ async def test_freeze_thaw(
     container.exec_run.return_value = (0, None)
     ha_ws_client.ha_version = AwesomeVersion("2022.1.0")
 
-    with patch.object(
-        AddonModel, "backup_pre", new=PropertyMock(return_value="pre_backup")
-    ), patch.object(
-        AddonModel, "backup_post", new=PropertyMock(return_value="post_backup")
+    with (
+        patch.object(
+            AddonModel, "backup_pre", new=PropertyMock(return_value="pre_backup")
+        ),
+        patch.object(
+            AddonModel, "backup_post", new=PropertyMock(return_value="post_backup")
+        ),
     ):
         # Run the freeze
         await coresys.backups.freeze_all()
@@ -1465,11 +1503,12 @@ async def test_restore_only_reloads_ingress_on_change(
     async def mock_is_running(*_) -> bool:
         return True
 
-    with patch.object(
-        HomeAssistantCore, "is_running", new=mock_is_running
-    ), patch.object(AddonModel, "_validate_availability"), patch.object(
-        DockerAddon, "attach"
-    ), patch.object(HomeAssistantAPI, "make_request") as make_request:
+    with (
+        patch.object(HomeAssistantCore, "is_running", new=mock_is_running),
+        patch.object(AddonModel, "_validate_availability"),
+        patch.object(DockerAddon, "attach"),
+        patch.object(HomeAssistantAPI, "make_request") as make_request,
+    ):
         make_request.return_value.__aenter__.return_value.status = 200
 
         # Has ingress before and after - not called
@@ -1518,8 +1557,9 @@ async def test_restore_new_addon(
     await coresys.addons.uninstall("local_example")
     assert "local_example" not in coresys.addons.local
 
-    with patch.object(AddonModel, "_validate_availability"), patch.object(
-        DockerAddon, "attach"
+    with (
+        patch.object(AddonModel, "_validate_availability"),
+        patch.object(DockerAddon, "attach"),
     ):
         assert await coresys.backups.do_restore_partial(
             backup, addons=["local_example"]
@@ -1554,8 +1594,9 @@ async def test_restore_preserves_data_config(
     assert install_addon_example.path_config.exists()
     assert test_config2.exists()
 
-    with patch.object(AddonModel, "_validate_availability"), patch.object(
-        DockerAddon, "attach"
+    with (
+        patch.object(AddonModel, "_validate_availability"),
+        patch.object(DockerAddon, "attach"),
     ):
         assert await coresys.backups.do_restore_partial(
             backup, addons=["local_example"]
@@ -1660,8 +1701,9 @@ async def test_skip_homeassistant_database(
     write_json_file(test_db, {"hello": "world"})
     write_json_file(test_db_wal, {"hello": "world"})
 
-    with patch.object(HomeAssistantCore, "update"), patch.object(
-        HomeAssistantCore, "start"
+    with (
+        patch.object(HomeAssistantCore, "update"),
+        patch.object(HomeAssistantCore, "start"),
     ):
         await coresys.backups.do_restore_partial(backup, homeassistant=True)
 
@@ -1735,8 +1777,9 @@ 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=[]
+    with (
+        patch("supervisor.backups.manager.Path.is_dir", new=mock_is_dir),
+        patch("supervisor.backups.manager.Path.glob", return_value=[]),
     ):
         err.errno = errno.EBUSY
         await coresys.backups.reload()
@@ -1787,3 +1830,39 @@ async def test_monitoring_after_partial_restore(
     backup_instance.restore_addons.assert_called_once_with([TEST_ADDON_SLUG])
     assert coresys.core.state == CoreState.RUNNING
     coresys.docker.unload.assert_not_called()
+
+
+@pytest.mark.parametrize(
+    "pre_backup_error",
+    [
+        {
+            "code": "pre_backup_actions_failed",
+            "message": "Database migration in progress",
+        },
+        {"code": "unknown_command", "message": "Unknown command."},
+    ],
+)
+async def test_core_pre_backup_actions_failed(
+    coresys: CoreSys,
+    ha_ws_client: AsyncMock,
+    caplog: pytest.LogCaptureFixture,
+    pre_backup_error: dict[str, str],
+    tmp_supervisor_data,
+    path_extern,
+):
+    """Test pre-backup actions failed in HA core stops backup."""
+    coresys.core.state = CoreState.RUNNING
+    coresys.hardware.disk.get_disk_free_space = lambda x: 5000
+    ha_ws_client.ha_version = AwesomeVersion("2024.7.0")
+    ha_ws_client.async_send_command.return_value = {
+        "error": pre_backup_error,
+        "id": 1,
+        "success": False,
+        "type": "result",
+    }
+
+    assert not await coresys.backups.do_backup_full()
+    assert (
+        f"Preparing backup of Home Assistant Core failed due to: {pre_backup_error['message']}"
+        in caplog.text
+    )