[#907] Answer the review of the base DN change ordering

maximthomas

yesterday 452a69ea42ac6987b0d4e05c7e39b328582a2975

[#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.

 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 ConfigChangeResult applyConfigurationChange(final PluggableBackendCfg newCfg)
  {
    final ConfigChangeResult ccr = new ConfigChangeResult();
    if (rootContainer == null)
    // 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)
    {
      return ccr;
    }
@@ -876,13 +879,13 @@
    // 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<>(rootContainer.getBaseDNs());
    final Set<DN> currentBaseDNs = new HashSet<>(rc.getBaseDNs());
    final List<EntryContainer> deleted = new ArrayList<>();
    for (DN baseDN : currentBaseDNs)
    {
      if (!newBaseDNs.contains(baseDN))
      {
        deleted.add(rootContainer.getEntryContainer(baseDN));
        deleted.add(rc.getEntryContainer(baseDN));
      }
    }
    final List<DN> added = new ArrayList<>();
@@ -893,14 +896,24 @@
        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<DN, EntryContainer> created = new LinkedHashMap<>();
    final List<EntryContainer> 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<EntryContainer> locked = new ArrayList<>(deleted.size());
    try
    {
@@ -912,55 +925,61 @@

      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();

            // 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);
            }
            for (DN baseDN : added)
            {
              created.put(baseDN, rootContainer.openEntryContainer(baseDN, txn, AccessMode.READ_WRITE));
            }
          }
        });
      }
      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 @@
    return ccr;
  }

  private void deregisterDeletedBaseDNs(List<EntryContainer> 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<EntryContainer> deleted,
      ConfigChangeResult ccr)
  {
    if (deleted.isEmpty())
    {
      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.
      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);
      }
      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);
        ccr.setAdminActionRequired(true);
        deregisterDeletedBaseDN(rc, ec, ccr);
      }
    }
  }

  private void registerNewBaseDNs(Map<DN, EntryContainer> created, ConfigChangeResult ccr)
  private static boolean allTreesStored(EntryContainer ec, Set<TreeName> storedTrees)
  {
    for (Map.Entry<DN, EntryContainer> entry : created.entrySet())
    for (Tree tree : ec.listTrees())
    {
      final DN baseDN = entry.getKey();
      if (!storedTrees.contains(tree.getName()))
      {
        return false;
      }
    }
    return true;
  }

  private void deregisterDeletedBaseDNs(RootContainer rc, List<EntryContainer> 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)
      {
        // 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(RootContainer rc, List<EntryContainer> created, ConfigChangeResult ccr)
  {
    for (EntryContainer ec : created)
    {
      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 @@
        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);
        }
      }
    }
  }

 opendj-server-legacy/src/main/java/org/opends/server/backends/pluggable/EntryContainer.java

@@ -2401,8 +2401,11 @@
  }

  /**
   * 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.

 opendj-server-legacy/src/messages/org/opends/messages/backend.properties

@@ -1114,3 +1114,9 @@
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

 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 @@
    }
  }

  /**
   * 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<TreeName> 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<TreeName> treesOf(EntryContainer ec)
  {
    final Set<TreeName> names = new HashSet<>();
@@ -342,13 +406,19 @@
      /** 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 @@
      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 @@
      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 @@
        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()