Fix cleanup in ON CLUSTER backups by vitlibar · Pull Request #83835 · ClickHouse/ClickHouse

vitlibar · 2025-07-16T12:28:27Z

Changelog category (leave one):

Not for changelog (changelog entry is not required)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fix cleanup in ON CLUSTER backups. We need to clean coordination nodes in ZooKeeper correctly after a backup or restore failed or got cancelled.

This PR is required to fix tests for ON CLUSTER backups and restores.

clickhouse-gh · 2025-07-16T12:29:22Z

Workflow [PR], commit [4c6f324]

Summary: ❌

job_name	test_name	status	info	comment
Stateless tests (amd_debug, AsyncInsert, s3 storage, parallel)		failure
	03352_concurrent_rename_alter	FAIL

jkartseva

How was this change tested? Can the test plan be reproduced and added as a test?

jkartseva · 2025-07-18T06:01:25Z

-            return;
+            for (auto & [host, host_info] : state.hosts)
+            {
+                if (!host_info.finished && (std::find(unfinished_hosts.begin(), unfinished_hosts.end(), host) == unfinished_hosts.end()))


Would host_info. finished == true be a valid state at this point? It looks like it wouldn't. Probably it's worth adding a debug message.

You're right, normally that isn't expected. However sometimes ZooKeeper performs an operation but reports a connection loss - so it's better to check if the operation is actually complete before retrying it again. I added a debug message and a comment here.

jkartseva · 2025-07-18T06:23:58Z

-    bool waitOtherHostsFinish(bool) const override { return true; }
-    bool finish(bool) override { return true; }
-    bool cleanup(bool) override { return true; }
+    void setError(std::exception_ptr, bool) override { is_error_set = true; }


Inform about which error was set through a debug message?

Function setError() is called only from BackupStarter::onException() and RestoreStarter::onException() after logging the error, so the error has been already logged as the point when setError() is called. I added a comment about that.

jkartseva · 2025-07-18T06:30:22Z

        }
+        else if (!get_node_failed_check_for_non_existence().empty())
+        {
+            show_error_before_next_attempt(fmt::format("Node {} exists", get_node_failed_check_for_non_existence()));


(Probably beyond the scope of this PR) The final exception

throw Exception(ErrorCodes::FAILED_TO_SYNC_BACKUP_OR_RESTORE, "Couldn't create the 'finish' node for {} after {} attempts", current_host_desc, max_attempts_after_bad_version);

lacks of information about the last error.

Good idea! I added this information to the exception.

jkartseva · 2025-07-18T06:43:41Z

+        if (backup && backup_is_corrupted && backups_worker.remove_backup_files_after_failure && backup_coordination
+            && backup_coordination->isErrorSet() && backup_coordination->finished()
+            && (!backup_coordination->isBackupQuerySentToOtherHosts() || backup_coordination->allHostsFinished()))


This condition is hard to read compared to the previous
bool should_remove_files_in_backup = backup && !is_internal_backup && backups_worker.remove_backup_files_after_failure;

Did I read it correctly that the corrupted files were never removed for internal backups?

Isn't backup_coordination->allHostsFinished() imply backup_coordination->finished() ?

I tried to improve readability, so bool should_remove_files_in_backup is back.

jkartseva · 2025-07-18T06:45:17Z

+        if (backup_coordination && backup_coordination->finished() &&
+            (!backup_coordination->isBackupQuerySentToOtherHosts() || backup_coordination->allHostsFinished()))


Same, isn't backup_coordination->allHostsFinished() imply backup_coordination->finished() ?

Yes, if backup_coordination->allHostsFinished() == true then backup_coordination->finished() == true.

However if backup_coordination->isBackupQuerySentToOtherHosts() == false then we shouldn't check backup_coordination->allHostsFinished() at all.

I changed the code to improve readability.

SaltTan · 2025-07-18T14:42:04Z

Will this PR fix #81968 by any chance?

vitlibar · 2025-07-20T10:24:05Z

CI failures are unrelated:

03352_concurrent_rename_alter - see 03352_concurrent_rename_alter is flaky #84081

clickhouse-gh Bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Jul 16, 2025

vitlibar changed the title ~~Fix cleanup in on cluster backups~~ Fix cleanup in ON CLUSTER backups Jul 16, 2025

vitlibar mentioned this pull request Jul 16, 2025

[WIP] Fix test_shutdown_waits_for_backup #83638

Closed

vitlibar assigned jkartseva Jul 16, 2025

vitlibar added 2 commits July 16, 2025 14:45

Improve cleanup code in ON CLUSTER backups and restores.

8d86004

Correct test_shutdown_waits_for_backup.

087a4ba

vitlibar force-pushed the fix-cleanup-in-on-cluster-backups branch from 4e80808 to 087a4ba Compare July 16, 2025 12:45

jkartseva reviewed Jul 17, 2025

View reviewed changes

jkartseva approved these changes Jul 18, 2025

View reviewed changes

Add more information to messages.

4c6f324

vitlibar enabled auto-merge July 21, 2025 07:03

vitlibar added this pull request to the merge queue Jul 21, 2025

Merged via the queue into ClickHouse:master with commit 1f9ca3e Jul 21, 2025
122 of 124 checks passed

vitlibar deleted the fix-cleanup-in-on-cluster-backups branch July 21, 2025 07:31

robot-ch-test-poll1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jul 21, 2025

		if (backup_coordination && backup_coordination->finished() &&
		(!backup_coordination->isBackupQuerySentToOtherHosts() \|\| backup_coordination->allHostsFinished()))

Conversation

vitlibar commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Uh oh!

clickhouse-gh Bot commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkartseva left a comment

Choose a reason for hiding this comment

Uh oh!

jkartseva Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

vitlibar Jul 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkartseva Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

vitlibar Jul 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkartseva Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

vitlibar Jul 19, 2025

Choose a reason for hiding this comment

Uh oh!

jkartseva Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

vitlibar Jul 19, 2025

Choose a reason for hiding this comment

Uh oh!

jkartseva Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

vitlibar Jul 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SaltTan commented Jul 18, 2025

Uh oh!

vitlibar commented Jul 20, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vitlibar commented Jul 16, 2025 •

edited

Loading

clickhouse-gh Bot commented Jul 16, 2025 •

edited

Loading

vitlibar Jul 19, 2025 •

edited

Loading

vitlibar Jul 19, 2025 •

edited

Loading

vitlibar Jul 19, 2025 •

edited

Loading