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

maximthomas

21 hours ago 452a69ea42ac6987b0d4e05c7e39b328582a2975

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