Merge pull request #927 from YunoHost/fix-backup-tests

[fix] Restoration of custom hooks / missing restore hooks

src/yunohost/backup.py

@@ -219,8 +219,8 @@ class BackupManager():
         backup_manager = BackupManager(name="mybackup", description="bkp things")
 
         # Add backup method to apply
-        backup_manager.add(BackupMethod.create('copy','/mnt/local_fs'))
+        backup_manager.add(BackupMethod.create('copy', backup_manager, '/mnt/local_fs'))
-        backup_manager.add(BackupMethod.create('tar','/mnt/remote_fs'))
+        backup_manager.add(BackupMethod.create('tar', backup_manager, '/mnt/remote_fs'))
 
         # Define targets to be backuped
         backup_manager.set_system_targets(["data"])
@@ -752,7 +752,7 @@ class BackupManager():
 
         for method in self.methods:
             logger.debug(m18n.n('backup_applying_method_' + method.method_name))
-            method.mount_and_backup(self)
+            method.mount_and_backup()
             logger.debug(m18n.n('backup_method_' + method.method_name + '_finished'))
 
     def _compute_backup_size(self):
@@ -851,7 +851,7 @@ class RestoreManager():
         self.info = backup_info(name, with_details=True)
         self.archive_path = self.info['path']
         self.name = name
-        self.method = BackupMethod.create(method)
+        self.method = BackupMethod.create(method, self)
         self.targets = BackupRestoreTargetsManager()
 
     #
@@ -956,6 +956,9 @@ class RestoreManager():
         # These are the hooks on the current installation
         available_restore_system_hooks = hook_list("restore")["hooks"]
 
+        custom_restore_hook_folder = os.path.join(CUSTOM_HOOK_FOLDER, 'restore')
+        filesystem.mkdir(custom_restore_hook_folder, 755, parents=True, force=True)
+
         for system_part in target_list:
             # By default, we'll use the restore hooks on the current install
             # if available
@@ -967,24 +970,25 @@ class RestoreManager():
                 continue
 
             # Otherwise, attempt to find it (or them?) in the archive
-            hook_paths = '{:s}/hooks/restore/*-{:s}'.format(self.work_dir, system_part)
-            hook_paths = glob(hook_paths)
 
             # If we didn't find it, we ain't gonna be able to restore it
-            if len(hook_paths) == 0:
+            if system_part not in self.info['system'] or\
+                    'paths' not in self.info['system'][system_part] or\
+                    len(self.info['system'][system_part]['paths']) == 0:
                 logger.exception(m18n.n('restore_hook_unavailable', part=system_part))
                 self.targets.set_result("system", system_part, "Skipped")
                 continue
 
+            hook_paths = self.info['system'][system_part]['paths']
+            hook_paths = [ 'hooks/restore/%s' % os.path.basename(p) for p in hook_paths ]
+
             # Otherwise, add it from the archive to the system
             # FIXME: Refactor hook_add and use it instead
-            custom_restore_hook_folder = os.path.join(CUSTOM_HOOK_FOLDER, 'restore')
-            filesystem.mkdir(custom_restore_hook_folder, 755, True)
             for hook_path in hook_paths:
                 logger.debug("Adding restoration script '%s' to the system "
                              "from the backup archive '%s'", hook_path,
                              self.archive_path)
-            shutil.copy(hook_path, custom_restore_hook_folder)
+                self.method.copy(hook_path, custom_restore_hook_folder)
 
     def set_apps_targets(self, apps=[]):
         """
@@ -1044,7 +1048,7 @@ class RestoreManager():
 
         filesystem.mkdir(self.work_dir, parents=True)
 
-        self.method.mount(self)
+        self.method.mount()
 
         self._read_info_files()
 
@@ -1499,19 +1503,19 @@ class BackupMethod(object):
         method_name
 
     Public methods:
-        mount_and_backup(self, backup_manager)
+        mount_and_backup(self)
-        mount(self, restore_manager)
+        mount(self)
         create(cls, method, **kwargs)
 
     Usage:
-        method = BackupMethod.create("tar")
+        method = BackupMethod.create("tar", backup_manager)
-        method.mount_and_backup(backup_manager)
+        method.mount_and_backup()
         #or
-        method = BackupMethod.create("copy")
+        method = BackupMethod.create("copy", restore_manager)
-        method.mount(restore_manager)
+        method.mount()
     """
 
-    def __init__(self, repo=None):
+    def __init__(self, manager, repo=None):
         """
         BackupMethod constructors
 
@@ -1524,6 +1528,7 @@ class BackupMethod(object):
                    BackupRepository object. If None, the default repo is used :
                    /home/yunohost.backup/archives/
         """
+        self.manager = manager
         self.repo = ARCHIVES_PATH if repo is None else repo
 
     @property
@@ -1569,18 +1574,13 @@ class BackupMethod(object):
         """
         return False
 
-    def mount_and_backup(self, backup_manager):
+    def mount_and_backup(self):
         """
         Run the backup on files listed by  the BackupManager instance
 
         This method shouldn't be overrided, prefer overriding self.backup() and
         self.clean()
-
-        Args:
-        backup_manager -- (BackupManager) A backup manager instance that has
-                          already done the files collection step.
         """
-        self.manager = backup_manager
         if self.need_mount():
             self._organize_files()
 
@@ -1589,17 +1589,13 @@ class BackupMethod(object):
         finally:
             self.clean()
 
-    def mount(self, restore_manager):
+    def mount(self):
         """
         Mount the archive from RestoreManager instance in the working directory
 
         This method should be extended.
-
-        Args:
-            restore_manager -- (RestoreManager) A restore manager instance
-                               contains an archive to restore.
         """
-        self.manager = restore_manager
+        pass
 
     def clean(self):
         """
@@ -1744,7 +1740,7 @@ class BackupMethod(object):
                 shutil.copy(path['source'], dest)
 
     @classmethod
-    def create(cls, method, *args):
+    def create(cls, method, manager, *args):
         """
         Factory method to create instance of BackupMethod
 
@@ -1760,7 +1756,7 @@ class BackupMethod(object):
         if not isinstance(method, basestring):
             methods = []
             for m in method:
-                methods.append(BackupMethod.create(m, *args))
+                methods.append(BackupMethod.create(m, manager, *args))
             return methods
 
         bm_class = {
@@ -1769,9 +1765,9 @@ class BackupMethod(object):
             'borg': BorgBackupMethod
         }
         if method in ["copy", "tar", "borg"]:
-            return bm_class[method](*args)
+            return bm_class[method](manager, *args)
         else:
-            return CustomBackupMethod(method=method, *args)
+            return CustomBackupMethod(manager, method=method, *args)
 
 
 class CopyBackupMethod(BackupMethod):
@@ -1781,8 +1777,8 @@ class CopyBackupMethod(BackupMethod):
     could be the inverse for restoring
     """
 
-    def __init__(self, repo=None):
+    def __init__(self, manager, repo=None):
-        super(CopyBackupMethod, self).__init__(repo)
+        super(CopyBackupMethod, self).__init__(manager, repo)
 
     @property
     def method_name(self):
@@ -1836,6 +1832,9 @@ class CopyBackupMethod(BackupMethod):
                         "&&", "umount", "-R", self.work_dir])
         raise YunohostError('backup_cant_mount_uncompress_archive')
 
+    def copy(self, file, target):
+        shutil.copy(file, target)
+
 
 class TarBackupMethod(BackupMethod):
 
@@ -1843,8 +1842,8 @@ class TarBackupMethod(BackupMethod):
     This class compress all files to backup in archive.
     """
 
-    def __init__(self, repo=None):
+    def __init__(self, manager, repo=None):
-        super(TarBackupMethod, self).__init__(repo)
+        super(TarBackupMethod, self).__init__(manager, repo)
 
     @property
     def method_name(self):
@@ -1904,7 +1903,7 @@ class TarBackupMethod(BackupMethod):
         if not os.path.isfile(link):
             os.symlink(self._archive_file, link)
 
-    def mount(self, restore_manager):
+    def mount(self):
         """
         Mount the archive. We avoid copy to be able to restore on system without
         too many space.
@@ -1914,9 +1913,10 @@ class TarBackupMethod(BackupMethod):
         backup_archive_corrupted -- Raised if the archive appears corrupted
         backup_archive_cant_retrieve_info_json -- If the info.json file can't be retrieved
         """
-        super(TarBackupMethod, self).mount(restore_manager)
+        super(TarBackupMethod, self).mount()
 
-        # Check the archive can be open
+        # Mount the tarball
+        logger.debug(m18n.n("restore_extracting"))
         try:
             tar = tarfile.open(self._archive_file, "r:gz")
         except:
@@ -1929,15 +1929,7 @@ class TarBackupMethod(BackupMethod):
         except IOError as e:
             raise YunohostError("backup_archive_corrupted", archive=self._archive_file, error=str(e))
 
-        # FIXME : Is this really useful to close the archive just to
+        if "info.json" in tar.getnames():
-        # reopen it right after this with the same options ...?
-        tar.close()
-
-        # Mount the tarball
-        logger.debug(m18n.n("restore_extracting"))
-        tar = tarfile.open(self._archive_file, "r:gz")
-
-        if "info.json" in files_in_archive:
             leading_dot = ""
             tar.extract('info.json', path=self.work_dir)
         elif "./info.json" in files_in_archive:
@@ -1992,7 +1984,15 @@ class TarBackupMethod(BackupMethod):
             ]
             tar.extractall(members=subdir_and_files, path=self.work_dir)
 
-        # FIXME : Don't we want to close the tar archive here or at some point ?
+        tar.close()
+
+    def copy(self, file, target):
+        tar = tarfile.open(self._archive_file, "r:gz")
+        file_to_extract = tar.getmember(file)
+        # Remove the path
+        file_to_extract.name = os.path.basename(file_to_extract.name)
+        tar.extract(file_to_extract, path=target)
+        tar.close()
 
 
 class BorgBackupMethod(BackupMethod):
@@ -2011,6 +2011,9 @@ class BorgBackupMethod(BackupMethod):
     def mount(self, mnt_path):
         raise YunohostError('backup_borg_not_implemented')
 
+    def copy(self, file, target):
+        raise YunohostError('backup_borg_not_implemented')
+
 
 class CustomBackupMethod(BackupMethod):
 
@@ -2020,8 +2023,8 @@ class CustomBackupMethod(BackupMethod):
     /etc/yunohost/hooks.d/backup_method/
     """
 
-    def __init__(self, repo=None, method=None, **kwargs):
+    def __init__(self, manager, repo=None, method=None, **kwargs):
-        super(CustomBackupMethod, self).__init__(repo)
+        super(CustomBackupMethod, self).__init__(manager, repo)
         self.args = kwargs
         self.method = method
         self._need_mount = None
@@ -2062,14 +2065,14 @@ class CustomBackupMethod(BackupMethod):
         if ret_failed:
             raise YunohostError('backup_custom_backup_error')
 
-    def mount(self, restore_manager):
+    def mount(self):
         """
         Launch a custom script to mount the custom archive
 
         Exceptions:
         backup_custom_mount_error -- Raised if the custom script failed
         """
-        super(CustomBackupMethod, self).mount(restore_manager)
+        super(CustomBackupMethod, self).mount()
         ret = hook_callback('backup_method', [self.method],
                             args=self._get_args('mount'))
 
@@ -2160,9 +2163,9 @@ def backup_create(name=None, description=None, methods=[],
 
     # Add backup methods
     if output_directory:
-        methods = BackupMethod.create(methods, output_directory)
+        methods = BackupMethod.create(methods, backup_manager, output_directory)
     else:
-        methods = BackupMethod.create(methods)
+        methods = BackupMethod.create(methods, backup_manager)
 
     for method in methods:
         backup_manager.add(method)

src/yunohost/tests/test_backuprestore.py

@@ -11,6 +11,7 @@ from yunohost.backup import backup_create, backup_restore, backup_list, backup_i
 from yunohost.domain import _get_maindomain
 from yunohost.user import user_permission_list, user_create, user_list, user_delete
 from yunohost.tests.test_permission import check_LDAP_db_integrity, check_permission_for_apps
+from yunohost.hook import CUSTOM_HOOK_FOLDER
 
 # Get main domain
 maindomain = ""
@@ -591,10 +592,30 @@ def test_restore_archive_with_bad_archive(mocker):
     clean_tmp_backup_directory()
 
 
+def test_restore_archive_with_custom_hook(mocker):
+
+    custom_restore_hook_folder = os.path.join(CUSTOM_HOOK_FOLDER, 'restore')
+    os.system("touch %s/99-yolo" % custom_restore_hook_folder)
+
+    # Backup with custom hook system
+    with message(mocker, "backup_created"):
+        backup_create(system=[], apps=None)
+    archives = backup_list()["archives"]
+    assert len(archives) == 1
+
+    # Restore system with custom hook
+    with message(mocker, "restore_complete"):
+        backup_restore(name=backup_list()["archives"][0],
+                        system=[],
+                        apps=None,
+                        force=True)
+
+    os.system("rm %s/99-yolo" % custom_restore_hook_folder)
+
+
 def test_backup_binds_are_readonly(mocker, monkeypatch):
 
-    def custom_mount_and_backup(self, backup_manager):
+    def custom_mount_and_backup(self):
-        self.manager = backup_manager
         self._organize_files()
 
         confssh = os.path.join(self.work_dir, "conf/ssh")