Repository navigation
Fix leftover S3 files when deleting native ClickHouse backups - #1590
Merged
Merged
Conversation
Collaborator
|
thanks for contribution, will review later |
…ix/native-backup-json-cleanup
Retention inside upload calls cleanEmbeddedAndObjectDiskRemoteIfSameLocalNotPresent
for older remote backups; assigning b.isEmbedded there leaked the tags of the
last deleted backup into the following local retention (hasObjectDisksLocal),
so use a local variable instead.
Read the backups_s3 object prefix from the {cluster}/{shard} macros in
TestEmbeddedS3Cleanup instead of hardcoding cluster/0, and add a ChangeLog entry.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Deleting a backup made with
use_embedded_backup_restoreand an S3 backup disk can report success while native backup files remain in S3. Files such asserialization.jsonwere skipped, and remote deletion could choose the wrong cleanup path after the local backup was removed.This change selects cleanup from the backup’s recorded type, includes native JSON files, and reads object references inside compressed data archives. It skips the tool’s own catalog, saved users, server settings, and named collections, which are ordinary files rather than references to native data. Native backups made on local disks keep their ordinary-file cleanup path.
The unit tests cover local deletion and remote deletion with uncompressed, tar, and gzip files. The new integration test creates a real
serialization.json, records the native data objects named by the backup, and checks both deletion orders. Those objects must remain while either the local or remote copy is kept, and all must disappear after both copies are deleted. The existing clustered backup and restore scenarios remain unchanged.ClickHouse 23.3 has a separate backup-creation bug that can leave files no longer named by any backup, fixed in ClickHouse/ClickHouse#51299. The integration test checks the files actually named by the backup instead of requiring deletion of those unrelated leftovers.
Validation:
go test ./pkg/backup ./pkg/storage/object_disk -count=1 -vandgo vet ./pkg/backup ./pkg/storage/object_diskpassed for the deletion changes.TestEmbeddedS3andTestEmbeddedS3Cleanupruns passed on ClickHouse 23.3 and 23.8. These ran the upstream test setup with local services and dummy credentials; the test runner was compiled for macOS ARM64 and the Linux backup binary included race checks and coverage.TestEmbeddedS3Cleanup/remote_firstfail specifically because its realserialization.jsonobject remains after deletion.