SOLR-18557: Restore source collection when REINDEXCOLLECTION cleanup fails

changelog/unreleased/SOLR-18557-reindex-abort-readonly.yml

@@ -0,0 +1,12 @@
+# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
+
+title: >
+  REINDEXCOLLECTION now restores the source collection when its cleanup can't delete the target or
+  checkpoint collection, instead of leaving the source read-only
+type: fixed
+authors:
+  - name: Boyi Dai
+    nick: SpookyJumpyBeans
+links:
+  - name: SOLR-18557
+    url: https://issues.apache.org/jira/browse/SOLR-18557

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

@@ -900,56 +900,62 @@ private void cleanup(
     }
     ClusterState clusterState = ccc.getSolrCloudManager().getClusterState();
     NamedList<Object> cmdResults = new NamedList<>();
-    if (createdTarget
-        && !collection.equals(targetCollection)
-        && clusterState.hasCollection(targetCollection)) {
-      log.debug(" -- removing {}", targetCollection);
-      ZkNodeProps cmd =
-          new ZkNodeProps(CommonParams.NAME, targetCollection, FOLLOW_ALIASES, "false");
-      new DeleteCollectionCmd(ccc)
-          .call(
-              adminCmdContext
-                  .subRequestContext(CollectionParams.CollectionAction.DELETE, null)
-                  .withClusterState(clusterState),
-              cmd,
-              cmdResults);
-      CollectionHandlingUtils.checkResults(
-          "CLEANUP: deleting target collection " + targetCollection, cmdResults, false);
-    }
-    // remove chk collection
-    if (clusterState.hasCollection(chkCollection)) {
-      log.debug(" -- removing {}", chkCollection);
-      ZkNodeProps cmd = new ZkNodeProps(CommonParams.NAME, chkCollection, FOLLOW_ALIASES, "false");
-      cmdResults = new NamedList<>();
-      new DeleteCollectionCmd(ccc)
-          .call(
-              adminCmdContext
-                  .subRequestContext(CollectionParams.CollectionAction.DELETE, null)
-                  .withClusterState(clusterState),
-              cmd,
-              cmdResults);
-      CollectionHandlingUtils.checkResults(
-          "CLEANUP: deleting checkpoint collection " + chkCollection, cmdResults, false);
-    }
-    log.debug(" -- turning readOnly mode off for {}", collection);
-    ZkNodeProps props =
-        new ZkNodeProps(
-            Overseer.QUEUE_OPERATION,
-            CollectionParams.CollectionAction.MODIFYCOLLECTION.toLower(),
-            ZkStateReader.COLLECTION_PROP,
-            collection,
-            ZkStateReader.READ_ONLY,
-            null);
-    if (ccc.getDistributedClusterStateUpdater().isDistributedStateUpdate()) {
-      ccc.getDistributedClusterStateUpdater()
-          .doSingleStateUpdate(
-              DistributedClusterStateUpdater.MutatingCommand.CollectionModifyCollection,
-              props,
-              ccc.getSolrCloudManager(),
-              ccc.getZkStateReader());
-    } else {
-      ccc.offerStateUpdate(props);
+    try {
+      if (createdTarget
+          && !collection.equals(targetCollection)
+          && clusterState.hasCollection(targetCollection)) {
+        log.debug(" -- removing {}", targetCollection);
+        ZkNodeProps cmd =
+            new ZkNodeProps(CommonParams.NAME, targetCollection, FOLLOW_ALIASES, "false");
+        new DeleteCollectionCmd(ccc)
+            .call(
+                adminCmdContext
+                    .subRequestContext(CollectionParams.CollectionAction.DELETE, null)
+                    .withClusterState(clusterState),
+                cmd,
+                cmdResults);
+        CollectionHandlingUtils.checkResults(
+            "CLEANUP: deleting target collection " + targetCollection, cmdResults, false);
+      }
+      // remove chk collection
+      if (clusterState.hasCollection(chkCollection)) {
+        log.debug(" -- removing {}", chkCollection);
+        ZkNodeProps cmd =
+            new ZkNodeProps(CommonParams.NAME, chkCollection, FOLLOW_ALIASES, "false");
+        cmdResults = new NamedList<>();
+        new DeleteCollectionCmd(ccc)
+            .call(
+                adminCmdContext
+                    .subRequestContext(CollectionParams.CollectionAction.DELETE, null)
+                    .withClusterState(clusterState),
+                cmd,
+                cmdResults);
+        CollectionHandlingUtils.checkResults(
+            "CLEANUP: deleting checkpoint collection " + chkCollection, cmdResults, false);
+      }
+    } finally {
+      // always restore the source collection, even if the temporary collections could not be
+      // removed (e.g. the target is referenced by an alias), otherwise it is left read-only
+      log.debug(" -- turning readOnly mode off for {}", collection);
+      ZkNodeProps props =
+          new ZkNodeProps(
+              Overseer.QUEUE_OPERATION,
+              CollectionParams.CollectionAction.MODIFYCOLLECTION.toLower(),
+              ZkStateReader.COLLECTION_PROP,
+              collection,
+              ZkStateReader.READ_ONLY,
+              null);
+      if (ccc.getDistributedClusterStateUpdater().isDistributedStateUpdate()) {
+        ccc.getDistributedClusterStateUpdater()
+            .doSingleStateUpdate(
+                DistributedClusterStateUpdater.MutatingCommand.CollectionModifyCollection,
+                props,
+                ccc.getSolrCloudManager(),
+                ccc.getZkStateReader());
+      } else {
+        ccc.offerStateUpdate(props);
+      }
+      removeReindexingState(collection);
     }
-    removeReindexingState(collection);
   }
 }

solr/core/src/test/org/apache/solr/cloud/ReindexCollectionTest.java

@@ -444,6 +444,42 @@ public void testAbort() throws Exception {
     assertEquals(status.toString(), "aborted", status.get("state"));
   }
 
+  @Test
+  public void testAbortWithAliasedTarget() throws Exception {
+    final String sourceCollection = "abortAliasedReindexing";
+    final String targetCollection = "abortAliasedReindexingTarget";
+    final String targetAlias = "abortAliasedReindexingAlias";
+    createCollection(sourceCollection, "conf1", 2, 1);
+
+    TestInjection.reindexLatch = new CountDownLatch(1);
+    CollectionAdminRequest.ReindexCollection req =
+        CollectionAdminRequest.reindexCollection(sourceCollection).setTarget(targetCollection);
+    String asyncId = req.processAsync(solrClient);
+    waitForState(
+        "source collection didn't become readOnly", sourceCollection, DocCollection::isReadOnly);
+
+    // an alias on the target makes the cleanup fail to delete it
+    CollectionAdminRequest.createAlias(targetAlias, targetCollection).process(solrClient);
+
+    req = CollectionAdminRequest.reindexCollection(sourceCollection);
+    req.setCommand("abort");
+    req.process(solrClient);
+    waitForReindexingState(sourceCollection, ReindexCollectionCmd.State.ABORTED);
+    TestInjection.reindexLatch.countDown();
+
+    // the source collection must still be restored
+    waitForState(
+        "source collection is in wrong state",
+        sourceCollection,
+        docCollection -> !docCollection.isReadOnly());
+    waitForReindexingState(sourceCollection, null);
+
+    // the failure to delete the aliased target is still reported
+    assertEquals(
+        RequestStatusState.FAILED,
+        CollectionAdminRequest.requestStatus(asyncId).waitFor(solrClient, 30));
+  }
+
   private void createCollection(String name, String config, int numShards, int numReplicas)
       throws Exception {
     CollectionAdminRequest.createCollection(name, config, numShards, numReplicas)