From 965c8607560f6f10ba9902981a75236a59cb3f1a Mon Sep 17 00:00:00 2001 From: maximthomas Date: Wed, 2 Sep 2026 15:03:04 +0300 Subject: [PATCH 1/3] [#907] Change the base DNs of a pluggable backend outside the write the storage replays Storage.write requires its WriteOperation to be idempotent, because an implementation replays it after a transaction conflict. applyConfigurationChange performed the registry work - which no rollback reaches - inside that operation, so a replay half applied the change: the removal path re-read the stale cfg and deregistered a base DN it had already deregistered, reporting an UNWILLING_TO_PERFORM against the operator's own DN rather than the conflict; the creation path skipped the DN its first attempt had registered, leaving a base DN registered with no trees at all and reporting success. The operation now only deletes and opens trees, which a rollback undoes, and the base DNs to remove and to add are worked out ahead of it, so no attempt sees different work to do than the one it replaces. The registries, baseDNs and cfg are updated once the write has committed. The entry containers of a removed base DN are held exclusively across the write, since their trees are now deleted while they are still registered. ReplayedConfigChangeTest drives the replay from PDBStorage's own retry loop, so that every attempt shares the storage implementation and the PersistIt exchanges a real conflict would. --- .../backends/pluggable/BackendImpl.java | 178 ++++-- .../pluggable/ReplayedConfigChangeTest.java | 553 ++++++++++++++++++ 2 files changed, 692 insertions(+), 39 deletions(-) create mode 100644 opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ReplayedConfigChangeTest.java diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/BackendImpl.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/BackendImpl.java index 03cd9303a7..dcacaec521 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/BackendImpl.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/BackendImpl.java @@ -18,14 +18,18 @@ package org.opends.server.backends.pluggable; import static org.forgerock.util.Reject.*; +import static org.forgerock.util.Utils.closeSilently; import static org.opends.messages.BackendMessages.*; import static org.opends.server.util.ServerConstants.*; import static org.opends.server.util.StaticUtils.*; import java.io.IOException; +import java.util.ArrayList; import java.util.Collections; import java.util.HashSet; +import java.util.LinkedHashMap; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.SortedSet; import java.util.concurrent.ExecutionException; @@ -845,83 +849,179 @@ public boolean isConfigurationChangeAcceptable(PluggableBackendCfg cfg, List + * {@link Storage#write(WriteOperation)} replays its operation after a transaction conflict, so + * the operation below is confined to work a rollback undoes: the trees are deleted and opened + * there, while the registries, which no rollback reaches, are updated once the write has + * committed. Getting this the wrong way round leaves the change half applied, and its replay + * reports the missing half rather than the conflict that caused it. + *

+ * What makes the operation replayable is that the base DNs to remove and to add are worked out + * once, ahead of the write, so that no attempt can see different work to do than the attempt it + * is replacing. + */ @Override public ConfigChangeResult applyConfigurationChange(final PluggableBackendCfg newCfg) { final ConfigChangeResult ccr = new ConfigChangeResult(); + if (rootContainer == null) + { + return ccr; + } + + final SortedSet newBaseDNs = newCfg.getBaseDN(); + // Ask the root container what this backend holds rather than the configuration it was last + // given: a base DN which an earlier, failed change left behind is work to do, and a + // configuration which was never applied is not. RootContainer.getBaseDNs() is a live view of + // the registered containers, so take a copy of it before anything registers one. + final Set currentBaseDNs = new HashSet<>(rootContainer.getBaseDNs()); + final List deleted = new ArrayList<>(); + for (DN baseDN : currentBaseDNs) + { + if (!newBaseDNs.contains(baseDN)) + { + deleted.add(rootContainer.getEntryContainer(baseDN)); + } + } + final List added = new ArrayList<>(); + for (DN baseDN : newBaseDNs) + { + if (!currentBaseDNs.contains(baseDN)) + { + added.add(baseDN); + } + } + // Opened by the write operation, registered only once it has committed. + final Map created = new LinkedHashMap<>(); + + // The trees of a removed base DN are now deleted while it is still registered, so hold its + // entry container exclusively for as long as the write runs, retries included, as + // RootContainer.close() does. That keeps out the operations which arrive during that window; an + // operation which had taken hold of the container before the lock still ends up in a closed + // one once it is released, as it did before this ordering. + final List locked = new ArrayList<>(deleted.size()); try { - if(rootContainer != null) + for (EntryContainer ec : deleted) + { + ec.lock(); + locked.add(ec); + } + + try { rootContainer.getStorage().write(new WriteOperation() { @Override public void run(WriteableTransaction txn) throws Exception { - SortedSet newBaseDNs = newCfg.getBaseDN(); + // Give up what a previous, rolled back attempt had opened: its trees are gone, and its + // entry containers still hold the configuration listeners they registered. + closeSilently(created.values()); + created.clear(); - // Check for changes to the base DNs. - removeDeletedBaseDNs(newBaseDNs, txn); - if (!createNewBaseDNs(newBaseDNs, ccr, txn)) + for (EntryContainer ec : deleted) { - return; + ec.delete(txn); + } + for (DN baseDN : added) + { + created.put(baseDN, rootContainer.openEntryContainer(baseDN, txn, AccessMode.READ_WRITE)); } - - baseDNs = new HashSet<>(newBaseDNs); - - // Put the new configuration in place. - cfg = newCfg; } }); } + catch (Exception e) + { + closeSilently(created.values()); + ccr.setResultCode(serverContext.getCoreConfigManager().getServerErrorResultCode()); + // Neither registry was touched, and on a storage engine whose deleteTree the rollback + // undoes with the rest - persistit, je, and the jdbc backend on postgresql and sql server - + // nothing at all has been applied. Where the DDL commits of its own accord (mysql, oracle) + // or where there is no transaction to roll back (cassandra), the trees of a base DN being + // removed may be gone already, and only a restart, which reopens the backend from the + // configuration that has been stored by now, puts that right. Either way the failure alone + // never says which base DNs the change was about, so name them. + ccr.addMessage(LocalizableMessage.raw( + "Backend %s could not change its base DNs (to remove: %s, to add: %s): %s", + getBackendID(), baseDNsOf(deleted), added, stackTraceToSingleLineString(e))); + return ccr; + } + + // The change is durable from here on, so every base DN is seen through even if one fails. + deregisterDeletedBaseDNs(deleted, ccr); + registerNewBaseDNs(created, ccr); + + baseDNs = new HashSet<>(newBaseDNs); + + // Put the new configuration in place. + cfg = newCfg; } - catch (Exception e) + finally { - ccr.setResultCode(serverContext.getCoreConfigManager().getServerErrorResultCode()); - ccr.addMessage(LocalizableMessage.raw(stackTraceToSingleLineString(e))); + for (EntryContainer ec : locked) + { + ec.unlock(); + } } return ccr; } - private void removeDeletedBaseDNs(SortedSet newBaseDNs, WriteableTransaction txn) throws DirectoryException + private void deregisterDeletedBaseDNs(List deleted, ConfigChangeResult ccr) { - for (DN baseDN : cfg.getBaseDN()) + for (EntryContainer ec : deleted) { - if (!newBaseDNs.contains(baseDN)) + final DN baseDN = ec.getBaseDN(); + try { - // The base DN was deleted. serverContext.getBackendConfigManager().deregisterBaseDN(baseDN); - EntryContainer ec = rootContainer.unregisterEntryContainer(baseDN); - ec.close(); - ec.delete(txn); + } + catch (Exception e) + { + logger.traceException(e); + + ccr.setResultCode(serverContext.getCoreConfigManager().getServerErrorResultCode()); + ccr.addMessage(LocalizableMessage.raw(stackTraceToSingleLineString(e))); + } + finally + { + // Its trees have been deleted, so it must stop being reachable whatever the registry said. + rootContainer.unregisterEntryContainer(baseDN); + closeSilently(ec); } } } - private boolean createNewBaseDNs(Set newBaseDNs, ConfigChangeResult ccr, WriteableTransaction txn) + private void registerNewBaseDNs(Map created, ConfigChangeResult ccr) { - for (DN baseDN : newBaseDNs) + for (Map.Entry entry : created.entrySet()) { - if (!rootContainer.getBaseDNs().contains(baseDN)) + final DN baseDN = entry.getKey(); + try { - try - { - // The base DN was added. - EntryContainer ec = rootContainer.openEntryContainer(baseDN, txn, AccessMode.READ_WRITE); - rootContainer.registerEntryContainer(baseDN, ec); - serverContext.getBackendConfigManager().registerBaseDN(baseDN, this, false); - } - catch (Exception e) - { - logger.traceException(e); + rootContainer.registerEntryContainer(baseDN, entry.getValue()); + serverContext.getBackendConfigManager().registerBaseDN(baseDN, this, false); + } + catch (Exception e) + { + logger.traceException(e); - ccr.setResultCode(serverContext.getCoreConfigManager().getServerErrorResultCode()); - ccr.addMessage(ERR_BACKEND_CANNOT_REGISTER_BASEDN.get(baseDN, e)); - return false; - } + ccr.setResultCode(serverContext.getCoreConfigManager().getServerErrorResultCode()); + ccr.addMessage(ERR_BACKEND_CANNOT_REGISTER_BASEDN.get(baseDN, e)); } } - return true; + } + + private static List baseDNsOf(List entryContainers) + { + final List baseDNs = new ArrayList<>(entryContainers.size()); + for (EntryContainer ec : entryContainers) + { + baseDNs.add(ec.getBaseDN()); + } + return baseDNs; } /** 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 new file mode 100644 index 0000000000..8d698b3196 --- /dev/null +++ b/opendj-server-legacy/src/test/java/org/opends/server/backends/pluggable/ReplayedConfigChangeTest.java @@ -0,0 +1,553 @@ +/* + * The contents of this file are subject to the terms of the Common Development and + * Distribution License (the License). You may not use this file except in compliance with the + * License. + * + * You can obtain a copy of the License at legal/CDDLv1.0.txt. See the License for the + * specific language governing permission and limitations under the License. + * + * When distributing Covered Software, include this CDDL Header Notice in each file and include + * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL + * Header, with the fields enclosed by brackets [] replaced by your own identifying + * information: "Portions Copyright [year] [name of copyright owner]". + * + * Portions Copyright 2026 3A Systems, LLC. + */ +package org.opends.server.backends.pluggable; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.forgerock.opendj.config.ConfigurationMock.mockCfg; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import static org.opends.server.backends.pluggable.State.IndexFlag.TRUSTED; +import static org.opends.server.backends.pluggable.SuffixContainer.STATE_INDEX_NAME; +import static org.opends.server.util.CollectionUtils.newTreeSet; + +import java.util.EnumSet; +import java.util.HashSet; +import java.util.Set; +import java.util.SortedSet; + +import org.forgerock.opendj.config.server.ConfigChangeResult; +import org.forgerock.opendj.config.server.ConfigException; +import org.forgerock.opendj.ldap.ByteSequence; +import org.forgerock.opendj.ldap.ByteString; +import org.forgerock.opendj.ldap.DN; +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.server.BackendIndexCfg; +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.State.IndexFlag; +import org.opends.server.backends.pluggable.spi.AccessMode; +import org.opends.server.backends.pluggable.spi.Cursor; +import org.opends.server.backends.pluggable.spi.Importer; +import org.opends.server.backends.pluggable.spi.ReadOperation; +import org.opends.server.backends.pluggable.spi.Storage; +import org.opends.server.backends.pluggable.spi.StorageStatus; +import org.opends.server.backends.pluggable.spi.TreeName; +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.ServerContext; +import org.opends.server.types.BackupConfig; +import org.opends.server.types.BackupDirectory; +import org.opends.server.types.DirectoryException; +import org.opends.server.types.RestoreConfig; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.Test; + +import com.persistit.exception.RollbackException; + +/** + * Tests that {@link BackendImpl#applyConfigurationChange} survives a replay of its + * {@link WriteOperation}. {@link Storage#write(WriteOperation)} may replay the operation after a + * transaction conflict, so every side effect it performs must either be transactional or be + * idempotent - see OpenDJ issue #907. + *

+ * The conflict is raised from inside the operation as the {@link RollbackException} PersistIt + * itself raises, so that the replay is driven by {@code PDBStorage.write}'s own retry loop rather + * than by a second call to it. That loop keeps one storage implementation - and with it its cache + * of PersistIt exchanges - across every attempt, which a second call would not. + */ +@SuppressWarnings("javadoc") +@Test(groups = { "precommit", "pluggablebackend" }, sequential = true) +public class ReplayedConfigChangeTest extends DirectoryServerTestCase +{ + private static final String BACKEND_ID = "ReplayedConfigChangeTest"; + private static final DN KEPT = DN.valueOf("dc=b907a,dc=com"); + private static final DN REMOVED = DN.valueOf("dc=b907b,dc=com"); + private static final DN ADDED = DN.valueOf("dc=b907c,dc=com"); + + private ServerContext serverContext; + private AttributeType cnType; + + @BeforeClass + public void startServer() throws Exception + { + TestCaseUtils.startServer(); + serverContext = TestCaseUtils.getServerContext(); + cnType = serverContext.getSchema().getAttributeType("cn"); + } + + /** + * These tests are designed to fail, and a failing one can leave a base DN behind in the server + * wide registry, where it would outlive the test and break the next one to use that DN. + */ + @AfterMethod + public void deregisterLeftoverBaseDNs() + { + for (DN baseDN : new DN[] { KEPT, REMOVED, ADDED }) + { + try + { + serverContext.getBackendConfigManager().deregisterBaseDN(baseDN); + } + catch (Exception alreadyGone) + { + // Which is what the test should have left behind. + } + } + } + + /** + * A base DN removal whose transaction conflicts before it touches the storage must be replayed + * without reporting a failure against the base DN it has already deregistered. + */ + @Test + public void removalIsReplayableWhenTheTransactionConflictsBeforeAnyStorageAccess() throws Exception + { + final ReplayingBackend backend = openBackend(newTreeSet(KEPT, REMOVED)); + try + { + final RootContainer rootContainer = backend.getRootContainer(); + assertThat(rootContainer.getBaseDNs()).contains(REMOVED); + + backend.storage.conflictAtFirstStorageAccess(1); + final ConfigChangeResult ccr = backend.applyConfigurationChange(backendCfg(newTreeSet(KEPT))); + + assertThat(backend.storage.attempts()).isEqualTo(2); + assertThat(ccr.getMessages()).isEmpty(); + assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(rootContainer.getBaseDNs()).doesNotContain(REMOVED); + assertThat(backend.getBaseDNs()).doesNotContain(REMOVED); + assertThat(serverContext.getBackendConfigManager().getLocalBackendWithBaseDN(REMOVED)).isNull(); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * A base DN addition whose transaction conflicts at commit time must be replayed, so that what + * the entry container it opens writes ends up committed rather than discarded by the rollback. + */ + @Test + public void additionIsReplayableWhenTheTransactionConflictsAtCommitTime() throws Exception + { + final ReplayingBackend backend = openBackend(newTreeSet(KEPT)); + try + { + final RootContainer rootContainer = backend.getRootContainer(); + assertThat(rootContainer.getBaseDNs()).doesNotContain(ADDED); + + backend.storage.conflictAtCommit(1); + final ConfigChangeResult ccr = backend.applyConfigurationChange(backendCfg(newTreeSet(KEPT, ADDED))); + + assertThat(backend.storage.attempts()).isEqualTo(2); + assertThat(ccr.getMessages()).isEmpty(); + assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(rootContainer.getBaseDNs()).contains(ADDED); + assertThat(backend.getBaseDNs()).contains(ADDED); + assertThat(serverContext.getBackendConfigManager().getLocalBackendWithBaseDN(ADDED)).isSameAs(backend); + // Everything the newly opened entry container wrote belongs to the rolled back transaction, + // so the storage has to be asked, not the entry container which remembers writing it. + final EntryContainer ec = rootContainer.getEntryContainer(ADDED); + final TreeName cnIndex = ec.getAttributeIndex(cnType).getNameToIndexes().values().iterator().next().getName(); + assertThat(rootContainer.getStorage().listTrees()).contains(cnIndex); + assertThat(persistedFlags(rootContainer, ec, cnIndex)).contains(TRUSTED); + // The entry container the rolled back attempt opened registered five configuration listeners, + // which only its close() takes back, so the replay has to give it up before opening another. + verify(backend.configuredWith, times(1)).removePluggableChangeListener(any()); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * The trees of a removed base DN are deleted by the operation itself, so a replay deletes trees a + * rolled back attempt had already deleted. This is the case which reaches the storage, and it + * removes and adds a base DN at once because that is what an operator editing the configuration + * does. + */ + @Test + public void aRemovalAndAnAdditionInOneChangeSurviveRepeatedReplay() throws Exception + { + final ReplayingBackend backend = openBackend(newTreeSet(KEPT, REMOVED)); + try + { + final RootContainer rootContainer = backend.getRootContainer(); + final Set removedTrees = treesOf(rootContainer.getEntryContainer(REMOVED)); + assertThat(rootContainer.getStorage().listTrees()).containsAll(removedTrees); + + // More than one conflict, because the contract is that the operation is replayed until it + // succeeds rather than that it survives a single replay. + backend.storage.conflictAtCommit(2); + final ConfigChangeResult ccr = backend.applyConfigurationChange(backendCfg(newTreeSet(KEPT, ADDED))); + + assertThat(backend.storage.attempts()).isEqualTo(3); + assertThat(ccr.getMessages()).isEmpty(); + assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(rootContainer.getBaseDNs()).contains(KEPT, ADDED).doesNotContain(REMOVED); + assertThat(backend.getBaseDNs()).contains(KEPT, ADDED).doesNotContain(REMOVED); + + final Set storedTrees = rootContainer.getStorage().listTrees(); + assertThat(storedTrees).doesNotContainAnyElementsOf(removedTrees); + assertThat(storedTrees).containsAll(treesOf(rootContainer.getEntryContainer(ADDED))); + assertThat(storedTrees).containsAll(treesOf(rootContainer.getEntryContainer(KEPT))); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * A failure the storage engine does not replay must leave the backend as it was and say which + * base DNs the change was about, since the failure itself never names them. + */ + @Test + public void aFailureWhichIsNotReplayedAppliesNothingAndNamesTheBaseDNs() throws Exception + { + final ReplayingBackend backend = openBackend(newTreeSet(KEPT, REMOVED)); + try + { + final RootContainer rootContainer = backend.getRootContainer(); + final Set removedTrees = treesOf(rootContainer.getEntryContainer(REMOVED)); + + backend.storage.failWithoutReplay(); + final ConfigChangeResult ccr = backend.applyConfigurationChange(backendCfg(newTreeSet(KEPT, ADDED))); + + assertThat(ccr.getResultCode()).isNotEqualTo(ResultCode.SUCCESS); + assertThat(ccr.getMessages().toString()).contains(REMOVED.toString()).contains(ADDED.toString()); + + // Nothing was registered, nothing was deregistered, and the rollback put the trees back. + assertThat(rootContainer.getBaseDNs()).contains(REMOVED).doesNotContain(ADDED); + assertThat(backend.getBaseDNs()).contains(REMOVED).doesNotContain(ADDED); + assertThat(serverContext.getBackendConfigManager().getLocalBackendWithBaseDN(REMOVED)).isSameAs(backend); + assertThat(serverContext.getBackendConfigManager().getLocalBackendWithBaseDN(ADDED)).isNull(); + assertThat(rootContainer.getStorage().listTrees()).containsAll(removedTrees); + } + finally + { + backend.finalizeBackend(); + } + } + + private static Set treesOf(EntryContainer ec) + { + final Set names = new HashSet<>(); + for (Tree tree : ec.listTrees()) + { + names.add(tree.getName()); + } + return names; + } + + /** Reads back the flags an index was given when it was opened, as they are stored. */ + private static EnumSet persistedFlags(RootContainer rootContainer, EntryContainer ec, TreeName index) + throws Exception + { + final State state = new State(new TreeName(ec.getTreePrefix(), STATE_INDEX_NAME)); + return rootContainer.getStorage().read(txn -> state.getIndexFlags(txn, index)); + } + + private ReplayingBackend openBackend(SortedSet baseDNs) throws Exception + { + final ReplayingBackend backend = new ReplayingBackend(); + backend.setBackendID(BACKEND_ID); + backend.configuredWith = backendCfg(baseDNs); + backend.configureBackend(backend.configuredWith, serverContext); + // Start from a pristine on-disk state so that a previous run cannot mask the defect. + backend.storage.removeStorageFiles(); + backend.openBackend(); + return backend; + } + + private PDBBackendCfg backendCfg(SortedSet baseDNs) throws ConfigException + { + final PDBBackendCfg cfg = mockCfg(PDBBackendCfg.class); + when(cfg.dn()).thenReturn(DN.valueOf("ds-cfg-backend-id=" + BACKEND_ID + ",cn=Backends,cn=config")); + when(cfg.getBackendId()).thenReturn(BACKEND_ID); + when(cfg.getDBDirectory()).thenReturn(BACKEND_ID); + when(cfg.getDBDirectoryPermissions()).thenReturn("755"); + when(cfg.getDBCacheSize()).thenReturn(0L); + 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); + return cfg; + } + + /** A backend whose storage makes the next write operation conflict, and so be replayed. */ + private static final class ReplayingBackend extends BackendImpl + { + private ReplayingStorage storage; + /** The configuration the entry containers register their listeners with. */ + private PDBBackendCfg configuredWith; + + @Override + protected Storage configureStorage(PDBBackendCfg cfg, ServerContext serverContext) throws ConfigException + { + storage = new ReplayingStorage(new PDBStorage(cfg, serverContext)); + return storage; + } + } + + /** A failure which no storage engine replays, unlike {@link RollbackException}. */ + private static final class UnreplayableFailure extends Exception + { + private static final long serialVersionUID = 1L; + } + + /** + * Decorates a {@link Storage} so that the next {@link Storage#write(WriteOperation)} conflicts a + * given number of times before it is let through. The conflict is raised from within the single + * {@code write} the delegate is asked for, so the delegate's own retry loop performs the replay. + */ + private static final class ReplayingStorage implements Storage + { + /** Where the conflict is raised, which decides how much of the operation has run. */ + private enum ConflictPoint + { + /** As soon as the operation first touches the transaction, before it has changed anything. */ + FIRST_STORAGE_ACCESS, + /** 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. */ + NO_REPLAY + } + + private final Storage delegate; + private ConflictPoint conflictPoint; + private int conflictsLeft; + private int attempts; + + ReplayingStorage(Storage delegate) + { + this.delegate = delegate; + } + + void conflictAtFirstStorageAccess(int conflicts) + { + arm(ConflictPoint.FIRST_STORAGE_ACCESS, conflicts); + } + + void conflictAtCommit(int conflicts) + { + arm(ConflictPoint.COMMIT, conflicts); + } + + void failWithoutReplay() + { + arm(ConflictPoint.NO_REPLAY, 1); + } + + private void arm(ConflictPoint where, int conflicts) + { + conflictPoint = where; + conflictsLeft = conflicts; + attempts = 0; + } + + /** How many times the armed operation was run, the first attempt included. */ + int attempts() + { + return attempts; + } + + @Override + public void write(final WriteOperation writeOperation) throws Exception + { + final ConflictPoint armed = conflictPoint; + if (armed == null) + { + delegate.write(writeOperation); + return; + } + conflictPoint = null; + // A single call, so that the replay is the delegate's own and keeps whatever the delegate + // holds for the duration of a write, rather than starting afresh as a second call would. + delegate.write(new WriteOperation() + { + @Override + public void run(WriteableTransaction txn) throws Exception + { + attempts++; + if (conflictsLeft-- <= 0) + { + writeOperation.run(txn); + return; + } + if (armed == ConflictPoint.FIRST_STORAGE_ACCESS) + { + writeOperation.run(new ConflictingTransaction()); + return; + } + writeOperation.run(txn); + if (armed == ConflictPoint.NO_REPLAY) + { + throw new UnreplayableFailure(); + } + throw new RollbackException(); + } + }); + } + + @Override + public Importer startImport() throws ConfigException + { + return delegate.startImport(); + } + + @Override + public void open(AccessMode accessMode) throws Exception + { + delegate.open(accessMode); + } + + @Override + public T read(ReadOperation readOperation) throws Exception + { + return delegate.read(readOperation); + } + + @Override + public void removeStorageFiles() + { + delegate.removeStorageFiles(); + } + + @Override + public StorageStatus getStorageStatus() + { + return delegate.getStorageStatus(); + } + + @Override + public boolean supportsBackupAndRestore() + { + return delegate.supportsBackupAndRestore(); + } + + @Override + public void createBackup(BackupConfig backupConfig) throws DirectoryException + { + delegate.createBackup(backupConfig); + } + + @Override + public void removeBackup(BackupDirectory backupDirectory, String backupID) throws DirectoryException + { + delegate.removeBackup(backupDirectory, backupID); + } + + @Override + public void restoreBackup(RestoreConfig restoreConfig) throws DirectoryException + { + delegate.restoreBackup(restoreConfig); + } + + @Override + public Set listTrees() + { + return delegate.listTrees(); + } + + @Override + public void close() + { + delegate.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() + { + return new RollbackException(); + } + + @Override + public void openTree(TreeName name, boolean createOnDemand) + { + throw conflict(); + } + + @Override + public void deleteTree(TreeName name) + { + throw conflict(); + } + + @Override + public void put(TreeName treeName, ByteSequence key, ByteSequence value) + { + throw conflict(); + } + + @Override + public boolean update(TreeName treeName, ByteSequence key, UpdateFunction f) + { + throw conflict(); + } + + @Override + public boolean delete(TreeName treeName, ByteSequence key) + { + throw conflict(); + } + + @Override + public ByteString read(TreeName treeName, ByteSequence key) + { + throw conflict(); + } + + @Override + public Cursor openCursor(TreeName treeName) + { + throw conflict(); + } + + @Override + public long getRecordCount(TreeName treeName) + { + throw conflict(); + } + + @Override + public boolean treeExists(TreeName treeName) + { + throw conflict(); + } + } +} From 452a69ea42ac6987b0d4e05c7e39b328582a2975 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Fri, 4 Sep 2026 09:39:42 +0300 Subject: [PATCH 2/3] [#907] Answer the review of the base DN change ordering Keep the write confined to what a rollback undoes, and make what happens outside it survive its own failures. - Open the base DNs being added before deleting the ones being removed, so that the failure this operation is most likely to meet is reached while everything is still there to roll back to. - When the write fails, ask the storage which of the removed containers actually lost their trees and give up exactly those. An engine which rolls a tree deletion back leaves the backend as it was; one which does not - cassandra, and the jdbc backend on mysql and oracle - would otherwise leave a base DN routed here with nothing behind it. - Close an entry container whose registration failed only when the root container did not take it, since nothing else can reclaim one it did. - Deregister, unregister and close a removed base DN together, rather than closing it in a finally which runs when the registry still routes to it. - Derive baseDNs from what the root container ended up holding, on the way out of every path, instead of from the configuration that was asked for. - Report the failures through backend.properties rather than a raw English string, name the base DN each one is about, and set adminActionRequired where a restart really is the remedy. - Skip the locks and the transaction altogether when no base DN moves. - Read rootContainer once, and say in EntryContainer.delete's javadoc what its contract actually is. --- .../backends/pluggable/BackendImpl.java | 182 +++++++++++++----- .../backends/pluggable/EntryContainer.java | 7 +- .../org/opends/messages/backend.properties | 6 + .../pluggable/ReplayedConfigChangeTest.java | 103 +++++++++- 4 files changed, 247 insertions(+), 51 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/BackendImpl.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/BackendImpl.java index dcacaec521..55db441cfa 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/BackendImpl.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/BackendImpl.java @@ -27,9 +27,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashSet; -import java.util.LinkedHashMap; import java.util.List; -import java.util.Map; import java.util.Set; import java.util.SortedSet; import java.util.concurrent.ExecutionException; @@ -54,9 +52,11 @@ import org.opends.server.backends.pluggable.spi.Storage; import org.opends.server.backends.pluggable.spi.StorageInUseException; import org.opends.server.backends.pluggable.spi.StorageRuntimeException; +import org.opends.server.backends.pluggable.spi.TreeName; 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.BackendConfigManager; import org.opends.server.core.DeleteOperation; import org.opends.server.core.DirectoryServer; import org.opends.server.core.ModifyDNOperation; @@ -866,7 +866,10 @@ public boolean isConfigurationChangeAcceptable(PluggableBackendCfg cfg, List currentBaseDNs = new HashSet<>(rootContainer.getBaseDNs()); + final Set currentBaseDNs = new HashSet<>(rc.getBaseDNs()); final List deleted = new ArrayList<>(); for (DN baseDN : currentBaseDNs) { if (!newBaseDNs.contains(baseDN)) { - deleted.add(rootContainer.getEntryContainer(baseDN)); + deleted.add(rc.getEntryContainer(baseDN)); } } final List added = new ArrayList<>(); @@ -893,14 +896,24 @@ public ConfigChangeResult applyConfigurationChange(final PluggableBackendCfg new added.add(baseDN); } } + if (deleted.isEmpty() && added.isEmpty()) + { + // The common case - index-entry-limit, db-cache-percent, preload-time-limit and the rest, + // which the entry containers apply through their own listeners. There is no storage work to + // do, so no transaction is opened to commit nothing. + baseDNs = new HashSet<>(newBaseDNs); + cfg = newCfg; + return ccr; + } // Opened by the write operation, registered only once it has committed. - final Map created = new LinkedHashMap<>(); + final List created = new ArrayList<>(); // The trees of a removed base DN are now deleted while it is still registered, so hold its // entry container exclusively for as long as the write runs, retries included, as - // RootContainer.close() does. That keeps out the operations which arrive during that window; an - // operation which had taken hold of the container before the lock still ends up in a closed - // one once it is released, as it did before this ordering. + // RootContainer.close(), EntryContainer's index delete listener and AttributeIndex all do. + // That keeps out the operations which arrive during that window; an operation which had taken + // hold of the container before the lock still ends up in a closed one once it is released, as + // it did before this ordering. final List locked = new ArrayList<>(deleted.size()); try { @@ -912,55 +925,61 @@ public ConfigChangeResult applyConfigurationChange(final PluggableBackendCfg new try { - rootContainer.getStorage().write(new WriteOperation() + rc.getStorage().write(new WriteOperation() { @Override public void run(WriteableTransaction txn) throws Exception { // Give up what a previous, rolled back attempt had opened: its trees are gone, and its // entry containers still hold the configuration listeners they registered. - closeSilently(created.values()); + closeSilently(created); created.clear(); - for (EntryContainer ec : deleted) + // Opening the added base DNs comes first, so that the failure this operation is most + // likely to meet is met while everything is still there to roll back to. Once a tree + // has been deleted, a storage engine which does not undo that has nothing to give + // back. + for (DN baseDN : added) { - ec.delete(txn); + created.add(rc.openEntryContainer(baseDN, txn, AccessMode.READ_WRITE)); } - for (DN baseDN : added) + for (EntryContainer ec : deleted) { - created.put(baseDN, rootContainer.openEntryContainer(baseDN, txn, AccessMode.READ_WRITE)); + ec.delete(txn); } } }); } catch (Exception e) { - closeSilently(created.values()); + logger.traceException(e); + + closeSilently(created); ccr.setResultCode(serverContext.getCoreConfigManager().getServerErrorResultCode()); - // Neither registry was touched, and on a storage engine whose deleteTree the rollback - // undoes with the rest - persistit, je, and the jdbc backend on postgresql and sql server - - // nothing at all has been applied. Where the DDL commits of its own accord (mysql, oracle) - // or where there is no transaction to roll back (cassandra), the trees of a base DN being - // removed may be gone already, and only a restart, which reopens the backend from the - // configuration that has been stored by now, puts that right. Either way the failure alone - // never says which base DNs the change was about, so name them. - ccr.addMessage(LocalizableMessage.raw( - "Backend %s could not change its base DNs (to remove: %s, to add: %s): %s", + // On a storage engine whose deleteTree the rollback undoes with the rest - persistit, je, + // and the jdbc backend on postgresql and sql server - nothing at all has been applied and + // neither registry is touched below. The failure alone never says which base DNs the + // change was about, so name them. + ccr.addMessage(ERR_BACKEND_CANNOT_CHANGE_BASEDNS.get( getBackendID(), baseDNsOf(deleted), added, stackTraceToSingleLineString(e))); + deregisterBaseDNsWhoseTreesAreGone(rc, deleted, ccr); return ccr; } // The change is durable from here on, so every base DN is seen through even if one fails. - deregisterDeletedBaseDNs(deleted, ccr); - registerNewBaseDNs(created, ccr); - - baseDNs = new HashSet<>(newBaseDNs); + deregisterDeletedBaseDNs(rc, deleted, ccr); + registerNewBaseDNs(rc, created, ccr); // Put the new configuration in place. cfg = newCfg; } finally { + // What the root container ended up holding, not what was asked for: a base DN whose + // registration failed is not one this backend serves, and getBaseDNs() is what the monitors, + // isIndexed() and closeBackend() are answered from. Taken on the way out of every path, the + // failed ones included, so that the two never disagree. + baseDNs = new HashSet<>(rc.getBaseDNs()); for (EntryContainer ec : locked) { ec.unlock(); @@ -969,39 +988,102 @@ public void run(WriteableTransaction txn) throws Exception return ccr; } - private void deregisterDeletedBaseDNs(List deleted, ConfigChangeResult ccr) + /** + * Gives up the base DNs whose trees the failed write took with it, which is what a storage engine + * that commits its DDL of its own accord (mysql, oracle) or has no transaction to roll back + * (cassandra) leaves behind. A base DN kept registered without its trees answers every operation + * with a storage error, where its removal was meant to leave a plain "no such entry"; one whose + * trees the rollback put back is left exactly as it was. + */ + private void deregisterBaseDNsWhoseTreesAreGone(RootContainer rc, List deleted, + ConfigChangeResult ccr) { + if (deleted.isEmpty()) + { + return; + } + final Set storedTrees; + try + { + storedTrees = rc.getStorage().listTrees(); + } + catch (Exception e) + { + // Nothing can be said about what survived, so nothing is given up on the strength of it. + logger.traceException(e); + ccr.setAdminActionRequired(true); + return; + } for (EntryContainer ec : deleted) { - final DN baseDN = ec.getBaseDN(); - try + if (!allTreesStored(ec, storedTrees)) { - serverContext.getBackendConfigManager().deregisterBaseDN(baseDN); + ccr.setAdminActionRequired(true); + deregisterDeletedBaseDN(rc, ec, ccr); } - catch (Exception e) - { - logger.traceException(e); + } + } - ccr.setResultCode(serverContext.getCoreConfigManager().getServerErrorResultCode()); - ccr.addMessage(LocalizableMessage.raw(stackTraceToSingleLineString(e))); + private static boolean allTreesStored(EntryContainer ec, Set storedTrees) + { + for (Tree tree : ec.listTrees()) + { + if (!storedTrees.contains(tree.getName())) + { + return false; } - finally + } + return true; + } + + private void deregisterDeletedBaseDNs(RootContainer rc, List deleted, ConfigChangeResult ccr) + { + for (EntryContainer ec : deleted) + { + deregisterDeletedBaseDN(rc, ec, ccr); + } + } + + private void deregisterDeletedBaseDN(RootContainer rc, EntryContainer ec, ConfigChangeResult ccr) + { + final DN baseDN = ec.getBaseDN(); + final BackendConfigManager backendConfigManager = serverContext.getBackendConfigManager(); + try + { + backendConfigManager.deregisterBaseDN(baseDN); + } + catch (Exception e) + { + logger.traceException(e); + + if (backendConfigManager.getLocalBackendWithBaseDN(baseDN) == this) { - // Its trees have been deleted, so it must stop being reachable whatever the registry said. - rootContainer.unregisterEntryContainer(baseDN); - closeSilently(ec); + // deregisterBaseDN puts its new registry in place only once it has succeeded, so this base + // DN is still routed here. Leave the entry container registered: closeBackend() reclaims a + // base DN through rootContainer.getBaseDNs(), and one taken out of there would stay claimed + // by a backend which no longer holds it until the server is restarted. + ccr.setResultCode(serverContext.getCoreConfigManager().getServerErrorResultCode()); + ccr.setAdminActionRequired(true); + ccr.addMessage(ERR_BACKEND_CANNOT_DEREGISTER_BASEDN.get(baseDN, stackTraceToSingleLineString(e))); + return; } + // It is not registered here, which is what an earlier change whose registerBaseDN failed + // leaves behind. Nothing routes to it, so there is nothing to hold on to. } + rc.unregisterEntryContainer(baseDN); + closeSilently(ec); } - private void registerNewBaseDNs(Map created, ConfigChangeResult ccr) + private void registerNewBaseDNs(RootContainer rc, List created, ConfigChangeResult ccr) { - for (Map.Entry entry : created.entrySet()) + for (EntryContainer ec : created) { - final DN baseDN = entry.getKey(); + final DN baseDN = ec.getBaseDN(); + boolean registered = false; try { - rootContainer.registerEntryContainer(baseDN, entry.getValue()); + rc.registerEntryContainer(baseDN, ec); + registered = true; serverContext.getBackendConfigManager().registerBaseDN(baseDN, this, false); } catch (Exception e) @@ -1009,7 +1091,15 @@ private void registerNewBaseDNs(Map created, ConfigChangeRes logger.traceException(e); ccr.setResultCode(serverContext.getCoreConfigManager().getServerErrorResultCode()); + ccr.setAdminActionRequired(true); ccr.addMessage(ERR_BACKEND_CANNOT_REGISTER_BASEDN.get(baseDN, e)); + if (!registered) + { + // Nothing else can reclaim it: closeBackend() and RootContainer.close() both work from + // the registered containers, and this one keeps the configuration listeners its + // constructor registered for as long as it is alive. + closeSilently(ec); + } } } } diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java index e9188abdd4..b40d8e8f43 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java @@ -2401,8 +2401,11 @@ private static boolean isManageDsaITOperation(Operation operation) } /** - * Delete this entry container from disk. The entry container should be - * closed before calling this method. + * Deletes this entry container from disk, that is, every tree {@link #listTrees()} enumerates. + * The entry container may be open or closed: the trees are taken from the attribute and VLV index + * maps, which {@link #close()} closes the indexes of but leaves populated, so the same set is + * deleted either way. A {@code close()} which cleared those maps would turn a call made after it + * into a partial deletion, silently. Either way the container is not to be used afterwards. * * @param txn a non null transaction * @throws StorageRuntimeException If an error occurs while removing the entry container. diff --git a/opendj-server-legacy/src/messages/org/opends/messages/backend.properties b/opendj-server-legacy/src/messages/org/opends/messages/backend.properties index eebfe842ef..74d5c55211 100644 --- a/opendj-server-legacy/src/messages/org/opends/messages/backend.properties +++ b/opendj-server-legacy/src/messages/org/opends/messages/backend.properties @@ -1114,3 +1114,9 @@ NOTE_COMPSCHEMA_MIGRATED_618=Migrated %d compressed schema definitions of backen ERR_COMPSCHEMA_CANNOT_MIGRATE_619=The compressed schema definitions of backend '%s' could not be migrated from \ the shared tree '%s' to '%s': %s. The backend cannot be opened, because its entries were encoded against the \ definitions that were not migrated and would decode as the wrong attributes +ERR_BACKEND_CANNOT_DEREGISTER_BASEDN_620=An error occurred while attempting to deregister base DN %s \ + from the Directory Server: %s +ERR_BACKEND_CANNOT_CHANGE_BASEDNS_621=The base DNs of backend %s could not be changed (to remove: %s, \ + to add: %s): %s. A storage engine which rolls a tree deletion back leaves the backend as it was; on one \ + which does not, the base DNs whose trees are gone have been given up, and the backend has to be restarted \ + for what it holds to match the configuration which has been stored 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 8d698b3196..c997cb3d78 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 @@ -9,9 +9,9 @@ * When distributing Covered Software, include this CDDL Header Notice in each file and include * the License file at legal/CDDLv1.0.txt. If applicable, add the following below the CDDL * Header, with the fields enclosed by brackets [] replaced by your own identifying - * information: "Portions Copyright [year] [name of copyright owner]". + * information: "Portions copyright [year] [name of copyright owner]". * - * Portions Copyright 2026 3A Systems, LLC. + * Copyright 2026 3A Systems, LLC. */ package org.opends.server.backends.pluggable; @@ -254,6 +254,70 @@ public void aFailureWhichIsNotReplayedAppliesNothingAndNamesTheBaseDNs() throws } } + /** + * A failure which the storage engine neither replays nor rolls back - the DDL of mysql and oracle + * commits of its own accord, and cassandra has no transaction at all - leaves the trees of a + * removed base DN gone. That base DN has to stop being reachable, or every operation against it + * meets a storage error rather than the "no such entry" its removal was meant to leave. + */ + @Test + public void aFailureWhichIsNotRolledBackGivesUpTheBaseDNsWhoseTreesAreGone() throws Exception + { + final ReplayingBackend backend = openBackend(newTreeSet(KEPT, REMOVED)); + try + { + final RootContainer rootContainer = backend.getRootContainer(); + final Set removedTrees = treesOf(rootContainer.getEntryContainer(REMOVED)); + + backend.storage.failAfterCommit(); + final ConfigChangeResult ccr = backend.applyConfigurationChange(backendCfg(newTreeSet(KEPT, ADDED))); + + assertThat(ccr.getResultCode()).isNotEqualTo(ResultCode.SUCCESS); + assertThat(ccr.adminActionRequired()).isTrue(); + assertThat(ccr.getMessages().toString()).contains(REMOVED.toString()).contains(ADDED.toString()); + + // The trees are gone, so the base DN is given up rather than left routed at them. + assertThat(rootContainer.getStorage().listTrees()).doesNotContainAnyElementsOf(removedTrees); + assertThat(rootContainer.getBaseDNs()).doesNotContain(REMOVED); + assertThat(backend.getBaseDNs()).doesNotContain(REMOVED); + assertThat(serverContext.getBackendConfigManager().getLocalBackendWithBaseDN(REMOVED)).isNull(); + + // The added base DN is not registered, since the change it belongs to failed. + assertThat(rootContainer.getBaseDNs()).doesNotContain(ADDED); + assertThat(backend.getBaseDNs()).doesNotContain(ADDED); + assertThat(serverContext.getBackendConfigManager().getLocalBackendWithBaseDN(ADDED)).isNull(); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * A configuration change which leaves the base DNs alone - every change to index-entry-limit, + * db-cache-percent and the rest - has no storage work to do, so it opens no transaction to + * commit nothing. + */ + @Test + public void aChangeWhichLeavesTheBaseDNsAloneOpensNoTransaction() throws Exception + { + final ReplayingBackend backend = openBackend(newTreeSet(KEPT, REMOVED)); + try + { + final int writesBefore = backend.storage.writes(); + final ConfigChangeResult ccr = backend.applyConfigurationChange(backendCfg(newTreeSet(KEPT, REMOVED))); + + assertThat(ccr.getResultCode()).isEqualTo(ResultCode.SUCCESS); + assertThat(ccr.getMessages()).isEmpty(); + assertThat(backend.storage.writes()).isEqualTo(writesBefore); + assertThat(backend.getBaseDNs()).contains(KEPT, REMOVED); + } + finally + { + backend.finalizeBackend(); + } + } + private static Set treesOf(EntryContainer ec) { final Set names = new HashSet<>(); @@ -342,13 +406,19 @@ private enum ConflictPoint /** 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. */ - NO_REPLAY + NO_REPLAY, + /** + * Once the operation has committed, as a failure which is not replayed either: what an engine + * whose tree deletions do not belong to the transaction leaves behind. + */ + NO_REPLAY_AFTER_COMMIT } private final Storage delegate; private ConflictPoint conflictPoint; private int conflictsLeft; private int attempts; + private int writes; ReplayingStorage(Storage delegate) { @@ -370,6 +440,11 @@ void failWithoutReplay() arm(ConflictPoint.NO_REPLAY, 1); } + void failAfterCommit() + { + arm(ConflictPoint.NO_REPLAY_AFTER_COMMIT, 1); + } + private void arm(ConflictPoint where, int conflicts) { conflictPoint = where; @@ -383,9 +458,16 @@ int attempts() return attempts; } + /** How many write operations this storage was asked for, armed or not. */ + int writes() + { + return writes; + } + @Override public void write(final WriteOperation writeOperation) throws Exception { + writes++; final ConflictPoint armed = conflictPoint; if (armed == null) { @@ -393,6 +475,21 @@ public void write(final WriteOperation writeOperation) throws Exception return; } conflictPoint = null; + if (armed == ConflictPoint.NO_REPLAY_AFTER_COMMIT) + { + // Committed, then reported as a failure: the operation's work outlives the failure, as it + // does where the storage engine does not roll a tree deletion back. + delegate.write(new WriteOperation() + { + @Override + public void run(WriteableTransaction txn) throws Exception + { + attempts++; + writeOperation.run(txn); + } + }); + throw new UnreplayableFailure(); + } // A single call, so that the replay is the delegate's own and keeps whatever the delegate // holds for the duration of a write, rather than starting afresh as a second call would. delegate.write(new WriteOperation() From cc756f7ab6346cbbf56c0424bfcf6896969ad9c6 Mon Sep 17 00:00:00 2001 From: maximthomas Date: Tue, 8 Sep 2026 09:18:02 +0300 Subject: [PATCH 3/3] [#907] Hold no entry container lock into the base DN registry The write which deletes and creates the trees of a base DN change now lives in changeBaseDNTrees, which takes the entry container locks and releases them in its own finally. BackendConfigManager guards its registry with a single lock the server already takes in the opposite order - shutdownLocalBackends, a backend being disabled and applyConfigurationDelete all hold it while finalizing a backend, which closes its root container and locks every entry container in turn - so holding a container lock into deregisterBaseDN deadlocked a base DN change against a shutdown, with no timeout on either side. The failure path reads the trees which survived and gives up a removed base DN whose trees are gone, rather than leaving it routed at trees which are not there. The trees created for a base DN which is not being added after all are left where they are: ConfigurationHandler.replaceEntry stores the entry before it notifies its change listeners and the failure does not take that back, so the next open of this backend opens that base DN from the stored configuration, adopting the trees which survived and creating the ones which did not. A base DN the root container no longer holds is answered exactly rather than with an ancestor's entry container - deleting the trees of one would take a base DN this backend still serves with it - and the change fails with a result rather than a NullPointerException. Messages 621 and 622 say what a failure left behind, 623 names a base DN which is no longer held, and a registration which fails is reported with the frames it was raised on. Claude-Session: https://claude.ai/code/session_01JDd647nAzukS35JDiycbqM --- .../backends/pluggable/BackendImpl.java | 214 ++++++++---- .../org/opends/messages/backend.properties | 18 +- .../pluggable/ReplayedConfigChangeTest.java | 321 +++++++++++++++++- 3 files changed, 473 insertions(+), 80 deletions(-) diff --git a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/BackendImpl.java b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/BackendImpl.java index 55db441cfa..d8374192e0 100644 --- a/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/BackendImpl.java +++ b/opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/BackendImpl.java @@ -885,7 +885,24 @@ public ConfigChangeResult applyConfigurationChange(final PluggableBackendCfg new { if (!newBaseDNs.contains(baseDN)) { - deleted.add(rc.getEntryContainer(baseDN)); + final EntryContainer ec = rc.getEntryContainer(baseDN); + // Answered exactly, never with an ancestor's container: getEntryContainer walks up the DN + // until it finds one, which is how an entry is routed to the base DN above it, and one + // backend holds hierarchically related base DNs whenever a registry which refused one left + // its container behind. Deleting the trees an ancestor answered with is deleting the trees + // of a base DN this backend is still serving. + if (ec == null || !baseDN.equals(ec.getBaseDN())) + { + // Unregistered since the copy above was taken, which is what closing the root container + // leaves behind: importLDIF, rebuildBackend and exportLDIF all do that, as does a backend + // being disabled. There is nothing to delete and nothing to say about the rest of the + // change, so none of it is attempted - and a result is returned rather than an exception, + // which is what the administration framework is owed whatever happens. + ccr.setResultCode(serverContext.getCoreConfigManager().getServerErrorResultCode()); + ccr.addMessage(ERR_BACKEND_BASEDN_NO_LONGER_HELD.get(getBackendID(), baseDN)); + return ccr; + } + deleted.add(ec); } } final List added = new ArrayList<>(); @@ -907,67 +924,41 @@ public ConfigChangeResult applyConfigurationChange(final PluggableBackendCfg new } // Opened by the write operation, registered only once it has committed. final List created = new ArrayList<>(); - - // The trees of a removed base DN are now deleted while it is still registered, so hold its - // entry container exclusively for as long as the write runs, retries included, as - // RootContainer.close(), EntryContainer's index delete listener and AttributeIndex all do. - // That keeps out the operations which arrive during that window; an operation which had taken - // hold of the container before the lock still ends up in a closed one once it is released, as - // it did before this ordering. - final List locked = new ArrayList<>(deleted.size()); try { - for (EntryContainer ec : deleted) - { - ec.lock(); - locked.add(ec); - } - try { - rc.getStorage().write(new WriteOperation() - { - @Override - public void run(WriteableTransaction txn) throws Exception - { - // Give up what a previous, rolled back attempt had opened: its trees are gone, and its - // entry containers still hold the configuration listeners they registered. - closeSilently(created); - created.clear(); - - // Opening the added base DNs comes first, so that the failure this operation is most - // likely to meet is met while everything is still there to roll back to. Once a tree - // has been deleted, a storage engine which does not undo that has nothing to give - // back. - for (DN baseDN : added) - { - created.add(rc.openEntryContainer(baseDN, txn, AccessMode.READ_WRITE)); - } - for (EntryContainer ec : deleted) - { - ec.delete(txn); - } - } - }); + changeBaseDNTrees(rc, deleted, added, created); } catch (Exception e) { logger.traceException(e); - closeSilently(created); ccr.setResultCode(serverContext.getCoreConfigManager().getServerErrorResultCode()); - // On a storage engine whose deleteTree the rollback undoes with the rest - persistit, je, - // and the jdbc backend on postgresql and sql server - nothing at all has been applied and - // neither registry is touched below. The failure alone never says which base DNs the - // change was about, so name them. + // The failure alone never says which base DNs the change was about, so name them. + // + // Only persistit and je roll the whole write back, leaving nothing at all applied and + // neither registry to touch. The jdbc backend does not, on any of its engines: its + // commitStatement() issues the statement and commits it, and commitsBeforeDdl() decides + // only which side of the statement the attempt stops being replayable on, never whether a + // completed "create table" or "drop table" survives the rollback of the write around it. + // Neither does cassandra, which has no transaction to roll back. + // giveUpBaseDNsWhoseTreesAreGone below reads what is actually left rather than trusting + // either answer. ccr.addMessage(ERR_BACKEND_CANNOT_CHANGE_BASEDNS.get( getBackendID(), baseDNsOf(deleted), added, stackTraceToSingleLineString(e))); - deregisterBaseDNsWhoseTreesAreGone(rc, deleted, ccr); + // Read before the entry containers are closed: what a container holds is what says which + // trees belong to it. + giveUpBaseDNsWhoseTreesAreGone(rc, deleted, ccr); + closeSilently(created); return ccr; } // The change is durable from here on, so every base DN is seen through even if one fails. - deregisterDeletedBaseDNs(rc, deleted, ccr); + for (EntryContainer ec : deleted) + { + deregisterDeletedBaseDN(rc, ec, ccr); + } registerNewBaseDNs(rc, created, ccr); // Put the new configuration in place. @@ -977,26 +968,101 @@ public void run(WriteableTransaction txn) throws Exception { // What the root container ended up holding, not what was asked for: a base DN whose // registration failed is not one this backend serves, and getBaseDNs() is what the monitors, - // isIndexed() and closeBackend() are answered from. Taken on the way out of every path, the - // failed ones included, so that the two never disagree. + // isIndexed() and closeBackend() are answered from. Taken on the way out of every path which + // reached the write, the failed ones included, so that the two never disagree. The change + // which had no storage work to do sets it from the new configuration above; the one which + // found a base DN this backend no longer holds leaves it alone, since the root container it + // would be read from is being closed underneath it. baseDNs = new HashSet<>(rc.getBaseDNs()); - for (EntryContainer ec : locked) + } + return ccr; + } + + /** + * Deletes the trees of the base DNs being removed and opens the ones being added, as the single + * write operation a storage engine may replay. + *

+ * The trees of a removed base DN are deleted while it is still registered, so its entry container + * is held exclusively for as long as the write runs, retries included, as + * {@link RootContainer#close()}, EntryContainer's index delete listener and AttributeIndex all do. + * That keeps out the operations which arrive during that window; an operation which had taken hold + * of the container before the lock still ends up in a closed one once it is released, as it did + * before this ordering. + *

+ * The locks are given up with the write and are never held into the registry work which follows it. + * {@link BackendConfigManager} guards its registry with a single lock which the server already + * takes in the opposite order - {@code shutdownLocalBackends}, a backend being disabled and + * {@code applyConfigurationDelete} all hold it while finalizing a backend, which closes its root + * container and locks every entry container in turn. Holding the container lock into + * {@code deregisterBaseDN} would deadlock a base DN change against a shutdown, with no timeout on + * either side. + */ + private void changeBaseDNTrees(final RootContainer rc, final List deleted, + final List added, final List created) throws Exception + { + for (EntryContainer ec : deleted) + { + // Taken outside the try, because EntryContainer.lock() has no throwing path - the write side + // of a ReentrantReadWriteLock, then a drain which swallows the interrupt - so every one of + // them is held by the time it is entered, and unlocking what was never locked cannot happen. + ec.lock(); + } + try + { + rc.getStorage().write(new WriteOperation() + { + @Override + public void run(WriteableTransaction txn) throws Exception + { + // Give up what a previous, rolled back attempt had opened: its trees are gone, and its + // entry containers still hold the configuration listeners they registered. + closeSilently(created); + created.clear(); + + // Opening the added base DNs comes first, so that the failure this operation is most + // likely to meet is met while everything is still there to roll back to. Once a tree + // has been deleted, a storage engine which does not undo that has nothing to give + // back. + for (DN baseDN : added) + { + created.add(rc.openEntryContainer(baseDN, txn, AccessMode.READ_WRITE)); + } + for (EntryContainer ec : deleted) + { + ec.delete(txn); + } + } + }); + } + finally + { + for (EntryContainer ec : deleted) { ec.unlock(); } } - return ccr; } /** - * Gives up the base DNs whose trees the failed write took with it, which is what a storage engine - * that commits its DDL of its own accord (mysql, oracle) or has no transaction to roll back - * (cassandra) leaves behind. A base DN kept registered without its trees answers every operation - * with a storage error, where its removal was meant to leave a plain "no such entry"; one whose - * trees the rollback put back is left exactly as it was. + * Gives up the base DNs whose trees the failed write took with it. There is anything to give up + * only on an engine which does not roll the write back whole: mysql and oracle commit their DDL of + * their own accord, cassandra has no transaction at all, and the jdbc backend commits after each + * statement on every engine it supports. A base DN kept registered without its trees answers every + * operation with a storage error, where its removal was meant to leave a plain "no such entry"; + * one whose trees the rollback put back is left exactly as it was. + *

+ * The trees the same write created for a base DN which is not being added after all are left where + * they are, and this is the only place which could have taken them back. The configuration naming + * that base DN was stored before this listener was called - {@code + * ConfigurationHandler.replaceEntry} writes the entry, and only then notifies its change listeners + * - and the failure does not take it back, so {@link RootContainer#open} opens that base DN again + * from it the next time this backend is opened, adopting the trees which survived and creating the + * ones which did not. Deleting them here would take away the trees of a base DN the stored + * configuration still asks this backend to serve, and would reach only the base DNs whose opening + * succeeded anyway: one which failed while being opened never became an entry container, and + * nothing but its own trees names them. */ - private void deregisterBaseDNsWhoseTreesAreGone(RootContainer rc, List deleted, - ConfigChangeResult ccr) + private void giveUpBaseDNsWhoseTreesAreGone(RootContainer rc, List deleted, ConfigChangeResult ccr) { if (deleted.isEmpty()) { @@ -1009,14 +1075,18 @@ private void deregisterBaseDNsWhoseTreesAreGone(RootContainer rc, List storedTrees) + private static Set treeNamesOf(EntryContainer ec) { + final Set names = new HashSet<>(); for (Tree tree : ec.listTrees()) { - if (!storedTrees.contains(tree.getName())) - { - return false; - } - } - return true; - } - - private void deregisterDeletedBaseDNs(RootContainer rc, List deleted, ConfigChangeResult ccr) - { - for (EntryContainer ec : deleted) - { - deregisterDeletedBaseDN(rc, ec, ccr); + names.add(tree.getName()); } + return names; } private void deregisterDeletedBaseDN(RootContainer rc, EntryContainer ec, ConfigChangeResult ccr) @@ -1061,7 +1121,11 @@ private void deregisterDeletedBaseDN(RootContainer rc, EntryContainer ec, Config // deregisterBaseDN puts its new registry in place only once it has succeeded, so this base // DN is still routed here. Leave the entry container registered: closeBackend() reclaims a // base DN through rootContainer.getBaseDNs(), and one taken out of there would stay claimed - // by a backend which no longer holds it until the server is restarted. + // by a backend which no longer holds it until the server is restarted. That is the opposite + // of what deregisterBaseDNsWhoseTreesAreGone does, and for the opposite reason: there the + // registry has already stopped routing to the base DN, so keeping the container only leaves + // a storage error where a "no such entry" was meant to be, while here the registry is still + // routing to it and dropping the container is what would leave that error behind. ccr.setResultCode(serverContext.getCoreConfigManager().getServerErrorResultCode()); ccr.setAdminActionRequired(true); ccr.addMessage(ERR_BACKEND_CANNOT_DEREGISTER_BASEDN.get(baseDN, stackTraceToSingleLineString(e))); @@ -1092,7 +1156,7 @@ private void registerNewBaseDNs(RootContainer rc, List created, ccr.setResultCode(serverContext.getCoreConfigManager().getServerErrorResultCode()); ccr.setAdminActionRequired(true); - ccr.addMessage(ERR_BACKEND_CANNOT_REGISTER_BASEDN.get(baseDN, e)); + ccr.addMessage(ERR_BACKEND_CANNOT_REGISTER_BASEDN.get(baseDN, stackTraceToSingleLineString(e))); if (!registered) { // Nothing else can reclaim it: closeBackend() and RootContainer.close() both work from diff --git a/opendj-server-legacy/src/messages/org/opends/messages/backend.properties b/opendj-server-legacy/src/messages/org/opends/messages/backend.properties index 74d5c55211..dbf5c55880 100644 --- a/opendj-server-legacy/src/messages/org/opends/messages/backend.properties +++ b/opendj-server-legacy/src/messages/org/opends/messages/backend.properties @@ -1117,6 +1117,18 @@ ERR_COMPSCHEMA_CANNOT_MIGRATE_619=The compressed schema definitions of backend ' ERR_BACKEND_CANNOT_DEREGISTER_BASEDN_620=An error occurred while attempting to deregister base DN %s \ from the Directory Server: %s ERR_BACKEND_CANNOT_CHANGE_BASEDNS_621=The base DNs of backend %s could not be changed (to remove: %s, \ - to add: %s): %s. A storage engine which rolls a tree deletion back leaves the backend as it was; on one \ - which does not, the base DNs whose trees are gone have been given up, and the backend has to be restarted \ - for what it holds to match the configuration which has been stored + to add: %s): %s. A storage engine which rolls the whole write back leaves the backend exactly as it \ + was; on one which does not, the base DNs whose trees are gone have been given up. The base DNs being \ + added are not being served, but the configuration which has been stored still names them, so the next \ + time this backend is opened it opens them from that configuration, keeping whatever trees the failed \ + change created for them and creating the ones it did not. A base DN being removed of which only some \ + trees survived is given up with those trees still in the storage, where nothing names them afterwards; \ + removing them means re-creating the backend +ERR_BACKEND_CANNOT_LIST_TREES_AFTER_BASEDN_CHANGE_622=The base DN change of backend %s failed, and the \ + trees which survived it could not be listed: %s. No base DN has been given up on the strength of that, \ + so this backend may still be serving one whose trees are gone, which answers every operation against it \ + with a storage error +ERR_BACKEND_BASEDN_NO_LONGER_HELD_623=The base DNs of backend %s could not be changed: base DN %s is no \ + longer one this backend holds, which is what closing its root container leaves behind - an LDIF import, \ + an index rebuild, an LDIF export and the backend being disabled all do that. Nothing has been changed; \ + submit the change again once that has finished 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 c997cb3d78..19c1aaae26 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 @@ -16,6 +16,9 @@ package org.opends.server.backends.pluggable; import static org.assertj.core.api.Assertions.assertThat; +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.forgerock.opendj.config.ConfigurationMock.mockCfg; import static org.mockito.Mockito.any; import static org.mockito.Mockito.mock; @@ -25,12 +28,16 @@ import static org.opends.server.backends.pluggable.State.IndexFlag.TRUSTED; import static org.opends.server.backends.pluggable.SuffixContainer.STATE_INDEX_NAME; import static org.opends.server.util.CollectionUtils.newTreeSet; +import static org.forgerock.util.Utils.closeSilently; import java.util.EnumSet; import java.util.HashSet; import java.util.Set; import java.util.SortedSet; +import java.util.TreeSet; +import java.util.concurrent.locks.ReentrantReadWriteLock; +import org.forgerock.i18n.LocalizableMessage; import org.forgerock.opendj.config.server.ConfigChangeResult; import org.forgerock.opendj.config.server.ConfigException; import org.forgerock.opendj.ldap.ByteSequence; @@ -50,6 +57,7 @@ import org.opends.server.backends.pluggable.spi.Importer; import org.opends.server.backends.pluggable.spi.ReadOperation; import org.opends.server.backends.pluggable.spi.Storage; +import org.opends.server.backends.pluggable.spi.StorageRuntimeException; import org.opends.server.backends.pluggable.spi.StorageStatus; import org.opends.server.backends.pluggable.spi.TreeName; import org.opends.server.backends.pluggable.spi.UpdateFunction; @@ -85,6 +93,8 @@ public class ReplayedConfigChangeTest extends DirectoryServerTestCase private static final DN KEPT = DN.valueOf("dc=b907a,dc=com"); private static final DN REMOVED = DN.valueOf("dc=b907b,dc=com"); 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"); private ServerContext serverContext; private AttributeType cnType; @@ -104,7 +114,7 @@ public void startServer() throws Exception @AfterMethod public void deregisterLeftoverBaseDNs() { - for (DN baseDN : new DN[] { KEPT, REMOVED, ADDED }) + for (DN baseDN : new DN[] { KEPT, REMOVED, ADDED, UNREGISTRABLE }) { try { @@ -293,6 +303,223 @@ public void aFailureWhichIsNotRolledBackGivesUpTheBaseDNsWhoseTreesAreGone() thr } } + /** + * The same failure leaves the trees it created for a base DN which is not being added after all + * exactly where they are. The configuration which names that base DN was stored before this + * listener was called - {@code ConfigurationHandler.replaceEntry} writes the entry, and only then + * notifies - and the failure does not take it back, so the next open of this backend opens that + * base DN again from it, adopting the trees which survived and creating the ones which did not. + * Deleting them here would take away the trees of a base DN the stored configuration still asks + * this backend to serve, and would buy nothing: that open re-creates them empty. + */ + @Test + public void aFailureWhichIsNotRolledBackLeavesTheTreesItCreated() throws Exception + { + final ReplayingBackend backend = openBackend(newTreeSet(KEPT, REMOVED)); + try + { + final RootContainer rootContainer = backend.getRootContainer(); + final Set storedBefore = new HashSet<>(rootContainer.getStorage().listTrees()); + + backend.storage.failAfterCommit(); + final ConfigChangeResult ccr = backend.applyConfigurationChange(backendCfg(newTreeSet(KEPT, ADDED))); + + assertThat(ccr.getResultCode()).isNotEqualTo(ResultCode.SUCCESS); + assertThat(rootContainer.getBaseDNs()).doesNotContain(ADDED); + // Named by the failure, since nothing in the running server names them any more. + assertThat(ccr.getMessages().toString()).contains(ADDED.toString()); + + final Set left = new HashSet<>(rootContainer.getStorage().listTrees()); + left.removeAll(storedBefore); + assertThat(left).as("the trees created for the base DN the stored configuration still names") + .isNotEmpty(); + for (TreeName tree : left) + { + assertThat(tree.getBaseDN()).isEqualTo(ADDED.toNormalizedUrlSafeString()); + } + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * Whether anything survived a failure is read from the trees the storage still holds, so a backend + * which cannot be asked for them reconciles nothing at all. The operator has to be told that, + * since it is the case where the failure alone says least about what the backend is left serving. + */ + @Test + public void aFailureWhoseSurvivingTreesCannotBeListedSaysSo() throws Exception + { + final ReplayingBackend backend = openBackend(newTreeSet(KEPT, REMOVED)); + try + { + backend.storage.onListTrees(new Runnable() + { + @Override + public void run() + { + throw new StorageRuntimeException("the trees cannot be listed"); + } + }); + + backend.storage.failAfterCommit(); + final ConfigChangeResult ccr = backend.applyConfigurationChange(backendCfg(newTreeSet(KEPT))); + backend.storage.onListTrees(null); + + assertThat(ccr.getResultCode()).isNotEqualTo(ResultCode.SUCCESS); + assertThat(ccr.adminActionRequired()).isTrue(); + assertThat(ordinalsOf(ccr)).contains(ERR_BACKEND_CANNOT_LIST_TREES_AFTER_BASEDN_CHANGE.ordinal()); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * A base DN this backend no longer holds must fail the change rather than the method: an entry + * container unregistered while the change was working out what to do leaves the root container + * with nothing to answer for that base DN, and the administration framework is owed a result + * whatever happens. + */ + @Test + public void aBaseDNTheBackendNoLongerHoldsFailsTheChangeRatherThanTheMethod() throws Exception + { + final ReplayingBackend backend = openBackend(newTreeSet(KEPT, REMOVED)); + try + { + final RootContainer rootContainer = backend.getRootContainer(); + final PDBBackendCfg newCfg = backendCfg(newTreeSet(KEPT)); + when(newCfg.getBaseDN()).thenReturn(new UnregisteringWhenAsked(rootContainer, REMOVED, newTreeSet(KEPT))); + + final ConfigChangeResult ccr = backend.applyConfigurationChange(newCfg); + + assertThat(ccr.getResultCode()).isNotEqualTo(ResultCode.SUCCESS); + assertThat(ordinalsOf(ccr)).contains(ERR_BACKEND_BASEDN_NO_LONGER_HELD.ordinal()); + assertThat(ccr.getMessages().toString()).contains(REMOVED.toString()); + // Nothing was applied, so the base DNs this backend serves are the ones it served before. + assertThat(backend.getBaseDNs()).contains(KEPT); + assertThat(rootContainer.getStorage().listTrees()).containsAll(treesOf(rootContainer.getEntryContainer(KEPT))); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * A base DN whose entry container is gone must not be answered with an ancestor's. + * {@link RootContainer#getEntryContainer} walks up the DN until it finds a container, which is how + * an entry is routed to the base DN above it; asked for a base DN the root container no longer + * holds, it hands back the container of the one it does. Deleting the trees of that container is + * deleting the trees of a base DN this backend is still serving. + *

+ * Two base DNs of one backend are hierarchically related only after a registration the registry + * refused, which leaves the entry container behind in the root container - see + * {@link #aBaseDNWhichCannotBeRegisteredReportsWhereItFailed}. + */ + @Test + public void anEntryContainerWhichIsGoneIsNotAnsweredWithItsParent() throws Exception + { + final ReplayingBackend backend = openBackend(newTreeSet(KEPT)); + try + { + final RootContainer rootContainer = backend.getRootContainer(); + // Refused by the registry, and so left in the root container underneath KEPT. + backend.applyConfigurationChange(backendCfg(newTreeSet(KEPT, UNREGISTRABLE))); + assertThat(rootContainer.getBaseDNs()).contains(KEPT, UNREGISTRABLE); + final Set keptTrees = treesOf(rootContainer.getEntryContainer(KEPT)); + + final PDBBackendCfg newCfg = backendCfg(newTreeSet(KEPT)); + when(newCfg.getBaseDN()).thenReturn(new UnregisteringWhenAsked(rootContainer, UNREGISTRABLE, newTreeSet(KEPT))); + final ConfigChangeResult ccr = backend.applyConfigurationChange(newCfg); + + // The base DN above the one which was gone is left alone, trees and routing both. + assertThat(rootContainer.getStorage().listTrees()) + .as("the trees of the base DN above the one which was gone").containsAll(keptTrees); + assertThat(rootContainer.getBaseDNs()).contains(KEPT); + assertThat(serverContext.getBackendConfigManager().getLocalBackendWithBaseDN(KEPT)).isSameAs(backend); + // And the change says which base DN stopped it. + assertThat(ccr.getResultCode()).isNotEqualTo(ResultCode.SUCCESS); + assertThat(ordinalsOf(ccr)).contains(ERR_BACKEND_BASEDN_NO_LONGER_HELD.ordinal()); + assertThat(ccr.getMessages().toString()).contains(UNREGISTRABLE.toString()); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * A base DN the registry refuses is reported with the whole of what refused it. The registry + * raises the same message for several reasons and from more than one place, so the exception's own + * text does not say which of them happened; the frames it was raised on do. + */ + @Test + public void aBaseDNWhichCannotBeRegisteredReportsWhereItFailed() throws Exception + { + final ReplayingBackend backend = openBackend(newTreeSet(KEPT)); + try + { + final ConfigChangeResult ccr = backend.applyConfigurationChange(backendCfg(newTreeSet(KEPT, UNREGISTRABLE))); + + assertThat(ccr.getResultCode()).isNotEqualTo(ResultCode.SUCCESS); + assertThat(ordinalsOf(ccr)).contains(ERR_BACKEND_CANNOT_REGISTER_BASEDN.ordinal()); + assertThat(ccr.getMessages().toString()) + .as("the reported cause never says where it was raised") + .contains("BackendConfigManager.java:"); + } + finally + { + backend.finalizeBackend(); + } + } + + /** + * The entry container locks are held for the write which deletes the trees, and no longer: + * everything below the write reaches {@code BackendConfigManager}, whose single registry lock the + * server already takes in the opposite order - {@code shutdownLocalBackends} and a backend being + * disabled both hold it while closing a root container, which locks every entry container in turn. + * Holding both in this order would deadlock a base DN change against a shutdown, with no timeout + * on either side. + */ + @Test + public void theRegistryIsNotTouchedWhileAnEntryContainerLockIsHeld() throws Exception + { + final ReplayingBackend backend = openBackend(newTreeSet(KEPT, REMOVED)); + try + { + final RootContainer rootContainer = backend.getRootContainer(); + final EntryContainer removed = rootContainer.getEntryContainer(REMOVED); + // Listing the surviving trees is the last thing the failure path does before it deregisters, + // so it is asked on the very thread, and at the very moment, the deadlock would be reached. + final boolean[] lockHeld = new boolean[] { false }; + final boolean[] asked = new boolean[] { false }; + backend.storage.onListTrees(new Runnable() + { + @Override + public void run() + { + asked[0] = true; + lockHeld[0] |= ((ReentrantReadWriteLock.WriteLock) removed.exclusiveLock).isHeldByCurrentThread(); + } + }); + + backend.storage.failAfterCommit(); + backend.applyConfigurationChange(backendCfg(newTreeSet(KEPT))); + backend.storage.onListTrees(null); + + assertThat(asked).as("the failure path never listed the surviving trees").containsExactly(true); + assertThat(lockHeld).as("the entry container lock was still held").containsExactly(false); + } + finally + { + backend.finalizeBackend(); + } + } + /** * A configuration change which leaves the base DNs alone - every change to index-entry-limit, * db-cache-percent and the rest - has no storage work to do, so it opens no transaction to @@ -318,6 +545,17 @@ public void aChangeWhichLeavesTheBaseDNsAloneOpensNoTransaction() throws Excepti } } + /** The messages a change result carries, by identity rather than by their formatted text. */ + private static Set ordinalsOf(ConfigChangeResult ccr) + { + final Set ordinals = new HashSet<>(); + for (LocalizableMessage message : ccr.getMessages()) + { + ordinals.add(message.ordinal()); + } + return ordinals; + } + private static Set treesOf(EntryContainer ec) { final Set names = new HashSet<>(); @@ -344,7 +582,37 @@ private ReplayingBackend openBackend(SortedSet baseDNs) throws Exception backend.configureBackend(backend.configuredWith, serverContext); // Start from a pristine on-disk state so that a previous run cannot mask the defect. backend.storage.removeStorageFiles(); - backend.openBackend(); + try + { + backend.openBackend(); + } + catch (Exception e) + { + // openBackend() opens the root container before it preloads, counts the entries, registers + // the base DNs and registers the monitor, so a failure in any of those leaves the volume open + // and the monitor registered. Every following test would then fail in openBackend() too, and + // the one which actually broke would be lost among them. + try + { + if (backend.getRootContainer() != null) + { + backend.finalizeBackend(); + } + else + { + backend.storage.close(); + } + } + catch (Exception cleanupFailure) + { + // openBackend() registers the root container monitor last of all, and closeBackend() + // deregisters it without a null check, so cleaning up after a failure before that throws a + // NullPointerException of its own. The failure being cleaned up after is the one worth + // reading. + e.addSuppressed(cleanupFailure); + } + throw e; + } return backend; } @@ -385,6 +653,44 @@ protected Storage configureStorage(PDBBackendCfg cfg, ServerContext serverContex } } + /** + * The base DNs a change asks for, which unregisters an entry container the first time it is asked + * whether it holds one. {@link BackendImpl#applyConfigurationChange} copies the base DNs the root + * container holds and then looks each of their entry containers up, and asks this set in between; + * an importLDIF, a rebuildBackend, an exportLDIF or the backend being disabled closes the root + * container in that window and unregisters every one of them. Done here rather than raced for, so + * that the window is closed on the same thread every time. + */ + private static final class UnregisteringWhenAsked extends TreeSet + { + private static final long serialVersionUID = 1L; + + private final transient RootContainer rootContainer; + private final DN toUnregister; + private boolean unregistered; + + UnregisteringWhenAsked(RootContainer rootContainer, DN toUnregister, SortedSet baseDNs) + { + super(baseDNs); + this.rootContainer = rootContainer; + this.toUnregister = toUnregister; + } + + @Override + public boolean contains(Object baseDN) + { + if (!unregistered) + { + unregistered = true; + // Closed here because nothing else will: the root container closes the containers it + // holds, and this one has just been taken out of it, with its configuration listeners + // still registered. + closeSilently(rootContainer.unregisterEntryContainer(toUnregister)); + } + return super.contains(baseDN); + } + } + /** A failure which no storage engine replays, unlike {@link RollbackException}. */ private static final class UnreplayableFailure extends Exception { @@ -415,6 +721,7 @@ private enum ConflictPoint } private final Storage delegate; + private Runnable onListTrees; private ConflictPoint conflictPoint; private int conflictsLeft; private int attempts; @@ -572,9 +879,19 @@ public void restoreBackup(RestoreConfig restoreConfig) throws DirectoryException delegate.restoreBackup(restoreConfig); } + /** Run whenever the trees which survived a failure are listed, and only then. */ + void onListTrees(Runnable probe) + { + this.onListTrees = probe; + } + @Override public Set listTrees() { + if (onListTrees != null) + { + onListTrees.run(); + } return delegate.listTrees(); }