Merge pull request #672 from YunoHost/fix-1308-output-directory-not-empty

[fix] Backup fails because output directory not empty

src/yunohost/backup.py

@@ -326,10 +326,19 @@ class BackupManager():
         if not os.path.isdir(self.work_dir):
             filesystem.mkdir(self.work_dir, 0o750, parents=True, uid='admin')
         elif self.is_tmp_work_dir:
-            logger.debug("temporary directory for backup '%s' already exists",
+
+            logger.debug("temporary directory for backup '%s' already exists... attempting to clean it",
                          self.work_dir)
-            # FIXME May be we should clean the workdir here
-            raise YunohostError('backup_output_directory_not_empty')
+
+            # Try to recursively unmount stuff (from a previously failed backup ?)
+            if not _recursive_umount(self.work_dir):
+                raise YunohostError('backup_output_directory_not_empty')
+            else:
+                # If umount succeeded, remove the directory (we checked that
+                # we're in /home/yunohost.backup/tmp so that should be okay...
+                # c.f. method clean() which also does this)
+                filesystem.rm(self.work_dir, recursive=True, force=True)
+                filesystem.mkdir(self.work_dir, 0o750, parents=True, uid='admin')
 
     #
     # Backup target management                                              #
@@ -911,7 +920,7 @@ class RestoreManager():
             ret = subprocess.call(["umount", self.work_dir])
             if ret != 0:
                 logger.warning(m18n.n('restore_cleaning_failed'))
-        filesystem.rm(self.work_dir, True, True)
+        filesystem.rm(self.work_dir, recursive=True, force=True)
 
     #
     # Restore target manangement                                            #
@@ -1526,34 +1535,12 @@ class BackupMethod(object):
                                   directories of the working directories
         """
         if self.need_mount():
-            if self._recursive_umount(self.work_dir) > 0:
+            if not _recursive_umount(self.work_dir):
                 raise YunohostError('backup_cleaning_failed')
 
         if self.manager.is_tmp_work_dir:
             filesystem.rm(self.work_dir, True, True)
 
-    def _recursive_umount(self, directory):
-        """
-        Recursively umount sub directories of a directory
-
-        Args:
-            directory -- a directory path
-        """
-        mount_lines = subprocess.check_output("mount").split("\n")
-
-        points_to_umount = [line.split(" ")[2]
-                            for line in mount_lines
-                            if len(line) >= 3 and line.split(" ")[2].startswith(directory)]
-        ret = 0
-        for point in reversed(points_to_umount):
-            ret = subprocess.call(["umount", point])
-            if ret != 0:
-                ret = 1
-                logger.warning(m18n.n('backup_cleaning_failed', point))
-                continue
-
-        return ret
-
     def _check_is_enough_free_space(self):
         """
         Check free space in repository or output directory before to backup
@@ -2039,6 +2026,7 @@ def backup_create(name=None, description=None, methods=[],
         # Check that output directory is empty
         if os.path.isdir(output_directory) and no_compress and \
                 os.listdir(output_directory):
+
             raise YunohostError('backup_output_directory_not_empty')
     elif no_compress:
         raise YunohostError('backup_output_directory_required')
@@ -2343,6 +2331,30 @@ def _call_for_each_path(self, callback, csv_path=None):
             callback(self, row['source'], row['dest'])
 
 
+def _recursive_umount(directory):
+    """
+    Recursively umount sub directories of a directory
+
+    Args:
+        directory -- a directory path
+    """
+    mount_lines = subprocess.check_output("mount").split("\n")
+
+    points_to_umount = [line.split(" ")[2]
+                        for line in mount_lines
+                        if len(line) >= 3 and line.split(" ")[2].startswith(directory)]
+
+    everything_went_fine = True
+    for point in reversed(points_to_umount):
+        ret = subprocess.call(["umount", point])
+        if ret != 0:
+            everything_went_fine = False
+            logger.warning(m18n.n('backup_cleaning_failed', point))
+            continue
+
+    return everything_went_fine
+
+
 def free_space_in_directory(dirpath):
     stat = os.statvfs(dirpath)
     return stat.f_frsize * stat.f_bavail

src/yunohost/tests/test_backuprestore.py

@@ -10,7 +10,7 @@ from moulinette import m18n
 from moulinette.core import init_authenticator
 from yunohost.app import app_install, app_remove, app_ssowatconf
 from yunohost.app import _is_installed
-from yunohost.backup import backup_create, backup_restore, backup_list, backup_info, backup_delete
+from yunohost.backup import backup_create, backup_restore, backup_list, backup_info, backup_delete, _recursive_umount
 from yunohost.domain import _get_maindomain
 from yunohost.utils.error import YunohostError
 
@@ -571,7 +571,7 @@ def test_backup_binds_are_readonly(monkeypatch):
 
         assert "Read-only file system" in output
 
-        if self._recursive_umount(self.work_dir) > 0:
+        if not _recursive_umount(self.work_dir):
             raise Exception("Backup cleaning failed !")
 
         self.clean()