[#956] Tell a failed entryUUID search apart from an entry which is not there - #968
[#956] Tell a failed entryUUID search apart from an entry which is not there#968vharseko wants to merge 3 commits into
Conversation
|
Ordinal moved: Six open branches had each read 310 as the first ordinal free in master and taken it - #935, #945, Nothing catches this on the way in. The additions land in different parts of the file, so git merges The open PRs which add to the file now hold 310-325 with nothing claimed twice:
No Java moved with it: the generated constant is the key name without its ordinal, so the rename is |
4110d0c to
bdb4f49
Compare
|
Rebased on master; the conflict was #948 moving the block this change rewrites (bdb4f49) Master carries #948 now, which put the whole in-place attempt of a replayed change - building the Resolved by dropping the copy this branch carried and applying its three edits to the block where
The 50 ms are now waited holding the replay read lock, which is where the
The diff is the same 270 insertions / 37 deletions over the same three files as before the rebase, |
bdb4f49 to
d68a79c
Compare
|
Rebased again, on master with #965 (d68a79c) #965 landed while this was waiting, and it reads the same entryUUID search this change is about: The conflict was in The diff is unchanged at 270 insertions / 37 deletions over the same three files. The The |
maximthomas
left a comment
There was a problem hiding this comment.
praise: the fix is small and lands where the read happens.
SearchFailedException is thrown at the one site that reads the data (findEntryDN),
every caller either answers SEARCH_FAILED or stops the pre-op with UNAVAILABLE, and the
replay loop needs one flag on top of the server-failure retry it already had. NamingConflictTest
is 10/10 at HEAD, and the OOME / alert contract below the loop is untouched. The comments at
LDAPReplicationDomain.java:2876-2894 and the PR body state the deliberate choices (the parse
failure retried, the error naming the conflict code) instead of leaving the reader to guess, and
ShortCircuitPlugin with a bounded maxTimes is the right tool for "fails, then serves again".
opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:3524
issue (blocking): findEntryDN() now throws on NO_SUCH_OBJECT, so a replayed Add of the
domain base entry into an empty replica never lands.
Two empty replicas share EMPTY_BACKEND_GENERATION_ID (48), so no initialize is needed and the
first change replayed is the base entry itself. Its pre-op hook runs findEntryDN(uuid) at
:1856, before the parentEntryUUID == null short-cut at :1875. A backend that serves the base
DN but has no base entry answers NO_SUCH_OBJECT on every route (EntryContainer.fetchBaseEntry);
there is no "SUCCESS with zero entries" path. At BASE that was null -> the Add went through. At
HEAD it is SearchFailedException -> UNAVAILABLE -> 10 attempts -> redeliveries until
replay-give-up-delay (300 s) -> the change is skipped with a "replica diverged" alert.
The same code comes back when no backend serves the DN (SearchOperationBasis:1211,
backend offline or being rebuilt), which is the case this PR must keep catching — so a bare
NO_SUCH_OBJECT whitelist would reopen #956. Ask the backend instead:
if (search.getResultCode() != ResultCode.SUCCESS)
{
if (search.getResultCode() == ResultCode.NO_SUCH_OBJECT && baseEntryIsAbsentFromALiveBackend())
{
// The backend serves the base DN and has no base entry yet: the search ran, and nothing
// is below a base entry which is not there. This is the empty replica about to receive it.
return null;
}
throw new SearchFailedException(uuid, ...);
}
private boolean baseEntryIsAbsentFromALiveBackend()
{
final LocalBackend<?> backend =
getServerContext().getBackendConfigManager().findLocalBackendForEntry(getBaseDN());
if (backend == null)
{
return false; // nothing serves the DN: offline or being rebuilt - the search did not run
}
try
{
return !backend.entryExists(getBaseDN());
}
catch (DirectoryException e)
{
return false; // the storage failed to answer - the search did not run
}
}And a test that would have caught it — no test in src/test/.../replication replays the
base-entry AddMsg into a backend without one:
@Test
public void baseEntryIsAddedToAnEmptyReplica() throws Exception
{
TestCaseUtils.initializeTestBackend(false); // the backend, without its base entry
final Entry base = TestCaseUtils.makeEntry("dn: " + TEST_ROOT_DN_STRING,
"objectClass: top", "objectClass: organization", "o: test");
final CSN csn = gen.newCSN();
replayMsg(addMsg(base, csn, null, "7c1a0d2e-4b6f-4c8a-9e1d-3f5b7a9c1e2d"));
assertTrue(DirectoryServer.entryExists(base.getName()),
"the base entry of an empty replica must land: its search found nothing, it did not fail");
assertTrue(domain.getServerState().cover(csn));
}opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:2899
issue (blocking): the exhaustion exit — || searchFailedResolvingConflict, the half of the
fix that closes #889 — is pinned by no test.
Both new cases fail the search 2 and 3 times against a budget of 10 and then succeed, so the loop
leaves on replayDone and the gate is never reached with this flag as the deciding term. Measured:
with the term removed, NamingConflictTest is still 10/10; with the whole case SEARCH_FAILED
arm deleted (falls into default:), still 10/10. A regression of the exact #889 shape — attempts
spent, CSN committed, change lost — passes CI.
@Test
public void modifyIsLeftOutOfTheServerStateWhenTheEntryUUIDSearchNeverRuns() throws Exception
{
final Entry entry = createAndAddEntry("modifyWhoseSearchNeverRuns");
final String entryUUID = getEntryUUID(entry.getName());
final DN staleDN = DN.valueOf("cn=movedAway," + TEST_ROOT_DN_STRING);
final CSN csn = gen.newCSN();
// No maxTimes: every attempt in place fails its search.
ShortCircuitPlugin.registerShortCircuit(OperationType.SEARCH, "PreParse", ResultCode.UNAVAILABLE.intValue());
try
{
replayMsg(new ModifyMsg(csn, staleDN, generatemods("telephonenumber", "01 02 45"), entryUUID));
}
finally
{
ShortCircuitPlugin.deregisterShortCircuit(OperationType.SEARCH, "PreParse");
}
assertFalse(domain.getServerState().cover(csn),
"a change whose search never ran is not in the data and must not advance the ServerState");
assertTrue(ShortCircuitPlugin.getShortCircuitCount(OperationType.SEARCH, "PreParse") >= 10,
"every attempt in place must have made its search");
}This assert kills the mutant: without the term the gate falls into the ERR_LOOP branch at
:2913-2924, which commits the CSN. (The exit requests a session restart; the fixture has no
replication server, so if that blocks, replay with a shutdown flag set to true so
runRequestedSessionRestarts(false) returns at once.)
opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/NamingConflictTest.java:328
issue (blocking): this comment, and the PR's "searches ... are treated the same way",
describe a flow the test does not run; the parent-search catch at
LDAPReplicationDomain.java:1884-1887 is never reached.
Every attempt makes the first findEntryDN(uuid) at :1856; under the short circuit it fails and
the hook returns before :1882. maxTimes=3 is three attempts, one failed search each; the
fourth attempt passes every search. Measured: reverting the parent catch to BASE semantics
(parentDnFromCtx = null) keeps the class 10/10. Of the three new production lines in the hook the
test sees one.
Minimum: fix the comment (one search per attempt, the first one) and the PR text, and say the
parent catch is pinned by symmetry with :1858, not by a test. ShortCircuitPlugin cannot select
by filter, so pinning the parent search itself needs a skip-first-N short circuit or a filter-aware
one — worth it only if the plugin grows that anyway.
opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:2765
suggestion (non-blocking): WARN_REPLAY_ENTRYUUID_SEARCH_FAILED is logged once per attempt
in place — up to 10 lines per delivery, plus 10 more for every redelivery
(WARN_REPLAY_RETRYING_CHANGE at :3296 is one per delivery). The sibling isServerFailure arm
at :2692 logs nothing per attempt. A backend offline for the whole 300 s budget on a busy domain
is changes x 10 x redeliveries lines. Log once per delivery — on the first SEARCH_FAILED, or at
the exhaustion exit where the failure is already reported.
opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/NamingConflictTest.java:291
suggestion (non-blocking): neither new case checks that the short circuit fired. A run where
it never does (plugin not loaded in the fixture, search routed elsewhere) stays green on the
sibling assertions. One line per case, after deregisterShortCircuit:
// The count includes the searches let through once maxTimes was spent: > 2 says the
// budget was used and the search after it ran.
assertTrue(ShortCircuitPlugin.getShortCircuitCount(OperationType.SEARCH, "PreParse") > 2,
"the short circuit must have been spent by the attempts in place");opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:2756
suggestion (non-blocking): the SearchFailedException -> SEARCH_FAILED switch serves all
four conflict resolutions, but only Modify and Add have a case; Delete and ModifyDN are pinned by
nothing. Same mechanism, so low risk — either one case each (a DeleteMsg / ModifyDNMsg on a
stale DN with maxTimes=2, same assertions as the Modify case), or a sentence in the PR saying
they ride on the shared switch.
opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:3517
note (non-blocking): a filter that does not parse is a permanent condition retried as a
transient one — 10 attempts, then redeliveries until the give-up budget, for an entryUUID that
will never parse. The PR body says this is deliberate and the cost is bounded, so nothing to
change; only noting that this branch is reached by no test, and that the unescaped
"entryuuid=" + uuid it guards is pre-existing.
…d, and look the entryUUID up as a value Review round 1 of OpenIdentityPlatform#968. A backend which serves the base DN and has no base entry answers NO_SUCH_OBJECT to a search under it, on every route - and so does a backend which is not there. findEntryDN() read every non-SUCCESS code as a search which did not run, so the base entry of a domain replayed into an empty replica was retried until the give-up budget skipped it: two empty replicas share the generation ID of an empty backend, and the base entry is the first change. baseEntryIsAbsentFromALiveBackend() asks the backend which of the two it is. The entryUUID is looked up as the value it is rather than read as part of a filter string: it comes off the wire and nothing validates it as one, and a value which does not parse as a filter was a search which never runs, retried as a transient failure for as long as the change was asked for. There is no filter to parse now, and no branch left. Nothing is logged per attempt in place any more, as nothing is for a storage which failed to serve the operation: the attempt keeps the search failure and the exhaustion exit reports it in the ERR_ERROR_REPLAYING_OPERATION line it already logs, in place of the error of the operation, which for this case only named the conflict. The Add hook puts the same text on the operation it stops. The message is ERR_REPLAY_ENTRYUUID_SEARCH_FAILED now that it is never logged on its own; the ordinal stays. Tests: the exhaustion exit is pinned by a case whose search never runs; Delete and ModifyDN get a case each; the Add hook is three cases, one per search, on a ShortCircuitPlugin which can let the first searches through before it applies; every bounded short circuit asserts, before it is deregistered, that its budget was spent and the search after it ran; the base entry of an empty replica and an entryUUID no filter string parses each get a case.
|
Review round 1 addressed (68cff84)
|
…an entry which is not there solveNamingConflict() decides an entry is gone by searching for its entryUUID and getting nothing back, and getFirstResult() answers the same thing for a search which found nothing and for a search which never ran. Every caller read that as "the entry has been deleted", which answers NOTHING_TO_DO - and that branch commits the CSN unconditionally, without the guard OpenIdentityPlatform#892 gave the FAILED branch next to it. A change which was never applied was recorded as replayed, the replication server never sent it again, and no alert was raised: the OpenIdentityPlatform#889 failure mode through a branch OpenIdentityPlatform#892 did not harden. findEntryDN() now reports a search which did not run rather than answering "no entry" out of it, and the replay takes that as the failure of the server it is: the change is retried in place and left out of the ServerState once the attempts are spent, so the replication server delivers it again. The searches which check a replayed Add for a conflict before it runs get the same treatment - reading them as a parent which is gone renamed the entry as a conflicting one, which an administrator has to repair by hand. Fixes OpenIdentityPlatform#956
…nothing else claims
…d, and look the entryUUID up as a value Review round 1 of OpenIdentityPlatform#968. A backend which serves the base DN and has no base entry answers NO_SUCH_OBJECT to a search under it, on every route - and so does a backend which is not there. findEntryDN() read every non-SUCCESS code as a search which did not run, so the base entry of a domain replayed into an empty replica was retried until the give-up budget skipped it: two empty replicas share the generation ID of an empty backend, and the base entry is the first change. baseEntryIsAbsentFromALiveBackend() asks the backend which of the two it is. The entryUUID is looked up as the value it is rather than read as part of a filter string: it comes off the wire and nothing validates it as one, and a value which does not parse as a filter was a search which never runs, retried as a transient failure for as long as the change was asked for. There is no filter to parse now, and no branch left. Nothing is logged per attempt in place any more, as nothing is for a storage which failed to serve the operation: the attempt keeps the search failure and the exhaustion exit reports it in the ERR_ERROR_REPLAYING_OPERATION line it already logs, in place of the error of the operation, which for this case only named the conflict. The Add hook puts the same text on the operation it stops. The message is ERR_REPLAY_ENTRYUUID_SEARCH_FAILED now that it is never logged on its own; the ordinal stays. Tests: the exhaustion exit is pinned by a case whose search never runs; Delete and ModifyDN get a case each; the Add hook is three cases, one per search, on a ShortCircuitPlugin which can let the first searches through before it applies; every bounded short circuit asserts, before it is deregistered, that its budget was spent and the search after it ran; the base entry of an empty replica and an entryUUID no filter string parses each get a case.
68cff84 to
5ab2187
Compare
|
Rebased on master; the conflict was Nine commits landed on master since the review round. The only conflict was in #972 is worth a note, since it reads the same situation the review's first point is about: a domain whose base entry is not in the backend now leaves its generationId unstored until the entry appears, rather than write it to the configuration entry. The base entry of an empty replica is what this branch lets land, and On the rebased tree: |
solveNamingConflict()decides an entry is gone by searching for its entryUUID and getting nothing back, andgetFirstResult()answers the same thing for a search which found nothing and for a search which never ran:Every caller in conflict resolution read that
nullas "the entry has been deleted" and answeredNOTHING_TO_DO, and that branch commits the CSN unconditionally - it never got the guard #892 added two lines below it, oncase FAILED. So a change which was never applied was recorded as replayed, the replication server never sent it again because this replica reported itself past that CSN, and no alert was raised: the #889 failure mode, through a branch #892 did not harden.What this changes
findEntryDN()reports a search which did not run instead of answering "no entry" out of it: a non-SUCCESSresult code is aSearchFailedException. One result code is looked at twice: a backend which serves the base DN and has no base entry answersNO_SUCH_OBJECTon every route (EntryContainer.searchIndexedfetches the base entry before it returns success,MemoryBackend.searchchecks it first) - and so does a backend which is not there. The backend itself tells the two apart,baseEntryIsAbsentFromALiveBackend(): the search ran over a backend which is there and empty, and it did not run over one which is gone or fails to answer. Without that, the base entry of a domain replayed into an empty replica - two empty replicas share the generation ID of an empty backend, so no initialization is needed and the base entry is the first change - would be retried until the give-up budget skipped it."entryuuid=" + uuidmade a value which does not parse - a dangling escape - a search which never runs, retried as a transient failure for as long as the change was asked for. Looked up as a value, such an entryUUID names no entry, which is what a search which ran and found nothing says. The// never happens because the filter is always validcomment is true now.ConflictResolution.SEARCH_FAILEDgets the in-place attempts a storage which failed gets, and the change is left out of the ServerState once they are spent, so the replication server delivers it again. The result code of the attempt says nothing of it - it is the conflict the operation failed on - so the attempt keeps the search failure and the exhaustion exit reports it, in theERR_ERROR_REPLAYING_OPERATIONline it already logs, in place of the error of the operation. Nothing is logged per attempt in place, as nothing is for a storage which failed to serve the operation: a backend which is down for a while fails every attempt of every change delivered meanwhile.handleConflictResolution(PreOperationAddOperation)) are treated the same way: the operation is stopped withUNAVAILABLEand the search which did not run as its error message, which is what the exhaustion exit reports. Reading the first of them as "not replayed here yet" adds an entry a second time when it was renamed since; reading the second as a parent which is gone hands the Add to conflict resolution as the naming conflict it is not, and renames the entry under the base DN as a conflicting entry when the search conflict resolution makes fails as well.findEntryUUID()is deliberately left alone: a search which fails there leaves a locally originated ModifyDN published without the entryUUID of its new superior, which is a bug on what this server sends rather than on what it records. It deserves an issue of its own.Rebased on master
The branch sits on master as it is now, which carries #948, #965 and - since the review round - #935, #959, #969, #970, #972, #973, #975, #976. The only conflict of the last rebase was
replication.properties, where #959 and #972 added 326 and 327 next to this branch's 322; both sides are kept. The Java merged on its own, and #972 reads the same situation this change lets through: a suffix whose base entry is not in the backend is "what a suffix waiting to be initialized looks like", and its generationId is now left unstored rather than written to the configuration entry -StateWithoutBaseEntryTestis 3/3 on this branch.#948 put the in-place attempt of a replayed change under the replay read lock it introduced, and
the
SEARCH_FAILEDretry is inside it: its 50 ms are waited where theFAILEDretry of #892already waits - a domain on its way down takes that lock exclusively and waits out the attempt in
flight, as it does for every other in-place retry.
#965 answers a ModifyDN whose entry is gone before the new superior is looked up, so
solveNamingConflict(ModifyDNOperation)reads the entryUUID search first and returns on it. Asearch which did not run now leaves that method rather than being read as an entry which is gone -
it declares
throws Exception, so theSearchFailedExceptionreaches thecatchin the replayloop, which is where it is turned into
SEARCH_FAILED.Tests
Nine cases in
NamingConflictTest, driven byShortCircuitPluginonSEARCH/PreParse. The plugin grew aregisterShortCircuit(..., letThroughFirst, maxTimes)so that a failure can start part way through the searches of an attempt - the second search failing while the first ran - and every case which registers a bounded short circuit asserts, before it is deregistered (which drops the count), that the budget was spent and the search after it ran.modifyIsRetriedWhileTheEntryUUIDSearchCanNotRun,deleteIsRetriedWhileTheEntryUUIDSearchCanNotRun,modifyDnIsRetriedWhileTheEntryUUIDSearchCanNotRun- a change on an entry which was renamed here, so that only the entryUUID search finds it; the search fails twice and is served on the third attempt. Without the fix:NOTHING_TO_DOafter a single search, the change dropped and the CSN recorded as replayed.addIsNotReplayedTwiceWhileTheEntryUUIDSearchCanNotRun- an Add delivered a second time whose entry was renamed since; the first search of the first attempt fails. Without the fix the entry is added a second time under its former DN, one entryUUID twice in the data.addIsRetriedWhileTheParentEntryUUIDSearchCanNotRun- the parent check fails while the search before it ran; pinned by the monitor: no naming conflict is counted for a search which read nothing. Without the fix conflict resolution counts one and rewrites the message to the DN it already carries.addIsRetriedWhileTheConflictResolutionSearchCanNotRun- the parent was renamed here, so the Add fails on a genuine conflict, and the search conflict resolution reads the data with fails. Without the fix the entry is renamed under the base DN as a conflicting entry; with it, the conflict is counted once, when it is solved.modifyIsLeftOutOfTheServerStateWhenTheEntryUUIDSearchNeverRuns- every attempt in place fails its search: the change is not in the ServerState and the entry untouched. This is the case which pins the exhaustion exit: with|| searchFailedResolvingConflict != nullremoved, the exit falls into theERR_LOOPbranch and commits the CSN.baseEntryIsAddedToAnEmptyReplica- the base entry replayed into a backend without one. Fails on the previous head of this branch: tenUNAVAILABLEattempts, the change left out of the ServerState, no base entry.anEntryUUIDWhichIsNotOneNamesNoEntry- an entryUUID no filter string parses; the change is resolved as one on an entry which is not in the data, and recorded.Mutation runs, each on the final tests: with
findEntryDN()answeringnullfor a search which did not run and the exhaustion term removed, seven of the nine fail (baseEntryIsAddedToAnEmptyReplicaand the filter case are the two that behaviour does not reach); with the two catches of the Add hook reverted to "no entry" and everything else kept,addIsNotReplayedTwice...fails on the duplicate entry andaddIsRetriedWhileTheParent...on the conflict counted.NamingConflictTestis 17/17 at 5ab2187. Theorg.opends.server.replication.**package ran on the review-round commit before the rebase: 3595 tests with 2 failures, bothsetUpof an embedded server which did not get the admin port it binds (Address already in useon 65534 and 65530 - test servers of other checkouts on the same machine), which took the 65 methods ofProtocolCompatibilityTestandFileChangeNumberIndexDBTestintoskipped; on the rebased tree the two classes are 58/58 and 5/5, andNamingConflictTest17/17 again.Not in this change
SUCCESS,NO_OPERATIONandBUSYbeing read before either guard is consulted.solveNamingConflict(ModifyDNOperation)when both the moved entry and its new parent are gone: fixed on master by [#955] Answer a ModifyDN whose entry is gone before the new superior is looked up #965, which this branch sits on. What this change adds there is that a search which did not run no longer reaches that decision at all.case NOTHING_TO_DOrefusing to commit the CSN while the result code is the configuredserver-error-result-code. With the search telling a failure from an empty answer,NOTHING_TO_DOis only reached when the search did run and the entry really is not in the data, and the guard would refuse legitimate no-ops - a Modify on an entry genuinely deleted elsewhere would be retried until the give-up budget raised a false "this replica diverged" alert.Fixes #956
Ordinal
ERR_REPLAY_ENTRYUUID_SEARCH_FAILED_322, moved off 310 in 4110d0c and renamed fromWARN_once it stopped being logged on its own: it is the error message of the operation the Add hook stops, and the error the exhaustion exit reports. Six open branches had each read310 as the first ordinal free in master and taken it, and git merges those additions without
reporting a conflict - they land in different places in the file - so the duplicate would only have
surfaced afterwards, as two unrelated messages sharing one support ID. The generator does not check
either: it keys on name and ordinal together, so both compile. The open PRs which add to
replication.propertiesnow hold 310-325 with nothing claimed twice.