diff --git a/documentation/sphinx/source/release-notes.rst b/documentation/sphinx/source/release-notes.rst index f168379162..c74d0439c2 100644 --- a/documentation/sphinx/source/release-notes.rst +++ b/documentation/sphinx/source/release-notes.rst @@ -46,7 +46,7 @@ Fixes * The ``fileconfigure`` command in ``fdbcli`` could fail with an unknown error if the file did not contain a valid JSON object. `(PR #2017) `_. * Configuring regions would fail with an internal error if the cluster contained storage servers that didn't set a datacenter ID. `(PR #2017) `_. * Clients no longer prefer reading from servers with the same zone ID, because it could create hot shards. [6.2.3] `(PR #2019) `_. -* Data distribution no longer gets stuck when storage servers' localities are misconfigured and later corrected. [6.2.5] `(PR #2110) `_. +* Data distribution could fail to start if any storage servers had misconfigured locality information. This problem could persist even after the offending storage servers were removed or fixed. [6.2.5] `(PR #2110) `_. Status ------ diff --git a/fdbserver/DataDistribution.actor.cpp b/fdbserver/DataDistribution.actor.cpp index 2ae79118c6..8627b8e576 100644 --- a/fdbserver/DataDistribution.actor.cpp +++ b/fdbserver/DataDistribution.actor.cpp @@ -42,6 +42,8 @@ class TCTeamInfo; struct TCMachineInfo; class TCMachineTeamInfo; +ACTOR Future checkAndRemoveInvalidLocalityAddr(DDTeamCollection* self); + struct TCServerInfo : public ReferenceCounted { UID id; StorageServerInterface lastKnownInterface; @@ -993,6 +995,12 @@ struct DDTeamCollection : ReferenceCounted { TraceEvent(SevWarnAlways, "MissingLocality") .detail("Server", i->first.uniqueID) .detail("Locality", i->first.locality.toString()); + auto addr = i->first.address(); + self->invalidLocalityAddr.insert(AddressExclusion(addr.ip, addr.port)); + if (self->checkInvalidLocalities.isReady()) { + self->checkInvalidLocalities = checkAndRemoveInvalidLocalityAddr(self); + self->addActor.send(self->checkInvalidLocalities); + } } self->addServer(i->first, i->second, self->serverTrackerErrorOut, 0); } @@ -3578,7 +3586,7 @@ ACTOR Future monitorStorageServerRecruitment(DDTeamCollection* self) { } ACTOR Future checkAndRemoveInvalidLocalityAddr(DDTeamCollection* self) { - state int checkCount = 0; + state double start = now(); state Transaction tr(self->cx); loop { @@ -3590,10 +3598,10 @@ ACTOR Future checkAndRemoveInvalidLocalityAddr(DDTeamCollection* self) { // Because worker's processId can be changed when its locality is changed, we cannot watch on the old // processId; This actor is inactive most time, so iterating all workers incurs little performance overhead. - Future> workers = getWorkers(&tr); - std::set existingAddrs; - for (int i = 0; i < workers.get().size(); i++) { - const ProcessData& workerData = workers.get()[i]; + state vector workers = wait(getWorkers(&tr)); + state std::set existingAddrs; + for (int i = 0; i < workers.size(); i++) { + const ProcessData& workerData = workers[i]; AddressExclusion addr(workerData.address.ip, workerData.address.port); existingAddrs.insert(addr); if (self->invalidLocalityAddr.count(addr) && @@ -3603,6 +3611,8 @@ ACTOR Future checkAndRemoveInvalidLocalityAddr(DDTeamCollection* self) { } } + wait(yield(TaskPriority::DataDistribution)); + // In case system operator permanently excludes workers on the address with invalid locality for (auto addr = self->invalidLocalityAddr.begin(); addr != self->invalidLocalityAddr.end();) { if (!existingAddrs.count(*addr)) { @@ -3617,10 +3627,9 @@ ACTOR Future checkAndRemoveInvalidLocalityAddr(DDTeamCollection* self) { break; } - checkCount++; - if (checkCount > 100) { - // The incorrect locality info is not properly correctly on time - TraceEvent(SevWarn, "InvalidLocality").detail("Addresses", self->invalidLocalityAddr.size()); + if (now() - start > 300) { // Report warning if invalid locality is not corrected within 300 seconds + // The incorrect locality info has not been properly corrected in a reasonable time + TraceEvent(SevWarn, "PersistentInvalidLocality").detail("Addresses", self->invalidLocalityAddr.size()); } } catch (Error& e) { wait(tr.onError(e)); diff --git a/fdbserver/Knobs.cpp b/fdbserver/Knobs.cpp index 80d6892e5e..69ab9acfe2 100644 --- a/fdbserver/Knobs.cpp +++ b/fdbserver/Knobs.cpp @@ -188,7 +188,7 @@ ServerKnobs::ServerKnobs(bool randomize, ClientKnobs* clientKnobs) { init( REBALANCE_MAX_RETRIES, 100 ); init( DD_OVERLAP_PENALTY, 10000 ); init( DD_VALIDATE_LOCALITY, true ); if( randomize && BUGGIFY ) DD_VALIDATE_LOCALITY = false; - init( DD_CHECK_INVALID_LOCALITY_DELAY, 60 ); if( randomize && BUGGIFY ) DD_CHECK_INVALID_LOCALITY_DELAY = 10 + deterministicRandom()->random01() * 600; + init( DD_CHECK_INVALID_LOCALITY_DELAY, 60 ); if( randomize && BUGGIFY ) DD_CHECK_INVALID_LOCALITY_DELAY = 1 + deterministicRandom()->random01() * 600; // TeamRemover TR_FLAG_DISABLE_MACHINE_TEAM_REMOVER = false; if( randomize && BUGGIFY ) TR_FLAG_DISABLE_MACHINE_TEAM_REMOVER = deterministicRandom()->random01() < 0.1 ? true : false; // false by default. disable the consistency check when it's true