[#1041] Claim the import context for the length of a session restart, and refuse the total update which lands across it - #1045
Open
vharseko wants to merge 7 commits into
Conversation
…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.
… the attempts in place Review round 2 of OpenIdentityPlatform#968. The mark an attempt leaves when the search conflict resolution reads the data with did not run outlived the attempt which set it: it was cleared only when conflict resolution rewrote the operation, so an attempt the server refused before it reached the data - BUSY, UNAVAILABLE from the operation itself, or the configured server-error-result-code out of conflict resolution - left it as it was, and the exhaustion exit reported the search of an earlier attempt next to the result code of the last one: two causes on one line. The CSN was left out of the ServerState either way. The mark is reset at the top of every attempt now, which is what the comment above the verdict promised, and the clear on a rewritten operation has nothing left to do. The assertion of addIsRetriedWhileTheConflictResolutionSearchCanNotRun which guards against the entry being renamed as a conflicting entry names the DN that rename produces - the entryUUID added to the RDN, under the base DN - rather than one the code never makes.
… this replica, and keep the replay out of the data it replaces A change queued for replay before the InitializeTargetMsg arrived was replayed into no backend: preBackendImport() takes the backend away without disabling the domain, so the operation got NO_SUCH_OBJECT, the entryUUID search did not run, and once the attempts in place were spent the session was restarted for the change to be delivered again - the session the import was reading. The import ended on the entries which had arrived and was reported as finished, since receiveEntryBytes() records no exception for a broker which is shutting down. importBackend() now holds the replay off the way disable() does - a flag the replay threads read where they read `disabled`, and the drain of the attempts in flight - before the backend is taken away; the restart after a failed or abandoned replay is refused for the whole of the total update (sessionHasAnOwner(): ownsItsSession() || importInProgress()), from the request rather than from the first entry; and once the state is loaded from the imported data, the pending changes, the restart request and the backoff are reset, as disable() resets them - a change given back during the import would otherwise stay listed and uncommitted, and hold the ServerState back for good, since the imported state already covers it. Review round 3, with the flush thread's Modify kept out of the MODIFY short circuit (a predicate on the replayed operation, pinned by the case making that Modify inside the window) and the text of the exhaustion exit asserted positively.
…quest, and describe the road as it is The importingData hold-off was pinned by no test: with `|| importingData` deleted from both goingDown reads, the change was attempted ten times into no backend and the session was saved by sessionHasAnOwner() alone, which the retry-warning check could not tell from the hold-off - that guard returns before the warning. aReplayDuringTheImportLeavesTheSessionToTheImport now asserts that no ERR_ERROR_REPLAYING_OPERATION record names the change: none at head, one under that mutant. sessionHasAnOwner() in recoverFromReplayFailure() was reached by neither case, since the hold-off gives the change back at the top of its first attempt. aRequestOnItsWayOwnsTheSessionTheAnswerArrivesOver reaches it: a total update this replica asked for, the answer held by the exporter, the backend live, a change whose entryUUID search never runs spent in place - the exhaustion exit reports it, no retry warning follows, and the import then runs to its end over the session the request was made over. With that guard on ownsItsSession() alone, the warning is logged and the case is red on it. Two comments described the road before the previous commit - the change given back is delivered again after the import restarts the session, not now, and the first case of ReplayDuringImportTest never spends an attempt - and the javadoc of sessionHasAnOwner() says that a change left listed holds the ServerState back, in memory and as persisted, until the next restart. Review round 4.
…f a session restart, and refuse the total update which lands across it The owner read of a session restart and the listener thread's claim of the import context for an InitializeTargetMsg share no lock, and cannot: disableService() waits for the listener thread under serviceStateLock, so the listener cannot take that lock before its claim. A restart which read no owner a few statements before the claim landed stopped the broker the import was about to read: the import ran over it, ended on nothing with no exception recorded, the suffix was replaced by the nothing which arrived, and the total update was reported as finished - or, the stop landing before the full update status was published, the listener thread died on a NullPointerException in signalStatusChange(). The two now contend on the one reference. disableServiceUnlessImportInProgress() claims importExportContext with SESSION_BEING_STOPPED before it stops anything, and releases it once disableService() returns - the listener thread is gone by then. The listener's claim fails against it and the InitializeTargetMsg is refused: ERR_INIT_REJECTED_SESSION_STOPPING to the exporter and to this server's log. An export in the context is not an owner and is stopped from under, as before; a total update which lands between the end of that export and the stop is refused by the listener re-reading, once its claim is made, that the broker is stopping - which also refuses the total update a disable() or the shutdown of the domain lands across. The claim is invisible to ieRunning(), importInProgress(), getImportExportContext() and every reader of the context; releaseIEContext() now releases only the context it is given, so a task which fails to claim no longer releases the one it failed against. Two test seams: the listener before its claim, and disableService() before the broker is stopped. ReplayDuringImportTest holds the listener there, drives a restart through the gap with a change whose attempts in place are spent, and releases the listener before the stop: the import must be refused, the exporter told, the data of this replica left alone. A second case disables the domain across the gap. Each pins its own guard: the restart case fails with the read in place of the claim, the disable case with the re-read of the broker removed.
vharseko
added
bug
replication
concurrency
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The owner read of a session restart and the listener thread's claim of the import context share no lock, and cannot share the one the restart holds:
disableService()waits for the listener thread underserviceStateLock, so a listener which took that lock before its claim would wait for a restart which is waiting for it. A restart which read no owner a few statements before the claim landed stopped the broker the import was about to read - and then waited for the listener thread to run that import to its end over the stopped broker:shutdown = truethere is one socket write, and the listener haspreBackendImport()to get through before it reads anything - the import ends on nothing,importLDIFreplaces the suffix with the nothing which arrived,loadDataState()finds no base entry and computes the generationId of an empty suffix, andNOTE_FULL_UPDATE_ENGAGED_FROM_REMOTE_ENDis logged with no error; the replica reconnects after the backoff with an empty state, and the exporter learns of it from the replication server (ERR_NO_REACHABLE_PEER_IN_THE_DOMAIN) - loud there, silent here, the data of this replica gone;setNewStatus(TO_FULL_UPDATE_STATUS_EVENT), the listener thread dies on aNullPointerExceptioninsignalStatusChange()-connectedRS.get().sessionis null once the broker is stopped - afterinitialize()'sfinallyhas reported the total update as finished. This is what the first run of the new test showed; the issue describes the first shape.What this changes
ReplicationDomain.disableServiceUnlessImportInProgress()claimsimportExportContextwith a context of its own,SESSION_BEING_STOPPED, before it stops anything, and releases it oncedisableService()has returned - the listener thread is gone by then, and it is the one thread which claims a total update this replica did not ask for.restartSession()calls it in place ofsessionHasAnOwner()+disableService(): a claim which fails against an import leaves the session alone, as the read did; one which fails against an export stops the session from under it, as before - an export is not an owner, and the exporter reports the cut.SESSION_BEING_STOPPED, and theInitializeTargetMsgis refused:ERR_INIT_REJECTED_SESSION_STOPPING(329) is sent to the exporter and logged on this server, which had nothing in its log for a total update it refused. A refusal published over a broker which is already stopped is silently not sent, and the exporter then learns of it from the replication server, as it did.disable()or by the shutdown of the domain, neither of which claims, and the total update which lands between the end of an export and the stop of a restart which found that export. Without it the claim wins, and what runs next publishes the full update status over a session which is gone.ieRunning(),importInProgress(),getImportExportContext()and every reader of the reference: theErrorMsgandInitializeRcvAckMsgarms ofreceive(), the self-initiated branch ofinitialize()-markInitStartReceived()on the claim would return true and run the import on a static context -abortStalledInitializeFromRemote(), the export, and the monitor.releaseIEContext()takes the context to release and releases only that one.initializeFromRemote()released the context on every failure, including the failure to acquire one: adsreplication initializerun while an import or export was in progress released that operation's context -receiveEntryBytes()then reads a null context - and, with this change, would have released the claim of a restart. Pre-existing, and on the road this change makes the claim depend on.Tests
Two seams, after
setReplayDrainTimeout():setImportClaimHook()runs on the listener thread between theInitializeTargetMsgit took off the session and its claim,setServiceStopHook()insidedisableService()before the broker is stopped. Nothing else can hold either thread there.Two cases in
ReplayDuringImportTest, on the fixture of #968 (the exporter is a broker of the test, the replay queue is synchronous):aRestartDecidedBeforeTheImportIsClaimedRefusesTheImport- the listener held before its claim; a Modify whose entryUUID search never runs (ShortCircuitPluginonSEARCH) spends its attempts in place on a thread of its own, decides the restart and is held before the stop; the listener is released in between, so the broker it finds is up and only the claim can refuse it. The refusal reaches the exporter as anErrorMsgwith that text,NOTE_FULL_UPDATE_ENGAGED_FROM_REMOTE_STARTis not logged, the listener thread does not die, the session comes back after the backoff, and the entry added before is still there. Without the claim -sessionHasAnOwner()read anddisableService()in its place - it fails on the start of the total update, the other four cases green.aDomainDisabledBeforeTheImportIsClaimedRefusesTheImport- the same hold,disable()across it,enable()after. With the re-read of the broker after the claim removed it fails on the start of the total update, the other four cases green.The first version of the restart case released the listener after the stop, and did not tell the claim from the re-read: the mutant with the read in place of the claim passed it, refused by the re-read. Hence the second seam.
ReplayDuringImportTestis 5/5, and thereplication/pluginpackage withUpdateOperationTest,AssuredReplicationPluginTest,InitOnLineTest,GenerationIdTest,ReSyncTest,ReplicationDomainTestandMonitorTest- the suites which drive a total update, and the ones which read the context - is 330/330 on this branch, nothing skipped.Built on #968
The claim replaces the read #968 introduced (
sessionHasAnOwner()) and the refusal is the road that read left open a few statements wide, so this is written on #968's head,00798abf33, which sits onorigin/master: the branch is that head plus the one commit[#1041]. The three[#956]commits it shows are #968's, and go away when #968 merges.Ordinal
ERR_INIT_REJECTED_SESSION_STOPPING_329: 310-327 are claimed by #968 and the branches around it, 328 by #1019; 329 is free on master and on every open branch which touchesreplication.properties.Not in this change
restartService(), the restart a configuration change asks for, still decides onownsItsSession(); the claim is the primitive it would take.disable()or a shutdown from being it before the first entry, but the shutdown of the server mid-import is still there.signalStatusChange()on a stopped broker throws aNullPointerExceptionout ofsetNewStatus()and ends the listener thread on it. This change keeps the total update from reaching it over a stopped broker; the null session itself is pre-existing and deserves an issue of its own.Fixes #1041