MDB-21949 Use tmp folder for restore/backup access control entities #155

MikhailBurdukov · 2024-05-24T13:01:19Z

No description provided.

MikhailBurdukov · 2024-05-24T13:09:45Z

tests/integration/features/access_control_replicated_backup.feature

@@ -45,14 +45,7 @@ Feature: Backup and restore functionality of replicated access control entities
    Then we got the following backups on clickhouse01
      | num | state   | data_count | link_count |
      | 0   | created | 1          | 0          |
-    When we execute command on clickhouse01


Not needed anymore, because we don't copy entities from the ZK to /var/lib/clickhouse/access

also, if we don't want to clean up access/tmp folders - should we change this step for checking tmp dir instead?

MikhailBurdukov · 2024-05-24T15:11:41Z

Fix for tests : #153

ekopanev · 2024-05-27T08:49:13Z

ch_backup/util.py

+    """
+    Interface for chown
+    """
+    shutil.chown(path, user, group)


do we really need to wrap this function with exact the same interface under a custom name then? originally it was done only for using our context

seems like unused function and can be deleted?

ekopanev · 2024-05-27T08:59:05Z

ch_backup/util.py

+
+    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)


btw, do we want to log this exception?

ekopanev · 2024-05-27T12:34:22Z

ch_backup/logic/access.py

+                self._backup_replicated(
+                    backup_tmp_path, access_control.acl_ids, context
+                )
+            self._backup_local(clickhouse_access_path, backup_tmp_path)


hm, am I right that now _backup_replicated() just copies data from ZK to tmp dir, while _backup_local() copies local files from access path to tmp and that's it?

And so:

_backup_local will always replace (in case of replicated storage) actual data from ZK with local version (because currently we do not clean them up)

we still will have a local (and soon "outdated") version of ACL in access folder anyway?

_backup_local will always replace (in case of replicated storage) actual data from ZK with local version (because currently we do not clean them up)

Yeah, seem we should do not replace it, if we already have the same one, but replicated.

we still will have a local (and soon "outdated") version of ACL in access folder anyway?

Yes, we will have it. I think make sense to cleanup /var/lib/clickhouse/access in restore operation.

Yes, we will have it. I think make sense to cleanup /var/lib/clickhouse/access in restore operation.

because I thought that originally we could have potential issue with local copies after each backup - in case of troubles with ZK we can't use replicated storage - so we can have a conflict with local sql files

so maybe we should clean it up after backup operation as well?

so maybe we should clean it up after backup operation as well?

Im not sure because suppose:

we have 2 acl entries: 1 - old one without zk, 2 - with zk

perform backup -> removing from /var/lib/clickhouse/acces.

So after ch-server restart we will lose 1 entry.

It seems fine to do cleanup on restore, but we will lose non-replicated records when we do a backup.

we have 2 acl entries: 1 - old one without zk, 2 - with zk

is it already possible to have 2 entities in different storages (local and replicated)? because I've tested it initially (22.x), and found that you can use only one storage type for that (if already no - maybe it makes sense)

and if 1 possible storage is still relevant - it's better to clean it up after a backup as well (think about your current state on the clusters - they all have local files), or move/remove them during migration from replicated to local

otherwise I think current approach works only for new clusters and with new ch-backup version

ch_backup/backup/layout.py

aalexfvk · 2024-05-27T12:24:31Z

ch_backup/logic/access.py

+        user = context.ch_ctl_conf["user"]
+        group = context.ch_ctl_conf["group"]
+
+        create_dir(clickhouse_access_path, user, group)


Actually, I don't see a necessity of creating separate function for this (creating folder and chown it). Two separate commands are relatively expressive.

clickhouse_access_path does not exist here ?

clickhouse_access_path does not exist here ?

Yes, clickhouse lazily creates /var/lib/clickhouse/access

aalexfvk · 2024-05-27T13:18:02Z

ch_backup/logic/access.py

+        group = context.ch_ctl_conf["group"]
+
+        create_dir(clickhouse_access_path, user, group)
+        with temporary_directory(backup_tmp_path, user, group):


Do we need to invent own class instead of the TemporaryDirectory

Does this class produce leftovers in case of exception and is it expected

For TemporaryDirectory there are no option to keep folder in case of exceptions.
https://github.com/python/cpython/blob/59630f92d8223f80993e3646b0f734d27f4b8dd4/Lib/tempfile.py#L944-L966

Why do we want to leave data in a temporary directory in case of an exception or a crash ? It might be corrupted

Because it might be useful in trouble shooting. If we want to re-run operation(backup/restore), then old files will be replaced with a new one. Also it make sense to cleanup tmp folder before operation, if it exist.

Ok, nevertheless this class is too specific for a general class(to be placed in ch_backup/util.py). Someone might want to accidentally reuse it for their own purposes
I see the following options:

Write a comment/docstring that this class is intended for using for access control backup procedure and write this motivation about keeping data in case of an exception

Rename it to smth like access_control_temp_directory.

Create a fully functional replacement for tempfile.TemporaryDirectory with keep parameter.
Simplified example:

@contextmanager def temporary_directory(keep=False): if keep: tmpdir = mkdtemp() yield tmpdir else: with TemporaryDirectory() as tmpdir: yield tmpdir

aalexfvk · 2024-05-27T14:12:50Z

ch_backup/logic/access.py

+            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
+                    )


Let's extract this to a separate function _download_access_control

aalexfvk · 2024-05-27T19:03:57Z

ch_backup/logic/access.py

+        clickhouse_access_path = context.ch_ctl_conf["access_control_path"]
+        restore_tmp_path = os.path.join(
+            context.ch_ctl_conf["tmp_path"], context.backup_meta.name
+        )
+        user = context.ch_ctl_conf["user"]
+        group = context.ch_ctl_conf["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)


It makes sense to extract this lines (except related tmp path) to a separate function with clean parameter.

MikhailBurdukov · 2024-05-30T09:27:06Z

@ekopanev any comments?

ekopanev

the rest LGTM

ekopanev · 2024-05-30T13:17:41Z

ch_backup/util.py

+    """
+    Interface for chown
+    """
+    shutil.chown(path, user, group)


seems like unused function and can be deleted?

MikhailBurdukov force-pushed the MDB-21949-Use_tmp_dir_for_access_backup_restore branch from b7e207c to 5bf74a7 Compare May 24, 2024 13:08

MikhailBurdukov commented May 24, 2024

View reviewed changes

MikhailBurdukov requested review from aalexfvk and ekopanev May 24, 2024 14:39

ekopanev reviewed May 27, 2024

View reviewed changes

aalexfvk reviewed May 27, 2024

View reviewed changes

yandex deleted a comment from MikhailBurdukov May 27, 2024

aalexfvk reviewed May 27, 2024

View reviewed changes

MikhailBurdukov added 4 commits May 29, 2024 14:49

MDB-21949 Use tmp folder for restore/backup access control entities

062a06f

Restart Ci

2db688a

Review fixes

9d1dbd8

fix

478c79f

MikhailBurdukov force-pushed the MDB-21949-Use_tmp_dir_for_access_backup_restore branch from f99c3d3 to 478c79f Compare May 29, 2024 14:50

Fix style

0dbe46b

aalexfvk approved these changes May 30, 2024

View reviewed changes

ekopanev approved these changes May 30, 2024

View reviewed changes

Remove unused

b975275

ekopanev approved these changes May 30, 2024

View reviewed changes

MikhailBurdukov merged commit 63e5fbb into main May 30, 2024
14 checks passed

MikhailBurdukov deleted the MDB-21949-Use_tmp_dir_for_access_backup_restore branch May 30, 2024 16:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MDB-21949 Use tmp folder for restore/backup access control entities #155

MDB-21949 Use tmp folder for restore/backup access control entities #155

MikhailBurdukov commented May 24, 2024

MikhailBurdukov May 24, 2024

ekopanev May 27, 2024

MikhailBurdukov commented May 24, 2024

ekopanev May 27, 2024

ekopanev May 30, 2024

ekopanev May 27, 2024

ekopanev May 27, 2024

MikhailBurdukov May 27, 2024

ekopanev May 27, 2024

MikhailBurdukov May 27, 2024

ekopanev May 30, 2024

aalexfvk May 27, 2024

MikhailBurdukov May 27, 2024

aalexfvk May 27, 2024 •

edited

Loading

MikhailBurdukov May 27, 2024

aalexfvk May 27, 2024 •

edited

Loading

MikhailBurdukov May 27, 2024

aalexfvk May 27, 2024 •

edited

Loading

aalexfvk May 27, 2024

aalexfvk May 27, 2024

MikhailBurdukov commented May 30, 2024

ekopanev left a comment

ekopanev May 30, 2024

MDB-21949 Use tmp folder for restore/backup access control entities #155

MDB-21949 Use tmp folder for restore/backup access control entities #155

Conversation

MikhailBurdukov commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikhailBurdukov commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalexfvk May 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalexfvk May 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalexfvk May 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikhailBurdukov commented May 30, 2024

ekopanev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalexfvk May 27, 2024 •

edited

Loading

aalexfvk May 27, 2024 •

edited

Loading

aalexfvk May 27, 2024 •

edited

Loading