From e4591672604695f18b43b85f3296e91e07f98352 Mon Sep 17 00:00:00 2001 From: michael stack Date: Sun, 26 Jul 2026 17:26:45 -0700 Subject: [PATCH] Make ShardsAffectedByTeamFailure::moveShard range-safe Lowering DD_MAX_PIPELINE_MOVES surfaced graceful excludes that hung for hours, and in one case indefinitely, *after* the excluded machine's data had already drained on schedule. Raising the cap made it go away. The cap is not the defect though: it changes how often the defect is hit. In principle the same stranding can happen with the cap wide open -- DDQueue produces partial-range moves on its own -- but that has not been demonstrated, so treat the cap as the trigger we have evidence for and the accounting bug below as the cause. The removal gate (waitForAllDataRemoved) needs both canRemoveStorageServer(), which reads the authoritative on-disk serverKeys, and getNumberOfShards() == 0, which reads the in-memory ShardsAffectedByTeamFailure map. They can disagree, and then the server is never removed and `fdbcli exclude` never returns. They disagree because moveShard() is not range-safe. When the move's key range fully covers a tracked range it erases the source teams; when it only partially covers one it appends the destination and never erases, and never splits the tracked range at the move boundary. erase() holds the only decrement of the per-server counter, so the drained servers stay counted until a later fully-covering move, a defineShard(), or a DD restart rebuilds the map from disk. Partial-range moves are routine: a queued relocation's keys go stale when a shard merge coarsens the tracked range underneath it, queueRelocation truncates queued relocations against overlapping newer ones, and launchQueuedWork launches the boundary fragments getRangesAffectedByInsertion returns, which are strict subsets of an older relocation's range by construction. A low cap holds requests outside DDQueue where its supersede logic cannot correct them, which is why a small cap raises the rate. Fix: split the tracked range at the move's boundaries first, so every affected range is fully contained and the source teams are erased for exactly the range that moved. cancelMove() already did this identical split for the same reason; factor it out as splitTrackedShardsAtBoundaries() and share it. Boundaries are added only where one is missing and the next defineShard() merges them back. Gated by DD_SPLIT_TRACKED_SHARDS_ON_MOVE (default true) since this is a data distribution hot path; the partial-overlap branch stays as a safety net with ASSERT_WE_THINK rather than becoming an assert, because dropping a destination team would be worse than over-counting a source. Also enforce at the removal gate the invariant DDTeamCollection.actor.cpp:5964 documents as a commented-out ASSERT and nothing checks: when the on-disk state says the server is empty, no team containing it should remain in the map. If one does, reconcile to the on-disk truth and trace loudly instead of hanging. Gated by DD_RECONCILE_SHARDS_ON_EXCLUDE (default true). The new scrubServer() drops the whole team reference rather than reusing removeFailedServerForRange(), whose helper edits team membership in place and would leave a shrunken team that exists nowhere in DDTeamCollection -- which teamTracker() then re-relocates forever at PRIORITY_TEAM_REDUNDANT. This was undiagnosable in production: waitForAllDataRemoved's trace was SevVerbose and the map's erase/insert traces are DisabledTraceEvent, so a server sat empty and registered for 33 hours with no signal. Add a repeating SevWarnAlways when canRemove is true but the count is not zero. SAF::check() cannot catch this on its own -- the two halves of the map stay mutually consistent and only the semantics are wrong. Tests cover a sub-range move erasing the source team for the moved half and retaining it for the other, the scrub leaving the range on the real replacement team, and the degenerate scrub paths. Simulation test excludes a machine with the cap pinned small. Validated on Joshua with 100k- and 10k-run correctness ensembles, both clean; the boundary split touches every data move, so a broad corpus run is the signal that matters. Known risk: the split adds tracked-range entries on a hot path. Growth should be bounded, but the steady-state count has not been measured at scale. --- fdbserver/core/ServerKnobs.cpp | 2 + fdbserver/core/include/fdbserver/core/Knobs.h | 12 ++ fdbserver/datadistributor/DDTxnProcessor.cpp | 62 ++++++- .../ShardsAffectedByTeamFailure.cpp | 152 +++++++++++++++--- .../ShardsAffectedByTeamFailure.h | 41 ++++- .../ShardsAffectedByTeamFailureTests.cpp | 146 +++++++++++++++++ tests/CMakeLists.txt | 1 + ...eIncludeStorageServersShardAccounting.toml | 35 ++++ 8 files changed, 419 insertions(+), 32 deletions(-) create mode 100644 tests/slow/ExcludeIncludeStorageServersShardAccounting.toml diff --git a/fdbserver/core/ServerKnobs.cpp b/fdbserver/core/ServerKnobs.cpp index d37d8d6ab9c..05c371fec89 100644 --- a/fdbserver/core/ServerKnobs.cpp +++ b/fdbserver/core/ServerKnobs.cpp @@ -285,6 +285,8 @@ void ServerKnobs::initialize(Randomize randomize, ClientKnobs* clientKnobs, IsSi // guess (thus 20 moves). Do not buggify this too small: testing under artificial scarcity results in // uninteresting degenerate cases. init( DD_MAX_PIPELINE_MOVES, 1000 ); if( randomize && buggify() ) DD_MAX_PIPELINE_MOVES = 20; + init( DD_RECONCILE_SHARDS_ON_EXCLUDE, true ); + init( DD_SPLIT_TRACKED_SHARDS_ON_MOVE, true ); init( DD_REBALANCE_RESET_AMOUNT, 30 ); init( INFLIGHT_PENALTY_HEALTHY, 1.0 ); init( INFLIGHT_PENALTY_UNHEALTHY, 500.0 ); diff --git a/fdbserver/core/include/fdbserver/core/Knobs.h b/fdbserver/core/include/fdbserver/core/Knobs.h index ca1c827cc73..7630f2bc0b6 100644 --- a/fdbserver/core/include/fdbserver/core/Knobs.h +++ b/fdbserver/core/include/fdbserver/core/Knobs.h @@ -160,6 +160,18 @@ class SWIFT_CXX_IMMORTAL_SINGLETON_TYPE ServerKnobs : public KnobsImpl 0, reconcile the map to the on-disk truth so removal is not + // blocked. When false, fall back to the legacy behavior of waiting for + // the count to reach 0 on its own. + bool DD_SPLIT_TRACKED_SHARDS_ON_MOVE; // Have ShardsAffectedByTeamFailure::moveShard() split the tracked + // shard at the move's key boundaries, so that a move of a strict + // sub-range of a tracked shard erases the source teams for the range + // that actually moved. When false, restore the legacy behavior of + // appending the destination team to the whole enclosing shard + // without erasing the source, which permanently over-counts drained + // source servers in storageServerShards. int DD_REBALANCE_RESET_AMOUNT; double INFLIGHT_PENALTY_HEALTHY; double INFLIGHT_PENALTY_REDUNDANT; diff --git a/fdbserver/datadistributor/DDTxnProcessor.cpp b/fdbserver/datadistributor/DDTxnProcessor.cpp index 223030a8168..7181b9d349f 100644 --- a/fdbserver/datadistributor/DDTxnProcessor.cpp +++ b/fdbserver/datadistributor/DDTxnProcessor.cpp @@ -853,13 +853,69 @@ class DDTxnProcessorImpl { if (ver > addedVersion + SERVER_KNOBS->MAX_READ_TRANSACTION_LIFE_VERSIONS) { bool canRemove = co_await canRemoveStorageServer(tr, serverID); auto shards = shardsAffectedByTeamFailure->getNumberOfShards(serverID); - TraceEvent(SevVerbose, "WaitForAllDataRemoved") + // Polled every ALL_DATA_REMOVED_DELAY for every server under removal, so it is + // suppressed; the diagnostic signal is ShardsAffectedStrandedOnExclude below. + TraceEvent(SevInfo, "WaitForAllDataRemoved") + .suppressFor(30.0) .detail("Server", serverID) .detail("CanRemove", canRemove) .detail("Shards", shards); ASSERT_GE(shards, 0); - if (canRemove && shards == 0) { - co_return; + if (canRemove) { + if (shards == 0) { + co_return; + } + // canRemoveStorageServer() consults the authoritative on-disk serverKeys: when it is + // true the server owns no data, and DDTeamCollection.actor.cpp:5964 documents the + // matching in-memory invariant as a commented-out ASSERT -- no team containing the + // server should remain in ShardsAffectedByTeamFailure. Nothing enforces it. The count + // can be stranded > 0 by any move whose key range only partially covers a tracked + // range: moveShard()'s partial-overlap branch appends the destination team without + // erasing the drained source team, and only a later fully-covering move, a + // defineShard(), or a DD restart rewrites that entry. Such a stale count blocks + // removeStorageServer() indefinitely. Since the on-disk state is authoritative and + // already says the server is empty, reconcile the map to it and trace loudly rather + // than hang. moveShard() is range-safe as of DD_SPLIT_TRACKED_SHARDS_ON_MOVE; this + // stays as enforcement, because we cannot prove every source of partial-range moves + // has been found. + // + // This event repeating is the "still stranded" signal; it is the one trace that makes + // this failure mode diagnosable from logs alone. + TraceEvent(SevWarnAlways, "ShardsAffectedStrandedOnExclude") + .suppressFor(60.0) + .detail("Server", serverID) + .detail("StrandedShards", shards) + .detail("TotalTrackedShards", shardsAffectedByTeamFailure->getNumberOfShards()) + .detail("WillReconcile", SERVER_KNOBS->DD_RECONCILE_SHARDS_ON_EXCLUDE); + if (SERVER_KNOBS->DD_RECONCILE_SHARDS_ON_EXCLUDE) { + CODE_PROBE(true, + "Reconciled a stranded ShardsAffectedByTeamFailure count on graceful " + "exclude", + probe::decoration::rare); + double start = now(); + auto scrub = shardsAffectedByTeamFailure->scrubServer(serverID); + TraceEvent(SevWarnAlways, "ShardsAffectedReconciledOnExclude") + .detail("Server", serverID) + .detail("StrandedShards", shards) + .detail("ShardsScanned", scrub.shardsScanned) + .detail("ShardsRewritten", scrub.shardsRewritten) + .detail("OwnerlessShards", scrub.ownerlessShards) + .detail("SampleRange", scrub.sampleRange) + .detail("Elapsed", now() - start); + auto remaining = shardsAffectedByTeamFailure->getNumberOfShards(serverID); + ASSERT_WE_THINK(remaining == 0); + if (remaining == 0) { + co_return; + } + // Unreachable by construction: scrubServer() drops every team naming the server, + // and only those teams contribute to the count. Rather than deregister a server + // the map still references, keep polling and make the anomaly loud. + TraceEvent(SevWarnAlways, "ShardsAffectedReconcileIncomplete") + .detail("Server", serverID) + .detail("RemainingShards", remaining); + } + // Knob disabled: fall through to the delay below and keep waiting for the count to + // reach 0 on its own (legacy behavior). } } diff --git a/fdbserver/datadistributor/ShardsAffectedByTeamFailure.cpp b/fdbserver/datadistributor/ShardsAffectedByTeamFailure.cpp index 86318c5a2c4..d2c410b74d2 100644 --- a/fdbserver/datadistributor/ShardsAffectedByTeamFailure.cpp +++ b/fdbserver/datadistributor/ShardsAffectedByTeamFailure.cpp @@ -19,6 +19,7 @@ */ #include "ShardsAffectedByTeamFailure.h" +#include "fdbserver/core/Knobs.h" std::vector ShardsAffectedByTeamFailure::getShardsFor(Team team) const { std::vector r; @@ -116,6 +117,40 @@ void ShardsAffectedByTeamFailure::defineShard(KeyRangeRef keys) { check(); } +void ShardsAffectedByTeamFailure::splitTrackedShardsAtBoundaries(KeyRangeRef keys) { + // Recreate only the boundary points of `keys`. defineShard() must not be used here: it would merge all + // tracked shards inside keys into one entry, losing the distinct destination teams of overlapping newer + // moves. team_shards is keyed by the EXACT (team, range) pair, so a tracked shard that straddles a + // boundary has to be removed from it before the split and its pieces re-added afterwards, or the + // team_shards <-> shard_teams invariant (see check()) breaks. + std::vector rangesToSplit; + auto beginRange = shard_teams.rangeContaining(keys.begin); + if (beginRange->begin() != keys.begin) { + rangesToSplit.push_back(beginRange->range()); + } + auto endRange = shard_teams.rangeContaining(keys.end); + if (endRange->begin() != keys.end && (rangesToSplit.empty() || rangesToSplit.back() != endRange->range())) { + rangesToSplit.push_back(endRange->range()); + } + if (rangesToSplit.empty()) { + // Both boundaries already exist; modify() would be a no-op. + return; + } + for (const auto& range : rangesToSplit) { + for (const auto& team : shard_teams.rangeContaining(range.begin)->value().first) { + erase(team, range); + } + } + shard_teams.modify(keys); + for (const auto& range : rangesToSplit) { + for (auto splitRange : shard_teams.containedRanges(range)) { + for (const auto& team : splitRange.value().first) { + insert(team, splitRange.range()); + } + } + } +} + void ShardsAffectedByTeamFailure::moveShard(KeyRangeRef keys, std::vector destinationTeams) { /*TraceEvent("ShardsAffectedByTeamFailureMove") .detail("KeyBegin", keys.begin) @@ -123,9 +158,31 @@ void ShardsAffectedByTeamFailure::moveShard(KeyRangeRef keys, std::vector .detail("NewTeamSize", destinationTeam.size()) .detail("NewTeam", describe(destinationTeam));*/ + // A relocation is frequently launched for a STRICT SUB-RANGE of a tracked shard, for three independent + // reasons. (1) A relocation's keys are captured when it is created, so an intervening defineShard() -- + // notably from a shard merge, which coarsens several tracked shards into one before sending its own + // relocation -- can widen the tracked shard underneath a request that is still queued. (2) DDQueue + // truncates already-queued relocations against overlapping newer ones (queueRelocation). (3) + // launchQueuedWork starts a relocator per range returned by getRangesAffectedByInsertion, which includes + // the pieces of a live relocator straddling the new range's boundaries, i.e. strict subsets of an older + // relocation's range. Without a boundary split such a move hits the partial-overlap branch below, which + // appends the destination team but never erases the drained source team -- so a source server stays counted + // in storageServerShards forever, and a gracefully excluded server is never removable because its removal + // gate requires getNumberOfShards(server) == 0. Splitting the tracked shard at the move's boundaries first + // makes every affected shard fully contained, so the source teams are erased for exactly the range that + // actually moved. The extra boundaries are transient: the next defineShard() from a shard-tracker + // split/merge over a coarser range merges them back. + if (SERVER_KNOBS->DD_SPLIT_TRACKED_SHARDS_ON_MOVE) { + splitTrackedShardsAtBoundaries(keys); + } + auto ranges = shard_teams.intersectingRanges(keys); std::vector, std::vector>, KeyRange>> modifiedShards; for (auto it = ranges.begin(); it != ranges.end(); ++it) { + // After splitTrackedShardsAtBoundaries() every intersecting range is contained in keys, so the + // partial-overlap branch below is unreachable. It is retained as a safety net rather than an assert + // because losing a destination team in production is worse than over-counting a source team. + ASSERT_WE_THINK(!SERVER_KNOBS->DD_SPLIT_TRACKED_SHARDS_ON_MOVE || keys.contains(it->range())); if (keys.contains(it->range())) { // erase the many teams that were associated with this one shard for (auto t = it->value().first.begin(); t != it->value().first.end(); ++t) { @@ -170,31 +227,9 @@ std::vector ShardsAffectedByTeamFailure::cancelMove(KeyRangeRef keys, const std::vector& destinationTeams, const std::vector& sourceTeams) { std::vector restoredRanges; - // A later shard split or merge can leave the cancelled move range strictly inside a tracked shard. Recreate only - // the move's boundary points before removing its destinations. defineShard() would merge all tracked shards inside - // keys, losing the distinct destinations of overlapping newer moves. - std::vector rangesToSplit; - auto beginRange = shard_teams.rangeContaining(keys.begin); - if (beginRange->begin() != keys.begin) { - rangesToSplit.push_back(beginRange->range()); - } - auto endRange = shard_teams.rangeContaining(keys.end); - if (endRange->begin() != keys.end && (rangesToSplit.empty() || rangesToSplit.back() != endRange->range())) { - rangesToSplit.push_back(endRange->range()); - } - for (const auto& range : rangesToSplit) { - for (const auto& team : shard_teams.rangeContaining(range.begin)->value().first) { - erase(team, range); - } - } - shard_teams.modify(keys); - for (const auto& range : rangesToSplit) { - for (auto splitRange : shard_teams.containedRanges(range)) { - for (const auto& team : splitRange.value().first) { - insert(team, splitRange.range()); - } - } - } + // A later shard split or merge can leave the cancelled move range strictly inside a tracked shard, so + // recreate the move's boundary points before removing its destinations. + splitTrackedShardsAtBoundaries(keys); auto ranges = shard_teams.containedRanges(keys); for (auto it = ranges.begin(); it != ranges.end(); ++it) { std::vector retainedTeams; @@ -311,6 +346,73 @@ void ShardsAffectedByTeamFailure::removeFailedServerForRange(KeyRangeRef keys, c check(); } +ShardsAffectedByTeamFailure::ScrubResult ShardsAffectedByTeamFailure::scrubServer(const UID& serverID) { + auto containsServer = [&serverID](const std::vector& teams) { + return std::any_of(teams.begin(), teams.end(), [&serverID](const Team& t) { return t.hasServer(serverID); }); + }; + auto without = [&serverID](const std::vector& teams) { + std::vector retained; + for (const auto& t : teams) { + if (!t.hasServer(serverID)) { + retained.push_back(t); + } + } + return retained; + }; + + ScrubResult result; + std::vector restartRanges; + // Values are mutated in place; no shard_teams range is inserted or erased, so the iteration stays valid. + auto rs = shard_teams.ranges(); + for (auto it = rs.begin(); it != rs.end(); ++it) { + ++result.shardsScanned; + auto& teams = it->value(); + const bool inCurrent = containsServer(teams.first); + if (!inCurrent && !containsServer(teams.second)) { + continue; + } + + const KeyRange range = it->range(); + std::vector retained = without(teams.first); + std::vector retainedPrev = without(teams.second); + + if (inCurrent) { + // Only the current-team list is mirrored in team_shards (and therefore in + // storageServerShards), so it is the only one that needs the erase/insert dance. + for (const auto& t : teams.first) { + erase(t, range); + } + if (retained.empty()) { + retained = retainedPrev; + retainedPrev.clear(); + } + for (const auto& t : retained) { + insert(t, range); + } + teams.first = retained; + if (retained.empty()) { + ++result.ownerlessShards; + restartRanges.push_back(range); + } + } + // getSourceServerIdsFor() prefers the previous-source list, so a drained server left there would + // still be handed out as a relocation source. + teams.second = retainedPrev; + + if (result.shardsRewritten == 0) { + result.sampleRange = range; + } + ++result.shardsRewritten; + } + check(); + + // Sent after the map is consistent: the receiver re-enters this object via defineShard(). + for (const auto& range : restartRanges) { + restartShardTracker.send(range); + } + return result; +} + auto ShardsAffectedByTeamFailure::intersectingRanges(KeyRangeRef keyRange) const -> decltype(shard_teams)::ConstRanges { return shard_teams.intersectingRanges(keyRange); } diff --git a/fdbserver/datadistributor/ShardsAffectedByTeamFailure.h b/fdbserver/datadistributor/ShardsAffectedByTeamFailure.h index 599816f7f2b..e5b31b811af 100644 --- a/fdbserver/datadistributor/ShardsAffectedByTeamFailure.h +++ b/fdbserver/datadistributor/ShardsAffectedByTeamFailure.h @@ -62,6 +62,14 @@ class ShardsAffectedByTeamFailure : public ReferenceCounted decltype(shard_teams)::ConstRanges; @@ -143,6 +158,24 @@ class ShardsAffectedByTeamFailure : public ReferenceCounted the gate opens and the server can be removed. + ASSERT_EQ(shards.getNumberOfShards(excluded), 0); + // The surviving replica's accounting is untouched, and -- the point of scrubServer() over + // removeFailedServerForRange() -- the shard is left attributed to the REAL replacement team only. A shrunken + // {UID(1,0), UID(2,0)} team here would exist nowhere in DDTeamCollection and would be re-relocated forever at + // PRIORITY_TEAM_REDUNDANT by teamTracker()'s "team not found" path. + auto teams = shards.getTeamsFor("e"_sr); + ASSERT_EQ(teams.first.size(), 1); + ASSERT(teams.first[0] == replacementTeam); + ASSERT_EQ(shards.getNumberOfShards(replacementTeam), 1); + ASSERT_EQ(shards.getNumberOfShards(UID(1, 0)), 0); + for (const auto& id : shards.getSourceServerIdsFor("e"_sr)) { + ASSERT(id != excluded); + } + shards.check(); + + return Void(); +} + +// The degenerate case the scrub has to have an answer for: the stranded server's team is the ONLY team the map +// has for that shard, so there is no real owner to fall back to. Dropping the reference must still clear the +// count (otherwise the exclude hangs) and must not leave a fabricated shrunken team behind. +TEST_CASE("/DataDistributor/ShardsAffectedByTeamFailure/ScrubServerOnlyTeam") { + ShardsAffectedByTeamFailure shards; + shards.setCheckMode(ShardsAffectedByTeamFailure::CheckMode::ForceCheck); + + const UID excluded(9, 0); + const ShardsAffectedByTeamFailure::Team teamWithExcluded({ UID(1, 0), UID(2, 0), excluded }, true); + const KeyRange shardRange = KeyRangeRef("e"_sr, "z"_sr); + + shards.assignRangeToTeams(shardRange, { teamWithExcluded }); + ASSERT_EQ(shards.getNumberOfShards(excluded), 1); + + auto scrub = shards.scrubServer(excluded); + ASSERT_EQ(scrub.shardsRewritten, 1); + // No surviving team and no previous-source team to promote: the entry is left with an empty current-team + // list, which is the state a freshly-initialized map is in, and the shard tracker is asked to restart. + ASSERT_EQ(scrub.ownerlessShards, 1); + ASSERT_EQ(shards.getNumberOfShards(excluded), 0); + ASSERT_EQ(shards.getNumberOfShards(UID(1, 0)), 0); + ASSERT(shards.getTeamsFor("e"_sr).first.empty()); + shards.check(); + + return Void(); +} + +// A stranded server can also be left in the PREVIOUS-SOURCE list, which is not mirrored in team_shards (so it +// does not affect getNumberOfShards) but IS what getSourceServerIdsFor() prefers -- a drained server left there +// would be handed back out as a relocation source. +TEST_CASE("/DataDistributor/ShardsAffectedByTeamFailure/ScrubServerPreviousSources") { + ShardsAffectedByTeamFailure shards; + shards.setCheckMode(ShardsAffectedByTeamFailure::CheckMode::ForceCheck); + + const UID excluded(9, 0); + const ShardsAffectedByTeamFailure::Team teamWithExcluded({ UID(1, 0), UID(2, 0), excluded }, true); + const ShardsAffectedByTeamFailure::Team replacementTeam({ UID(3, 0), UID(4, 0), UID(5, 0) }, true); + const KeyRange shardRange = KeyRangeRef("e"_sr, "z"_sr); + + // A full-shard move leaves the old team in the previous-source list (no finishMove yet). + shards.assignRangeToTeams(shardRange, { teamWithExcluded }); + shards.moveShard(shardRange, { replacementTeam }); + ASSERT_EQ(shards.getNumberOfShards(excluded), 0); // the count itself was reconciled by the full-shard move + auto sources = shards.getSourceServerIdsFor("e"_sr); + ASSERT(std::find(sources.begin(), sources.end(), excluded) != sources.end()); // still a candidate source + + auto scrub = shards.scrubServer(excluded); + ASSERT_EQ(scrub.shardsRewritten, 1); + ASSERT_EQ(scrub.ownerlessShards, 0); + ASSERT_EQ(shards.getNumberOfShards(excluded), 0); + for (const auto& id : shards.getSourceServerIdsFor("e"_sr)) { + ASSERT(id != excluded); + } + // The current team is untouched. + auto teams = shards.getTeamsFor("e"_sr); + ASSERT_EQ(teams.first.size(), 1); + ASSERT(teams.first[0] == replacementTeam); + shards.check(); + + return Void(); +} diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 72d5f3f5cd7..9219e765cac 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -424,6 +424,7 @@ if(WITH_PYTHON) add_fdb_test(TEST_FILES slow/DDBalanceAndRemoveStatus.toml) add_fdb_test(TEST_FILES slow/DifferentClustersSameRV.toml) add_fdb_test(TEST_FILES slow/ExcludeIncludeStorageServers.toml) + add_fdb_test(TEST_FILES slow/ExcludeIncludeStorageServersShardAccounting.toml) add_fdb_test(TEST_FILES slow/FastTriggeredWatches.toml) add_fdb_test(TEST_FILES slow/LongRunning.toml LONG_RUNNING) add_fdb_test(TEST_FILES slow/LowLatencyWithFailures.toml) diff --git a/tests/slow/ExcludeIncludeStorageServersShardAccounting.toml b/tests/slow/ExcludeIncludeStorageServersShardAccounting.toml new file mode 100644 index 00000000000..caa6c736464 --- /dev/null +++ b/tests/slow/ExcludeIncludeStorageServersShardAccounting.toml @@ -0,0 +1,35 @@ +[configuration] +# Keep one spare machine per datacenter so excluding a storage server cannot exhaust a replicated team. +extraMachineCountDC = 1 + +[[knobs]] +# Regression coverage for the graceful-exclude finalize stall: a relocation whose key range only partially +# covers a tracked ShardsAffectedByTeamFailure range used to leave the drained server counted in that map, +# which blocks removeStorageServer() indefinitely even though the server's on-disk serverKeys are empty. +# moveShard() now splits the tracked range at the move boundary so the source team is erased, and the removal +# gate reconciles any residual stale count against the on-disk truth. Either way the exclude must complete. +# +# The cap is pinned small (the buggified value) so the scenario is exercised on every run instead of +# occasionally: holding relocation requests outside DDQueue, where its supersede logic cannot correct a range +# that has gone stale, raises the rate of partial-range moves. It is not a precondition -- DDQueue's own +# truncation and boundary-fragment launching produce them at any cap. +# +# To see the pre-fix behaviour locally: --knobs dd_split_tracked_shards_on_move=false, plus +# dd_reconcile_shards_on_exclude=false to reproduce the hang itself rather than the repair. +dd_max_pipeline_moves = 20 +# Large teams would let a shard be assigned more servers than the replication factor, adding a second unrelated +# source of multi-team ranges; keep it off so the pipeline cap is the only variable under test. +dd_max_shards_on_large_teams = 0 + +[[test]] +testTitle = 'ExcludeIncludeStorageServersShardAccounting' + + # Continuously write and verify data so excluded storage servers actually own shards that must drain, + # generating the re-replication traffic that, under the small pipeline cap, reproduces the stranding. + [[test.workload]] + testName = 'Cycle' + transactionsPerSecond = 2500.0 + testDuration = 120.0 + + [[test.workload]] + testName = 'ExcludeIncludeStorageServers'