Repository navigation
Refuse a bulkload restore of a partitioned-log backup - #14311
Closed
saintstack wants to merge 1 commit into
Closed
saintstack wants to merge 1 commit into
saintstack wants to merge 1 commit into
Conversation
StartFullRestoreTaskFunc reads mutationLogType and then takes the !useRangeFileRestore branch without consulting it, so the partitioned dispatcher is unreachable for a bulkload restore and RestoreDispatchTask is handed partitioned log files. Nothing refused the combination: mutationLogType appears nowhere in submitBackup, none of the existing bulkload refusals mentioned it, and the CLI accepts --mode bulkdump with --mutation-log-type partitioned-log followed by --mode bulkload. The data is never corrupted -- decodeMutationLogFileBlock rejects a plog on its first block, because the block format version is not BACKUP_AGENT_MLOG_VERSION. But a restore task that throws is logged and retried rather than aborted: TaskBucket records lastError and never finishes the task, so it is re-dispatched on every timeout with no retry cap and no transition to ERestoreState::ABORTED. The restore sits at zero log bytes applied, repeating restore_unsupported_file_version, instead of reporting that the layout cannot be replayed. Refuse it at submit time through bulkLoadRestoreRefusal, which already receives mutationLogType by way of submitRestore. The value comes from desc.mutationLogType, so this keys off the layout describeBackup found in the container rather than off a flag the caller passed. Note this does not cover a container that holds partitioned logs while desc.mutationLogType comes back DEFAULT, which can happen because describeBackup only sets it when the plogs listing for its scan window is non-empty. That detection gap is a separate problem. Covered by the existing bulkLoadRestoreRefusal unit test: a partitioned log is refused for bulkload, a classic log is not, and rangefile mode is unaffected. With the check disabled the test fails.
Contributor
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Contributor
Result of foundationdb-pr-clang on Linux RHEL 9
|
Contributor
Result of foundationdb-pr-cluster-tests on Linux RHEL 9
|
Contributor
Result of foundationdb-pr on Linux RHEL 9
|
Contributor
Result of foundationdb-pr-clang-arm on Linux RHEL 9
|
Contributor
Result of foundationdb-pr-macos-m1 on macOS 14.x
|
Contributor
Result of foundationdb-pr-macos on macOS 14.x
|
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.
Problem
StartFullRestoreTaskFuncreadsmutationLogTypeand then takes the!useRangeFileRestorebranch without consulting it, soRestoreDispatchPartitionedTaskFuncis unreachable for a bulkload restore and the plainRestoreDispatchTaskFuncis handed partitioned log files.Nothing refused the combination:
mutationLogTypeappears nowhere insubmitBackup, none of the existing bulkload refusals mentioned it, and the CLI acceptsfdbbackup start --mode bulkdump --mutation-log-type partitioned-logfollowed byfdbrestore start --mode bulkload.No data is corrupted.
decodeMutationLogFileBlockrejects a plog on its first block, since the block format version isPARTITIONED_MLOG_VERSION(4110) rather thanBACKUP_AGENT_MLOG_VERSION(2001).But the restore never fails either. A restore task that throws is logged and retried, not aborted:
TaskBucketrecordslastErrorand neverfinish()es the task, so it is re-dispatched on every timeout expiry with no retry cap and no transition toERestoreState::ABORTED. The observable result is a restore sitting at zero log bytes applied, repeatingrestore_unsupported_file_version, rather than one that reports the layout cannot be replayed.Fix
Refuse it at submit time through
bulkLoadRestoreRefusal(), which already receivesmutationLogTypeby way ofsubmitRestore— so no new plumbing is required.That value comes from
desc.mutationLogType, which means the refusal keys off the layoutdescribeBackupactually found in the container, not off a flag the caller happened to pass. That matters here:getRestoreSetselects the partitioned layout from container contents (if (!plogs.empty())), so the hazard does not require the user to have requested anything.Known gap
This does not cover a container that holds partitioned logs while
desc.mutationLogTypecomes backDEFAULT, which can happen becausedescribeBackuponly sets the property when theplogs/listing for its scan window is non-empty. That detection gap is a separate problem and is not addressed here.A belt-and-braces alternative would be a consumer-side check in the classic reader, for which the data is already plumbed —
RestoreConfig::RestoreFilecarriestagId/totalTags, andLogFile::isPartitionedLog()exists but currently has zero callers. I have left that out to keep this change to the submit-time refusal; happy to add it if preferred.Testing
Covered by the existing
TEST_CASE("/backup/restore/bulkLoadRestoreRefusal"), extended with three cases: a partitioned log is refused for bulkload, a classic log is not, and rangefile mode is unaffected.Not vacuous: with the new check replaced by
if (false), the test fails (0 tests passed; 1 tests failed).fdbclient_test --simulation -f /backup/→ 32 passed, 1 failed. The failure is/backup/time(parseTime(formatTime(ts)) == ts), which fails identically on unmodifiedmainon this machine and is unrelated.No existing test combines partitioned logs with bulkload, so nothing regresses.