[#953] Refuse a server-error-result-code which does not report a failure - #980
Conversation
…es not report a failure
|
The red
It looks like #924 - the window in |
…-server-error-result-code
|
@maximthomas the branch conflicted with master and is merged with it now (4ddf3e8, What the merge changed, so the diff does not have to be re-read from scratch:
Re-run on the merged tree: The description is updated to the merged line numbers. |
maximthomas
left a comment
There was a problem hiding this comment.
praise: The production change is the right shape and does what the description says. Refusing the five success codes in isConfigurationChangeAcceptable() closes #953 at the one place both readers — replay() and synchronize() — are covered at once, and letting an unknown code through keeps the private-code freedom. Replacing the NO_OPERATION read in replay() with conflictResolutionFoundNothingToDo() / isConflictResolutionNoOp() is a cleaner design than what it replaces: the replay reads what conflict resolution decided, not the integer that carried it, and the three no-op sites are marked uniformly. The boot fallback to OTHER with a warning is the right call for an old config.ldif — substitute, say so, start. The ServerErrorResultCodeTestCase refusal table is well built, and the javadoc on isServerErrorResultCodeAcceptable() explains the why better than most. Five sweeps, one mutation run and green CI on all five Linux failsafe cells found no production defect.
issue (blocking): changeAlreadyAppliedIsRecordedAsReplayed pins an outcome the FAILED road produces too; the attachment mechanism is pinned by nothing.
opendj-server-legacy/src/test/java/org/opends/server/replication/plugin/NamingConflictTest.java:236
The only assertion is domain.getServerState().cover(csn). Without the attachment the replay goes solveNamingConflict(Add) → FAILED → skipUnreplayableChange() → updateError() → remotePendingChanges.commit(csn) — the same bit. Measured: with the setAttachment at the add site (opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:1873) removed, the class is 9/9 green, this case included. The modifyDN (:2039) and modify (:2094) sites are reached by no test. A forgotten setAttachment at any of them turns every genuine no-op into an "unreplayable change" with a divergence alert, under a green suite.
Assert what only the no-op road produces — the replayed-updates-failed counter — and add one case per site:
final int failedBefore = failedReplayedUpdates();
replayMsg(addMsg(entry, csn, parentUUID, entryUUID));
assertTrue(domain.getServerState().cover(csn),
"a change which is already in the data was not recorded as replayed");
assertEquals(failedReplayedUpdates(), failedBefore,
"a change which is already in the data was counted as one the replay could not apply");private int failedReplayedUpdates()
{
final MonitorData monitor = new MonitorData();
domain.addAdditionalMonitoring(monitor);
for (Attribute attribute : monitor)
{
if ("replayed-updates-failed".equals(attribute.getAttributeDescription().getNameOrOID()))
{
return Integer.parseInt(attribute.iterator().next().toString());
}
}
throw new AssertionError("replayed-updates-failed is not in the monitor data");
}If the pin is not wanted, the description's "pins the other half of the replay change" should read "pins that the CSN is recorded" — that is all the case does today.
issue (blocking): the boot fallback serverErrorResultCode(int) is reached by no test; return ResultCode.valueOf(configured); survives all 7 cases.
opendj-server-legacy/src/main/java/org/opends/server/core/CoreConfigManager.java:453-461
Two guards on one door. Every test road goes through isConfigurationChangeAcceptable() (:426) first: the refusal rows never reach the helper with a success code, and the two positive rows carry exceptional codes the helper returns exactly as valueOf() would. So the BASE behaviour — start on NO_OPERATION, no warning — survives the whole class by construction, and the fallback is the only thing between an old config.ldif holding 16654 and #953 again.
Cheapest pin — make the helper package-private and test it directly:
@Test
public void aCodeWhichIsNotAFailureFallsBackOnOtherAtStartUp()
{
assertEquals(CoreConfigManager.serverErrorResultCode(ResultCode.NO_OPERATION.intValue()), ResultCode.OTHER);
assertEquals(CoreConfigManager.serverErrorResultCode(ResultCode.UNWILLING_TO_PERFORM.intValue()),
ResultCode.UNWILLING_TO_PERFORM);
assertEquals(CoreConfigManager.serverErrorResultCode(9999), ResultCode.valueOf(9999));
}suggestion (non-blocking): the boot warning never reaches logs/errors, though the javadoc and the description say "in the error log".
opendj-server-legacy/src/main/java/org/opends/server/core/CoreConfigManager.java:448 (the logger.warn at :459)
initializeCoreConfig() runs before initializeLoggerConfig() (opendj-server-legacy/src/main/java/org/opends/server/core/DirectoryServer.java:1434 / :1446); at that moment the only publisher is the start-up stdout one and nothing is buffered, so the line lands in server.out or on the console. Either say so — "on the server's standard output at start-up" — in the javadoc and the description, or defer the message until the error logger is up.
suggestion (non-blocking): the refusal rows never set resultCodeToRestore, so on a regression the success code leaks into every later test class.
opendj-server-legacy/src/test/java/org/opends/server/core/ServerErrorResultCodeTestCase.java:84
tearDown() restores only when the field is set (:57); only the two positive cases set it. The day isConfigurationChangeAcceptable() stops refusing — the regression this class exists to catch — the last row (NO_OPERATION) stays the server-error code for the rest of the failsafe JVM and the failure surfaces as replay cascades in the replication classes rather than here. The restore is idempotent:
final ResultCode inForce = getServerErrorResultCode();
resultCodeToRestore = inForce.intValue();
assertNotEquals(setServerErrorResultCode(resultCode.intValue()), 0,
"the server accepted " + resultCode + " as the code it puts on an internal error");…and the start-up fallback of the server error code
|
@maximthomas round 2 is pushed (ec9c1e3), one commit on top of the merged tree; the description is brought up to date.
The boot fallback The boot warning never reaches The refusal rows never set Through failsafe: |
…-server-error-result-code
|
@maximthomas the branch conflicted with master again and is merged with it (fcb49be, The one conflict was an import in Re-run on the merged tree: The description is updated to the merged line numbers. |
maximthomas
left a comment
There was a problem hiding this comment.
praise: The delta closes each round-1 item with a pin rather than a promise, and the merge over #968 reads right.
NamingConflictTest.assertRecordedAsReplayedAndNotGivenUpOn(:339-343) assertsreplayed-updates-failedbesidecover(csn): the add-site and the modify-site mutant each go red at:342in exactly their own case (20/21 twice, 21/21 at the head), measured.CoreConfigManager.serverErrorResultCode(int)package-private and pinned directly: five fallback rows,UNWILLING_TO_PERFORMandvalueOf(9999)taken as they are (ServerErrorResultCodeTestCase.java:107-118).resultCodeToRestore = inForce.intValue()before the attempt (:91): a regression can no longer leave a success code in force for the classes after this one.- The helper javadoc (
CoreConfigManager.java:451-453) now says where the boot warning goes — stdout,logs/server.out. - The hand-merged add-site hunk keeps #968's
searchDidNotRunreturn ahead of the mark (LDAPReplicationDomain.java:1927-1936).
suggestion (non-blocking): The refusal rows pin rc != 0 only; the exact code and the reason are pinned by nothing.
opendj-server-legacy/src/test/java/org/opends/server/core/ServerErrorResultCodeTestCase.java:93-96
inForce is the default OTHER in every row, so with the isConfigurationChangeAcceptable check reverted the value goes applyGlobalConfiguration → serverErrorResultCode(0) → OTHER == inForce and the second assertion stays green; only assertNotEquals(rc, 0) turns red. UNWILLING_TO_PERFORM and ERR_CONFIG_CORE_SERVER_ERROR_RESULT_CODE_NOT_A_FAILURE can regress to any other non-zero code and text unnoticed.
assertEquals(setServerErrorResultCode(resultCode.intValue()), ResultCode.UNWILLING_TO_PERFORM.intValue(),
"the server accepted " + resultCode + " as the code it puts on an internal error");Or, to pin the reason too, go through an internal modify instead of applyModifications:
final ModifyOperation op = getRootConnection().processModify(newModifyRequest("cn=config")
.addModification(REPLACE, "ds-cfg-server-error-result-code", String.valueOf(resultCode.intValue())));
assertEquals(op.getResultCode(), ResultCode.UNWILLING_TO_PERFORM);
assertThat(op.getErrorMessage().toString()).contains(
ERR_CONFIG_CORE_SERVER_ERROR_RESULT_CODE_NOT_A_FAILURE.get(resultCode.intValue(), resultCode).toString());suggestion (non-blocking): The boot fallback's warning is pinned by nothing; the five fallback rows assert the substituted code only.
opendj-server-legacy/src/main/java/org/opends/server/core/CoreConfigManager.java:469, opendj-server-legacy/src/test/java/org/opends/server/core/ServerErrorResultCodeTestCase.java:107-110
serverErrorResultCode(int) has two outputs, OTHER and WARN_CONFIG_CORE_SERVER_ERROR_RESULT_CODE_NOT_A_FAILURE; the rows read the first. Deleting the logger.warn line is green 13/13 by construction — the substitution is pinned, the line which tells the operator what was ignored is not.
final List<LocalizableMessage> warned = new ArrayList<>();
final ErrorLogPublisher<ErrorLogPublisherCfg> capture = new ErrorLogPublisher<ErrorLogPublisherCfg>()
{
@Override public void log(String category, Severity severity, LocalizableMessage message, Throwable e)
{ warned.add(message); }
@Override public boolean isEnabledFor(String category, Severity severity) { return severity == Severity.WARNING; }
@Override public void initializeLogPublisher(ErrorLogPublisherCfg config, ServerContext serverContext) {}
@Override public void close() {}
@Override public DN getDN() { return null; }
};
ErrorLogger.getInstance().addLogPublisher(capture);
try
{
assertEquals(CoreConfigManager.serverErrorResultCode(resultCode.intValue()), ResultCode.OTHER);
assertEquals(warned.size(), 1);
assertEquals(warned.get(0).toString(),
WARN_CONFIG_CORE_SERVER_ERROR_RESULT_CODE_NOT_A_FAILURE.get(resultCode.intValue(), ResultCode.OTHER).toString());
}
finally
{
ErrorLogger.getInstance().removeLogPublisher(capture);
}nitpick (non-blocking): The description and the javadoc name two readers of the code; the third is the one which makes the refusal matter for cn=config itself.
opendj-server-legacy/src/main/java/org/opends/server/core/CoreConfigManager.java:594-598
applyConfigurationChange puts getServerErrorResultCode() on the ConfigChangeResult when the apply throws, then swaps coreAttributes in only when the result is SUCCESS. Before this PR, with 0 configured, a failed apply was reported as a success and applied all the same. Worth one sentence next to the replay and synchronize readers; no test reaches this catch, so it stays a text item.
nitpick (non-blocking): "Purely structural" holds for the three in-tree producers of NO_OPERATION, not for a plugin.
opendj-server-legacy/src/main/java/org/opends/server/replication/plugin/LDAPReplicationDomain.java:2947
At the base any NO_OPERATION after a replay took the silent no-op arm; at the head only an operation carrying CONFLICT_RESOLUTION_NO_OP does, and every other NO_OPERATION goes solveNamingConflict → FAILED → skipUnreplayableChange — counted in replayed-updates-failed, logged, alerted, the CSN committed either way. The three in-tree producers are all marked, so nothing in the tree diverges; a third-party pre-operation plugin returning NO_OPERATION for a replayed change is the one input which now behaves differently. The direction looks right — say so in the description rather than "structural".
…arning of the start-up fallback The refusal rows read the result code and the reason off an internal modify rather than an ldapmodify exit code which is not zero: UNWILLING_TO_PERFORM, and a reason which names the attribute and the code. The five start-up rows read the warning off the error log next to the substituted code, and a code taken as it is warns about nothing. The javadoc of the acceptability check names the third reader of the code, applyConfigurationChange(), for which a code of 0 reported a failed change to cn=config as a success and applied it all the same.
|
@maximthomas round 3 is pushed (9eb3ff5), one commit on top of the merged tree; the description is brought up to date. The refusal rows pin The boot fallback's warning is pinned by nothing — pinned, through the test writer rather than a publisher of its own: The third reader — a sentence in the javadoc of "Purely structural" — the description now says what the mark changes rather than calling it structural: for the configured code the branch is unreachable once the validation is in; a Through failsafe: |
maximthomas
left a comment
There was a problem hiding this comment.
praise: Every item of the previous round is closed at this head, and the closures hold under mutation.
- The refusal is pinned on its own result code and reason:
opendj-server-legacy/src/test/java/org/opends/server/core/ServerErrorResultCodeTestCase.java:101-107assertsUNWILLING_TO_PERFORMandcontains(ERR_CONFIG_CORE_SERVER_ERROR_RESULT_CODE_NOT_A_FAILURE.get(...)); with the reason left out ofunacceptableReasons, exactly the five refusal rows go red at:105, measured. - The boot fallback's warning is pinned:
errorLogRecords(WARN_CONFIG_CORE_SERVER_ERROR_RESULT_CODE_NOT_A_FAILURE...)at:126; with thelogger.warnatopendj-server-legacy/src/main/java/org/opends/server/core/CoreConfigManager.java:469deleted, exactly the five start-up rows go red, measured. 13/13 at the head. - The javadoc at
CoreConfigManager.java:484-490now names the configuration itself as the third reader of the code. - The start-up rows read
TestCaseUtils.ERROR_TEXT_WRITERafter other classes have restarted the server in-core: in the CI log of this head three in-core restarts precede this class in the same JVM and it is 13/13 there, so the "writer detached by a restart" worry does not hold.
Fixes #953.
ds-cfg-server-error-result-codeis a plain integer with no validation (GlobalConfiguration.xml,<adm:integer lower-limit="0"/>, default 80), so it can be set to one of the five codesResultCoderegisters as reporting a success: 0, 5, 6, 14 and 16654. Each of those means something of its own to whoever reads a result code, and the reader then acts on that meaning while the operation it came from failed.What the readers do with it today
(Line numbers are those of
masterat eef0757, which this branch is merged with.)LDAPReplicationDomain.replay(), on a change an internal error kept out of the backend:NO_OPERATION(16654):ResultCode.valueOf(16654)hands back the registered singleton, so the reference comparison atreplay():2932holds. The guard reads "conflict resolution found this change already applied",recordChangeResolved()commits the CSN, the replication server never sends the change again, and no alert is raised. That is the silent divergence Replication replay records a failed operation as applied: the ServerState advances past the change and the assured ack reports success #889 exists to prevent, reached through the one branch which runs before the two guards [#889] Keep a change the replay could not apply out of the ServerState #892 and Five of the six codes in CONFLICT_RESULT_CODES are never exercised: pin isServerFailure() directly #939 taught about the configured code —isServerFailure(NO_OPERATION, NO_OPERATION)answerstruecorrectly, it is simply never asked.SUCCESS(0): the change is lost beforereplay()looks at it.LocalBackendModifyOperationcatches theDirectoryException, callssetResponseData(de)— which copies the code verbatim — and still runsprocessSynchPostOperationPlugins()in itsfinally, soLDAPReplicationDomain.synchronize():2309seesSUCCESSand commits the CSN. The same branch does the mirror damage for a local write which failed internally:synchronize():2325-2351builds anLDAPUpdateMsgfrom it and callscommitAndPushCommittedChanges(), publishing to the whole topology a change this replica does not have.BUSY(51) is the same shape but benign: the in-place attempts are bounded byretryCountand the give-up branch already namesBUSYexplicitly, so the change stays out of the ServerState.The configuration is a reader of its own:
CoreConfigManager.applyConfigurationChange()puts the code on a change tocn=configwhich failed to apply, then keeps the new core attributes only when the result isSUCCESS— so with0configured a failed apply was reported as a success and applied all the same. No test reaches that catch; it is named because it is the reader which makes the refusal matter forcn=configitself.The fix
Refuse the value in the configuration rather than work around it at each reader:
CoreConfigManager.isConfigurationChangeAcceptable()turns down a code which is notisExceptional(), naming the attribute and the code in the reason.applyGlobalConfiguration()— the startup path, which does not go through the acceptability check — does not refuse to start on a configuration written before this, or edited outside the server: it logs a warning naming the value it ignored and usesResultCode.OTHER(80), the setting's own default. The warning goes to the standard output of the server —logs/server.outunderstart-ds— rather than tologs/errors: the core configuration is applied before the error loggers are up (DirectoryServer.startServer(),initializeCoreConfig()beforeinitializeLoggerConfig()), and at that point the start-up publisher on stdout is the only one there is.valueOf()answers an unknown code which does), so a private code stays configurable.This closes both halves at the root, including the
SUCCESSone, which no fix insidereplay()can reach.replay()additionally reads the no-op decision off the operation conflict resolution marked — a new attachment set at the threehandleConflictResolution()sites which answerNO_OPERATION— rather than off the result code which carries it. For the configured code that branch is unreachable once the validation is in; what the mark changes is the reading of any otherNO_OPERATIONa replayed operation comes back with. In the tree there is none — the no-op control never rides on a replayed operation, which is built without controls — but a third-party pre-operation plugin can answer it, and a change such a plugin stopped is not in the data. At the base it was recorded as replayed without a word; now it goes through conflict resolution and, unsolved, is reported as a change this replica could not apply — alerted and counted inreplayed-updates-failed, the CSN recorded either way. That is the intended direction: the question of "is this change in the data" is answered by what conflict resolution decided, not by a value an administrator or a plugin owns.The
CONFLICT_RESULT_CODEScarve-out stays as it is: every code conflict resolution owns reports a failure, so the configuration still admits them, which is what #892 and #938 cover.Worth knowing before merging
The check is strict: a server which started on a bad value will refuse any change to
cn=configuntil that value is fixed —isConfigurationChangeAcceptable()receives the wholeGlobalCfgand cannot tell what changed. The message names the attribute and the reason, and the value is a nonsense one to begin with (the default is 80), but it is a behaviour change worth stating.Tests
ServerErrorResultCodeTestCase(new): each of the five success codes is refused withUNWILLING_TO_PERFORMand a reason which names the attribute and the code, and the code in force is unchanged; an error code and a code the server does not know are both accepted. The change goes through an internal modify rather thanldapmodify, so that a refusal is read in full — the code and the reason — rather than as an exit code which is not zero. The start-up fallbackserverErrorResultCode(int)— package private, since no change to a running server can reach it past the acceptability check — is pinned directly: each of the five falls back onOTHERand says which value it ignored, read off the error-log records of the test writer by message ID and text; an error code and an unknown one are taken as they are, with no warning. The refusal rows remember the code in force, so a regression of the check shows here rather than as a success code left in force for the classes which run after. 13/13.NamingConflictTest: one case per site which marks a conflict-resolution answer as a no-op —changeAlreadyAppliedIsRecordedAsReplayed(an add whose entryUUID is already in the data),modifyDnOlderThanARenameIsRecordedAsReplayed(a ModifyDN older than a rename the entry has been through) andmodifyOfExcludedAttributesOnlyIsRecordedAsReplayed(on a fractional replica, a modify of attributes it does not replicate;DomainFakeCfglearntds-cfg-fractional-excludefor it). Each pins that the change is recorded as replayed and not counted inreplayed-updates-failed: recording the CSN alone does not tell the no-op road from the one which gives a change up, since that one records the CSN too. 21/21 for the class on the merged tree, with the A failed entryUUID search reads as a deleted entry, and conflict resolution records the change as replayed #956 cases master added.UpdateOperationTest(Replication replay records a failed operation as applied: the ServerState advances past the change and the assured ack reports success #889): 31/31 on the merged tree, with the Replication: a change whose replay throws is left owned by a thread which is gone #922, Replication: the permissive-modify check dereferences a null entry DN when the DN does not parse #928 and A failed entryUUID search reads as a deleted entry, and conflict resolution records the change as replayed #956 cases master added.IsServerFailureTest([#939] Pin isServerFailure() directly, so the conflict result codes are guarded #960, master): 23/23 — the javadoc ofisServerFailure()is the only thing this branch touches there.Verified failing first: with the validation reverted, the five refusal rows of
ServerErrorResultCodeTestCasefail on exactly the five codes ("the server accepted Success as the code it puts on an internal error"). Mutations, each caught by its own case and by nothing else: the mark removed at the add site →changeAlreadyAppliedIsRecordedAsReplayed; at the ModifyDN site →modifyDnOlderThanARenameIsRecordedAsReplayed(simultaneousModrdnConflictreaches that site too, and stays green — it pins nothing there); at the modify site →modifyOfExcludedAttributesOnlyIsRecordedAsReplayed; the fallback replaced byreturn ResultCode.valueOf(configured)→ the five start-up rows; thelogger.warnof the fallback removed → the five start-up rows ("the server did not say which value it ignored"); the reason left out ofunacceptableReasons→ the five refusal rows ("the refusal does not name the attribute and the code it turned down").Merged with master
origin/master(eef0757) is merged in, the second merge of this branch after the one at 21d03d5. The one conflict was an import inNamingConflictTest: this branch addsModifyMsgat the spot where #968 addedOperationContext, and both are kept. Everything else merged on its own, and the diff against master is the same eight files: the seven of the first round andDomainFakeCfg.What master changed next to the line this branch touches in
replay(): #956/#968 put aSearchFailedExceptioncatch aroundsolveNamingConflict()with aSEARCH_FAILEDoutcome of its own, and at the add site the entryUUID search now reports a failure it could not run (searchDidNotRun()) before thereplayedEntryDN != nullcheck this branch marks. TheisConflictResolutionNoOp(op)branch stays where it was — first underresult != SUCCESS, inside thereplayReadLockblock #945/#908 put it in — and the three marked sites are the same three. #928 changed the permissive-modify DN comparison a few lines above it.On the merged tree:
ServerErrorResultCodeTestCase13/13,NamingConflictTest21/21,UpdateOperationTest31/31,IsServerFailureTest23/23.