SOLR-18391: Clean up a collection when creation fails partway, and name the cause in the error

changelog/unreleased/SOLR-18391-collection-creation-failure-cleanup.yml

@@ -0,0 +1,8 @@
+# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
+title: A collection creation that fails part way no longer leaves a half-created collection behind, and the error returned to the client names the cause; a create whose final alias write fails now removes the completed collection instead of leaving it behind
+type: fixed
+authors:
+  - name: Nick Shanin
+links:
+  - name: SOLR-18391
+    url: https://issues.apache.org/jira/browse/SOLR-18391

solr/core/src/java/org/apache/solr/cloud/api/collections/CreateCollectionCmd.java

@@ -21,7 +21,6 @@
 import static org.apache.solr.common.params.CollectionAdminParams.COLL_CONF;
 import static org.apache.solr.common.params.CollectionParams.CollectionAction.ADDREPLICA;
 import static org.apache.solr.common.params.CollectionParams.CollectionAction.CREATE;
-import static org.apache.solr.common.params.CollectionParams.CollectionAction.DELETE;
 import static org.apache.solr.common.params.CommonAdminParams.ASYNC;
 import static org.apache.solr.common.params.CommonAdminParams.WAIT_FOR_FINAL_STATE;
 import static org.apache.solr.common.params.CommonParams.NAME;
@@ -80,6 +79,7 @@
 import org.apache.solr.common.params.ModifiableSolrParams;
 import org.apache.solr.common.util.EnvUtils;
 import org.apache.solr.common.util.NamedList;
+import org.apache.solr.common.util.RetryUtil;
 import org.apache.solr.common.util.SimpleOrderedMap;
 import org.apache.solr.common.util.Utils;
 import org.apache.solr.core.ConfigSetService;
@@ -98,6 +98,11 @@ public class CreateCollectionCmd implements CollApiCmds.CollectionApiCommand {
 
   public static final String PRS_DEFAULT_PROP = "solr.cloud.prs.enabled";
 
+  // The alias write at the end of a create is retried a few times on a ZooKeeper error: the
+  // collection itself is complete by then, and a transient error should not fail the create.
+  private static final int ALIAS_CREATION_ATTEMPTS = 3;
+  private static final long ALIAS_CREATION_RETRY_PAUSE_MS = 200;
+
   public CreateCollectionCmd(CollectionCommandContext ccc) {
     this.ccc = ccc;
   }
@@ -152,6 +157,9 @@ public void call(AdminCmdContext adminCmdContext, ZkNodeProps message, NamedList
 
     DocCollection newColl = null;
     final String collectionPath = DocCollection.getCollectionPath(collectionName);
+    // True once this command may have written the collection state. Any later failure then deletes
+    // the collection again, whatever the failure is, so that a failed create leaves nothing behind.
+    boolean stateWritten = false;
 
     try {
       ZkStateReader zkStateReader = ccc.getZkStateReader();
@@ -201,6 +209,7 @@ public void call(AdminCmdContext adminCmdContext, ZkNodeProps message, NamedList
             new ClusterStateMutator(ccc.getSolrCloudManager()).createCollection(clusterState, m);
         byte[] data = Utils.toJSON(Map.of(collectionName, command.collection));
         ccc.getZkStateReader().getZkClient().create(collectionPath, data, CreateMode.PERSISTENT);
+        stateWritten = true;
         clusterState = clusterState.copyWith(collectionName, command.collection);
         newColl = command.collection;
         ccc.submitIntraProcessMessage(new RefreshCollectionMessage(collectionName));
@@ -216,6 +225,17 @@ public void call(AdminCmdContext adminCmdContext, ZkNodeProps message, NamedList
               e);
         }
       } else {
+        // The check at the top of this command reads this node's cluster state view, which can
+        // lag behind ZooKeeper. The state update below creates the collection's state.json, so
+        // if one is already in ZooKeeper it belongs to an existing collection, and a failed
+        // create must not delete a collection this command did not create. This command holds
+        // the collection lock, so state found here is not its own.
+        if (zkStateReader.getZkClient().exists(collectionPath)) {
+          throw new SolrException(
+              SolrException.ErrorCode.BAD_REQUEST, "collection already exists: " + collectionName);
+        }
+        // set before submitting: a submission that fails may still have been applied
+        stateWritten = true;
         if (ccc.getDistributedClusterStateUpdater().isDistributedStateUpdate()) {
           // The message has been crafted by CollectionsHandler.CollectionOperation.CREATE_OP and
           // defines the QUEUE_OPERATION to be CollectionParams.CollectionAction.CREATE.
@@ -243,28 +263,22 @@ public void call(AdminCmdContext adminCmdContext, ZkNodeProps message, NamedList
         // refresh cluster state (value read below comes from Zookeeper watch firing following the
         // update done previously, be it by Overseer or by this thread when updates are distributed)
         clusterState = ccc.getSolrCloudManager().getClusterState();
+        // The wait above saw the collection through a watch. This is a second, separate read, and
+        // while the watch is being released the collection can be absent from it for a moment.
+        // Replica assignment reads this cluster state, so make sure it holds what the wait saw.
+        if (!clusterState.hasCollection(collectionName)) {
+          clusterState = clusterState.copyWith(collectionName, newColl);
+        }
       }
 
-      final List<ReplicaPosition> replicaPositions;
-      try {
-        replicaPositions =
-            buildReplicaPositions(
-                ccc.getCoreContainer(),
-                ccc.getSolrCloudManager(),
-                clusterState,
-                message,
-                shardNames,
-                numReplicas);
-      } catch (Assign.AssignmentException e) {
-        ZkNodeProps deleteMessage = new ZkNodeProps("name", collectionName);
-        new DeleteCollectionCmd(ccc)
-            .call(
-                adminCmdContext.subRequestContext(DELETE).withClusterState(clusterState),
-                deleteMessage,
-                results);
-        // unwrap the exception
-        throw new SolrException(ErrorCode.BAD_REQUEST, e.getMessage(), e.getCause());
-      }
+      final List<ReplicaPosition> replicaPositions =
+          buildReplicaPositions(
+              ccc.getCoreContainer(),
+              ccc.getSolrCloudManager(),
+              clusterState,
+              message,
+              shardNames,
+              numReplicas);
 
       if (replicaPositions.isEmpty()) {
         log.debug("Finished create command for collection: {}", collectionName);
@@ -428,6 +442,12 @@ public void call(AdminCmdContext adminCmdContext, ZkNodeProps message, NamedList
       boolean failure =
           results.get("failure") != null
               && ((SimpleOrderedMap<?>) results.get("failure")).size() > 0;
+      String failureDetail = null;
+      if (failure) {
+        // Name the first failure only. The map holds one entry per failed core, and dumping all
+        // of them puts every node's error text, URLs and paths included, in the client message.
+        failureDetail = String.valueOf(((SimpleOrderedMap<?>) results.get("failure")).getVal(0));
+      }
       if (isPRS) {
         TimeOut timeout =
             new TimeOut(
@@ -446,18 +466,20 @@ public void call(AdminCmdContext adminCmdContext, ZkNodeProps message, NamedList
           // we have successfully found all replicas to be ACTIVE
         } else {
           failure = true;
+          if (failureDetail == null) {
+            failureDetail = "not all replicas became active";
+          }
         }
       }
       if (failure) {
-        // Let's cleanup as we hit an exception
-        // We shouldn't be passing 'results' here for the cleanup as the response would then contain
-        // 'success' element, which may be interpreted by the user as a positive ack
-        CollectionHandlingUtils.cleanupCollection(
-            adminCmdContext, collectionName, new NamedList<>(), ccc);
-        log.info("Cleaned up artifacts for failed create collection for [{}]", collectionName);
+        // the collection is cleaned up where this exception is caught, below
         throw new SolrException(
             ErrorCode.BAD_REQUEST,
-            "Underlying core creation failed while creating collection: " + collectionName);
+            "Underlying core creation failed while creating collection: "
+                + collectionName
+                + " ("
+                + failureDetail
+                + ")");
       } else {
         ccc.submitIntraProcessMessage(new RefreshCollectionMessage(collectionName));
         log.debug("Finished create command on all shards for collection: {}", collectionName);
@@ -480,15 +502,94 @@ public void call(AdminCmdContext adminCmdContext, ZkNodeProps message, NamedList
 
       // create an alias pointing to the new collection, if different from the collectionName
       if (!alias.equals(collectionName)) {
-        ccc.getZkStateReader()
-            .aliasesManager
-            .applyModificationAndExportToZk(a -> a.cloneWithCollectionAlias(alias, collectionName));
+        runWithBoundedRetries(
+            () ->
+                ccc.getZkStateReader()
+                    .aliasesManager
+                    .applyModificationAndExportToZk(
+                        a -> a.cloneWithCollectionAlias(alias, collectionName)),
+            ALIAS_CREATION_ATTEMPTS,
+            ALIAS_CREATION_RETRY_PAUSE_MS);
       }
 
-    } catch (SolrException ex) {
-      throw ex;
     } catch (Exception ex) {
-      throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, null, ex);
+      if (ex instanceof InterruptedException) {
+        Thread.currentThread().interrupt();
+      }
+      // An interrupted thread cannot complete the ZooKeeper calls the cleanup needs
+      if (stateWritten && !Thread.currentThread().isInterrupted()) {
+        cleanupFailedCreate(adminCmdContext, collectionName, ex);
+      }
+      if (ex instanceof SolrException solrException) {
+        throw solrException;
+      }
+      if (ex instanceof Assign.AssignmentException) {
+        // unwrap the exception
+        throw new SolrException(ErrorCode.BAD_REQUEST, ex.getMessage(), ex.getCause());
+      }
+      throw new SolrException(
+          ErrorCode.SERVER_ERROR, "Could not create collection " + collectionName + ": " + ex, ex);
+    }
+  }
+
+  /**
+   * Runs {@code op}, retrying it if it fails with a {@link ZooKeeperException}, for up to {@code
+   * maxAttempts} attempts in total and pausing {@code pauseMillis} between attempts. Any other
+   * failure propagates at once, and so does a ZooKeeper failure on the last attempt. An interrupt
+   * is never retried: the interrupt flag is restored and the {@link InterruptedException}
+   * propagates.
+   */
+  static void runWithBoundedRetries(RetryUtil.RetryCmd op, int maxAttempts, long pauseMillis)
+      throws Exception {
+    int attempt = 0;
+    while (true) {
+      attempt++;
+      try {
+        op.execute();
+        return;
+      } catch (InterruptedException e) {
+        Thread.currentThread().interrupt();
+        throw e;
+      } catch (ZooKeeperException e) {
+        if (attempt >= maxAttempts) {
+          throw e;
+        }
+        log.warn(
+            "Attempt {} of {} failed with a ZooKeeper error; retrying after a pause",
+            attempt,
+            maxAttempts,
+            e);
+        try {
+          Thread.sleep(pauseMillis);
+        } catch (InterruptedException interruptedDuringPause) {
+          Thread.currentThread().interrupt();
+          throw interruptedDuringPause;
+        }
+      }
+    }
+  }
+
+  /**
+   * Deletes what a failed create left behind. A failure of the cleanup itself is logged and added
+   * to {@code cause} as suppressed, so that the client still gets the original error.
+   */
+  private void cleanupFailedCreate(
+      AdminCmdContext adminCmdContext, String collectionName, Exception cause) {
+    try {
+      // We shouldn't be passing 'results' here for the cleanup as the response would then contain
+      // 'success' element, which may be interpreted by the user as a positive ack
+      CollectionHandlingUtils.cleanupCollection(
+          adminCmdContext, collectionName, new NamedList<>(), ccc);
+      log.info("Cleaned up artifacts for failed create collection for [{}]", collectionName);
+    } catch (Exception cleanupFailure) {
+      if (cleanupFailure instanceof InterruptedException) {
+        Thread.currentThread().interrupt();
+      }
+      log.error(
+          "Could not clean up after failed create collection for [{}]",
+          collectionName,
+          cleanupFailure);
+      cause.addSuppressed(cleanupFailure);
     }
   }
 

solr/core/src/java/org/apache/solr/cluster/placement/impl/PlacementPluginAssignStrategy.java

@@ -27,6 +27,7 @@
 import org.apache.solr.cloud.api.collections.Assign;
 import org.apache.solr.cluster.Node;
 import org.apache.solr.cluster.Replica.ReplicaType;
+import org.apache.solr.cluster.SolrCollection;
 import org.apache.solr.cluster.placement.BalanceRequest;
 import org.apache.solr.cluster.placement.DeleteCollectionRequest;
 import org.apache.solr.cluster.placement.DeleteReplicasRequest;
@@ -70,7 +71,7 @@ public List<ReplicaPosition> assign(
       placementRequests.add(
           PlacementRequestImpl.toPlacementRequest(
               placementContext.getCluster(),
-              placementContext.getCluster().getCollection(assignRequest.collectionName),
+              getCollection(placementContext, assignRequest.collectionName),
               assignRequest));
     }
 
@@ -171,6 +172,20 @@ public void verifyDeleteReplicas(
     }
   }
 
+  /**
+   * Looks up the collection replicas are requested for. It can be missing from the cluster state
+   * this request sees, for example when it was deleted concurrently.
+   */
+  private static SolrCollection getCollection(
+      PlacementContext placementContext, String collectionName) throws IOException {
+    SolrCollection collection = placementContext.getCluster().getCollection(collectionName);
+    if (collection == null) {
+      throw new Assign.AssignmentException(
+          "Collection " + collectionName + " not found in cluster state; cannot assign replicas");
+    }
+    return collection;
+  }
+
   /** Very minimal placement logic for System collections */
   private static List<ReplicaPosition> computeSystemCollectionPositions(
       PlacementContext placementContext, Assign.AssignRequest assignRequest) throws IOException {
@@ -186,7 +201,7 @@ private static List<ReplicaPosition> computeSystemCollectionPositions(
     }
     PlacementRequestImpl request =
         new PlacementRequestImpl(
-            placementContext.getCluster().getCollection(assignRequest.collectionName),
+            getCollection(placementContext, assignRequest.collectionName),
             new HashSet<>(assignRequest.shardNames),
             nodes,
             assignRequest.numReplicas);