From 2c22d96fcf2a6c9323d331e1197337fec239bc88 Mon Sep 17 00:00:00 2001
From: Maxim Thomas <maxim.thomas@gmail.com>
Date: Wed, 09 Sep 2026 07:21:53 +0000
Subject: [PATCH] [#907] Change the base DNs of a pluggable backend outside the write the storage replays (#914)
---
opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/BackendImpl.java | 352 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
1 files changed, 303 insertions(+), 49 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 03cd930..d837419 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,11 +18,13 @@
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.List;
@@ -50,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;
@@ -845,83 +849,333 @@
return true;
}
+ /**
+ * {@inheritDoc}
+ * <p>
+ * {@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.
+ * <p>
+ * 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();
- try
+ // Read once: importLDIF, rebuildBackend, exportLDIF and verifyBackend all assign this field
+ // and null it out again, and this method now goes on using it past the commit.
+ final RootContainer rc = rootContainer;
+ if (rc == null)
{
- if(rootContainer != null)
+ return ccr;
+ }
+
+ final SortedSet<DN> 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<DN> currentBaseDNs = new HashSet<>(rc.getBaseDNs());
+ final List<EntryContainer> deleted = new ArrayList<>();
+ for (DN baseDN : currentBaseDNs)
+ {
+ if (!newBaseDNs.contains(baseDN))
{
- rootContainer.getStorage().write(new WriteOperation()
+ 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()))
{
- @Override
- public void run(WriteableTransaction txn) throws Exception
- {
- SortedSet<DN> newBaseDNs = newCfg.getBaseDN();
-
- // Check for changes to the base DNs.
- removeDeletedBaseDNs(newBaseDNs, txn);
- if (!createNewBaseDNs(newBaseDNs, ccr, txn))
- {
- return;
- }
-
- baseDNs = new HashSet<>(newBaseDNs);
-
- // Put the new configuration in place.
- cfg = newCfg;
- }
- });
+ // 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);
}
}
- catch (Exception e)
+ final List<DN> added = new ArrayList<>();
+ for (DN baseDN : newBaseDNs)
{
- ccr.setResultCode(serverContext.getCoreConfigManager().getServerErrorResultCode());
- ccr.addMessage(LocalizableMessage.raw(stackTraceToSingleLineString(e)));
+ if (!currentBaseDNs.contains(baseDN))
+ {
+ 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 List<EntryContainer> created = new ArrayList<>();
+ try
+ {
+ try
+ {
+ changeBaseDNTrees(rc, deleted, added, created);
+ }
+ catch (Exception e)
+ {
+ logger.traceException(e);
+
+ ccr.setResultCode(serverContext.getCoreConfigManager().getServerErrorResultCode());
+ // 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)));
+ // 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.
+ for (EntryContainer ec : deleted)
+ {
+ deregisterDeletedBaseDN(rc, ec, 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 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());
}
return ccr;
}
- private void removeDeletedBaseDNs(SortedSet<DN> newBaseDNs, WriteableTransaction txn) throws DirectoryException
+ /**
+ * 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.
+ * <p>
+ * 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.
+ * <p>
+ * 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<EntryContainer> deleted,
+ final List<DN> added, final List<EntryContainer> created) throws Exception
{
- for (DN baseDN : cfg.getBaseDN())
+ for (EntryContainer ec : deleted)
{
- if (!newBaseDNs.contains(baseDN))
+ // 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()
{
- // The base DN was deleted.
- serverContext.getBackendConfigManager().deregisterBaseDN(baseDN);
- EntryContainer ec = rootContainer.unregisterEntryContainer(baseDN);
- ec.close();
- ec.delete(txn);
+ @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();
}
}
}
- private boolean createNewBaseDNs(Set<DN> newBaseDNs, ConfigChangeResult ccr, WriteableTransaction txn)
+ /**
+ * 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.
+ * <p>
+ * 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 giveUpBaseDNsWhoseTreesAreGone(RootContainer rc, List<EntryContainer> deleted, ConfigChangeResult ccr)
{
- for (DN baseDN : newBaseDNs)
+ if (deleted.isEmpty())
{
- if (!rootContainer.getBaseDNs().contains(baseDN))
+ return;
+ }
+ final Set<TreeName> 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 - and
+ // that is the case where the failure itself says least about what this backend is left
+ // serving, so it is said outright rather than left to a bare admin action.
+ logger.traceException(e);
+ ccr.setAdminActionRequired(true);
+ ccr.addMessage(ERR_BACKEND_CANNOT_LIST_TREES_AFTER_BASEDN_CHANGE.get(
+ getBackendID(), stackTraceToSingleLineString(e)));
+ return;
+ }
+ for (EntryContainer ec : deleted)
+ {
+ if (!storedTrees.containsAll(treeNamesOf(ec)))
{
- 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);
+ ccr.setAdminActionRequired(true);
+ deregisterDeletedBaseDN(rc, ec, ccr);
+ }
+ }
+ }
- ccr.setResultCode(serverContext.getCoreConfigManager().getServerErrorResultCode());
- ccr.addMessage(ERR_BACKEND_CANNOT_REGISTER_BASEDN.get(baseDN, e));
- return false;
+ private static Set<TreeName> treeNamesOf(EntryContainer ec)
+ {
+ final Set<TreeName> names = new HashSet<>();
+ for (Tree tree : ec.listTrees())
+ {
+ names.add(tree.getName());
+ }
+ return names;
+ }
+
+ 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)
+ {
+ // 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. 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)));
+ 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(RootContainer rc, List<EntryContainer> created, ConfigChangeResult ccr)
+ {
+ for (EntryContainer ec : created)
+ {
+ final DN baseDN = ec.getBaseDN();
+ boolean registered = false;
+ try
+ {
+ rc.registerEntryContainer(baseDN, ec);
+ registered = true;
+ serverContext.getBackendConfigManager().registerBaseDN(baseDN, this, false);
+ }
+ catch (Exception e)
+ {
+ logger.traceException(e);
+
+ ccr.setResultCode(serverContext.getCoreConfigManager().getServerErrorResultCode());
+ ccr.setAdminActionRequired(true);
+ 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
+ // the registered containers, and this one keeps the configuration listeners its
+ // constructor registered for as long as it is alive.
+ closeSilently(ec);
}
}
}
- return true;
+ }
+
+ private static List<DN> baseDNsOf(List<EntryContainer> entryContainers)
+ {
+ final List<DN> baseDNs = new ArrayList<>(entryContainers.size());
+ for (EntryContainer ec : entryContainers)
+ {
+ baseDNs.add(ec.getBaseDN());
+ }
+ return baseDNs;
}
/**
--
Gitblit v1.10.0