guard delete against breaking incremental backups chains - #1494
Merged
Conversation
`delete local|remote <backup_name>` deleted a backup which other backups
reference via `required_backup` without any check: the breakage surfaced
only later, when a descendant was downloaded or restored, `list remote`
kept printing `+<parent>` for a backup which no longer existed, and for
object disks the descendant `required` parts blobs were deleted together
with the `<object_disk_path>/<backup_name>` prefix of the parent.
`backups_to_keep_remote` retention already protected such chains, only
the explicit delete didn't.
Both delete paths now collect the backups whose `required_backup` points
to the deleted one and refuse to delete, naming the dependents. `--force`
(`force=1` for `POST /backup/delete/{where}/{name}`) restores the old
behavior, and `general.rebase_during_delete` rebases every dependent
increment first (same as the `rebase` command) so the chain stays
restorable - opt-in, because rebase copies the parent parts into the
dependents and turns an O(1) delete into an unbounded data copy.
Retention, watch and cleanup paths pass force=true, so their behavior is
unchanged. Integration tests which deleted a parent while a descendant
was still present now delete children first, or pass --force where they
break the chain on purpose.
…itions restore_remote of the increment downloads it locally, so the local fullBackupName it requires can't be deleted without --force anymore.
…493-delete-required-backup-guard
…493-delete-required-backup-guard
…493-delete-required-backup-guard
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.
Fix #1493.
delete local|remote <backup_name>deleted a backup which other backups reference viarequired_backupwithout any check. The breakage surfaced only later, when a descendant was downloaded or restored (ReadBackupMetadataRemote(RequiredBackup)),list remotekept printing+<parent>for a backup which no longer existed, and for object disks the descendantrequiredparts blobs were deleted together with the<object_disk_path>/<backup_name>prefix of the parent.backups_to_keep_remoteretention already protected such chains (storage.GetBackupsToDeleteRemote), only the explicit delete didn't.Behavior
Both delete paths (CLI,
POST /backup/delete/{where}/{name},system.backup_actions) now collect the backups whoserequired_backuppoints at the deleted one and refuse to delete, naming the dependents:--force(force=1/force=truefor the API) keeps the old chain-breaking behavior. Note it must precede the positional arguments:delete --force remote <name>.New
general.rebase_during_delete(envREBASE_DURING_DELETE, defaultfalse): rebase every dependent increment first (same as therebasecommand), so the chain stays restorable and the backup becomes deletable. Opt-in, because rebase copies the deleted backup parts into its dependents and turns an O(1) delete into an unbounded data copy; a rebase failure aborts the delete.backups_to_keep_local/backups_to_keep_remoteretention,watch,clean_local_broken/clean_remote_broken,createrollback and--delete-sourcepassforce=true, so their behavior is unchanged.Not covered:
remote_storage: custom—RemoveBackupRemotedelegates tocustom.DeleteRemotebefore any metadata is read, so there is nothing to check there.Tests
TestFindDependentBackups(local and remote chain link detection, self-reference, multiple children).TestDeleteRequiredBackup— guard ondelete local, backup survives the refused delete,--force, guard ondelete remote, thenREBASE_DURING_DELETE=truerebases the increment and the rebased increment restores after its ancestor is gone.childrenFirstintest/integration/utils.go,fullCleanup, rebase/restoreResolveIncrement/downloadRequiredBackupConnection/keepBackupRemote/hardlinks cleanups,cloud_storage.py), or pass--forcewhere they break the chain on purpose (testBackupSpecifiedPartitions,hardlinksExistsFiles).Local runs (macOS/arm64, ClickHouse 26.3):
make testgreen;TestS3,TestDeleteRequiredBackup,TestRebaseS3,TestHardlinksExistsFiles,TestKeepBackupRemoteAndDiffFromRemote,TestKeepBackupRemoteWithRebase,TestRestoreResolveIncrementS3,TestDownloadRequiredBackupConnectionS3,TestProjections,TestDiffFromChecksums,TestResumeOperationsAfterRestartS3,TestWatchSchedule,TestServerAPIRebaseall pass.