wbtiger
/
foundationdb
mirror of https://github.com/apple/foundationdb.git
0
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

movekeys: rewrite removeOldDestinations to comply with krmSetRangeCoalescing (#13200) 

Problem: FailedAssertion="value != endValue || endKey == maxWithPrefix.end" seen in a test cluster which regularly has a lot of data deleted (and hence, shard deletions).

Initial repro: PR #13177

The problems seems to be with how removeOldDestinations interacts with krmSetRangeCoalesting_, which has a very ominious but nonspecific comment about "use with care". We believe the correct solution is to have removeOldDestinations modify the entries one at a time, because other calls made on the same transaction read and update overlapping rows. This PR does that and also updates the comment in krmSetRangeCoalescing_ about what the actual requirements are.

This PR also includes an updated test (relative to PR#13177) to reproduce the problem directly in removeOldDestinations when run against the old version (it's left in the code for reference and as a cautionary example, enabled only behind a very loud ifdef).

The updated test case is complicated due to the need to run in simulation and to deal with various injected errors that are simply a fact of life there. All it really wants to do is arrange the race in removeOldDestinations() and ensure the outcome is correct (or the bug is reproduced, if it's present in removeOldDestinations).

removeOldDestinations is moved to the end of the file to avoid a "multiple definitions" compile error, which may possibly be a clang bug. There is a newly-added prototype in a header which is identical to the definition. Neither the agent or I could figure it out. Moving the definition of the function to after the call site in MoveKeys.cpp avoided the compile error.

Testing:

20260512-020318-gglass-30749afe25d28060 compressed=True data_size=37003190 duration=4183985 ended=100000 fail_fast=10 max_runs=100000 pass=100000 priority=100 remaining=0 runtime=0:49:06 sanity=False started=100000 stopped=20260512-025224 submitted=20260512-020318 timeout=5400 username=gglass

* Update comment on krmSetRangeCoalescing(). Update removeOldDestinations() to call it in a way which does not generate corrupt metadata.  Convert the test case from PR13177 to test removeOldDestinations() directly.

* Retry loops around transactions

* avoid getting hosed by short reads

* formatting

* Address copilot review comments
This commit is contained in:
gxglass 2026-05-20 15:05:14 -07:00 committed by GitHub
parent c7c2af544b
commit 9ea25b1142
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 290 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

18
fdbclient/KeyRangeMap.cpp
View File

@ -213,9 +213,21 @@ Future<Void> krmSetRange(Reference<ReadYourWritesTransaction> tr, Key mapPrefix,
tr->set(withPrefix.end, oldValue);
}
// Sets a range of keys in a key range map, coalescing with adjacent regions if the values match
// Ranges outside of maxRange will not be coalesced
// CAUTION: use care when attempting to coalesce multiple ranges in the same prefix in a single transaction
// Sets a range of keys in a key range map, coalescing with adjacent regions if the values match.
// Ranges outside of maxRange will not be coalesced.
//
// REQUIREMENTS FOR CALLERS:
// - Multiple calls on the SAME prefix within a single transaction MUST be executed sequentially
// (co_await each one before starting the next). They MUST NOT be launched concurrently via
// waitForAll or similar. Each call does snapshot reads to determine coalescing boundaries and
// then writes clears/sets that extend beyond the requested range. Concurrent calls on the same
// prefix see the same snapshot state, compute overlapping clear ranges, and corrupt each other's
// boundary writes — producing adjacent same-value entries (fragmentation) that violate KRM
// invariants and crash downstream code.
// - Multiple calls on non-overlapping prefixes within a single transaction are safe to run
// concurrently because their reads and writes are disjoint.
// - The transaction MUST be a ReadYourWritesTransaction if sequential calls on the same prefix
// need each call to see the prior call's writes for correct coalescing.
template <class Transaction>
static Future<Void> krmSetRangeCoalescing_(Transaction* tr,
Key mapPrefix,

73
fdbserver/core/MoveKeys.cpp
View File

@ -784,28 +784,6 @@ Future<Void> cleanUpSingleShardDataMove(Database occ,
TraceEvent(SevInfo, "CleanUpSingleShardDataMoveEnd", dataMoveId).detail("Range", keys);
}
Future<Void> removeOldDestinations(Reference<ReadYourWritesTransaction> tr,
UID oldDest,
VectorRef<KeyRangeRef> shards,
KeyRangeRef currentKeys) {
KeyRef beginKey = currentKeys.begin;
std::vector<Future<Void>> actors;
for (int i = 0; i < shards.size(); i++) {
if (beginKey < shards[i].begin)
actors.push_back(krmSetRangeCoalescing(
tr, serverKeysPrefixFor(oldDest), KeyRangeRef(beginKey, shards[i].begin), allKeys, serverKeysFalse));
beginKey = shards[i].end;
}
if (beginKey < currentKeys.end)
actors.push_back(krmSetRangeCoalescing(
tr, serverKeysPrefixFor(oldDest), KeyRangeRef(beginKey, currentKeys.end), allKeys, serverKeysFalse));
return waitForAll(actors);
}
Future<std::vector<UID>> addReadWriteDestinations(KeyRangeRef shard,
std::vector<StorageServerInterface> srcInterfs,
std::vector<StorageServerInterface> destInterfs,
@ -1124,15 +1102,14 @@ static Future<Void> startMoveKeys(Database occ,
std::set<UID>::iterator oldDest;
// Remove old dests from serverKeys. In order for krmSetRangeCoalescing to work correctly in the
// same prefix for a single transaction, we must do most of the coalescing ourselves. Only the
// shards on the boundary of currentRange are actually coalesced with the ranges outside of
// currentRange. For all shards internal to currentRange, we overwrite all consecutive keys whose
// value is or should be serverKeysFalse in a single write
// Remove old dests from serverKeys. Each removeOldDestinations call
// executes its krmSetRangeCoalescing calls sequentially so that each
// sees the prior call's writes through the RYW transaction.
std::vector<Future<Void>> actors;
for (oldDest = oldDests.begin(); oldDest != oldDests.end(); ++oldDest)
if (std::find(servers.begin(), servers.end(), *oldDest) == servers.end())
actors.push_back(removeOldDestinations(tr, *oldDest, shardMap[*oldDest], currentKeys));
actors.push_back(removeOldDestinations(
tr, serverKeysPrefixFor(*oldDest), shardMap[*oldDest], currentKeys));
// Update serverKeys to include keys (or the currently processed subset of keys) for each SS in
// servers
@ -3524,3 +3501,43 @@ TEST_CASE("/fdbserver/MoveKeys/finishMoveKeysBackoff") {
return Void();
}
Future<Void> removeOldDestinations(Reference<ReadYourWritesTransaction> tr,
Key prefix,
VectorRef<KeyRangeRef> shards,
KeyRangeRef currentKeys) {
KeyRef beginKey = currentKeys.begin;
for (int i = 0; i < shards.size(); i++) {
if (beginKey < shards[i].begin) {
co_await krmSetRangeCoalescing(
tr, prefix, KeyRangeRef(beginKey, shards[i].begin), allKeys, serverKeysFalse);
}
beginKey = shards[i].end;
}
if (beginKey < currentKeys.end) {
co_await krmSetRangeCoalescing(tr, prefix, KeyRangeRef(beginKey, currentKeys.end), allKeys, serverKeysFalse);
}
#if DO_NOT_ENABLE_THIS_EXCEPT_TO_REPRODUCE_BUG_IN_TESTING
// BAD OLD LOGIC: fdbserver/workloads/KRMCoalescingFragmentation.cpp will
// generate improperly coalesced KRM entries if you comment out the above
// and uncomment this version.
std::vector<Future<Void>> actors;
for (int i = 0; i < shards.size(); i++) {
if (beginKey < shards[i].begin)
actors.push_back(
krmSetRangeCoalescing(tr, prefix, KeyRangeRef(beginKey, shards[i].begin), allKeys, serverKeysFalse));
beginKey = shards[i].end;
}
if (beginKey < currentKeys.end)
actors.push_back(
krmSetRangeCoalescing(tr, prefix, KeyRangeRef(beginKey, currentKeys.end), allKeys, serverKeysFalse));
co_await waitForAll(actors);
#endif
}

5
fdbserver/core/include/fdbserver/core/MoveKeys.h
View File

@ -198,4 +198,9 @@ Future<Void> checkMoveKeysLock(Transaction* tr,
const DDEnabledState* ddEnabledState,
bool isWrite = true);
Future<Void> removeOldDestinations(Reference<ReadYourWritesTransaction> tr,
Key prefix,
VectorRef<KeyRangeRef> shards,
KeyRangeRef currentKeys);
#endif

219
fdbserver/workloads/KRMCoalescingFragmentation.cpp Normal file
View File

@ -0,0 +1,219 @@
/*
* KRMCoalescingFragmentation.cpp
*
* This source file is part of the FoundationDB open source project
*
* Copyright 2013-2026 Apple Inc. and the FoundationDB project authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
#include "fdbclient/KeyRangeMap.h"
#include "fdbclient/NativeAPI.actor.h"
#include "fdbclient/ReadYourWrites.h"
#include "fdbclient/SystemData.h"
#include "fdbserver/core/MoveKeys.h"
#include "fdbserver/tester/workloads.h"
// Regression test for the krmSetRangeCoalescing fragmentation bug in
// removeOldDestinations() (MoveKeys.cpp).
//
// Calls the real removeOldDestinations with a test-only KRM prefix (in user
// keyspace, not system keyspace) so the test exercises the actual production
// code path without disturbing any system metadata.
//
// On UNFIXED code (concurrent waitForAll): produces adjacent same-value entries.
// On FIXED code (sequential co_await): produces a correctly coalesced map.
struct KRMCoalescingFragmentationWorkload : TestWorkload {
static constexpr auto NAME = "KRMCoalescingFragmentation";
Key testPrefix;
bool success;
explicit KRMCoalescingFragmentationWorkload(WorkloadContext const& wcx)
: TestWorkload(wcx), testPrefix("KRMFragTest/"_sr), success(false) {}
Future<Void> setup(Database const& cx) override { return Void(); }
Future<Void> start(Database const& cx) override {
if (clientId != 0)
return Void();
return runTest(cx);
}
Future<bool> check(Database const& cx) override {
if (clientId != 0)
return true;
return success;
}
void getMetrics(std::vector<PerfMetric>& m) override {}
private:
Future<Void> runTest(Database cx) {
// Step 1: Establish initial KRM state with alternating values.
// "" -> "1" "d" -> "" "j" -> "1" "\xff\xff" -> ""
{
auto tr = makeReference<ReadYourWritesTransaction>(cx);
loop {
Error err;
try {
co_await krmSetRange(tr, testPrefix, allKeys, serverKeysTrue);
co_await tr->commit();
break;
} catch (Error& e) {
err = e;
}
co_await tr->onError(err);
}
}
{
auto tr = makeReference<ReadYourWritesTransaction>(cx);
loop {
Error err;
try {
co_await krmSetRange(tr, testPrefix, KeyRangeRef("d"_sr, "j"_sr), serverKeysFalse);
co_await tr->commit();
break;
} catch (Error& e) {
err = e;
}
co_await tr->onError(err);
}
}
// Verify setup.
{
auto tr = makeReference<ReadYourWritesTransaction>(cx);
loop {
Error err;
try {
RangeResult result = co_await krmGetRanges(tr, testPrefix, allKeys);
if (result.back().key < allKeys.end) {
tr->reset();
continue;
}
TraceEvent evt("KRMFragTestSetupVerify");
evt.detail("NumEntries", result.size());
for (int i = 0; i < result.size(); i++) {
evt.detail(format("Key%d", i), result[i].key);
evt.detail(format("Val%d", i), result[i].value);
}
ASSERT(result.size() == 4);
ASSERT(result[0].value == serverKeysTrue);
ASSERT(result[1].key == "d"_sr && result[1].value == serverKeysFalse);
ASSERT(result[2].key == "j"_sr && result[2].value == serverKeysTrue);
TraceEvent("KRMFragTestSetupDone").detail("Entries", result.size());
break;
} catch (Error& e) {
err = e;
}
co_await tr->onError(err);
}
}
// Step 2: Call removeOldDestinations with the multi-gap pattern.
// Shards to keep: ["a","c"), ["e","g"), ["k","m")
// Gaps to clear: ["c","e") and ["g","k")
// currentKeys: ["a","m")
//
// The existing "" entry at "d" causes both gap clears to extend their
// coalescing through it, producing overlapping clears on unfixed code.
{
Arena arena;
VectorRef<KeyRangeRef> shards;
shards.push_back(arena, KeyRangeRef("a"_sr, "c"_sr));
shards.push_back(arena, KeyRangeRef("e"_sr, "g"_sr));
shards.push_back(arena, KeyRangeRef("k"_sr, "m"_sr));
KeyRangeRef currentKeys("a"_sr, "m"_sr);
auto tr = makeReference<ReadYourWritesTransaction>(cx);
loop {
Error err;
try {
co_await removeOldDestinations(tr, testPrefix, shards, currentKeys);
co_await tr->commit();
break;
} catch (Error& e) {
err = e;
}
co_await tr->onError(err);
}
}
// Step 3: Read back and verify the exact expected KRM state.
//
// Sequential coalescing should produce:
// "" -> "1" (server owns ["", "c"))
// "c" -> "" (gaps ["c","e") and ["g","k") coalesced with existing "" region ["d","j"))
// "k" -> "1" (server owns ["k", ...))
// <terminator>
//
// Gap 1 ["c","e") coalesces right through "d"->"" to "j", then Gap 2 ["g","k")
// coalesces left through the now-extended "" region back to "c".
{
auto tr = makeReference<ReadYourWritesTransaction>(cx);
loop {
Error err;
try {
RangeResult result = co_await krmGetRanges(tr, testPrefix, allKeys);
if (result.back().key < allKeys.end) {
tr->reset();
continue;
}
TraceEvent evt("KRMFragTestResult");
evt.detail("NumEntries", result.size());
for (int i = 0; i < result.size(); i++) {
evt.detail(format("Key%d", i), result[i].key);
evt.detail(format("Val%d", i), result[i].value);
}
bool fragmented = false;
for (int i = 1; i < (int)result.size() - 1; i++) {
if (result[i].value == result[i - 1].value) {
fragmented = true;
TraceEvent(SevError, "KRMFragTestFragmentationDetected")
.detail("Key1", result[i - 1].key)
.detail("Key2", result[i].key)
.detail("Value", result[i].value);
}
}
if (fragmented) {
TraceEvent(SevError, "KRMFragTestFailed")
.detail("Message", "removeOldDestinations produced fragmented KRM entries");
break;
}
// Verify the exact expected transitions: the gaps were cleared and
// coalesced, and the kept-shard boundaries are correct.
ASSERT_EQ(result.size(), 4);
ASSERT(result[0].key == ""_sr && result[0].value == serverKeysTrue);
ASSERT(result[1].key == "c"_sr && result[1].value == serverKeysFalse);
ASSERT(result[2].key == "k"_sr && result[2].value == serverKeysTrue);
success = true;
TraceEvent("KRMFragTestPassed").detail("Entries", result.size());
break;
} catch (Error& e) {
err = e;
}
co_await tr->onError(err);
}
}
}
};
WorkloadFactory<KRMCoalescingFragmentationWorkload> KRMCoalescingFragmentationWorkloadFactory;

1
tests/CMakeLists.txt
View File

@ -176,6 +176,7 @@ if(WITH_PYTHON)
add_fdb_test(TEST_FILES fast/IncrementTest.toml)
add_fdb_test(TEST_FILES fast/InventoryTestAlmostReadOnly.toml)
add_fdb_test(TEST_FILES fast/InventoryTestSomeWrites.toml)
add_fdb_test(TEST_FILES fast/KRMCoalescingFragmentation.toml)
add_fdb_test(TEST_FILES fast/LocalRatekeeper.toml)
add_fdb_test(TEST_FILES fast/LongStackWriteDuringRead.toml)
add_fdb_test(TEST_FILES fast/LowLatency.toml)

5
tests/fast/KRMCoalescingFragmentation.toml Normal file
View File

@ -0,0 +1,5 @@
[[test]]
testTitle = 'KRMCoalescingFragmentation'
[[test.workload]]
testName = 'KRMCoalescingFragmentation'