* fdbcli audit_storage metadata_encoding: fix counting of format-neutral entries
The audit tool's status line reported MIGRATION IN PROGRESS
indefinitely on any running cluster in the FORWARD direction — the
FORWARD COMPLETE terminal state was unreachable. Root cause: two
categories of format-neutral entries were being counted as old-format:
1. Empty-value keyServers entries. These are KRM boundary sentinels
(see krmDecodeRanges in fdbclient/KeyRangeMap.cpp:70-84 for how
they are emitted at range ends). They mark the edge between
adjacent same-valued ranges and do not carry any keyServers
assignment. Every cluster has some — they cannot be migrated away.
2. serverKeysFalse ("this server does not own this range"). Written
by both finishMoveKeys (old format, MoveKeys.cpp:1787) AND
finishMoveShards (new format, MoveKeys.cpp:2138) on the drop-side
of any move. Also written as range boundary markers around every
assigned range (see unassignServerKeys in MoveKeys.cpp:91,127).
Every running cluster has many — they cannot be migrated away.
Under the old counting logic, keyServersOld and serverKeysOld were
therefore never zero, and the terminal-state condition
(keyServersOld == 0 && serverKeysOld == 0) that gates FORWARD COMPLETE
never triggered. ROLLBACK COMPLETE was unaffected because its
condition looks at *New* counts.
Fix: exclude format-neutral entries from the counts. keyServers skips
empty values. serverKeys uses two new small helpers added to
SystemData.h/cpp:
isServerKeysUnassigned(value) — true for empty + serverKeysFalse
isServerKeysOldFormatAssigned(value) — true for serverKeysTrue +
serverKeysTrueEmptyRange
Not caught earlier because the audit command has no automated
callers — it is a manual operator command, and manual users would
see MIGRATION IN PROGRESS and assume DD needed more time. The first
automated consumer that asserts the FORWARD terminal state is the
k8s test test_shardencode_rollover_load, which hangs indefinitely
without this fix.
* audit_storage metadata_encoding: workload counterpart + review fixes
Round out PR1 with the sim-workload half of the counting fix plus four
fixes surfaced by code review of the audit-tool changes:
1. Sim workload counting fix. fdbserver/workloads/CheckMetadataEncoding.cpp
had the identical miscounting bug as the fdbcli command — counted KRM
boundary sentinels as keyServersOld and serverKeysFalse as
serverKeysOld. Now uses the same isServerKeysUnassigned /
isServerKeysOldFormatAssigned classifiers introduced in this PR.
2. Wrap the fdbcli dataMoves scan in a retry loop. Previously a
retryable getRange error would swallow via tr.onError() without
re-reading, leaving dataMovesCount at 0 and causing a false
"ROLLBACK COMPLETE — safe to downgrade binary" report while data
moves were still in flight.
3. Move terminal-state assertions (requireForwardComplete /
requireRollbackComplete) out of the shardEncodeExpected branches.
Previously the assertions were nested inside the branch that matched
the knob, so a TOML that requested the assertion while the knob
override failed to take effect would silently skip the check. Now
the assertions always run when requested, and mismatched
knob/option combinations fail loudly with an explicit
misconfiguration error.
4. Emit the DataMoves trace detail only when the dataMoves range was
actually scanned. Previously the trace unconditionally logged
DataMoves=0 even when no scan ran (requireRollbackComplete=false),
poisoning downstream log parsers that assume 0 means "no data
moves in flight" rather than "not measured".
Also adds tests/fast/CheckMetadataEncodingForward.toml requiring
requireForwardComplete=true — the direct regression test for the
counting fix (without the fix, KRM sentinels/serverKeysFalse are
miscounted as old-format and FORWARD COMPLETE is unreachable).
* Add metadata encoding audit, rollback support, and tests
Adds tooling and code to support safe SHARD_ENCODE_LOCATION_METADATA
rollback and migration verification:
audit_storage metadata_encoding:
New AuditType that scans keyServers and serverKeys to report encoding
format counts (old tag-based vs new UID-based). Reports migration
status: FORWARD COMPLETE / ROLLBACK IN PROGRESS / ROLLBACK COMPLETE.
DD startup rewrite (DDTxnProcessor.cpp):
When SHARD_ENCODE=false, clears stale DataMoveMetaData and rewrites
shard-encoded keyServers entries to old format. serverKeys entries are
left in place (readable in both formats, drain naturally).
MoveKeys graceful bail (MoveKeys.cpp):
Functions that require shard-encoding throw dd_config_changed instead
of asserting when they detect the knob flipped or DataMoveMetaData is
unexpectedly empty from a concurrent DD restart.
Error handling (DataDistribution.cpp, DDRelocationQueue.actor.cpp):
dd_config_changed added to normalDDQueueErrors to prevent SevError
logging for expected operational restarts.
Knob infrastructure (ServerKnobs.cpp):
SHARD_ENCODE randomization respects explicitlySetKnobs so TOML
overrides take effect in simulation tests.
Tests:
- CheckMetadataEncodingForward.toml: verifies new-format entries
- CheckMetadataEncodingOldPath.toml: verifies old-format entries
- ShardEncodeRollback.toml: full rollback test with Attrition,
Rollback (TLog recovery), data consistency check
Documentation:
- design/shard-encode-location-metadata.md: full feature description
* Address review feedback: fix doc, remove stale test refs, add pagination comment
- Fix doc: use correct command name (metadata_encoding not validate_metadata_encoding),
remove get_audit_status references (audit runs client-side, no persisted state)
- Remove stale ShardEncodeRollback-1/2.toml refs from CMakeLists (files don't exist)
- Remove metadata_encoding from GetAuditStatusCommand (not queryable after the fact)
- Add comment explaining pagination: rewrite loops via caller until all entries converted
* Add more on how metadata handling changes particular regards coalescing
* Formatting
* Address review feedback on shard-encode rollback
Bundled review fixes for the design doc and the DD rollback path:
- Drop the version field from the DataMoveMetaData ASCII diagram. The
field is serialized for evolvability but has no readers (only
invalidVersion is ever written; the metaData.version reads in the
tree are on CheckpointMetaData, a different struct). Removing it
from the diagram avoids implying semantics that arent there.
- Clarify the S3 path lifetime in the LOGICAL_BULKLOAD section: the
path lives in BulkLoadTaskState at \xff/bulkLoadTask/<range> and is
fixed for the lifetime of a given dataMoveId. Same dataMoveId on DD
restart is a continuation; a different dataMoveId for the same
range signals a new task and possibly a new path.
- Rename ROLLBACK IN PROGRESS to MIGRATION IN PROGRESS in
CheckMetadataEncodingCommand and the design doc. A snapshot of the
keyspace cannot distinguish forward-in-progress from rollback-in-
progress (both look like mixed format), so the previous label was
over-claiming a direction the audit cannot actually verify. The
status line now reports new-format counts only, with no "remaining"
framing.
- Define the four states reported by audit_storage metadata_encoding
(FORWARD COMPLETE, MIGRATION IN PROGRESS, ROLLBACK COMPLETE, NOT
STARTED) at the first mention, with the trigger condition for each.
Also note that ROLLBACK COMPLETE is what a cluster that never
enabled the knob will show, since the conditions dont distinguish
those cases.
- Expand the rewriteShardEncodedMetadata comment to describe the
bounded scope honestly: Phase 2 rewrites only the first 1000
keyServers entries per call, and clusters with more than that
remnant rely on shard movement / wiggle for the bulk rewrite (per
the "Migration for downgrade" section of the design doc). The
function is safe to call on every DD init when knob is false; the
no-op path is write-cost-free, three reads.
- Replace four ASSERTs in cleanUpSingleShardDataMove and the
raw{Start,CheckFetching,Finish}Movement dispatch helpers with
if-throws. cleanUpSingleShardDataMove now throws operation_cancelled
when krmGetRanges paginates (the data-move range was subdivided into
more shards than fit in one page; the caller's view is stale). The
three raw* helpers now throw dd_config_changed when the
MoveKeysParams field expected by the current SHARD_ENCODE_LOCATION_
METADATA branch is missing, matching the existing forward-direction
guards. All four guarded conditions become invalid when the knob
flips while a data-move's params are in flight: params built under
the old knob value reach dispatch under the new one. The asymmetry
was pre-existing but only surfaced now because ShardEncodeRollback
flips the knob mid-test.
Trevor Clinkenbeard