Repository navigation
Add fdbrestore status --json - #14141
Merged
saintstack merged 4 commits intoOct 23, 2026
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
saintstack
force-pushed
the
dev/stack/fdbrestore-status-json
branch
from
September 26, 2026 04:47
3ee39e9 to
c5712a4
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
saintstack
force-pushed
the
dev/stack/fdbrestore-status-json
branch
from
September 26, 2026 06:29
c5712a4 to
b97c13b
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
added 3 commits
October 13, 2026 21:39
fdbrestore status was text-only, so automation had to regex-parse it, and the two snapshot engines emit different formats. fdb-kubernetes-tests carries a standing TODO for this and parses both shapes by hand today. Rather than add a second reader, the reads move into one RestoreConfig::StatusSnapshot and the text and JSON outputs become pure renderers over it, sharing the snapshot-phase decision. getProgress() and getFullStatus() keep their signatures and their output byte-identical. The previous JSON defect in this area -- fdbbackup status --json serializing an empty Errors array -- came from a JSON path written separately from the text path it was meant to mirror. The document reports only the counters that apply to the engine in use: Blocks/FileCount for rangefile, Tasks for bulkload. Zero-filling the inapplicable set would let a consumer read "not applicable" as "no progress". An empty tag reports every restore, so the result is one object with a Restores array rather than a document per tag.
The JSON renderer guarded on the integer quotient while the text renderer guarded on the double, so for 0 < bytesWritten < triggeredTasks the text printed "Avg bytes/task: 0 bytes" while the JSON omitted the key entirely. Hoist the average into a shared helper alongside statusPhases() so the two cannot disagree about whether to emit it, and emit it as an integer since JsonBuilder renders doubles with "%g" and would lose precision on a large average.
stateText(tr) is map(stateEnum().getD(tr), restoreStateText), so reading it alongside stateEnum() fetched the same key twice in the same transaction. Derive the text from the enum instead: identical value within one snapshot, and one fewer read on all three status paths.
saintstack
force-pushed
the
dev/stack/fdbrestore-status-json
branch
from
October 13, 2026 20:39
6f0b078 to
b8cae66
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
akankshamahajan15
self-requested a review
October 22, 2026 17:38
| printf(" The restore tag to act on. Default is 'default'\n"); | ||
| printf("\n"); | ||
| printf(" Options for status:\n\n"); | ||
| printf(" --json Emit the status as a JSON document instead of text.\n"); |
Contributor
There was a problem hiding this comment.
I think we can remove "instead of text" as it is evident from the json word itself
Contributor
Author
There was a problem hiding this comment.
Done (both places)
akankshamahajan15
approved these changes
Oct 22, 2026
| Future<Optional<bool>> useRangeFileRestore = restore.useRangeFileRestore().get(tr); | ||
| Future<Optional<bool>> bulkLoadComplete = restore.bulkLoadComplete().get(tr); | ||
| // {snapshot, mutation log} phase, or {nullptr, nullptr} when the state has no phase to report. | ||
| // Shared by the text and JSON renderers so the two cannot disagree. |
Contributor
There was a problem hiding this comment.
Curious about the wording "so the two cannot disagree.", why would they disagree? And is this statement needed anymore?
Contributor
Author
There was a problem hiding this comment.
Dropped. Not something needed in code (refactor rationale).
neethuhaneesha
approved these changes
Oct 22, 2026
Contributor
Result of foundationdb-pr-clang-ide on Linux RHEL 9
|
Contributor
Result of foundationdb-pr-clang-arm on Linux RHEL 9
|
Contributor
Result of foundationdb-pr 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-macos on macOS 14.x
|
This comment has been minimized.
This comment has been minimized.
Contributor
Result of foundationdb-pr-macos-m1 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.
fdbrestore statusis text-only. Automation has to regex-parse it, and it emits two different formats depending on the snapshot engine — blocks and files for a rangefile restore, task counters for a bulkload one.fdb-kubernetes-testscarries a standing TODO for exactly this and parses both shapes by hand today (tests/fixtures/backup_restore_fixture.go:1152-1157).fdbbackup status --jsonandfdbbackup describe --jsonalready exist; restore was the gap.Approach
RestoreConfig::getProgress_implandgetFullStatus_impleach performed their own reads and built text byformat()concatenation, with the latter calling the former. Rather than add a second, independent JSON reader, the reads move into oneRestoreConfig::StatusSnapshotand all three outputs become pure renderers over it, sharing the snapshot-phase decision viastatusPhases().This shape is deliberate. The last JSON defect in this area —
fdbbackup status --jsonserializing an emptyErrorsarray (#14124) — was caused precisely by a JSON path written separately from the text path it was meant to mirror, and drifting. One source of data makes that class of bug structurally harder.getProgress()andgetFullStatus()keep their signatures and their output is byte-identical, so no call site needed changing.One consequence worth recording, because "callers are unaffected" would be too strong: the consolidated read set is a superset of what
getProgress()alone used to read, andgetProgress()'s only caller iswaitRestore(), which commits its transaction — it registers a watch onstateEnumand commits a few lines later. So six config reads that the progress renderer never consumes now enter that transaction's read-conflict set:addPrefix,removePrefix,sourceContainerURL,restoreVersion,bulkLoadComplete, andgetRestoreRangesOrDefault(). The last is a paginated range read over therestoreRangeSetsubspace rather than a point read, so it contributes a range conflict rather than a single key. (stateEnumis not an addition —getProgress()already read it viastateText().)The practical cost is nevertheless very low.
bulkLoadCompleteis the only one plausibly written mid-restore, it flips once, the read-version-to-commit window is milliseconds, and that loop already readsstateEnum— the very key that changes to end the wait — so it is conflict-prone by construction and already recovers throughtr->onError(). Expect well under one extra retry per restore, and only onfdbrestore waitandstart --waitfordone, the paths that passVerbose::True.Output
Only the counters that apply to the engine in use are emitted. Zero-filling the inapplicable set would let a consumer read "not applicable" as "no progress" — note the
Taskscounters below are legitimately all-zero early in a bulkload restore, so an emittedBlocks: 0/0alongside them would be genuinely ambiguous.An empty tag reports every restore, so the result is one object with a
Restoresarray rather than a document per tag. That also means a single document can contain both engine shapes, which the concatenated text format cannot express cleanly.Captured live from the
s3_backup_bulkdump_bulkloadctest, which runs a bulkload restore alongside a rangefile validation restore:{ "SchemaVersion": "1.0.0", "Restores": [ { "Tag": "test_backup_bulkdump", "UID": "cd179a2e3c8cdfaf4492e9df4ba99d99", "State": "running", "SnapshotMethod": "bulkload", "URL": "blobstore://127.0.0.1:8081/ctests/94135/test_bulkdump_bulkload?bucket=test-bucket®ion=us-east-1&secure_connection=0", "Ranges": [ { "Begin": "", "End": "\\xff" } ], "AddPrefix": "", "RemovePrefix": "", "RestoreVersion": 21723915, "CurrentVersion": 1853165, "FirstConsistentVersion": 1853165, "BytesWritten": 0, "Tasks": { "Submitted": 0, "Triggered": 0, "Running": 0, "Total": 0 }, "SnapshotPhase": "in_progress", "MutationLogPhase": "not_started" }, { "Tag": "test_backup_bulkdump_validate", "UID": "1bc3c8f007212f64f74b0ab9377ea53a", "State": "completed", "SnapshotMethod": "rangefile", "URL": "blobstore://127.0.0.1:8081/ctests/94135/test_bulkdump_bulkload?bucket=test-bucket®ion=us-east-1&secure_connection=0", "Ranges": [ { "Begin": "", "End": "\\xff" } ], "AddPrefix": "\\xff\\x02/rlog/", "RemovePrefix": "", "RestoreVersion": 21723915, "CurrentVersion": 21723915, "FirstConsistentVersion": 1853165, "BytesWritten": 330, "Blocks": { "Finished": 1, "Total": 1, "InProgress": 0 }, "FileCount": 2, "ApplyVersionLag": 0, "SnapshotPhase": "complete", "MutationLogPhase": "complete" } ] }And a single-tag rangefile restore, before and after completion, from a
file://container:{"SchemaVersion":"1.0.0","Restores":[{"Tag":"testtag","UID":"629eef09732e61b06863dabeeb7d5faf", "State":"queued","SnapshotMethod":"rangefile","URL":"file:///...","Ranges":[{"Begin":"","End":"\\xff"}], "AddPrefix":"","RemovePrefix":"","RestoreVersion":24125773,"CurrentVersion":-1, "FirstConsistentVersion":0,"BytesWritten":0,"Blocks":{"Finished":0,"Total":0,"InProgress":0}, "FileCount":0,"ApplyVersionLag":0}]}{"SchemaVersion":"1.0.0","Restores":[{"Tag":"testtag","UID":"629eef09732e61b06863dabeeb7d5faf", "State":"completed",...,"BytesWritten":222,"Blocks":{"Finished":1,"Total":1,"InProgress":0}, "FileCount":2,"ApplyVersionLag":0,"SnapshotPhase":"complete","MutationLogPhase":"complete"}]}Notes on the shape:
SnapshotPhase/MutationLogPhaseare emitted only forRUNNINGandCOMPLETED, matching the text path — hence absent whilequeued.CurrentVersion: -1while queued is pre-existinggetCurrentVersion()behaviour surfaced as-is, not introduced here. Consumers should read it as "not yet known".SchemaVersion, followingfdbbackup status --jsonanddescribe --json. (fdbbackup queryuses snake_case with no schema version; that inconsistency is pre-existing and not propagated here.)Testing
Both in-tree backup ctests pass on this change, re-run after rebasing onto current
main(which upgraded Boost to 1.89.0), and built withWITH_ROCKSDB=ONso the fixture's hardcodedssd-rocksdb-v1engine is available:dir_backup_tests(rangefile,file://)exit=0,PASSED test_dir_backup_and_restores3_backup_bulkdump_bulkload_tests(bulkload, MockS3)exit=0,PASSED test_bulkdump_bulkload, plusAudit validation PASSED - BulkLoad produces identical results to traditional restoreBackupTests(option parsing over the realCSimpleOpttables)exit=0These two ctests are also the regression gate for the text output, since both grep it (
backup_tests_common.shmatchesState:.*completedandPhase:). Text was verified byte-identical by inspection as well: the two original functions computed the bulkload flag with opposite polarity (!present \|\| valueingetProgress_implvspresent && !valueingetFullStatus_impl); those are exact complements, so collapsing them into oneuseRangeFilefield is behaviour-preserving at every use site.Additionally verified by hand against a local loopback cluster: full backup → clear → restore → data verified, with
--jsonpolled mid-restore and after completion (the single-tag output above), and both documents confirmed parseable.Incidental fixes
getProgress_implcomputed anerrstrfromlastErrorand never used it, sofdbrestore statushas never displayed the last error despite reading it. The dead local is gone and the value is now surfaced asLastError: {Message, SecondsAgo}in the JSON. This makes JSON strictly more informative than text here; aligning the text output is deliberately left out of scope to keep it byte-identical.getFullStatuspreviously readuseRangeFileRestoretwice, once directly and once viagetProgress. Now read once.stateText(tr)ismap(stateEnum().getD(tr), restoreStateText), so reading it alongsidestateEnum()fetched the same key twice in one transaction. The text is now derived from the enum: identical value within a single snapshot, and one fewer read on all three status paths.Not in scope
fdbdr statusstill has no JSON at all —DatabaseBackupAgenthas nogetStatusJSONcounterpart.fdbbackuperror paths are still plain text on stderr regardless of--json, so a--jsonconsumer cannot distinguish "empty result" from "failed".fdbbackup list,tagsanddumpremain text-only.