SOLR-18589: Lock the resolved collection when followAliases=true

changelog/unreleased/solr-18589-alias-lock-key.yml

@@ -0,0 +1,7 @@
+title: "Collection API commands with followAliases=true now lock the collection the alias points to, not the alias name."
+type: fixed
+authors:
+  - name: Amrit Sarkar
+links:
+  - name: SOLR-18589
+    url: https://issues.apache.org/jira/browse/SOLR-18589

solr/core/src/java/org/apache/solr/cloud/OverseerTaskProcessor.java

@@ -37,6 +37,7 @@
 import java.util.function.Predicate;
 import org.apache.solr.cloud.Overseer.LeaderStatus;
 import org.apache.solr.cloud.OverseerTaskQueue.QueueEvent;
+import org.apache.solr.cloud.api.collections.CollectionHandlingUtils;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.cloud.SolrZkClient;
 import org.apache.solr.common.cloud.ZkNodeProps;
@@ -345,7 +346,7 @@ public void run() {
               log.error(
                   "Error occurred while trying to acquire lock for task [{}]", head.getId(), e);
               NamedList<Object> errResp = new NamedList<>();
-              errResp.add("exception", e.getMessage());
+              CollectionHandlingUtils.addExceptionToNamedList(operation, e, errResp);
               OverseerSolrResponse response = new OverseerSolrResponse(errResp);
               if (asyncId != null) {
                 failureMap.put(asyncId, OverseerSolrResponseSerializer.serialize(response));

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

@@ -49,6 +49,7 @@
 import org.apache.solr.cloud.overseer.ClusterStateMutator;
 import org.apache.solr.cloud.overseer.OverseerAction;
 import org.apache.solr.common.SolrException;
+import org.apache.solr.common.cloud.Aliases;
 import org.apache.solr.common.cloud.ClusterState;
 import org.apache.solr.common.cloud.DocCollection;
 import org.apache.solr.common.cloud.DocCollection.CollectionStateProps;
@@ -109,6 +110,16 @@ public class CollectionHandlingUtils {
   public static final Map<String, Object> COLLECTION_PROPS_AND_DEFAULTS =
       Collections.unmodifiableMap(makeCollectionPropsAndDefaults());
 
+  /**
+   * The collection a command locks: {@code collName}, resolved through {@code aliases} when the
+   * message sets {@code followAliases}, so the lock matches the collection the command acts on.
+   */
+  static String lockCollectionName(ZkNodeProps message, String collName, Aliases aliases) {
+    return message.getBool(CollectionAdminParams.FOLLOW_ALIASES, false)
+        ? aliases.resolveSimpleAlias(collName)
+        : collName;
+  }
+
   private static Map<String, Object> makeCollectionPropsAndDefaults() {
     Map<String, Object> propsAndDefaults =
         Utils.makeMap(

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

@@ -393,6 +393,9 @@ public OverseerSolrResponse call() {
 
       NamedList<Object> results = new NamedList<>();
       try {
+        String lockCollName =
+            CollectionHandlingUtils.lockCollectionName(
+                message, collName, ccc.getZkStateReader().getAliases());
         // Create API lock for executing the command. This call is non blocking (not blocked on
         // waiting for a lock to be acquired anyway, might be blocked on access to ZK etc) We create
         // a new CollectionApiLockFactory using a new ZkDistributedCollectionLockFactory because
@@ -407,7 +410,7 @@ public OverseerSolrResponse call() {
                 ccc.getZkStateReader().getZkClient(), ZK_COLLECTION_LOCKS);
         DistributedMultiLock lock =
             new CollectionApiLockFactory(lockFactory)
-                .createCollectionApiLock(adminCmdContext, collName, shardId, replicaName);
+                .createCollectionApiLock(adminCmdContext, lockCollName, shardId, replicaName);
 
         try {
           if (log.isDebugEnabled()) {
@@ -461,7 +464,7 @@ public OverseerSolrResponse call() {
             }
           }
           if (adminCmdContext.getAction() == CollectionAction.DELETE) {
-            deleteEmptyLockDirs(lockFactory, collName);
+            deleteEmptyLockDirs(lockFactory, lockCollName);
           }
         }
       } catch (Exception e) {

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

@@ -198,7 +198,8 @@ public Lock lockTask(ZkNodeProps message, long batchSessionId, String callingLoc
     return lockSession.lock(
         getCollectionAction(message.getStr(Overseer.QUEUE_OPERATION)),
         Arrays.asList(
-            getTaskKey(message),
+            CollectionHandlingUtils.lockCollectionName(
+                message, getTaskKey(message), zkStateReader.getAliases()),
             message.getStr(ZkStateReader.SHARD_ID_PROP),
             message.getStr(ZkStateReader.REPLICA_PROP)),
         callingLockId);

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

@@ -64,6 +64,7 @@
 import org.apache.solr.cluster.placement.PlacementPluginFactory;
 import org.apache.solr.cluster.placement.plugins.SimplePlacementFactory;
 import org.apache.solr.common.MapWriter;
+import org.apache.solr.common.SolrException;
 import org.apache.solr.common.cloud.Aliases;
 import org.apache.solr.common.cloud.ClusterState;
 import org.apache.solr.common.cloud.DocCollection;
@@ -1593,4 +1594,79 @@ protected LeaderStatus amILeader() {
       }
     };
   }
+
+  @Test
+  public void testLockTaskResolvesFollowedAlias() throws Exception {
+    commonMocks(2, false);
+    when(zkStateReaderMock.getAliases())
+        .thenReturn(Aliases.EMPTY.cloneWithCollectionAlias("aliasA", "collA"));
+
+    try (OverseerCollectionMessageHandler collHandler =
+        new OverseerCollectionMessageHandler(
+            zkStateReaderMock,
+            "1234",
+            shardHandlerFactoryMock,
+            ADMIN_PATH,
+            new Stats(),
+            overseerMock,
+            new OverseerNodePrioritizer(
+                zkStateReaderMock, overseerMock, ADMIN_PATH, shardHandlerFactoryMock))) {
+      OverseerMessageHandler.Lock collALock =
+          collHandler.lockTask(
+              new ZkNodeProps(
+                  Overseer.QUEUE_OPERATION,
+                  CollectionParams.CollectionAction.MOCK_COLL_TASK.toLower(),
+                  "name",
+                  "collA"),
+              1,
+              null);
+      assertNotNull(collALock);
+
+      OverseerMessageHandler.Lock aliasLock =
+          collHandler.lockTask(
+              new ZkNodeProps(
+                  Overseer.QUEUE_OPERATION,
+                  CollectionParams.CollectionAction.MOCK_SHARD_TASK.toLower(),
+                  ZkStateReader.COLLECTION_PROP,
+                  "aliasA",
+                  ZkStateReader.SHARD_ID_PROP,
+                  "shard1",
+                  CollectionAdminParams.FOLLOW_ALIASES,
+                  "true"),
+              1,
+              null);
+      assertNull("aliasA resolves to collA, which is locked", aliasLock);
+      collALock.unlock();
+    }
+  }
+
+  @Test
+  public void testLockTaskMultiCollectionAliasReturnsBadRequest() throws Exception {
+    commonMocks(2, false);
+    when(zkStateReaderMock.getAliases())
+        .thenReturn(Aliases.EMPTY.cloneWithCollectionAlias("aliasAB", "collA,collB"));
+    createAndStartProcessor(null);
+
+    ZkNodeProps props =
+        new ZkNodeProps(
+            Overseer.QUEUE_OPERATION,
+            CollectionParams.CollectionAction.MOCK_SHARD_TASK.toLower(),
+            ZkStateReader.COLLECTION_PROP,
+            "aliasAB",
+            ZkStateReader.SHARD_ID_PROP,
+            "shard1",
+            CollectionAdminParams.FOLLOW_ALIASES,
+            "true");
+    queue.add(
+        new QueueEvent("multiAliasTask", Utils.toJSON(props), null) {
+          @Override
+          public void setBytes(byte[] bytes) {
+            lastProcessMessageResult = OverseerSolrResponseSerializer.deserialize(bytes);
+          }
+        });
+    waitForEmptyQueue();
+
+    SolrException e = (SolrException) lastProcessMessageResult.getException();
+    assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code());
+  }
 }

solr/core/src/test/org/apache/solr/cloud/api/collections/AliasCollectionApiLockTest.java

@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cloud.api.collections;
+
+import java.util.concurrent.TimeUnit;
+import org.apache.solr.client.solrj.SolrRequest;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.client.solrj.request.GenericSolrRequest;
+import org.apache.solr.client.solrj.response.RequestStatusState;
+import org.apache.solr.cloud.DistributedLock;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.cloud.ZkDistributedCollectionLockFactory;
+import org.apache.solr.common.cloud.Slice;
+import org.apache.solr.common.params.CollectionAdminParams;
+import org.apache.solr.common.params.CollectionParams.CollectionAction;
+import org.apache.solr.common.params.CollectionParams.LockLevel;
+import org.apache.solr.common.params.CommonAdminParams;
+import org.apache.solr.common.params.CoreAdminParams;
+import org.apache.solr.common.util.TimeSource;
+import org.apache.solr.util.TimeOut;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+/** With Overseer disabled, a command naming an alias locks the collection the alias resolves to. */
+public class AliasCollectionApiLockTest extends SolrCloudTestCase {
+  private static final String COLLECTION = "aliasLockColl";
+  private static final String ALIAS = "aliasLockAlias";
+  private static final long WAIT_SECONDS = 30;
+
+  @BeforeClass
+  public static void setupCluster() throws Exception {
+    configureCluster(1)
+        .addConfig("conf", configset("cloud-minimal"))
+        .withOverseer(false)
+        .configure();
+    CollectionAdminRequest.createCollection(COLLECTION, "conf", 1, 2)
+        .process(cluster.getSolrClient());
+    cluster.waitForActiveCollection(COLLECTION, 1, 2);
+    CollectionAdminRequest.createAlias(ALIAS, COLLECTION).process(cluster.getSolrClient());
+  }
+
+  @Test
+  public void testAliasedCommandWaitsForCollectionLock() throws Exception {
+    Slice slice = getCollectionState(COLLECTION).getSlices().iterator().next();
+    String replica = slice.getReplicas().iterator().next().getName();
+    String asyncId = "deleteReplicaViaAlias";
+    DistributedLock held =
+        new ZkDistributedCollectionLockFactory(
+                cluster.getZkClient(),
+                DistributedCollectionConfigSetCommandRunner.ZK_COLLECTION_LOCKS)
+            .createLock(true, LockLevel.COLLECTION, COLLECTION, null, null, null);
+    held.waitUntilAcquired();
+    try {
+      new GenericSolrRequest(
+              SolrRequest.METHOD.GET,
+              "/admin/collections",
+              params(
+                  CoreAdminParams.ACTION,
+                  CollectionAction.DELETEREPLICA.toLower(),
+                  CollectionAdminParams.COLLECTION,
+                  ALIAS,
+                  CoreAdminParams.SHARD,
+                  slice.getName(),
+                  CoreAdminParams.REPLICA,
+                  replica,
+                  CollectionAdminParams.FOLLOW_ALIASES,
+                  "true",
+                  CommonAdminParams.ASYNC,
+                  asyncId))
+          .process(cluster.getSolrClient());
+
+      String lockDir = held.getLockId().substring(0, held.getLockId().lastIndexOf('/'));
+      new TimeOut(WAIT_SECONDS, TimeUnit.SECONDS, TimeSource.NANO_TIME)
+          .waitFor(
+              "DELETEREPLICA neither waited for nor ran past the lock",
+              () -> state(asyncId) != RequestStatusState.SUBMITTED || waiters(lockDir) > 0);
+      assertEquals(
+          "DELETEREPLICA on " + ALIAS + " ran while " + COLLECTION + " was locked",
+          RequestStatusState.SUBMITTED,
+          state(asyncId));
+    } finally {
+      held.release();
+    }
+    assertEquals(
+        RequestStatusState.COMPLETED,
+        CollectionAdminRequest.requestStatus(asyncId)
+            .waitFor(cluster.getSolrClient(), WAIT_SECONDS));
+    assertNull(getCollectionState(COLLECTION).getReplica(replica));
+  }
+
+  private static RequestStatusState state(String asyncId) {
+    try {
+      return CollectionAdminRequest.requestStatus(asyncId)
+          .process(cluster.getSolrClient())
+          .getRequestStatus();
+    } catch (Exception e) {
+      throw new RuntimeException(e);
+    }
+  }
+
+  private static int waiters(String lockDir) {
+    try {
+      return cluster.getZkClient().getChildren(lockDir, null).size() - 1;
+    } catch (Exception e) {
+      throw new RuntimeException(e);
+    }
+  }
+}