Skip to content

Reject --incremental combined with the bulk modes - #14200

Open
saintstack wants to merge 5 commits into
apple:mainfrom
saintstack:dev/stack/bulk-incremental-reject
Open

Reject --incremental combined with the bulk modes#14200
saintstack wants to merge 5 commits into
apple:mainfrom
saintstack:dev/stack/bulk-incremental-reject

Conversation

@saintstack

@saintstack saintstack commented Oct 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Stacked on #14199. That PR is not merged yet, so this one's diff also contains its two commits
(Make the bulk key-range test measure its own claim, Honor --keys on the bulkdump/bulkload path).
Review only the three commits below; I will rebase onto main once #14199 lands, at which point the
diff shrinks to just those.

Validated: ensemble 20261002-050347-stack-ef7c73ef1a946e81 on 782a71048c —
99997 pass / 1 fail of 99998 reported, fail_fast never tripped. The single failure is an
ExternalTimeout: one slow seed hit the harness's 1800 s wall-clock kill at TestRunCount=89.
Both guards fired in that run too, now logged as StdErrOutput Severity="30" — which is the
StderrSeverity fix visible in the failure record itself.

The bug

--incremental means "logs only, no base snapshot" — one CLI flag setting both incrementalBackupOnly
and onlyApplyMutationLogs (fdbbackup/backup.cpp:3727-3730). The bulk modes exist to produce
(bulkdump) and consume (bulkload) exactly the snapshot that --incremental declines, so the
combination is incoherent.

Both sides accepted it silently and resolved it in opposite directions:

Backup. StartFullBackupTaskFunc skips the entire snapshot block when incrementalBackupOnly is
set (FileBackupAgent.cpp:4175), so --mode bulkdump --incremental produced a log-only backup with
no SSTs. Nothing warned. The operator found out only when a later bulkload restore aborted with
"backup has no bulkdump data", which reads as a restore fault but is a backup-side omission hours
earlier.

Restore. StartFullRestoreTaskFunc selects the bulkload branch from useRangeFileRestore alone
(:6824), without consulting the logs-only flag, so --mode bulkload --incremental ingested the
whole snapshot the caller asked to skip. It then set onlyApplyMutationLogs itself (:6858),
overwriting the caller's value so even status misreported the restore.

The change

Reject both pairs at submit time rather than picking a winner, matching the range-count guards added
in #14199. A logs-only restore is --mode rangefile --incremental, which is unaffected.

The remaining assignment at :6858 is sequencing, not user intent — the SSTs carry the range data, so
the dispatch that follows must replay logs only. With the guard in place it can only ever overwrite
false, and it is now commented as such.

SevWarnAlways rather than SevError, for the same reason as #14199: a simulation workload can
legitimately submit the rejected combination, and SevError would fail the run for correctly-refused
input.

Test status

The second commit adds an opt-in probe (expectBulkIncrementalRejection) and a dedicated toml that
attempts both rejected pairs and asserts the error codes. It is kept separate from
BackupS3BlobBulkLoadRestoreKeyRange.toml deliberately: the backup probe creates a container as a
side effect, and that test is the regression gate for the --keys data-correctness bug in #14199.

It first failed 10/10 (20261001-230113-stack-cf51b914e5414d23). The diagnosis, from the failure
records: FailReason="ProducedErrors", no failed assertion and no Severity=40 event, and both
guard messages present in StdErrOutput. So both guards fired and both probes passed — TestHarness2
simply scores anything on stderr at severity 40, and these guards write their user-facing message
there. The test was failing on precisely the output it exists to provoke.

Fixed in the third commit with StderrSeverity = 30, the mechanism seven existing tests already use
for the same reason (FuzzApiCorrectness, WriteDuringRead and friends). That commit also passes the
real lockUID to the restore probe rather than a fresh random one; the database is locked by then, so
had the guard not fired, the probe would have died in checkDatabaseLock with database_locked
instead of reporting anything useful.

Verified locally that the backup guard fires as intended
(BS3BCW_IncrementalBackupRejection … ErrorCode="2300" Accepted="0") and that the toml option reaches
the run (StderrSeverity … NewSeverity="30"). A full run of this toml cannot be done on Apple Silicon
— WITH_ROCKSDB=OFF is mandatory there and makes newRocksDBSstFileWriter() return nullptr, which
writeKVSToSSTFile dereferences — so the end-to-end confirmation is a Joshua rerun.

Resolved: the restore-side guard is reachable

An earlier version of this description flagged a risk that the restore guard might be unreachable,
since restore() calls getRestoreSet(..., onlyApplyMutationLogs, ...) before submitRestore and
throws restore_invalid_version when the set is absent. That concern is settled and was wrong:
the failing run's own output contains the restore guard's message, so submitRestore is reached and
the guard fires. Both halves of the first commit are confirmed working.

Commit note

The three commits here are kept separate on purpose: the guards, their coverage, and the harness fix
for that coverage. The third exists only because the second was wrong about how stderr is scored, and
squashing it would hide that the guards were never at fault.

michael stack added 4 commits September 24, 2026 20:46
michael stack
Make the bulk key-range test measure its own claim
5c5e7a1 
BackupS3BlobBulkLoadRestoreMultiRange asserted "Data outside backup
ranges is not affected", but BackupS3BlobCorrectness::_setup appended
normalKeys to backupRanges and restoreRanges unconditionally, after the
constructor had built the random ranges from backupRangesCount. With
normalKeys in the list there is no data outside the backup ranges, so
that assertion could not fail and backupRangesCount had no observable
effect.

Drop the append and scope the test to a single range, which is the widest
coverage a bulkdump/bulkload job can express. Sentinel keys are planted
outside the backup range and rewritten once the backup has stopped, so a
restore that widens beyond the range reverts them and fails. The rewrite
has to follow the backup: a value the backup captured would be restored
to the value the check expects, hiding the widening.

The other tomls using this workload all set backupRangesCount = -1 and
already take normalKeys from the constructor branch, so the append was
redundant for them.

Renamed to BackupS3BlobBulkLoadRestoreKeyRange.toml: it no longer tests
multiple ranges, and multi-range bulk backup is not expressible.
michael stack
Honor --keys on the bulkdump/bulkload path
f9c9237 
The bulk backup/restore integration ignored the caller's key ranges:
createBulkDumpJob, createBulkLoadJob and the log-replay key-version map
were all hardcoded to normalKeys. A range-scoped bulkdump therefore
dumped the whole keyspace, and a bulkload restore wrote all of it back,
overwriting keys the caller never named. restoreRanges was read and then
never used.

Pass the caller's range to all three sites. A bulk job carries exactly
one key range and the cluster admits one bulk job at a time, so
submitBackup and submitRestore reject any other range count rather than
silently widening; supporting several ranges needs a change to the
persisted bulk job metadata.

The rejections are SevWarnAlways, not the SevError used by the
neighbouring option-conflict checks: a simulation workload can
legitimately submit a multi-range bulkdump, and SevError would fail the
run for input that was correctly refused.
michael stack
Reject --incremental combined with the bulk modes
b1b0ceb 
--incremental and the bulk modes make contradictory requests, and both
sides resolved the contradiction silently and in opposite directions.

On backup, StartFullBackupTaskFunc skips the entire snapshot block when
incrementalBackupOnly is set, so --mode bulkdump --incremental produced a
log-only backup with no SSTs. Nothing reported this; the operator learned
of it only when a later bulkload restore aborted with "backup has no
bulkdump data" and appeared to blame the restore.

On restore, StartFullRestoreTaskFunc selects the bulkload branch from
useRangeFileRestore alone, without consulting the logs-only flag, so
--mode bulkload --incremental ingested the whole snapshot the caller had
asked to skip. It then set onlyApplyMutationLogs itself, overwriting the
caller's value so even status misreported the restore.

Reject both pairs at submit time rather than picking a winner. A
logs-only restore is --mode rangefile --incremental, which is unaffected.

The remaining assignment in the bulkload branch is sequencing, not user
intent: the SSTs carry the range data, so the dispatch that follows must
replay logs only. With the new guard it can only ever overwrite false.
michael stack
Cover the --incremental rejections on both bulk paths
e809548 
The guards added in the previous commit had no test: no toml combines
--incremental with a bulk mode, so neither submit-time rejection was ever
exercised. The opposite direction was already covered incidentally --
tests/fast/IncrementalBackup.toml proves a rangefile incremental backup
is still accepted -- but nothing proved the rejections actually fire.

Add an opt-in probe to the workload that attempts both rejected pairs and
asserts the error: submitBackup with --incremental and snapshotMode 1 or
2 must throw backup_error, and submitRestore with --incremental and
bulkload must throw restore_error. The restore probe runs after the real
backup so the container is describable and a throw can only come from the
guard.

This lives in its own toml rather than in the key-range test. The backup
probe creates a container as a side effect, and the key-range test is the
regression gate for the --keys data-correctness bug; a side effect there
would be paid for in the one place we can least afford it.
@saintstack saintstack added the Backup_v3 Range Partitioned Backup label Oct 2, 2026
@foundationdb-ci

This comment has been minimized.

Sign in to view
@foundationdb-ci

This comment has been minimized.

Sign in to view
@foundationdb-ci

This comment has been minimized.

Sign in to view
@foundationdb-ci

This comment has been minimized.

Sign in to view
@foundationdb-ci

This comment has been minimized.

Sign in to view
@foundationdb-ci

This comment has been minimized.

Sign in to view
@foundationdb-ci

This comment has been minimized.

Sign in to view
michael stack
Stop the rejection test failing on its own expected stderr
782a710 
The test failed 10/10 on Joshua with FailReason="ProducedErrors" and no
failed assertion: both guards fired correctly and both probes passed, but
the guards write their user-facing message to stderr, and TestHarness2
scores stderr at severity 40 by default. The test was failing on exactly
the output it exists to provoke.

Set StderrSeverity = 30, the mechanism seven existing tests already use
for the same reason (FuzzApiCorrectness, WriteDuringRead and friends).

Also pass the real lockUID to the restore probe instead of a fresh random
one. The database is locked by then, so had the guard not fired the probe
would have died in checkDatabaseLock with database_locked rather than
reporting anything useful about the guard.
@foundationdb-ci

foundationdb-ci commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang-ide on Linux RHEL 9

  • Commit ID: 782a710
  • Duration 0:30:56
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

foundationdb-ci commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang-arm on Linux RHEL 9

  • Commit ID: 782a710
  • Duration 0:50:01
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

foundationdb-ci commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-clang on Linux RHEL 9

  • Commit ID: 782a710
  • Duration 1:07:23
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

foundationdb-ci commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr on Linux RHEL 9

  • Commit ID: 782a710
  • Duration 1:14:38
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@saintstack
saintstack marked this pull request as ready for review October 2, 2026 05:49
@foundationdb-ci

foundationdb-ci commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-cluster-tests on Linux RHEL 9

  • Commit ID: 782a710
  • Duration 1:48:26
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)
  • Cluster Test Logs zip file of the test logs (available for 30 days)

@foundationdb-ci

foundationdb-ci commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-macos-m1 on macOS 14.x

  • Commit ID: 782a710
  • Duration 2:19:31
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

@foundationdb-ci

foundationdb-ci commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Result of foundationdb-pr-macos on macOS 14.x

  • Commit ID: 782a710
  • Duration 4:35:52
  • Result: ✅ SUCCEEDED
  • Error: N/A
  • Build Log terminal output (available for 30 days)
  • Build Workspace zip file of the working directory (available for 30 days)

This was referenced Oct 2, 2026
Open
Open

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Backup_v3 Range Partitioned Backup

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@saintstack @foundationdb-ci