From ac8dbcfc1cd86171a333d7c4788d85c5d53483be Mon Sep 17 00:00:00 2001 From: Sunil Mohan Adapa Date: Wed, 26 Mar 2025 20:01:18 -0700 Subject: [PATCH] backups: Add ability to cleanup files before restoring a backup - Many times, merging old and new data folders is not ideal and could lead to unexpected outcomes. Perhaps removing all the backup folders and files before restore is ideal. However, this patch tries to introduce that approach slowly on an experimental basis. Tests: - Unit tests work. Signed-off-by: Sunil Mohan Adapa Reviewed-by: James Valleroy --- plinth/modules/backups/components.py | 25 +++++++++++++- plinth/modules/backups/privileged.py | 17 ++++++++++ .../modules/backups/tests/test_components.py | 33 +++++++++++++++++-- 3 files changed, 72 insertions(+), 3 deletions(-) diff --git a/plinth/modules/backups/components.py b/plinth/modules/backups/components.py index 6c668587b..4a8e020b7 100644 --- a/plinth/modules/backups/components.py +++ b/plinth/modules/backups/components.py @@ -57,11 +57,21 @@ def _validate_settings(settings): assert isinstance(setting, str) +def _validate_paths(paths): + """Validate a list of files or directories.""" + if not paths: + return + + assert isinstance(paths, list) + for path in paths: + assert isinstance(path, str) + + class BackupRestore(app.FollowerComponent): """Component to backup/restore an app.""" def __init__(self, component_id, config=None, data=None, secrets=None, - services=None, settings=None): + services=None, settings=None, delete_before_restore=None): """Initialize the backup/restore component.""" super().__init__(component_id) @@ -75,6 +85,8 @@ class BackupRestore(app.FollowerComponent): self.services = services or [] _validate_settings(settings) self.settings = settings or [] + _validate_paths(delete_before_restore) + self.delete_before_restore = delete_before_restore or [] self.has_data = (bool(config) or bool(data) or bool(secrets) or bool(settings)) @@ -112,6 +124,9 @@ class BackupRestore(app.FollowerComponent): if self.settings: manifest['settings'] = self.settings + if self.delete_before_restore: + manifest['delete_before_restore'] = self.delete_before_restore + return manifest def backup_pre(self, packet): @@ -123,6 +138,7 @@ class BackupRestore(app.FollowerComponent): def restore_pre(self, packet): """Perform any special operations before restore.""" + self._files_restore_pre() def restore_post(self, packet): """Perform any special operations after restore.""" @@ -151,6 +167,13 @@ class BackupRestore(app.FollowerComponent): privileged.dump_settings(self.app_id, data) + def _files_restore_pre(self): + """Delete some files and directories before restoring.""" + if not self.delete_before_restore: + return + + privileged.delete_before_restore(self.app_id) + def _settings_restore_post(self): """Read from a file and restore keys to kvstore.""" if not self.settings: diff --git a/plinth/modules/backups/privileged.py b/plinth/modules/backups/privileged.py index 2a8118978..26691023f 100644 --- a/plinth/modules/backups/privileged.py +++ b/plinth/modules/backups/privileged.py @@ -6,12 +6,15 @@ import json import os import pathlib import re +import shutil import subprocess import tarfile from django.utils.translation import gettext_lazy as _ from plinth import action_utils +from plinth import app as app_module +from plinth import module_loader from plinth.actions import privileged, secret_str from plinth.utils import Version @@ -481,6 +484,20 @@ def load_settings(app_id: str) -> dict[str, int | float | bool | str]: return {} +@privileged +def delete_before_restore(app_id: str): + """Delete some paths before restoring an app.""" + module_loader.load_modules() + app_module.apps_init() + app = app_module.App.get(app_id) + + from plinth.modules.backups.components import BackupRestore + components = app.get_components_of_type(BackupRestore) + for component in components: + for path in component.delete_before_restore: + shutil.rmtree(path, ignore_errors=True) + + def _get_env(encryption_passphrase: str | None = None): """Create encryption and ssh kwargs out of given arguments.""" env = dict(os.environ, BORG_RELOCATED_REPO_ACCESS_IS_OK='yes', diff --git a/plinth/modules/backups/tests/test_components.py b/plinth/modules/backups/tests/test_components.py index e5cc39b96..a42829443 100644 --- a/plinth/modules/backups/tests/test_components.py +++ b/plinth/modules/backups/tests/test_components.py @@ -21,8 +21,10 @@ def fixture_backup_restore(): value = {'files': ['a', 'b'], 'directories': ['a', 'b']} services = ['service-1', {'type': 'system', 'name': 'service-2'}] settings = ['setting-1', 'setting-2'] + delete_before_restore = ['path1', 'path2'] return BackupRestore('test-backup-restore', config=value, data=value, - secrets=value, services=services, settings=settings) + secrets=value, services=services, settings=settings, + delete_before_restore=delete_before_restore) @pytest.mark.parametrize('section', [ @@ -156,6 +158,20 @@ def test_invalid_services(services): components._validate_services(services) +@pytest.mark.parametrize('paths', [ + 10, + 'invalid', + [None], + [10], + [[]], + [{}], +]) +def test_invalid_paths(paths): + """Test invalid values for paths.""" + with pytest.raises(AssertionError): + components._validate_paths(paths) + + def test_backup_restore_init_default_arguments(): """Test initialization of the backup restore object.""" component = BackupRestore('test-backup-restore') @@ -230,7 +246,6 @@ def test_backup_restore_hooks(backup_restore): """Test running hooks on backup restore object.""" packet = None backup_restore.backup_post(packet) - backup_restore.restore_pre(packet) @pytest.mark.django_db @@ -249,6 +264,20 @@ def test_backup_restore_backup_pre(dump_settings, backup_restore): dump_settings.assert_has_calls([call('testapp', {'setting-1': 'value-1'})]) +@patch('plinth.modules.backups.privileged.delete_before_restore') +def test_backup_restore_restore_pre(delete_before_restore, backup_restore): + """Test running restore-pre hook.""" + packet = None + backup_restore.app_id = 'testapp' + + component = BackupRestore('test-backup-restore') + component.restore_pre(packet) + delete_before_restore.assert_has_calls([]) + + backup_restore.restore_pre(packet) + delete_before_restore.assert_has_calls([call('testapp')]) + + @pytest.mark.django_db @patch('plinth.modules.backups.privileged.load_settings') def test_backup_restore_restore_post(load_settings, backup_restore):