Handle holds for vanished dirs - #4899
Conversation
There was a problem hiding this comment.
Pull request overview
Adds event-based hold handling for vanished SSTable directories and retained temporary manifests.
Changes:
- Releases holds from vanished backup directories.
- Ignores shadowed temporary manifests during restore, validation, purge, and deletion.
- Adds unit and integration coverage for retention modes and temporary manifests.
Integration-test review:
- Coverage is broad, but misses cross-task shared SSTable protection.
- Table-driven tests lack required scenario descriptions.
- The new integer-ID SSTable restriction is undocumented.
- Existing test coverage was not reduced.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/service/restore/worker_manifest.go |
Filters shadowed temporary manifests. |
pkg/service/restore/service_restore_integration_test.go |
Tests temporary manifest validation. |
pkg/service/restore/restore_integration_test.go |
Tests WORM backup restoration. |
pkg/service/restore/helper_integration_test.go |
Supports event-hold test targets. |
pkg/service/one2onerestore/worker_validate_integration_test.go |
Tests temporary manifest handling. |
pkg/service/one2onerestore/worker_manifest.go |
Filters shadowed temporary manifests. |
pkg/service/one2onerestore/service_integration_test.go |
Tests WORM 1-1 restoration. |
pkg/service/one2onerestore/helpers_integration_test.go |
Supports event-hold test targets. |
pkg/service/backup/worker_retention_lock.go |
Separates hold and object-lock workflows. |
pkg/service/backup/worker_retention_lock_test.go |
Moves event-hold tests elsewhere. |
pkg/service/backup/worker_purge.go |
Filters shadowed manifests during purge. |
pkg/service/backup/worker_manifest.go |
Copies manifests for event holds. |
pkg/service/backup/worker_event_based_hold.go |
Implements vanished-directory hold handling. |
pkg/service/backup/worker_event_based_hold_test.go |
Tests event-hold helpers. |
pkg/service/backup/worker_deduplicate.go |
Adjusts retention-mode compatibility checks. |
pkg/service/backup/validation.go |
Skips shadowed manifests during validation. |
pkg/service/backup/service.go |
Filters manifests during snapshot deletion. |
pkg/service/backup/service_backup_integration_test.go |
Covers purge, deletion, and validation. |
pkg/service/backup/purger.go |
Deletes shadowed temporary manifests. |
pkg/service/backup/list.go |
Adds shadowed-manifest filtering. |
pkg/service/backup/list_test.go |
Tests manifest filtering. |
pkg/service/backup/event_based_hold_interceptor_integration_test.go |
Tests vanished directories and retained manifests. |
pkg/rclone/rcserver/rc.go |
Allows optional remote directory paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for _, m := range filterShadowedTemporaryManifests(manifestInfos(manifests)) { | ||
| if m.TaskID != taskID { | ||
| continue |
There was a problem hiding this comment.
It's not a problem:
- vanished dirs are relatively rare
- stage dedup applies missing holds, so if we removed too many holds, they will be re-applied. This does not affect backup WORM protection, just increases req cnt
- if task A encountered vanished dir caused by table deletion, task B would also encounter it, so task A just does task's B job (task B would still do the listings, but no hold removal). The same goes for vanished dir caused by node removal. One note - tasks could encounter vanished dir which wouldn't be vanished from other task POV, but only when someone plays with backup task filtering params across backup task executions (--keyspace, --dc). This is rare, and even if we included manifests from other tasks here, it wouldn't solve this "problem" even for a single task configuration, where user executed backup on dc1,dc2, then on dc1, then on dc1,dc2.
|
tests are passing - the only comments are basically about code comments - I will address them when a not stacked version of this PR is made. |
Trying to fill event based hold info in error responses could overwrite the error response all together. Interceptor should only fill event based hold info on successful calls.
Since we always look for objects by joining remote and paths, it is natural to treat unset remote as fs root. This is helpful when dealing with clients using omitempty json tag.
deef8d0 to
e14a54e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (4)
pkg/service/backup/worker_event_based_hold.go:182
- These maps retain every distinct directory from all current and held manifests for the entire location, so
MaxManifestInMemorylimits parsing concurrency but not the resulting cluster-wide state. On large clusters this can create substantial Scylla Manager memory pressure. Process manifests grouped by node ID and compute/release each node's difference independently so only a bounded number of per-node directory sets are retained.
var (
currentDirs = make(map[sstableDir]struct{})
oldDirs = make(map[sstableDir]struct{})
pkg/service/backup/event_based_hold_interceptor_integration_test.go:522
- The table-drop and node/DC-removal cases should run as separate table-driven subtests. Applying both mutations before one backup never demonstrates either case independently, so regressions that only succeed when both kinds of vanished directories coexist can pass this test.
Print("When: vanished dir is caused by table drop")
ExecStmt(t, clusterSession, fmt.Sprintf("DROP TABLE %q.%q", keyspace, droppedTable))
Print("And: vanished dir is caused by filtered out node")
props["dc"] = []string{"dc1"}
pkg/service/backup/worker_event_based_hold.go:420
- Correct the grammatical error in this comment.
// Remote objects can already have holds applies from default bucket settings
pkg/service/backup/worker_event_based_hold.go:443
- Correct the typo in this comment.
// To do that, we first cache all local files and apply the holds on they fly when reading
At first, I thought that the whole event based hold implementation would fit into worker_retention_lock file, but it would need to contain both eventBasedHoldHandler and additional machinery needed for cleaning holds from vanished dirs, which is a little too much. This commit simply moves what can be moved without any changes to dedicated worker_event_based_hold, and the next commits will aim have fully separate execution paths for different modes, as the vanished dir cleanup changes the manifest cleanup quite a lot.
… implementations RetentionLock stays as the StageRetentionLock handler, but it now serves as a crossroad for decoupled object retention lock and event based hold approaches. This prepares us for more changes in event based hold implementation, which would start getting messy when still coupled with object retention lock codebase. Most implementation was just moved to a different file, but waitRetentionJob was additionally refactored so that it will be easy to reuse it with event based hold backup and plug different metric updates there.
This commit reworks event based hold backup part which is responsible for handling SM manifests holds. Previously, we just listed all manifests from with current task ID and removed holds from those with older snapshot tags. Now we also handle vanished dirs - sstable dirs in backup location which had holds applied during previous task execution, but for one reason or another, were not part of the current task execution and were missed during deduplication stage, which is the main point for handling sstable holds. To do that, we first list all older manifests which still have hold applied. Then, we parse them to learn about all dirs that were a part of their snapshot. Then we compare those dirs with the current ones, find the vanished ones, and release all hold still placed in such dirs. All of that happens parallel for location and up to MaxManifestInMemory per location. Fixes https://scylladb.atlassian.net/browse/CLOUD-3223
e14a54e to
ae7ff16
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (3)
pkg/service/backup/worker_event_based_hold.go:429
- Use plural agreement for “actions.”
// The table below shows what actions needs to be taken in which scenarios:
pkg/service/backup/worker_event_based_hold.go:189
- The concurrency limit bounds manifest parsing, but these two maps retain every directory from every current and held manifest until the whole location is processed. On a large cluster this creates cluster-wide memory growth—the resource this limit is intended to protect—and
vanishedthen duplicates part of it. Partition comparison by node (or stream per-node results directly to hold release) so directory sets can be discarded incrementally.
var (
currentDirs = make(map[sstableDir]struct{})
oldDirs = make(map[sstableDir]struct{})
pkg/service/backup/worker_event_based_hold.go:38
- Add the missing article and infinitive marker in this comment.
This issue also appears on line 429 of the same file.
// MaxManifestInMemory is cluster wide limit, so we need divide it across
|
PR is ready for review - the failures are caused by failing TestRestoreFullAlternatorIntegration against scylla nightly - I'm investigating that and will create an issue about it, but it shouldn't block this PR review. |
|
The CI problem is technically caused by https://scylladb.atlassian.net/browse/CLOUD-3399, but while investigating it I run into https://scylladb.atlassian.net/browse/SCYLLADB-3642 as well. |
This PR handles the vanished dirs during event based hold backup procedure.
Vanished dirs are the remote sstable dirs which were a part of the previous snapshot (SM handled holds on their objects), but for some reason (e.g., node or table deletion) are no longer a part of the current snapshot (SM didn't handle their holds in stage deduplicate).
In such case, SM would need to establish which dirs are vanished (read and compare old manifests with the current ones. To avoid re-reading all manifests on ever backup task execution, we read only those which still have holds applied and treat older manifests without holds as already checked for vanished dirs during the previous backup task executions) and remove all holds placed on their objects, as we know that they also won't be a part of next backup task executions. Here is a more detailed comment answering copilot question on how does it work when multiple backup tasks are configured.
Fixes https://scylladb.atlassian.net/browse/CLOUD-3223