Review fixes

ch_backup/backup/layout.py

@@ -120,7 +120,7 @@ def upload_access_control_file(self, backup_name: str, file_name: str) -> None:
             raise StorageError(msg) from e
 
     def upload_access_control_files(
-        self, path: str, backup_name: str, local_path: str, file_names: List[str]
+        self, local_path: str, backup_name: str, file_names: List[str]
     ) -> None:
         """
         Upload access control list.
@@ -131,7 +131,7 @@ def upload_access_control_files(
         try:
             logging.debug('Uploading access control data "{}"', local_path)
             self._storage_loader.upload_files_tarball(
-                path,
+                local_path,
                 remote_path,
                 files=file_names,
                 encryption=True,

ch_backup/logic/access.py

@@ -4,6 +4,7 @@
 
 import os
 import re
+import shutil
 from typing import Any, Dict, List, Sequence, Union
 
 from kazoo.client import KazooClient
@@ -15,9 +16,7 @@
 from ch_backup.logic.backup_manager import BackupManager
 from ch_backup.util import (
     chown_dir_contents,
-    chown_path,
     copy_directory_content,
-    create_dir,
     temporary_directory,
 )
 
@@ -41,7 +40,9 @@ def backup(self, context: BackupContext) -> None:
         user = context.ch_ctl_conf["user"]
         group = context.ch_ctl_conf["group"]
 
-        create_dir(clickhouse_access_path, user, group)
+        os.makedirs(clickhouse_access_path, exist_ok=True)
+        shutil.chown(clickhouse_access_path, user, group)
+
         with temporary_directory(backup_tmp_path, user, group):
             objects = context.ch_ctl.get_access_control_objects()
             context.backup_meta.set_access_control(objects)
@@ -63,7 +64,6 @@ def backup(self, context: BackupContext) -> None:
             context.backup_layout.upload_access_control_files(
                 backup_tmp_path,
                 context.backup_meta.name,
-                backup_tmp_path,
                 acl_file_names,
             )
 
@@ -87,20 +87,14 @@ def restore(self, context: BackupContext) -> None:
         user = context.ch_ctl_conf["user"]
         group = context.ch_ctl_conf["group"]
 
-        create_dir(clickhouse_access_path, user, group)
+        if os.path.exists(clickhouse_access_path):
+            shutil.rmtree(clickhouse_access_path)
+
+        os.makedirs(clickhouse_access_path)
+        shutil.chown(clickhouse_access_path, user, group)
+
         with temporary_directory(restore_tmp_path, user, group):
-            if (
-                context.backup_meta.access_control.backup_format
-                == BackupStorageFormat.TAR
-            ):
-                context.backup_layout.download_access_control(
-                    restore_tmp_path, context.backup_meta.name
-                )
-            else:
-                for name in _get_access_control_files(acl_ids):
-                    context.backup_layout.download_access_control_file(
-                        restore_tmp_path, context.backup_meta.name, name
-                    )
+            self._download_access_control_list(context, restore_tmp_path, acl_ids)
 
             if has_replicated_access:
                 self._restore_replicated(restore_tmp_path, acl_ids, acl_meta, context)
@@ -178,6 +172,19 @@ def fix_admin_user(self, context: BackupContext, dry_run: bool = True) -> None:
                 except FileNotFoundError:
                     logging.debug(f"File {file_path} not found.")
 
+    def _download_access_control_list(
+        self, context: BackupContext, restore_tmp_path: str, acl_ids: List[str]
+    ) -> None:
+        if context.backup_meta.access_control.backup_format == BackupStorageFormat.TAR:
+            context.backup_layout.download_access_control(
+                restore_tmp_path, context.backup_meta.name
+            )
+        else:
+            for name in _get_access_control_files(acl_ids):
+                context.backup_layout.download_access_control_file(
+                    restore_tmp_path, context.backup_meta.name, name
+                )
+
     def _clean_user_uuid(self, raw_str: str) -> str:
         return re.sub(r"EXCEPT ID\('(.+)'\)", "", raw_str)
 
@@ -263,7 +270,7 @@ def _mark_to_rebuild(
         mark_file = os.path.join(clickhouse_access_path, CH_MARK_FILE)
         with open(mark_file, "a", encoding="utf-8"):
             pass
-        chown_path(mark_file, user, group)
+        shutil.chown(mark_file, user, group)
 
     def _has_replicated_access(self, context: BackupContext) -> bool:
         return (

ch_backup/util.py

@@ -466,6 +466,7 @@ def __eq__(self, other):
             if not getattr(self, slot) == getattr(other, slot):
                 return False
         return True
+
 def chown_path(path: str, user: str, group: str) -> None:
     """
     Interface for chown
@@ -480,18 +481,14 @@ def copy_directory_content(from_path_dir: str, to_path_dir: str) -> None:
     if to_path_dir[-1] != "/":
         to_path_dir += "/"
     for subpath in os.listdir(from_path_dir):
-        subpath = os.path.join(from_path_dir, subpath)
-        shutil.copy(subpath, to_path_dir)
-
-
-def create_dir(path: str, user: str, group: str) -> None:
-    """
-    Create dir and chown it.
-    """
-    os.makedirs(path, exist_ok=True)
-    chown_path(path, user, group)
+        subpath_from = os.path.join(from_path_dir, subpath)
+        subpath_to = os.path.join(to_path_dir, subpath)
+        if not os.path.exists(subpath_to):
+            shutil.copy(subpath_from, to_path_dir)
 
 
+# tempfile.TemporaryDirectory : https://docs.python.org/3.11/library/tempfile.html#tempfile.TemporaryDirectory
+# It is not possible to keep directory content only when exception occurs.
 class temporary_directory:
     """
     Class to automatically create and remove temporary directory.
@@ -505,10 +502,19 @@ def __init__(self, path: str, user: str, group: str):
         self._group = group
 
     def __enter__(self) -> None:
-        create_dir(self._path, self._user, self._group)
+        if os.path.exists(self._path):
+            shutil.rmtree(self._path)
+
+        os.makedirs(self._path)
+        shutil.chown(self._path, self._user, self._group)
 
     def __exit__(self, exc_type, exc_value, traceback):
         if exc_type is not None:
-            logging.warning("Dont remove tmp dir {} due to exception.", self._path)
+            logging.warning(
+                "Dont remove tmp dir {} due to exception. {}: {}",
+                self._path,
+                exc_type.__name__,
+                exc_value,
+            )
             return
         shutil.rmtree(self._path)