Repository navigation
feat: add clean_broken_retention CLI command - #1371
Merged
Merged
Conversation
Walks top-level of remote `path` and `object_disks_path` and batch-deletes (with retry, via the existing BatchDeleter pipeline) every entry that is not present in the live BackupList and not matched by any --keep=<glob>. Dry-run by default; --commit performs the deletes. Useful for cleaning up orphans left behind by failed retention runs (e.g. the GCS 503 scenario from #1356).
Plant orphans directly under /minio/data/clickhouse for both `path` and `object_disks_path`, then verify: - dry-run lists them without deleting, - --commit removes them from both locations, - --keep=<glob> preserves matching entries, - the live backup is never touched.
…ckends
Table-driven, with one sub-test per backend:
- S3 (minio), SFTP (sshd), FTP (proftpd), GCS_EMULATOR (fake-gcs-server)
use direct container-FS plant/assert,
- AZBLOB plants via az-cli docker run against azurite,
- real GCS and COS skip themselves unless GCS_TESTS / QA_TENCENT_SECRET_KEY
is set (their plant helpers fail loudly if reached without infra).
For each backend: create a real backup that must survive, plant 3 orphans
in path/object_disks_path, verify dry-run lists them without deleting,
verify --commit + --keep glob deletes only unmatched orphans, then a
second --commit clears the rest.
…roken_retention GCS: spins up google/cloud-sdk:slim with --volumes-from clickhouse-backup to access /etc/clickhouse-backup/credentials.json, authenticates the service account, and uses gsutil cp/ls. COS: spins up amazon/aws-cli:latest with --endpoint-url pointing at the regional COS endpoint and QA_TENCENT_SECRET_ID/KEY as AWS credentials, since COS supports the S3 API. Setup hook renders config.yml from the copied template via envsubst. Both run only when their credential env var is set.
- Collapse the three container-FS factories (S3/SFTP/FTP/GCS_EMULATOR) into a single containerFSCase helper. - Extract dockerRunSh and an azList closure to remove repetition in the AZBLOB case; reuse a single gsutil/awsRun wrapper for GCS and COS. - Drop the redundant storageType field; derive the table name from name. - Loop the plant+assertExists step over a small table instead of open-coding four calls. - Pull repeated keep-glob string into a const, drop fmt.Sprintf with no format args.
Signed-off-by: slach <bloodjazman@gmail.com>
…ash in Walk names
The command previously left orphans untouched because:
1. `BackupList(ctx, false, ...)` uses the on-disk metadata cache; on the
second invocation (e.g. dry-run → --commit) the cache returns orphan
directories with Broken="", so my Broken!="" filter let them into the
keep-set and zero orphans were detected. Now passes parseMetadata=true
so every top-level entry is stat'd for metadata.json on each call.
2. `bd.Walk("/", false, …)` emits names with a leading slash on S3 (from
TrimPrefix mismatch); the previous `strings.Contains(name, "/")` filter
rejected them as nested. Switched to strings.Trim("/") so leading and
trailing slashes are both stripped before the top-level check.
test/integration:
- Split TestCleanBrokenRetention into one top-level test per backend so
each can be run independently via RUN_TESTS=TestCleanBrokenRetention<Backend>.
- S3 case now plants orphans via 'mc cp' instead of raw file writes —
MinIO only sees objects placed through its S3 API; direct files on disk
are invisible to ListObjectsV2 and the cleanup batch finds nothing.
Signed-off-by: slach <bloodjazman@gmail.com>
…lean_broken_retention
…lean_broken_retention # Conflicts: # test/testflows/clickhouse_backup/tests/snapshots/cli.py.cli.snapshot
Signed-off-by: slach <bloodjazman@gmail.com>
…ntics BackupList discovers all top-level entries under backup path and marks directories without metadata.json as "broken" backups. Since broken backups must be kept (they may be in-progress uploads), entries under the backup path can never be true orphans. Genuine orphans only exist under object_disks_path, which BackupList does not scan. The tests now correctly: - Plant a broken entry under pathRoot, assert it is preserved throughout - Plant orphans only under objRoot, assert they are cleaned - Use clean_remote_broken to clean up the broken entry at the end
Add NewErrNotFound(key) helper that wraps ErrNotFound with the actual key path, so 'key not found: backup_name/shadow/db/table/part.tar' replaces the uninformative 'key not found' in download/upload errors.
…place --keep with --exclude/--include - findOrphanTopLevelNames now scans getBackupPath() instead of "/" (avoids walking the entire bucket root on shared storage) - Replace --keep flag with --exclude (preserve matched orphans) and --include (scope cleanup to matching names only) - Test isolation: all TestCleanBrokenRetention* use --include=cbr_* to avoid interfering with parallel tests in the same backend bucket
…s-test deletion CleanRemoteBroken previously deleted all broken remote backups without any name filtering. When multiple CI matrix jobs share the same GCS bucket, a concurrent TestCleanBrokenRetentionGCS could delete mid-upload backups from other tests (e.g. TestGCS) that temporarily lack metadata.json. Add --include flag (same path.Match syntax as clean_broken_retention) so callers can scope deletion to a name pattern. API/server callers pass nil for backward compatibility. Update the integration test to use --include=cbr_* so clean_remote_broken only touches backups owned by the retention test.
- Include CLICKHOUSE_VERSION in cbr_* glob patterns and backup names so parallel CI jobs with different CH versions don't interfere with each other in shared GCS/Azure buckets - Move DROP TABLE to t.Cleanup via docker exec instead of Go client, avoids 'connection is closed' when defer runs after env.Cleanup - Add shell command to gsutil error message for easier debugging
Coverage Report for CI Build 26110299144Warning No base build found for commit Coverage: 63.609%Details
Uncovered Changes
Coverage RegressionsRequires a base build to compare against. How to fix this → 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
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.
Summary
New CLI command
clean_broken_retentionthat walks top-level of remotepathandobject_disks_path, batch-deletes (with retry) every entry that is not a live backup and is not excluded by--excludeglobs and is matched by--includeglobs (if provided). Object disk orphans are deleted in parallel with progress tracking. Pass--committo actually delete; without it the command only logs what would be deleted.--excludeglobs (path.Matchsyntax, repeatable);--includeglobs scope cleanup to matching names only--commitperforms the actual deletesBackupDestination.RemoveBackupRemote(forpath) andcleanBackupObjectDisks(forobject_disks_path) — both already use batched delete with retry/exponential-backoffUsage
Test plan
--commitrun — verify the orphan is removed from bothpathandobject_disks_path, batched, with retry on transient 5xx