From 34939cfe521d2b096f66206597722d5c8e148b6e Mon Sep 17 00:00:00 2001 From: Stefan Agner Date: Wed, 19 Feb 2025 16:00:17 +0100 Subject: [PATCH] Remove I/O in event loop for backup load, import and remove (#5647) * Avoid IO in event loop when removing backup * Refactor backup size calculation Currently size is lazy loaded when required via properties. This however is blocking the async event loop. Backup sizes don't change. Instead of lazy loading the size of a backup simply determine it on loading/after creation. * Fix tests for backup size change * Avoid IO in event loop when loading backups * Avoid IO in event loop when importing a backup --- supervisor/api/backups.py | 6 +-- supervisor/api/const.py | 1 - supervisor/backups/backup.py | 46 ++++++++-------- supervisor/backups/manager.py | 27 +++++++--- supervisor/const.py | 1 + supervisor/misc/tasks.py | 2 +- .../fixups/system_clear_full_backup.py | 2 +- tests/api/test_backups.py | 54 +++++++++++-------- tests/backups/test_backup.py | 8 +-- tests/backups/test_manager.py | 22 ++++---- tests/conftest.py | 8 ++- 11 files changed, 102 insertions(+), 75 deletions(-) diff --git a/supervisor/api/backups.py b/supervisor/api/backups.py index b4dbc3e54..371312950 100644 --- a/supervisor/api/backups.py +++ b/supervisor/api/backups.py @@ -39,6 +39,7 @@ from ..const import ( ATTR_PROTECTED, ATTR_REPOSITORIES, ATTR_SIZE, + ATTR_SIZE_BYTES, ATTR_SLUG, ATTR_SUPERVISOR_VERSION, ATTR_TIMEOUT, @@ -58,7 +59,6 @@ from .const import ( ATTR_BACKGROUND, ATTR_LOCATION_ATTRIBUTES, ATTR_LOCATIONS, - ATTR_SIZE_BYTES, CONTENT_TYPE_TAR, ) from .utils import api_process, api_validate @@ -155,7 +155,7 @@ class APIBackups(CoreSysAttributes): return { loc if loc else LOCATION_LOCAL: { ATTR_PROTECTED: backup.all_locations[loc][ATTR_PROTECTED], - ATTR_SIZE_BYTES: backup.location_size(loc), + ATTR_SIZE_BYTES: backup.all_locations[loc][ATTR_SIZE_BYTES], } for loc in backup.locations } @@ -457,7 +457,7 @@ class APIBackups(CoreSysAttributes): else: self._validate_cloud_backup_location(request, backup.location) - self.sys_backups.remove(backup, locations=locations) + await self.sys_backups.remove(backup, locations=locations) @api_process async def download(self, request: web.Request): diff --git a/supervisor/api/const.py b/supervisor/api/const.py index aa89b0a20..75d939680 100644 --- a/supervisor/api/const.py +++ b/supervisor/api/const.py @@ -60,7 +60,6 @@ ATTR_REVISION = "revision" ATTR_SAFE_MODE = "safe_mode" ATTR_SEAT = "seat" ATTR_SIGNED = "signed" -ATTR_SIZE_BYTES = "size_bytes" ATTR_STARTUP_TIME = "startup_time" ATTR_STATUS = "status" ATTR_SUBSYSTEM = "subsystem" diff --git a/supervisor/backups/backup.py b/supervisor/backups/backup.py index c1ba7b4fc..726327b07 100644 --- a/supervisor/backups/backup.py +++ b/supervisor/backups/backup.py @@ -6,7 +6,6 @@ from collections.abc import AsyncGenerator, Awaitable from contextlib import asynccontextmanager from copy import deepcopy from datetime import timedelta -from functools import lru_cache import io import json import logging @@ -40,6 +39,7 @@ from ..const import ( ATTR_PROTECTED, ATTR_REPOSITORIES, ATTR_SIZE, + ATTR_SIZE_BYTES, ATTR_SLUG, ATTR_SUPERVISOR_VERSION, ATTR_TYPE, @@ -67,12 +67,6 @@ from .validate import SCHEMA_BACKUP _LOGGER: logging.Logger = logging.getLogger(__name__) -@lru_cache -def _backup_file_size(backup: Path) -> int: - """Get backup file size.""" - return backup.stat().st_size if backup.is_file() else 0 - - def location_sort_key(value: str | None) -> str: """Sort locations, None is always first else alphabetical.""" return value if value else "" @@ -88,6 +82,7 @@ class Backup(JobGroup): slug: str, location: str | None, data: dict[str, Any] | None = None, + size_bytes: int = 0, ): """Initialize a backup.""" super().__init__( @@ -102,6 +97,7 @@ class Backup(JobGroup): location: { ATTR_PATH: tar_file, ATTR_PROTECTED: data.get(ATTR_PROTECTED, False) if data else False, + ATTR_SIZE_BYTES: size_bytes, } } @@ -236,7 +232,7 @@ class Backup(JobGroup): @property def size_bytes(self) -> int: """Return backup size in bytes.""" - return self.location_size(self.location) + return self._locations[self.location][ATTR_SIZE_BYTES] @property def is_new(self) -> bool: @@ -260,14 +256,6 @@ class Backup(JobGroup): """Returns a copy of the data.""" return deepcopy(self._data) - def location_size(self, location: str | None) -> int: - """Get size of backup in a location.""" - if location not in self.all_locations: - return 0 - - backup = self.all_locations[location][ATTR_PATH] - return _backup_file_size(backup) - def __eq__(self, other: Any) -> bool: """Return true if backups have same metadata.""" if not isinstance(other, Backup): @@ -416,23 +404,24 @@ class Backup(JobGroup): async def load(self): """Read backup.json from tar file.""" - if not self.tarfile.is_file(): - _LOGGER.error("No tarfile located at %s", self.tarfile) - return False - def _load_file(): - """Read backup.json.""" - with tarfile.open(self.tarfile, "r:") as backup: + def _load_file(tarfile_path: Path): + """Get backup size and read backup metadata.""" + size_bytes = tarfile_path.stat().st_size + with tarfile.open(tarfile_path, "r:") as backup: if "./snapshot.json" in [entry.name for entry in backup.getmembers()]: # Old backups stil uses "snapshot.json", we need to support that forever json_file = backup.extractfile("./snapshot.json") else: json_file = backup.extractfile("./backup.json") - return json_file.read() + return size_bytes, json_file.read() # read backup.json try: - raw = await self.sys_run_in_executor(_load_file) + size_bytes, raw = await self.sys_run_in_executor(_load_file, self.tarfile) + except FileNotFoundError: + _LOGGER.error("No tarfile located at %s", self.tarfile) + return False except (tarfile.TarError, KeyError) as err: _LOGGER.error("Can't read backup tarfile %s: %s", self.tarfile, err) return False @@ -457,6 +446,7 @@ class Backup(JobGroup): if self._data[ATTR_PROTECTED]: self._locations[self.location][ATTR_PROTECTED] = True + self._locations[self.location][ATTR_SIZE_BYTES] = size_bytes return True @@ -480,6 +470,11 @@ class Backup(JobGroup): ) return outer_secure_tarfile, outer_secure_tarfile.open() + def _close_outer_tarfile() -> int: + """Close outer tarfile.""" + self._outer_secure_tarfile.close() + return self.tarfile.stat().st_size + self._outer_secure_tarfile, outer_tarfile = await self.sys_run_in_executor( _open_outer_tarfile ) @@ -487,7 +482,8 @@ class Backup(JobGroup): yield finally: await self._create_cleanup(outer_tarfile) - await self.sys_run_in_executor(self._outer_secure_tarfile.close) + size_bytes = await self.sys_run_in_executor(_close_outer_tarfile) + self._locations[self.location][ATTR_SIZE_BYTES] = size_bytes self._outer_secure_tarfile = None @asynccontextmanager diff --git a/supervisor/backups/manager.py b/supervisor/backups/manager.py index 212fcb4aa..a64037100 100644 --- a/supervisor/backups/manager.py +++ b/supervisor/backups/manager.py @@ -14,6 +14,7 @@ from ..const import ( ATTR_DAYS_UNTIL_STALE, ATTR_PATH, ATTR_PROTECTED, + ATTR_SIZE_BYTES, FILE_HASSIO_BACKUPS, FOLDER_HOMEASSISTANT, CoreState, @@ -258,7 +259,12 @@ class BackupManager(FileConfiguration, JobGroup): else: backups[backup.slug] = Backup( - self.coresys, tar_file, backup.slug, location_name, backup.data + self.coresys, + tar_file, + backup.slug, + location_name, + backup.data, + backup.size_bytes, ) return True @@ -272,7 +278,9 @@ class BackupManager(FileConfiguration, JobGroup): tasks = [ self.sys_create_task(_load_backup(_location, tar_file)) for _location, path in locations.items() - for tar_file in self._list_backup_files(path) + for tar_file in await self.sys_run_in_executor( + self._list_backup_files, path + ) ] _LOGGER.info("Found %d backup files", len(tasks)) @@ -305,7 +313,7 @@ class BackupManager(FileConfiguration, JobGroup): return True - def remove( + async def remove( self, backup: Backup, locations: list[LOCATION_TYPE] | None = None, @@ -324,7 +332,7 @@ class BackupManager(FileConfiguration, JobGroup): for location in targets: backup_tarfile = backup.all_locations[location][ATTR_PATH] try: - backup_tarfile.unlink() + await self.sys_run_in_executor(backup_tarfile.unlink) del backup.all_locations[location] except FileNotFoundError as err: self.sys_create_task(self.reload(location)) @@ -397,7 +405,11 @@ class BackupManager(FileConfiguration, JobGroup): backup.all_locations.update( { - loc: {ATTR_PATH: path, ATTR_PROTECTED: backup.protected} + loc: { + ATTR_PATH: path, + ATTR_PROTECTED: backup.protected, + ATTR_SIZE_BYTES: backup.size_bytes, + } for loc, path in all_new_locations.items() } ) @@ -426,8 +438,7 @@ class BackupManager(FileConfiguration, JobGroup): tar_file = Path(self._get_base_path(location), f"{backup.slug}.tar") try: - backup.tarfile.rename(tar_file) - + await self.sys_run_in_executor(backup.tarfile.rename, tar_file) except OSError as err: if err.errno == errno.EBADMSG and location in {LOCATION_CLOUD_BACKUP, None}: self.sys_resolution.unhealthy = UnhealthyReason.OSERROR_BAD_MESSAGE @@ -444,7 +455,7 @@ class BackupManager(FileConfiguration, JobGroup): ) if not await backup.load(): # Remove invalid backup from location it was moved to - backup.tarfile.unlink() + await self.sys_run_in_executor(backup.tarfile.unlink) return None _LOGGER.info("Successfully imported %s", backup.slug) diff --git a/supervisor/const.py b/supervisor/const.py index 42a7fee4a..4bfde2906 100644 --- a/supervisor/const.py +++ b/supervisor/const.py @@ -295,6 +295,7 @@ ATTR_SESSION_DATA_USER = "user" ATTR_SESSION_DATA_USER_ID = "user_id" ATTR_SIGNAL = "signal" ATTR_SIZE = "size" +ATTR_SIZE_BYTES = "size_bytes" ATTR_SLUG = "slug" ATTR_SOURCE = "source" ATTR_SQUASH = "squash" diff --git a/supervisor/misc/tasks.py b/supervisor/misc/tasks.py index 37af8a9de..46b847e3d 100644 --- a/supervisor/misc/tasks.py +++ b/supervisor/misc/tasks.py @@ -370,6 +370,6 @@ class Tasks(CoreSysAttributes): ] for backup in old_backups: try: - self.sys_backups.remove(backup, [LOCATION_CLOUD_BACKUP]) + await self.sys_backups.remove(backup, [LOCATION_CLOUD_BACKUP]) except BackupFileNotFoundError as err: _LOGGER.debug("Can't remove backup %s: %s", backup.slug, err) diff --git a/supervisor/resolution/fixups/system_clear_full_backup.py b/supervisor/resolution/fixups/system_clear_full_backup.py index 6d09989de..79a0a1dcf 100644 --- a/supervisor/resolution/fixups/system_clear_full_backup.py +++ b/supervisor/resolution/fixups/system_clear_full_backup.py @@ -34,7 +34,7 @@ class FixupSystemClearFullBackup(FixupBase): : -1 * MINIMUM_FULL_BACKUPS ]: try: - self.sys_backups.remove(backup) + await self.sys_backups.remove(backup) except BackupFileNotFoundError as err: _LOGGER.debug("Can't remove backup %s: %s", backup.slug, err) diff --git a/tests/api/test_backups.py b/tests/api/test_backups.py index 39fc8758b..1658fe46b 100644 --- a/tests/api/test_backups.py +++ b/tests/api/test_backups.py @@ -571,7 +571,9 @@ async def test_cloud_backup_core_only(api_client: TestClient, mock_full_backup: assert resp.status == 403 # pylint: disable-next=protected-access - mock_full_backup._locations = {".cloud_backup": {"path": None, "protected": False}} + mock_full_backup._locations = { + ".cloud_backup": {"path": None, "protected": False, "size_bytes": 10240} + } assert mock_full_backup.location == ".cloud_backup" resp = await api_client.post(f"/backups/{mock_full_backup.slug}/restore/full") @@ -650,8 +652,8 @@ async def test_backup_to_multiple_locations( assert orig_backup.exists() assert copy_backup.exists() assert coresys.backups.get(slug).all_locations == { - None: {"path": orig_backup, "protected": False}, - ".cloud_backup": {"path": copy_backup, "protected": False}, + None: {"path": orig_backup, "protected": False, "size_bytes": 10240}, + ".cloud_backup": {"path": copy_backup, "protected": False, "size_bytes": 10240}, } assert coresys.backups.get(slug).location is None @@ -711,8 +713,8 @@ async def test_upload_to_multiple_locations( assert orig_backup.exists() assert copy_backup.exists() assert coresys.backups.get("7fed74c8").all_locations == { - None: {"path": orig_backup, "protected": False}, - ".cloud_backup": {"path": copy_backup, "protected": False}, + None: {"path": orig_backup, "protected": False, "size_bytes": 10240}, + ".cloud_backup": {"path": copy_backup, "protected": False, "size_bytes": 10240}, } assert coresys.backups.get("7fed74c8").location is None @@ -726,7 +728,7 @@ async def test_upload_duplicate_backup_new_location( orig_backup = Path(copy(backup_file, coresys.config.path_backup)) await coresys.backups.reload() assert coresys.backups.get("7fed74c8").all_locations == { - None: {"path": orig_backup, "protected": False} + None: {"path": orig_backup, "protected": False, "size_bytes": 10240} } with backup_file.open("rb") as file, MultipartWriter("form-data") as mp: @@ -743,8 +745,8 @@ async def test_upload_duplicate_backup_new_location( assert orig_backup.exists() assert copy_backup.exists() assert coresys.backups.get("7fed74c8").all_locations == { - None: {"path": orig_backup, "protected": False}, - ".cloud_backup": {"path": copy_backup, "protected": False}, + None: {"path": orig_backup, "protected": False, "size_bytes": 10240}, + ".cloud_backup": {"path": copy_backup, "protected": False, "size_bytes": 10240}, } assert coresys.backups.get("7fed74c8").location is None @@ -781,7 +783,7 @@ async def test_upload_with_filename( orig_backup = coresys.config.path_backup / filename assert orig_backup.exists() assert coresys.backups.get("7fed74c8").all_locations == { - None: {"path": orig_backup, "protected": False} + None: {"path": orig_backup, "protected": False, "size_bytes": 10240} } assert coresys.backups.get("7fed74c8").location is None @@ -814,8 +816,8 @@ async def test_remove_backup_from_location(api_client: TestClient, coresys: Core await coresys.backups.reload() assert (backup := coresys.backups.get("7fed74c8")) assert backup.all_locations == { - None: {"path": location_1, "protected": False}, - ".cloud_backup": {"path": location_2, "protected": False}, + None: {"path": location_1, "protected": False, "size_bytes": 10240}, + ".cloud_backup": {"path": location_2, "protected": False, "size_bytes": 10240}, } resp = await api_client.delete( @@ -826,7 +828,9 @@ async def test_remove_backup_from_location(api_client: TestClient, coresys: Core assert location_1.exists() assert not location_2.exists() assert coresys.backups.get("7fed74c8") - assert backup.all_locations == {None: {"path": location_1, "protected": False}} + assert backup.all_locations == { + None: {"path": location_1, "protected": False, "size_bytes": 10240} + } @pytest.mark.usefixtures("tmp_supervisor_data") @@ -838,7 +842,7 @@ async def test_remove_backup_file_not_found(api_client: TestClient, coresys: Cor await coresys.backups.reload() assert (backup := coresys.backups.get("7fed74c8")) assert backup.all_locations == { - None: {"path": location, "protected": False}, + None: {"path": location, "protected": False, "size_bytes": 10240}, } location.unlink() @@ -866,8 +870,8 @@ async def test_download_backup_from_location( await coresys.backups.reload() assert (backup := coresys.backups.get("7fed74c8")) assert backup.all_locations == { - None: {"path": location_1, "protected": False}, - ".cloud_backup": {"path": location_2, "protected": False}, + None: {"path": location_1, "protected": False, "size_bytes": 10240}, + ".cloud_backup": {"path": location_2, "protected": False, "size_bytes": 10240}, } # The use case of this is user might want to pick a particular mount if one is flaky @@ -981,8 +985,12 @@ async def test_restore_backup_unencrypted_after_encrypted( backup = coresys.backups.get("d9c48f8b") assert backup.all_locations == { - None: {"path": Path(enc_tar), "protected": True}, - ".cloud_backup": {"path": Path(unc_tar), "protected": False}, + None: {"path": Path(enc_tar), "protected": True, "size_bytes": 10240}, + ".cloud_backup": { + "path": Path(unc_tar), + "protected": False, + "size_bytes": 10240, + }, } # pylint: disable=fixme @@ -1095,8 +1103,12 @@ async def test_backup_mixed_encryption(api_client: TestClient, coresys: CoreSys) backup = coresys.backups.get("d9c48f8b") assert backup.all_locations == { - None: {"path": Path(enc_tar), "protected": True}, - ".cloud_backup": {"path": Path(unc_tar), "protected": False}, + None: {"path": Path(enc_tar), "protected": True, "size_bytes": 10240}, + ".cloud_backup": { + "path": Path(unc_tar), + "protected": False, + "size_bytes": 10240, + }, } resp = await api_client.get("/backups") @@ -1255,7 +1267,7 @@ async def test_missing_file_removes_location_from_cache( assert resp.status == 404 # Wait for reload task to complete and confirm location is removed - await asyncio.sleep(0) + await asyncio.sleep(0.01) assert coresys.backups.get(slug).all_locations.keys() == {None} @@ -1310,7 +1322,7 @@ async def test_missing_file_removes_backup_from_cache( assert resp.status == 404 # Wait for reload task to complete and confirm backup is removed - await asyncio.sleep(0) + await asyncio.sleep(0.01) assert not coresys.backups.list_backups diff --git a/tests/backups/test_backup.py b/tests/backups/test_backup.py index d2098cc4a..1340a5e67 100644 --- a/tests/backups/test_backup.py +++ b/tests/backups/test_backup.py @@ -47,7 +47,9 @@ async def test_consolidate_conflict_varied_encryption( f"Backup d9c48f8b exists in two files in locations None. Ignoring {enc_tar.as_posix()}" in caplog.text ) - assert enc_backup.all_locations == {None: {"path": unc_tar, "protected": False}} + assert enc_backup.all_locations == { + None: {"path": unc_tar, "protected": False, "size_bytes": 10240} + } async def test_consolidate( @@ -72,8 +74,8 @@ async def test_consolidate( not in caplog.text ) assert enc_backup.all_locations == { - None: {"path": enc_tar, "protected": True}, - "backup_test": {"path": unc_tar, "protected": False}, + None: {"path": enc_tar, "protected": True, "size_bytes": 10240}, + "backup_test": {"path": unc_tar, "protected": False, "size_bytes": 10240}, } diff --git a/tests/backups/test_manager.py b/tests/backups/test_manager.py index b11b8bfba..0154bea1d 100644 --- a/tests/backups/test_manager.py +++ b/tests/backups/test_manager.py @@ -1737,12 +1737,12 @@ async def test_backup_remove_error( err.errno = errno.EBUSY with pytest.raises(BackupError): - coresys.backups.remove(backup) + await coresys.backups.remove(backup) assert coresys.core.healthy is True err.errno = errno.EBADMSG with pytest.raises(BackupError): - coresys.backups.remove(backup) + await coresys.backups.remove(backup) assert coresys.core.healthy is healthy_expected @@ -1986,11 +1986,11 @@ async def test_backup_remove_multiple_locations(coresys: CoreSys): await coresys.backups.reload() assert (backup := coresys.backups.get("7fed74c8")) assert backup.all_locations == { - None: {"path": location_1, "protected": False}, - ".cloud_backup": {"path": location_2, "protected": False}, + None: {"path": location_1, "protected": False, "size_bytes": 10240}, + ".cloud_backup": {"path": location_2, "protected": False, "size_bytes": 10240}, } - coresys.backups.remove(backup) + await coresys.backups.remove(backup) assert not location_1.exists() assert not location_2.exists() assert not coresys.backups.get("7fed74c8") @@ -2006,15 +2006,17 @@ async def test_backup_remove_one_location_of_multiple(coresys: CoreSys): await coresys.backups.reload() assert (backup := coresys.backups.get("7fed74c8")) assert backup.all_locations == { - None: {"path": location_1, "protected": False}, - ".cloud_backup": {"path": location_2, "protected": False}, + None: {"path": location_1, "protected": False, "size_bytes": 10240}, + ".cloud_backup": {"path": location_2, "protected": False, "size_bytes": 10240}, } - coresys.backups.remove(backup, locations=[".cloud_backup"]) + await coresys.backups.remove(backup, locations=[".cloud_backup"]) assert location_1.exists() assert not location_2.exists() assert coresys.backups.get("7fed74c8") - assert backup.all_locations == {None: {"path": location_1, "protected": False}} + assert backup.all_locations == { + None: {"path": location_1, "protected": False, "size_bytes": 10240} + } @pytest.mark.usefixtures("tmp_supervisor_data") @@ -2062,4 +2064,4 @@ async def test_remove_non_existing_backup_raises( err.errno = errno.ENOENT with pytest.raises(BackupFileNotFoundError): - coresys.backups.remove(backup) + await coresys.backups.remove(backup) diff --git a/tests/conftest.py b/tests/conftest.py index aaccd8a84..5b0294e3e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -575,7 +575,9 @@ def install_addon_example(coresys: CoreSys, repository): @pytest.fixture async def mock_full_backup(coresys: CoreSys, tmp_path) -> Backup: """Mock a full backup.""" - mock_backup = Backup(coresys, Path(tmp_path, "test_backup.tar"), "test", None) + mock_backup = Backup( + coresys, Path(tmp_path, "test_backup.tar"), "test", None, None, 10240 + ) mock_backup.new("Test", utcnow().isoformat(), BackupType.FULL) mock_backup.repositories = ["https://github.com/awesome-developer/awesome-repo"] mock_backup.docker = {} @@ -600,7 +602,9 @@ async def mock_full_backup(coresys: CoreSys, tmp_path) -> Backup: @pytest.fixture async def mock_partial_backup(coresys: CoreSys, tmp_path) -> Backup: """Mock a partial backup.""" - mock_backup = Backup(coresys, Path(tmp_path, "test_backup.tar"), "test", None) + mock_backup = Backup( + coresys, Path(tmp_path, "test_backup.tar"), "test", None, None, 10240 + ) mock_backup.new("Test", utcnow().isoformat(), BackupType.PARTIAL) mock_backup.repositories = ["https://github.com/awesome-developer/awesome-repo"] mock_backup.docker = {}