Repository navigation
Conversation
RemoveOldBackupsRemote runs after the new backup is completely uploaded. Its failure made CreateToRemote return an error which watch treated as a failed upload: prevBackupName was not advanced, so the next increment was diffed from a stale base although the fresh backup exists on remote, and createRemoteErrCount grew until watch aborted with "too many errors". The retention error is now wrapped into remoteRetentionError, and watch (interval and --schedule modes) uses watchBackupUploaded() to advance the chain and not count the iteration towards abort, while still logging the error and reporting it via status and callback. With --schedule the full_type=rebase and delete_previous_cycle steps proceed as well. One-shot create_remote/upload keep returning the error (non-zero exit code). Fixes #1535 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Local retention (backups_to_keep_local) and --delete-source also run after the new backup is completely on remote storage, so their failure must not make watch rebuild the chain from a stale base or abort with "too many errors" either. remoteRetentionError becomes postUploadError and wraps these errors too, including the local retention at the end of create_remote --streaming. One-shot create_remote/upload still fail. Refs #1535 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Two old remote backups tagged `embedded` are put to FTP and watch runs with an embedded_backup_disk that doesn't exist, so every backups_to_keep_remote retention fails after the new backup is uploaded (RemoveBackupRemote and object disk cleanup errors are only warnings, this is a deterministic failing path). watch must keep running until killed by `timeout`, log the retention error and upload the increment with required_backup = the full backup of the first iteration. Refs #1535 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
watch reloads the config file on every iteration (watch.go LoadConfig) without re-applying --env overrides, so REMOTE_STORAGE / retention / embedded_backup_disk passed with --env were lost after the first reload: the upload failed with remote_storage "none" and backups_to_keep_remote=0 from the file made watch abort after the first error. Use config_modifier like other scenarios and restore the config in cleanup. Refs #1535 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`delete remote` of the planted embedded-tagged backups fails for the same reason as the retention (embedded_backup_disk doesn't exist), so cleanup left them on remote storage. Delete them with FTP DELE/RMD the same way they were planted, and restore the original config even when the final assertion fails. Refs #1535 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- embedded ON CLUSTER initiator: on a post-upload error still wait for the worker nodes, a worker failure is returned unwrapped, so watch never advances the chain (or deletes the previous schedule cycle) on a backup which is incomplete cluster-wide - uploadEpilogue: remote retention, local retention and --delete-source all run even when a previous step failed, errors are joined into one postUploadError, so a permanent remote retention failure doesn't leak a local backup per watch iteration - create_remote --streaming: a post-upload failure is no longer reported as a partial backup to resume or delete, shadow cleanup and local retention still run - --schedule full_type=rebase: a rebase failure keeps the post-upload error message but stays "not uploaded" - ChangeLog: never-abort and metrics behavior, worker-side limitation Refs #1535 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- run watch in the background with PID / exit code files and poll `list remote` until an increment with uploaded metadata.json appears (bounded deadline) instead of a fixed 50s `timeout`, so a slow increment upload isn't killed midway; the check is that watch was still running, the exit code after our SIGTERM depends on its handler - parse `list --format=json remote` instead of text columns - cleanup stops watch by its PID file and always restores the config - CREATE TABLE IF NOT EXISTS for reruns after a failed cleanup Refs #1535 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The background job of the interactive testflows bash printed `[1]+ Done ...` into the output of a later command, which broke parsing in cleanup. Start it from a child bash, the same way BackupNode.start_server does. Refs #1535 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Coverage Report for CI Build 39210929648Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage at 70.287% (no base build to compare)Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
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
2 participants
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.
Fixes #1535
Problem
When a backup is fully uploaded but a step after the upload fails (remote retention
RemoveOldBackupsRemote, local retention, or--delete-sourceremoval),create_remotereturned an error andwatchtreated it like a failed upload:prevBackupNamewas not advanced (next increment diffed from a stale base),createRemoteErrCountgrew, andwatcheventually aborted with "too many errors".Change
postUploadError;watchBackupUploaded(err)classifies them.uploadEpiloguenow runs all three post-upload steps even if an earlier one fails and joins their errors.watch(interval mode and--schedule): on a post-upload error the backup counts as uploaded — chain advances, error doesn't count towards aborts, still logged at error level and reported to/backup/status,system.backup_actionsand the callback.--schedule: iffull_type=rebasefails after a post-upload error, both errors are reported and the backup is treated as not uploaded (nodelete_previous_cycle).create_remote --streaming: a post-upload failure no longer reports "partial backup ... delete it from both".create_remote/uploadkeep the non-zero exit code.Known limitation
Post-upload failures on embedded ON CLUSTER worker nodes still count as failed uploads (the error type is lost through
system.backup_actions); this errs on the safe side. Metrics:failed_create_remoteskeeps growing while backups are uploaded (documented in ChangeLog).Tests
TestWatchBackupUploaded,TestNewPostUploadError,TestRebaseFailedError;go vet,go test ./pkg/backup/... ./pkg/server/...pass./clickhouse backup/watch/retention failure keeps chain(forces remote retention failure via stale embedded backups with a missingembedded_backup_disk) — passes locally (71/71 steps); fails on pre-fix code because watch aborts with "too many errors" before any increment exists.🤖 Generated with Claude Code