diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java index a63026c8a9..86ddbea13e 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/AttributeIndex.java @@ -14,6 +14,7 @@ * Copyright 2006-2010 Sun Microsystems, Inc. * Portions Copyright 2011-2016 ForgeRock AS. * Portions Copyright 2014 Manuel Gaupp + * Portions Copyright 2026 3A Systems, LLC. */ package org.opends.server.backends.pluggable; @@ -924,18 +925,53 @@ public synchronized ConfigChangeResult applyConfigurationChange(final BackendInd // indexIdToIndexes newIndexIdToIndexes.putAll(updatedIndexes); + // What the new configuration asks of the indexes which stay is decided here, before any of + // the three writes below, and reported here as well. Decided before the write which applies + // it: neither the entry limit an index holds nor its in-memory trusted flag is rolled back + // with the transaction, while the removal of the persisted TRUSTED flag is, so an attempt + // which rolls back would leave the raised limit in place, and a replay of it would compare + // that limit against itself, find nothing to rebuild, and commit an index whose entry limit + // was raised and which the storage still records as trusted. Reported before the writes + // rather than once they have committed, because the instruction holds whichever way they go: + // the configuration entry already holds the raised limit when this listener runs, and the + // next open of the index applies it to a tree whose keys were given up under the lower one. + // Only the limit itself waits for the write which untrusts the index to commit. + final List indexesToUntrust = new ArrayList<>(); + final List rebuildMessages = new ArrayList<>(); + planIndexUpdates(updatedIndexes.values(), newConfiguration, indexesToUntrust, rebuildMessages); + for (LocalizableMessage rebuildMessage : rebuildMessages) + { + ccr.setAdminActionRequired(true); + ccr.addMessage(rebuildMessage); + } + // Open added indexes *before* adding them to indexIdToIndexes + final List addedIndexesToRebuild = new ArrayList<>(); entryContainer.getRootContainer().getStorage().write(new WriteOperation() { @Override public void run(WriteableTransaction txn) throws Exception { + // Emptied at the start of every attempt: the storage may replay this operation, and what + // has to be reported is what the attempt which commits found, not what every attempt did. + addedIndexesToRebuild.clear(); for (MatchingRuleIndex addedIndex : addedIndexes.values()) { - createIndex(txn, addedIndex, ccr); + if (createIndex(txn, addedIndex)) + { + addedIndexesToRebuild.add(addedIndex.getName()); + } } } }); + // Reported once that write has committed, since a message an attempt which rolls back added + // to the result stays there, and the operator would be told once per attempt. + // EntryContainer.applyConfigurationAdd reports the index it adds the same way. + for (TreeName addedIndex : addedIndexesToRebuild) + { + ccr.setAdminActionRequired(true); + ccr.addMessage(NOTE_INDEX_ADD_REQUIRES_REBUILD.get(addedIndex)); + } config = newConfiguration; indexingOptions = newIndexingOptions; @@ -962,17 +998,28 @@ public void run(WriteableTransaction txn) throws Exception entryContainer.unlock(); } - entryContainer.getRootContainer().getStorage().write(new WriteOperation() + // The only part of what the indexes which stay are asked for that is written down. A change + // which untrusts none of them - a lowered limit - opens no transaction, rather than one a + // bounded storage could give up on with nothing to give up; VLVIndex guards its write the + // same way. + if (!indexesToUntrust.isEmpty()) { - @Override - public void run(WriteableTransaction txn) throws Exception + entryContainer.getRootContainer().getStorage().write(new WriteOperation() { - for (final Index updatedIndex : updatedIndexes.values()) + @Override + public void run(WriteableTransaction txn) throws Exception { - updateIndex(updatedIndex, newConfiguration, ccr, txn); + for (final Index updatedIndex : indexesToUntrust) + { + updatedIndex.setTrusted(txn, false); + } } - } - }); + }); + } + for (final Index updatedIndex : updatedIndexes.values()) + { + updatedIndex.setIndexEntryLimit(newConfiguration.getIndexEntryLimit()); + } } catch (Exception e) { @@ -983,36 +1030,45 @@ public void run(WriteableTransaction txn) throws Exception return ccr; } - private static void createIndex(WriteableTransaction txn, MatchingRuleIndex index, ConfigChangeResult ccr) + /** + * Opens an index this change adds, and answers whether it has to be rebuilt before it is used. + * Answered to the caller rather than reported from here: this runs inside a {@link WriteOperation} + * the storage may replay, and the report belongs to the attempt which commits. + */ + private static boolean createIndex(WriteableTransaction txn, MatchingRuleIndex index) { index.open(txn, true); - if (!index.isTrusted()) - { - ccr.setAdminActionRequired(true); - ccr.addMessage(NOTE_INDEX_ADD_REQUIRES_REBUILD.get(index.getName())); - } + return !index.isTrusted(); } - private static void updateIndex(Index updatedIndex, BackendIndexCfg newConfig, ConfigChangeResult ccr, - WriteableTransaction txn) + /** + * Works out what the new configuration asks of the indexes which stay: which of them may no longer + * be trusted, and what the operator has to be told about each of them. Decided from the state the + * indexes are in before anything is applied to them, so that a write the storage replays reaches + * the same answer on every attempt. + */ + private static void planIndexUpdates(Collection updatedIndexes, BackendIndexCfg newConfig, + List indexesToUntrust, List rebuildMessages) { - // This index could still be used since a new smaller index size limit doesn't impact validity of the results. - boolean newLimitRequiresRebuild = updatedIndex.setIndexEntryLimit(newConfig.getIndexEntryLimit()); - if (newLimitRequiresRebuild) - { - ccr.setAdminActionRequired(true); - ccr.addMessage(NOTE_CONFIG_INDEX_ENTRY_LIMIT_REQUIRES_REBUILD.get(updatedIndex.getName())); - } - // This index could still be used when disabling confidentiality. - boolean newConfidentialityRequiresRebuild = updatedIndex.setConfidential(newConfig.isConfidentialityEnabled()); - if (newConfidentialityRequiresRebuild) - { - ccr.setAdminActionRequired(true); - ccr.addMessage(NOTE_CONFIG_INDEX_CONFIDENTIALITY_REQUIRES_REBUILD.get(updatedIndex.getName())); - } - if (newLimitRequiresRebuild || newConfidentialityRequiresRebuild) + for (Index updatedIndex : updatedIndexes) { - updatedIndex.setTrusted(txn, false); + // This index could still be used since a new smaller index size limit doesn't impact validity of the results. + boolean newLimitRequiresRebuild = updatedIndex.getIndexEntryLimit() < newConfig.getIndexEntryLimit(); + if (newLimitRequiresRebuild) + { + rebuildMessages.add(NOTE_CONFIG_INDEX_ENTRY_LIMIT_REQUIRES_REBUILD.get(updatedIndex.getName())); + } + // This index could still be used when disabling confidentiality. Asked rather than told: for an + // index this only compares the configuration with the parameters its crypto suite holds. + boolean newConfidentialityRequiresRebuild = updatedIndex.setConfidential(newConfig.isConfidentialityEnabled()); + if (newConfidentialityRequiresRebuild) + { + rebuildMessages.add(NOTE_CONFIG_INDEX_CONFIDENTIALITY_REQUIRES_REBUILD.get(updatedIndex.getName())); + } + if (newLimitRequiresRebuild || newConfidentialityRequiresRebuild) + { + indexesToUntrust.add(updatedIndex); + } } } diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VLVIndex.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VLVIndex.java index ca84641b19..ba19cb18ad 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VLVIndex.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/VLVIndex.java @@ -65,7 +65,6 @@ import org.opends.server.controls.ServerSideSortRequestControl; import org.opends.server.controls.VLVRequestControl; import org.opends.server.controls.VLVResponseControl; -import org.opends.server.core.DirectoryServer; import org.opends.server.core.SearchOperation; import org.opends.server.protocols.ldap.LDAPResultCode; import org.opends.server.types.Attribute; @@ -73,7 +72,6 @@ import org.opends.server.types.Entry; import org.opends.server.types.Modification; import org.opends.server.types.SearchFilter; -import org.opends.server.util.StaticUtils; /** * This class represents a VLV index. @@ -229,71 +227,103 @@ private static SearchFilter parseSearchFilter(final BackendVLVIndexCfg cfg, Stri @Override public synchronized ConfigChangeResult applyConfigurationChange(final BackendVLVIndexCfg cfg) { - try - { - final ConfigChangeResult ccr = new ConfigChangeResult(); - storage.write(new WriteOperation() - { - @Override - public void run(final WriteableTransaction txn) throws Exception - { - applyConfigurationChange0(txn, cfg, ccr); - } - }); - return ccr; - } - catch (final Exception e) - { - throw new StorageRuntimeException(e); - } - } - - private synchronized void applyConfigurationChange0( - final WriteableTransaction txn, final BackendVLVIndexCfg cfg, final ConfigChangeResult ccr) - { - // Update base DN only if changed - if (!config.getBaseDN().equals(cfg.getBaseDN())) + final ConfigChangeResult ccr = new ConfigChangeResult(); + /* + * What this change asks for is worked out here, before the write which applies it, and what it + * changes is published after that write has committed. Asked and answered from within a + * WriteOperation the storage may replay, every question below is asked of the configuration + * this vlvIndex holds and answered into the result of the change, and neither is rolled back + * with the transaction: an attempt which rolls back leaves this vlvIndex already holding the + * new definition, so the replay of it finds nothing changed, and it leaves the result already + * asking for the rebuild, which is the only thing that keeps the replay removing the TRUSTED + * flag the rollback put back. The operator is told to rebuild the index once per attempt, and + * what stops the storage from committing a vlvIndex it still records as trusted - answering a + * sorted search after a restart out of a tree built for the definition it no longer has - is + * that repetition. See OpenDJ issue #991, which reports this of AttributeIndex, where the + * index itself holds the answer and the replay does commit a stale index as trusted. + */ + final boolean baseDNChanged = !config.getBaseDN().equals(cfg.getBaseDN()); + if (baseDNChanged) { - this.baseDN = cfg.getBaseDN(); ccr.setAdminActionRequired(true); } - - // Update scope only if changed - if (!config.getScope().equals(cfg.getScope())) + final boolean scopeChanged = !config.getScope().equals(cfg.getScope()); + if (scopeChanged) { - this.scope = convertScope(cfg.getScope()); ccr.setAdminActionRequired(true); } - - // Update the filter only if changed - if (!config.getFilter().equals(cfg.getFilter())) + // parseSearchFilter() asks for the administrative action itself, and only once it has parsed. + final boolean filterChanged = !config.getFilter().equals(cfg.getFilter()); + final SearchFilter newFilter = filterChanged ? parseSearchFilter(cfg, getName().toString(), ccr) : filter; + final boolean sortOrderChanged = !config.getSortOrder().equals(cfg.getSortOrder()); + final List newSortKeys; + if (sortOrderChanged) { - this.filter = parseSearchFilter(cfg, getName().toString(), ccr); + newSortKeys = parseSortKeys(cfg.getSortOrder(), ccr); + ccr.setAdminActionRequired(true); } - - // Update the sort order only if changed - if (!config.getSortOrder().equals(cfg.getSortOrder())) + else { - this.sortKeys = parseSortKeys(cfg.getSortOrder(), ccr); - ccr.setAdminActionRequired(true); + newSortKeys = sortKeys; } - if (ccr.adminActionRequired()) + final boolean requiresRebuild = ccr.adminActionRequired(); + if (requiresRebuild) { - trusted = false; + // Reported outside the write rather than from within it, since a message an attempt which + // rolls back added to the result stays there and the operator would be told once per + // attempt; and before the write rather than once it has committed, because the instruction + // holds whichever way the write goes: the configuration entry already holds the new + // definition when this listener runs, and the next open of this vlvIndex applies it to a + // tree built for the definition it no longer has. ccr.addMessage(NOTE_INDEX_ADD_REQUIRES_REBUILD.get(getName())); + + // The only part of this change which is written down. A change asking for nothing this + // vlvIndex has to be rebuilt for opens no transaction, rather than one a bounded storage + // could give up on with nothing to give up. A conflict raised by the flag removal is left to + // the storage, whose retry loop replays the operation: caught inside the operation, as it + // used to be, it was swallowed where the storage was waiting to be told to replay, and the + // attempt committed having done nothing. What the storage gives up on is reported the way + // AttributeIndex reports it, with the result built so far - the rebuild asked for above + // holds on that road too - rather than thrown past ConfigurationHandler, which catches + // nothing a listener throws and would discard that result whole. try { - state.removeFlagsFromIndex(txn, getName(), IndexFlag.TRUSTED); + storage.write(new WriteOperation() + { + @Override + public void run(final WriteableTransaction txn) throws Exception + { + setTrusted(txn, false); + } + }); } - catch (final StorageRuntimeException de) + catch (final Exception e) { - ccr.addMessage(LocalizableMessage.raw(StaticUtils.stackTraceToSingleLineString(de))); - ccr.setResultCodeIfSuccess(DirectoryServer.getCoreConfigManager().getServerErrorResultCode()); + ccr.setResultCode(getCoreConfigManager().getServerErrorResultCode()); + ccr.addMessage(LocalizableMessage.raw(stackTraceToSingleLineString(e))); + return ccr; } } + if (baseDNChanged) + { + this.baseDN = cfg.getBaseDN(); + } + if (scopeChanged) + { + this.scope = convertScope(cfg.getScope()); + } + if (filterChanged) + { + this.filter = newFilter; + } + if (sortOrderChanged) + { + this.sortKeys = newSortKeys; + } this.config = cfg; + return ccr; } private List parseSortKeys(final String sortOrder, ConfigChangeResult ccr) diff --git a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ReplayedConfigChangeTest.java b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ReplayedConfigChangeTest.java index 19c1aaae26..e76671930e 100644 --- a/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ReplayedConfigChangeTest.java +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ReplayedConfigChangeTest.java @@ -19,6 +19,8 @@ import static org.opends.messages.BackendMessages.ERR_BACKEND_BASEDN_NO_LONGER_HELD; import static org.opends.messages.BackendMessages.ERR_BACKEND_CANNOT_LIST_TREES_AFTER_BASEDN_CHANGE; import static org.opends.messages.BackendMessages.ERR_BACKEND_CANNOT_REGISTER_BASEDN; +import static org.opends.messages.BackendMessages.NOTE_CONFIG_INDEX_ENTRY_LIMIT_REQUIRES_REBUILD; +import static org.opends.messages.BackendMessages.NOTE_INDEX_ADD_REQUIRES_REBUILD; import static org.forgerock.opendj.config.ConfigurationMock.mockCfg; import static org.mockito.Mockito.any; import static org.mockito.Mockito.mock; @@ -46,11 +48,14 @@ import org.forgerock.opendj.ldap.ResultCode; import org.forgerock.opendj.ldap.schema.AttributeType; import org.forgerock.opendj.server.config.meta.BackendIndexCfgDefn.IndexType; +import org.forgerock.opendj.server.config.meta.BackendVLVIndexCfgDefn.Scope; import org.forgerock.opendj.server.config.server.BackendIndexCfg; +import org.forgerock.opendj.server.config.server.BackendVLVIndexCfg; import org.forgerock.opendj.server.config.server.PDBBackendCfg; import org.opends.server.DirectoryServerTestCase; import org.opends.server.TestCaseUtils; import org.opends.server.backends.pdb.PDBStorage; +import org.opends.server.backends.pluggable.AttributeIndex.MatchingRuleIndex; import org.opends.server.backends.pluggable.State.IndexFlag; import org.opends.server.backends.pluggable.spi.AccessMode; import org.opends.server.backends.pluggable.spi.Cursor; @@ -63,6 +68,7 @@ import org.opends.server.backends.pluggable.spi.UpdateFunction; import org.opends.server.backends.pluggable.spi.WriteOperation; import org.opends.server.backends.pluggable.spi.WriteableTransaction; +import org.opends.server.core.AddOperation; import org.opends.server.core.ServerContext; import org.opends.server.types.BackupConfig; import org.opends.server.types.BackupDirectory; @@ -95,6 +101,8 @@ public class ReplayedConfigChangeTest extends DirectoryServerTestCase private static final DN ADDED = DN.valueOf("dc=b907c,dc=com"); /** Hierarchically related to {@link #KEPT}, which one backend is not allowed to serve as well. */ private static final DN UNREGISTRABLE = DN.valueOf("dc=b907d,dc=b907a,dc=com"); + /** Held in lower case, which is how an entry container keys the vlvIndexes it holds. */ + private static final String VLV_INDEX_NAME = "b907vlv"; private ServerContext serverContext; private AttributeType cnType; @@ -545,6 +553,16 @@ public void aChangeWhichLeavesTheBaseDNsAloneOpensNoTransaction() throws Excepti } } + private static Set treesOf(EntryContainer ec) + { + final Set names = new HashSet<>(); + for (Tree tree : ec.listTrees()) + { + names.add(tree.getName()); + } + return names; + } + /** The messages a change result carries, by identity rather than by their formatted text. */ private static Set ordinalsOf(ConfigChangeResult ccr) { @@ -556,14 +574,263 @@ private static Set ordinalsOf(ConfigChangeResult ccr) return ordinals; } - private static Set treesOf(EntryContainer ec) + /** + * An index a change adds is opened untrusted while the backend holds entries, and the operator is + * told to rebuild it. That message belongs to the attempt which commits: added to the result from + * inside the operation, a replay repeats it once per attempt. + */ + @Test + public void anIndexAddedByAChangeIsReportedOnceWhenTheTransactionIsReplayed() throws Exception { - final Set names = new HashSet<>(); - for (Tree tree : ec.listTrees()) + final ReplayingBackend backend = openBackend(newTreeSet(KEPT)); + try { - names.add(tree.getName()); + addBaseEntry(backend, KEPT, "b907a"); + final AttributeIndex index = backend.getRootContainer().getEntryContainer(KEPT).getAttributeIndex(cnType); + + // The first of the three writes a change makes is the one which opens the indexes it adds. + backend.storage.conflictAtCommit(1); + final ConfigChangeResult ccr = + index.applyConfigurationChange(indexCfg(newTreeSet(IndexType.EQUALITY, IndexType.PRESENCE), 4000)); + + assertThat(backend.storage.attempts()).isEqualTo(2); + assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(ccr.adminActionRequired()).isTrue(); + assertThat(ccr.getMessages()).as("the rebuild the added index needs, asked for once").hasSize(1); + assertThat(ordinalsOf(ccr)).containsOnly(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal()); + assertThat(index.isIndexed(IndexType.PRESENCE)).as("the index type the change added").isTrue(); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * Raising the entry limit of an index leaves it holding the keys it gave up under the lower limit, + * so it has to be rebuilt and may not be trusted until it is. The limit an index holds is not + * rolled back with the transaction while the removal of its persisted TRUSTED flag is, so an + * attempt which rolls back must not be what decides that the limit went up. + */ + @Test + public void aRaisedEntryLimitUntrustsTheIndexWhenTheTransactionIsReplayed() throws Exception + { + final ReplayingBackend backend = openBackend(newTreeSet(KEPT)); + try + { + final RootContainer rootContainer = backend.getRootContainer(); + final EntryContainer ec = rootContainer.getEntryContainer(KEPT); + final AttributeIndex index = ec.getAttributeIndex(cnType); + final MatchingRuleIndex cnIndex = index.getNameToIndexes().values().iterator().next(); + assertThat(persistedFlags(rootContainer, ec, cnIndex.getName())).contains(TRUSTED); + + // The third write is the one which removes the flag: the first two open the indexes the + // change adds and delete the ones it removes, and it neither adds nor removes any. + final int writesBefore = backend.storage.writes(); + backend.storage.conflictAtCommitOnWrite(3, 1); + final ConfigChangeResult ccr = index.applyConfigurationChange(indexCfg(newTreeSet(IndexType.EQUALITY), 8000)); + + assertThat(backend.storage.writes()).as("the armed write was the last of three").isEqualTo(writesBefore + 3); + assertThat(backend.storage.attempts()).isEqualTo(2); + assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(ccr.adminActionRequired()).isTrue(); + assertThat(ccr.getMessages()).as("the rebuild the raised limit needs, asked for once").hasSize(1); + assertThat(ordinalsOf(ccr)).containsOnly(NOTE_CONFIG_INDEX_ENTRY_LIMIT_REQUIRES_REBUILD.ordinal()); + assertThat(cnIndex.getIndexEntryLimit()).as("the limit the change applied").isEqualTo(8000); + assertThat(cnIndex.isTrusted()).isFalse(); + assertThat(persistedFlags(rootContainer, ec, cnIndex.getName())) + .as("the flag the attempt which committed had to remove").doesNotContain(TRUSTED); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * A vlvIndex whose sort order changes holds a tree sorted under the order it no longer has, so it + * may not be trusted until it is rebuilt. Every question it asks is asked of the configuration it + * holds, and no rollback takes that configuration back, so the replay of an attempt which rolled + * back finds nothing changed: the rebuild it goes on asking for is the administrative action the + * rolled back attempt left in the result, told to the operator once per attempt. + */ + @Test + public void aChangedSortOrderUntrustsTheVlvIndexWhenTheTransactionIsReplayed() throws Exception + { + final ReplayingBackend backend = openBackendWithVlvIndex(); + try + { + final RootContainer rootContainer = backend.getRootContainer(); + final EntryContainer ec = rootContainer.getEntryContainer(KEPT); + final VLVIndex vlvIndex = ec.getVLVIndex(VLV_INDEX_NAME); + assertThat(persistedFlags(rootContainer, ec, vlvIndex.getName())).contains(TRUSTED); + + backend.storage.conflictAtCommit(1); + final ConfigChangeResult ccr = vlvIndex.applyConfigurationChange(vlvIndexCfg("+sn")); + + assertThat(backend.storage.attempts()).isEqualTo(2); + assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(ccr.adminActionRequired()).isTrue(); + assertThat(ccr.getMessages()).as("the rebuild the new sort order needs, asked for once").hasSize(1); + assertThat(ordinalsOf(ccr)).containsOnly(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal()); + assertThat(vlvIndex.isTrusted()).isFalse(); + assertThat(persistedFlags(rootContainer, ec, vlvIndex.getName())) + .as("the flag the attempt which committed had to remove").doesNotContain(TRUSTED); + + // The definition the change applied is the one this vlvIndex holds from now on, so asking for + // it a second time asks for nothing, and opens no transaction to commit nothing. + final int writesBefore = backend.storage.writes(); + final ConfigChangeResult again = vlvIndex.applyConfigurationChange(vlvIndexCfg("+sn")); + assertThat(again.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(again.adminActionRequired()).as("a change which changes nothing").isFalse(); + assertThat(again.getMessages()).isEmpty(); + assertThat(backend.storage.writes()).as("a change which changes nothing opens no transaction") + .isEqualTo(writesBefore); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * A conflict the flag removal itself raises is reported to the operation by the transaction, as + * a {@link StorageRuntimeException} wrapping it, and belongs to the storage's retry loop. Caught + * inside the operation and turned into a message, it was a conflict swallowed where the storage + * was waiting to be told to replay: the attempt committed having removed nothing, and the change + * reported a failure against a vlvIndex the storage still recorded as trusted. + */ + @Test + public void aConflictRaisedByTheRemovalOfTheVlvIndexFlagIsReplayed() throws Exception + { + final ReplayingBackend backend = openBackendWithVlvIndex(); + try + { + final RootContainer rootContainer = backend.getRootContainer(); + final EntryContainer ec = rootContainer.getEntryContainer(KEPT); + final VLVIndex vlvIndex = ec.getVLVIndex(VLV_INDEX_NAME); + assertThat(persistedFlags(rootContainer, ec, vlvIndex.getName())).contains(TRUSTED); + + backend.storage.conflictAsTheTransactionReportsIt(1); + final ConfigChangeResult ccr = vlvIndex.applyConfigurationChange(vlvIndexCfg("+sn")); + + assertThat(backend.storage.attempts()).as("replayed by the storage, not answered from inside").isEqualTo(2); + assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(ccr.adminActionRequired()).isTrue(); + assertThat(ccr.getMessages()).as("the rebuild the new sort order needs, asked for once").hasSize(1); + assertThat(ordinalsOf(ccr)).containsOnly(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal()); + assertThat(vlvIndex.isTrusted()).isFalse(); + assertThat(persistedFlags(rootContainer, ec, vlvIndex.getName())) + .as("the flag the replayed attempt removed").doesNotContain(TRUSTED); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * A raised entry limit has to be rebuilt for whether or not the write which untrusts the index is + * applied: the configuration entry holds the raised limit before the listener runs, and the next + * open of the index applies it to a tree whose keys were given up under the lower one. So the + * instruction is given before that write, and a write the storage gives up on - the last attempt + * of its retry loop, or a failure it does not replay at all - leaves it in the result, next to + * the failure, rather than dropping it together with the limit it could not apply. + */ + @Test + public void aRaisedEntryLimitIsReportedWhenTheWriteWhichUntrustsTheIndexGivesUp() throws Exception + { + final ReplayingBackend backend = openBackend(newTreeSet(KEPT)); + try + { + final RootContainer rootContainer = backend.getRootContainer(); + final EntryContainer ec = rootContainer.getEntryContainer(KEPT); + final AttributeIndex index = ec.getAttributeIndex(cnType); + final MatchingRuleIndex cnIndex = index.getNameToIndexes().values().iterator().next(); + assertThat(persistedFlags(rootContainer, ec, cnIndex.getName())).contains(TRUSTED); + + backend.storage.failWithoutReplayOnWrite(3); + final ConfigChangeResult ccr = index.applyConfigurationChange(indexCfg(newTreeSet(IndexType.EQUALITY), 8000)); + + assertThat(ccr.getResultCode()).isNotEqualTo(ResultCode.SUCCESS); + assertThat(ccr.adminActionRequired()).as("the rebuild the raised limit needs, on the road which failed").isTrue(); + assertThat(ordinalsOf(ccr)).contains(NOTE_CONFIG_INDEX_ENTRY_LIMIT_REQUIRES_REBUILD.ordinal()); + assertThat(cnIndex.getIndexEntryLimit()).as("the limit the failed write did not apply").isEqualTo(4000); + assertThat(persistedFlags(rootContainer, ec, cnIndex.getName())) + .as("what the restart will read").contains(TRUSTED); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * A lowered entry limit leaves every key the index holds valid, so it asks for no rebuild, and it + * untrusts nothing, so it opens no transaction: nothing for a bounded storage to give up on. + */ + @Test + public void aLoweredEntryLimitNeedsNoRebuildAndOpensNoTransaction() throws Exception + { + final ReplayingBackend backend = openBackend(newTreeSet(KEPT)); + try + { + final AttributeIndex index = backend.getRootContainer().getEntryContainer(KEPT).getAttributeIndex(cnType); + final MatchingRuleIndex cnIndex = index.getNameToIndexes().values().iterator().next(); + + final int writesBefore = backend.storage.writes(); + final ConfigChangeResult ccr = index.applyConfigurationChange(indexCfg(newTreeSet(IndexType.EQUALITY), 2000)); + + assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(ccr.adminActionRequired()).as("a lowered limit needs no rebuild").isFalse(); + assertThat(ccr.getMessages()).isEmpty(); + assertThat(cnIndex.isTrusted()).isTrue(); + assertThat(cnIndex.getIndexEntryLimit()).as("the limit the change applied").isEqualTo(2000); + assertThat(backend.storage.writes()).as("the two writes which add and remove indexes, and no third") + .isEqualTo(writesBefore + 2); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * The same road for a vlvIndex: the write which untrusts it is the only one the change makes, and + * a failure of it is reported with the rebuild the change asked for, rather than thrown out of + * the listener with that result discarded. + */ + @Test + public void aChangedSortOrderIsReportedWhenTheWriteWhichUntrustsTheVlvIndexGivesUp() throws Exception + { + final ReplayingBackend backend = openBackendWithVlvIndex(); + try + { + final RootContainer rootContainer = backend.getRootContainer(); + final EntryContainer ec = rootContainer.getEntryContainer(KEPT); + final VLVIndex vlvIndex = ec.getVLVIndex(VLV_INDEX_NAME); + assertThat(persistedFlags(rootContainer, ec, vlvIndex.getName())).contains(TRUSTED); + + backend.storage.failWithoutReplay(); + final ConfigChangeResult ccr = vlvIndex.applyConfigurationChange(vlvIndexCfg("+sn")); + + assertThat(ccr.getResultCode()).isNotEqualTo(ResultCode.SUCCESS); + assertThat(ccr.adminActionRequired()) + .as("the rebuild the new sort order needs, on the road which failed").isTrue(); + assertThat(ordinalsOf(ccr)).contains(NOTE_INDEX_ADD_REQUIRES_REBUILD.ordinal()); + assertThat(persistedFlags(rootContainer, ec, vlvIndex.getName())) + .as("what the restart will read").contains(TRUSTED); + + // The definition the failed change did not publish is still a change when asked for again. + final ConfigChangeResult again = vlvIndex.applyConfigurationChange(vlvIndexCfg("+sn")); + assertThat(again.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(again.adminActionRequired()).as("the definition the failed write did not publish").isTrue(); + assertThat(persistedFlags(rootContainer, ec, vlvIndex.getName())).doesNotContain(TRUSTED); + } + finally + { + backend.finalizeBackend(); } - return names; } /** Reads back the flags an index was given when it was opened, as they are stored. */ @@ -575,10 +842,21 @@ private static EnumSet persistedFlags(RootContainer rootContainer, En } private ReplayingBackend openBackend(SortedSet baseDNs) throws Exception + { + return openBackend(baseDNs, false); + } + + /** The vlvIndex is opened only where a test is about one, since every base DN gets a copy of it. */ + private ReplayingBackend openBackendWithVlvIndex() throws Exception + { + return openBackend(newTreeSet(KEPT), true); + } + + private ReplayingBackend openBackend(SortedSet baseDNs, boolean withVlvIndex) throws Exception { final ReplayingBackend backend = new ReplayingBackend(); backend.setBackendID(BACKEND_ID); - backend.configuredWith = backendCfg(baseDNs); + backend.configuredWith = backendCfg(baseDNs, withVlvIndex); backend.configureBackend(backend.configuredWith, serverContext); // Start from a pristine on-disk state so that a previous run cannot mask the defect. backend.storage.removeStorageFiles(); @@ -617,6 +895,11 @@ private ReplayingBackend openBackend(SortedSet baseDNs) throws Exception } private PDBBackendCfg backendCfg(SortedSet baseDNs) throws ConfigException + { + return backendCfg(baseDNs, false); + } + + private PDBBackendCfg backendCfg(SortedSet baseDNs, boolean withVlvIndex) throws ConfigException { final PDBBackendCfg cfg = mockCfg(PDBBackendCfg.class); when(cfg.dn()).thenReturn(DN.valueOf("ds-cfg-backend-id=" + BACKEND_ID + ",cn=Backends,cn=config")); @@ -627,17 +910,52 @@ private PDBBackendCfg backendCfg(SortedSet baseDNs) throws ConfigException when(cfg.getDBCachePercent()).thenReturn(20); when(cfg.getBaseDN()).thenReturn(baseDNs); when(cfg.listBackendIndexes()).thenReturn(new String[] { "cn" }); - when(cfg.listBackendVLVIndexes()).thenReturn(new String[0]); - - final BackendIndexCfg indexCfg = mock(BackendIndexCfg.class); - when(indexCfg.getIndexType()).thenReturn(newTreeSet(IndexType.EQUALITY)); - when(indexCfg.getAttribute()).thenReturn(cnType); - when(indexCfg.getIndexEntryLimit()).thenReturn(4000); - when(indexCfg.getSubstringLength()).thenReturn(6); - when(cfg.getBackendIndex("cn")).thenReturn(indexCfg); + // Built before it is handed over: stubbing a mock from inside a when() of another mock leaves + // that when() unfinished, and Mockito fails the next test to touch either of them. + final BackendIndexCfg cnIndexCfg = indexCfg(newTreeSet(IndexType.EQUALITY), 4000); + when(cfg.getBackendIndex("cn")).thenReturn(cnIndexCfg); + if (withVlvIndex) + { + final BackendVLVIndexCfg vlvCfg = vlvIndexCfg("+cn"); + when(cfg.listBackendVLVIndexes()).thenReturn(new String[] { VLV_INDEX_NAME }); + when(cfg.getBackendVLVIndex(VLV_INDEX_NAME)).thenReturn(vlvCfg); + } + else + { + when(cfg.listBackendVLVIndexes()).thenReturn(new String[0]); + } + return cfg; + } + + private BackendIndexCfg indexCfg(SortedSet indexTypes, int indexEntryLimit) + { + final BackendIndexCfg cfg = mock(BackendIndexCfg.class); + when(cfg.getIndexType()).thenReturn(indexTypes); + when(cfg.getAttribute()).thenReturn(cnType); + when(cfg.getIndexEntryLimit()).thenReturn(indexEntryLimit); + when(cfg.getSubstringLength()).thenReturn(6); + return cfg; + } + + private BackendVLVIndexCfg vlvIndexCfg(String sortOrder) + { + final BackendVLVIndexCfg cfg = mock(BackendVLVIndexCfg.class); + when(cfg.getName()).thenReturn(VLV_INDEX_NAME); + when(cfg.getBaseDN()).thenReturn(KEPT); + when(cfg.getScope()).thenReturn(Scope.WHOLE_SUBTREE); + when(cfg.getFilter()).thenReturn("(objectClass=*)"); + when(cfg.getSortOrder()).thenReturn(sortOrder); return cfg; } + /** An index of an empty backend is trusted when it is opened, whatever its tree holds. */ + private static void addBaseEntry(ReplayingBackend backend, DN baseDN, String domainComponent) throws Exception + { + backend.addEntry( + TestCaseUtils.makeEntry("dn: " + baseDN, "objectClass: top", "objectClass: domain", "dc: " + domainComponent), + mock(AddOperation.class)); + } + /** A backend whose storage makes the next write operation conflict, and so be replayed. */ private static final class ReplayingBackend extends BackendImpl { @@ -709,6 +1027,13 @@ private enum ConflictPoint { /** As soon as the operation first touches the transaction, before it has changed anything. */ FIRST_STORAGE_ACCESS, + /** + * As soon as the operation first touches the transaction, in the form the transaction of + * {@code PDBStorage} reports a conflict: a {@link StorageRuntimeException} wrapping the + * {@link RollbackException}. What an operation catching the former from inside sees, where + * {@link #FIRST_STORAGE_ACCESS} raises the bare conflict such a catch never meets. + */ + FIRST_STORAGE_ACCESS_AS_THE_TRANSACTION_REPORTS_IT, /** Once the operation has run to completion, as a conflict reported by {@code commit()}. */ COMMIT, /** Once the operation has run to completion, as a failure which is not replayed at all. */ @@ -723,6 +1048,8 @@ private enum ConflictPoint private final Storage delegate; private Runnable onListTrees; private ConflictPoint conflictPoint; + /** Which write, counted over the life of this storage, is armed; zero for the next one. */ + private int armedWrite; private int conflictsLeft; private int attempts; private int writes; @@ -737,16 +1064,46 @@ void conflictAtFirstStorageAccess(int conflicts) arm(ConflictPoint.FIRST_STORAGE_ACCESS, conflicts); } + /** + * Conflicts at the first storage access of the operation, in the form the transaction itself + * reports one: what the operation sees where it asks the transaction for something, rather than + * what the retry loop sees once the operation has let it through. + */ + void conflictAsTheTransactionReportsIt(int conflicts) + { + arm(ConflictPoint.FIRST_STORAGE_ACCESS_AS_THE_TRANSACTION_REPORTS_IT, conflicts); + } + void conflictAtCommit(int conflicts) { arm(ConflictPoint.COMMIT, conflicts); } + /** + * Conflicts at commit time on the {@code nth} write asked for from now on, the next one being + * the first: a configuration change which makes several writes has to arm the one it is about. + */ + void conflictAtCommitOnWrite(int nth, int conflicts) + { + arm(ConflictPoint.COMMIT, conflicts); + armedWrite = writes + nth; + } + void failWithoutReplay() { arm(ConflictPoint.NO_REPLAY, 1); } + /** + * Fails without a replay on the {@code nth} write asked for from now on, the next one being + * the first: what the last attempt of a retry loop which gave up leaves the caller holding. + */ + void failWithoutReplayOnWrite(int nth) + { + arm(ConflictPoint.NO_REPLAY, 1); + armedWrite = writes + nth; + } + void failAfterCommit() { arm(ConflictPoint.NO_REPLAY_AFTER_COMMIT, 1); @@ -756,6 +1113,7 @@ private void arm(ConflictPoint where, int conflicts) { conflictPoint = where; conflictsLeft = conflicts; + armedWrite = 0; attempts = 0; } @@ -776,12 +1134,13 @@ public void write(final WriteOperation writeOperation) throws Exception { writes++; final ConflictPoint armed = conflictPoint; - if (armed == null) + if (armed == null || (armedWrite != 0 && writes != armedWrite)) { delegate.write(writeOperation); return; } conflictPoint = null; + armedWrite = 0; if (armed == ConflictPoint.NO_REPLAY_AFTER_COMMIT) { // Committed, then reported as a failure: the operation's work outlives the failure, as it @@ -812,7 +1171,12 @@ public void run(WriteableTransaction txn) throws Exception } if (armed == ConflictPoint.FIRST_STORAGE_ACCESS) { - writeOperation.run(new ConflictingTransaction()); + writeOperation.run(new ConflictingTransaction(false)); + return; + } + if (armed == ConflictPoint.FIRST_STORAGE_ACCESS_AS_THE_TRANSACTION_REPORTS_IT) + { + writeOperation.run(new ConflictingTransaction(true)); return; } writeOperation.run(txn); @@ -905,9 +1269,21 @@ public void close() /** A transaction which conflicts as soon as it is used, without ever reaching the storage. */ private static final class ConflictingTransaction implements WriteableTransaction { - private static RollbackException conflict() + /** + * Whether the conflict is raised as {@code PDBStorage}'s transaction raises it - wrapped in a + * {@link StorageRuntimeException}, which that storage's retry loop unwraps - or bare. + */ + private final boolean asTheTransactionReportsIt; + + ConflictingTransaction(boolean asTheTransactionReportsIt) + { + this.asTheTransactionReportsIt = asTheTransactionReportsIt; + } + + private RuntimeException conflict() { - return new RollbackException(); + final RollbackException conflict = new RollbackException(); + return asTheTransactionReportsIt ? new StorageRuntimeException(conflict) : conflict; } @Override