SOLR-18301 overseer election does not converge

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

@@ -18,8 +18,13 @@
 
 import java.io.Closeable;
 import java.lang.invoke.MethodHandles;
+import java.util.List;
+import org.apache.curator.framework.api.transaction.CuratorTransactionResult;
+import org.apache.curator.framework.api.transaction.OperationType;
 import org.apache.solr.common.cloud.SolrZkClient;
+import org.apache.solr.common.cloud.ZkMaintenanceUtils;
 import org.apache.solr.common.cloud.ZkNodeProps;
+import org.apache.zookeeper.CreateMode;
 import org.apache.zookeeper.KeeperException;
 import org.apache.zookeeper.KeeperException.NoNodeException;
 import org.slf4j.Logger;
@@ -31,9 +36,21 @@ public abstract class ElectionContext implements Closeable {
   final ZkNodeProps leaderProps;
   final String id;
   final String leaderPath;
+
+  /** Parent of {@link #leaderPath}; derived once since {@link #leaderPath} is final. */
+  final String leaderParentPath;
+
   volatile String leaderSeqPath;
   private SolrZkClient zkClient;
 
+  /**
+   * Version of {@link #leaderPath}'s parent captured when this context registered as leader (see
+   * {@link #registerLeaderNode}); {@link #deleteLeaderNode} uses it on cancel to remove only our
+   * own registration (ABA-safe). Null until/after registration. Thread-safety is provided by each
+   * subclass's own election lock — there is no base-level lock guarding this field.
+   */
+  protected Integer leaderZkNodeParentVersion;
+
   public ElectionContext(
       final String coreNodeName,
       final String electionPath,
@@ -44,6 +61,7 @@ public ElectionContext(
     this.id = coreNodeName;
     this.electionPath = electionPath;
     this.leaderPath = leaderPath;
+    this.leaderParentPath = ZkMaintenanceUtils.getZkParent(leaderPath);
     this.leaderProps = leaderProps;
     this.zkClient = zkClient;
   }
@@ -81,4 +99,48 @@ public void joinedElectionFired() {}
   public ElectionContext copy() {
     throw new UnsupportedOperationException("copy");
   }
+
+  /**
+   * Registers {@link #leaderPath} as an ephemeral leader node in a single multi transaction that
+   * also bumps the version of the leader node's parent (via a {@code setData}), capturing it into
+   * {@link #leaderZkNodeParentVersion} so {@link #deleteLeaderNode()} can later remove <em>only our
+   * own</em> registration, ABA-safe. The transaction also sanity-checks that {@link #leaderSeqPath}
+   * still exists.
+   *
+   * <p>Does the ZooKeeper work only; callers own any retry, locking, and error handling around it
+   * (which legitimately differ between overseer and shard leader election).
+   */
+  protected void registerLeaderNode(byte[] leaderData)
+      throws KeeperException, InterruptedException {
+    List<CuratorTransactionResult> results =
+        zkClient.multi(
+            op -> op.check().withVersion(-1).forPath(leaderSeqPath),
+            op -> op.create().withMode(CreateMode.EPHEMERAL).forPath(leaderPath, leaderData),
+            op -> op.setData().withVersion(-1).forPath(leaderParentPath, null));
+    leaderZkNodeParentVersion =
+        results.stream()
+            .filter(
+                CuratorTransactionResult.ofTypeAndPath(OperationType.SET_DATA, leaderParentPath))
+            .findFirst()
+            .orElseThrow(
+                () ->
+                    new RuntimeException(
+                        "Could not set data for parent path in ZK: " + leaderParentPath))
+            .getResultStat()
+            .getVersion();
+  }
+
+  /**
+   * Deletes {@link #leaderPath} guarded by {@link #leaderZkNodeParentVersion}, so it only removes a
+   * registration whose parent version still matches — i.e. our own, never a newer lineage's/host's.
+   * Must only be called when {@link #leaderZkNodeParentVersion} is non-null. Callers own locking,
+   * any exception handling (e.g. treating {@link KeeperException.BadVersionException}/{@link
+   * NoNodeException} as "not ours / already gone"), and clearing {@link #leaderZkNodeParentVersion}
+   * afterward.
+   */
+  protected void deleteLeaderNode() throws KeeperException, InterruptedException {
+    zkClient.multi(
+        op -> op.check().withVersion(leaderZkNodeParentVersion).forPath(leaderParentPath),
+        op -> op.delete().withVersion(-1).forPath(leaderPath));
+  }
 }

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

@@ -25,7 +25,6 @@
 import java.util.ArrayDeque;
 import java.util.HashSet;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.CopyOnWriteArrayList;
 import org.apache.solr.client.solrj.cloud.SolrCloudManager;
@@ -66,7 +65,6 @@
 import org.apache.solr.update.UpdateShardHandler;
 import org.apache.zookeeper.CreateMode;
 import org.apache.zookeeper.KeeperException;
-import org.apache.zookeeper.data.Stat;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -182,6 +180,9 @@ private class ClusterStateUpdater implements SolrInfoBean, Runnable, Closeable {
     private final Compressor compressor;
 
     private boolean isClosed = false;
+    // Set when this overseer is told to step down via an explicit QUIT (roles handoff). Read in
+    // run()'s finally to decide whether to spawn the OverseerExitThread to rejoin the election.
+    private volatile boolean quitReceived = false;
 
     public ClusterStateUpdater(
         final ZkStateReader reader,
@@ -234,6 +235,7 @@ public void run() {
       if (log.isInfoEnabled()) {
         log.info("Starting to work on the main queue : {}", LeaderElector.getNodeName(myId));
       }
+      boolean crashed = false;
       try {
         ZkStateWriter zkStateWriter = null;
         ClusterState clusterState = null;
@@ -391,14 +393,29 @@ public void run() {
             refreshClusterState = true; // it might have been a bad version error
           }
         }
+      } catch (Throwable t) {
+        // The main loop terminated abnormally -- not a clean close, not a session-expiry return,
+        // not
+        // a QUIT. Rejoin below so we recover instead of leaving a dead overseer still holding the
+        // /overseer_elect/leader znode with nothing behind it.
+        crashed = true;
+        log.error("Overseer main loop terminated unexpectedly", t);
       } finally {
         if (log.isInfoEnabled()) {
           log.info("Overseer Loop exiting : {}", LeaderElector.getNodeName(myId));
         }
-        // do this in a separate thread because any wait is interrupted in this main thread
-        Thread checkLeaderThread = new Thread(this::checkIfIamStillLeader, "OverseerExitThread");
-        checkLeaderThread.setDaemon(true);
-        checkLeaderThread.start();
+        // Only spawn the exit thread to rejoin the election when nobody else will: an explicit QUIT
+        // (roles handoff) or an unexpected crash. On a clean close or a ZK session-expiry
+        // reconnect,
+        // the ZkController reconnect handler owns re-election, so spawning here would just race it
+        // and
+        // risk two competing overseer lineages.
+        if (quitReceived || crashed) {
+          // do this in a separate thread because any wait is interrupted in this main thread
+          Thread checkLeaderThread = new Thread(this::checkIfIamStillLeader, "OverseerExitThread");
+          checkLeaderThread.setDaemon(true);
+          checkLeaderThread.start();
+        }
       }
     }
 
@@ -455,44 +472,15 @@ private void checkIfIamStillLeader() {
           && (zkController.getCoreContainer().isShutDown() || zkController.isClosed())) {
         return; // shutting down no need to go further
       }
-      Stat stat = new Stat();
-      final String path = OVERSEER_ELECT + "/leader";
-      byte[] data;
-      try {
-        data = zkClient.getData(path, null, stat);
-      } catch (IllegalStateException | KeeperException.NoNodeException e) {
-        return;
-      } catch (Exception e) {
-        log.warn("Error communicating with ZooKeeper", e);
-        return;
-      }
+      // We only reach here after a QUIT (roles handoff) or an unexpected crash, i.e. cases where no
+      // Zk reconnect handler will re-drive the election. The rejoin below cancels our context,
+      // which is what removes our leader registration.
       try {
-        Map<?, ?> m = (Map<?, ?>) Utils.fromJSON(data);
-        String id = (String) m.get(ID);
-        if (overseerCollectionConfigSetProcessor.getId().equals(id)) {
-          try {
-            log.warn(
-                "I (id={}) am exiting, but I'm still the leader",
-                overseerCollectionConfigSetProcessor.getId());
-            zkClient.delete(path, stat.getVersion());
-          } catch (KeeperException.BadVersionException e) {
-            // no problem ignore it some other Overseer has already taken over
-          } catch (Exception e) {
-            log.error("Could not delete my leader node {}", path, e);
-          }
-
-        } else {
-          log.info("somebody else (id={}) has already taken up the overseer position", id);
-        }
-      } finally {
-        // if I am not shutting down, Then I need to rejoin election
-        try {
-          if (zkController != null && !zkController.getCoreContainer().isShutDown()) {
-            zkController.rejoinOverseerElection(null, false);
-          }
-        } catch (Exception e) {
-          log.warn("Unable to rejoinElection ", e);
+        if (zkController != null && !zkController.getCoreContainer().isShutDown()) {
+          zkController.rejoinOverseerElection(null, false);
         }
+      } catch (Exception e) {
+        log.warn("Unable to rejoinElection ", e);
       }
     }
 
@@ -572,6 +560,7 @@ private List<ZkWriteCommand> processMessage(
               if (log.isInfoEnabled()) {
                 log.info("Quit command received {} {}", message, LeaderElector.getNodeName(myId));
               }
+              quitReceived = true;
               IOUtils.closeQuietly(overseerCollectionConfigSetProcessor);
               IOUtils.closeQuietly(this);
             } else {

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

@@ -26,7 +26,6 @@
 import org.apache.solr.common.cloud.ZkMaintenanceUtils;
 import org.apache.solr.common.cloud.ZkNodeProps;
 import org.apache.solr.common.util.Utils;
-import org.apache.zookeeper.CreateMode;
 import org.apache.zookeeper.KeeperException;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -61,18 +60,41 @@ void runLeaderProcess(boolean weAreReplacement) throws KeeperException, Interrup
     final String id = leaderSeqPath.substring(leaderSeqPath.lastIndexOf('/') + 1);
     ZkNodeProps myProps = new ZkNodeProps(ID, id);
 
-    zkClient.makePath(leaderPath, Utils.toJSON(myProps), CreateMode.EPHEMERAL);
-
+    // Register and start under the same lock close() takes, so a close() cannot land between them
+    // and leave a leader znode with no overseer behind it. Registration also captures the parent
+    // version so cancelElection() only deletes our own. Mirrors ShardLeaderElectionContextBase.
     synchronized (this) {
-      if (!this.isClosed && !overseer.getZkController().getCoreContainer().isShutDown()) {
+      boolean shutDown = overseer.getZkController().getCoreContainer().isShutDown();
+      if (!this.isClosed && !shutDown) {
+        registerLeaderNode(Utils.toJSON(myProps));
+        log.info("Created overseer leader registration {} -> {}", leaderPath, id);
         overseer.start(id);
+      } else {
+        log.info(
+            "Not registering as overseer leader for {}: isClosed={}, shutDown={}",
+            leaderPath,
+            this.isClosed,
+            shutDown);
       }
     }
   }
 
   @Override
   public void cancelElection() throws InterruptedException, KeeperException {
     super.cancelElection();
+    // Delete only our own registration, guarded by the parent version captured at registration, so
+    // we can never remove a newer lineage's (ABA-safe). Mirrors ShardLeaderElectionContextBase.
+    synchronized (this) {
+      if (leaderZkNodeParentVersion != null) {
+        try {
+          deleteLeaderNode();
+        } catch (KeeperException.BadVersionException | KeeperException.NoNodeException e) {
+          // A newer lineage already re-registered (parent version bumped) or the node is already
+          // gone -- either way the leader znode is not ours to remove.
+        }
+        leaderZkNodeParentVersion = null;
+      }
+    }
     overseer.close();
   }
 

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

@@ -164,7 +164,8 @@ void runLeaderProcess(boolean weAreReplacement) throws KeeperException, Interrup
 
       if (shouldAbort()) {
         // Solr is shutting down or the ZooKeeper session expired while waiting for replicas. If the
-        // later, we cannot be sure we are still the leader, so we should bail out. The OnReconnect
+        // later, we cannot be sure we are still the leader, so we should bail out. The
+        // OnExpiredReconnection
         // handler will re-register the cores and handle a new leadership election.
         return;
       }

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

@@ -18,11 +18,8 @@
 package org.apache.solr.cloud;
 
 import java.lang.invoke.MethodHandles;
-import java.util.List;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
-import org.apache.curator.framework.api.transaction.CuratorTransactionResult;
-import org.apache.curator.framework.api.transaction.OperationType;
 import org.apache.solr.cloud.overseer.OverseerAction;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.SolrException.ErrorCode;
@@ -51,7 +48,6 @@ class ShardLeaderElectionContextBase extends ElectionContext {
   protected LeaderElector leaderElector;
   protected ZkStateReader zkStateReader;
   protected ZkController zkController;
-  protected Integer leaderZkNodeParentVersion;
 
   // Prevents a race between cancelling and becoming leader.
   private final Object lock = new Object();
@@ -76,11 +72,11 @@ public ShardLeaderElectionContextBase(
     this.shardId = shardId;
     this.collection = collection;
 
-    String parent = ZkMaintenanceUtils.getZkParent(leaderPath);
     // only if /collections/{collection} exists already do we succeed in creating this path
-    log.info("make sure parent is created {}", parent);
+    log.info("make sure parent is created {}", leaderParentPath);
     try {
-      ZkMaintenanceUtils.ensureExists(parent, (byte[]) null, CreateMode.PERSISTENT, zkClient, 2);
+      ZkMaintenanceUtils.ensureExists(
+          leaderParentPath, (byte[]) null, CreateMode.PERSISTENT, zkClient, 2);
     } catch (KeeperException e) {
       throw new RuntimeException(e);
     } catch (InterruptedException e) {
@@ -104,10 +100,7 @@ public void cancelElection() throws InterruptedException, KeeperException {
               "Removing leader registration node on cancel: {} {}",
               leaderPath,
               leaderZkNodeParentVersion);
-          String parent = ZkMaintenanceUtils.getZkParent(leaderPath);
-          zkClient.multi(
-              op -> op.check().withVersion(leaderZkNodeParentVersion).forPath(parent),
-              op -> op.delete().withVersion(-1).forPath(leaderPath));
+          deleteLeaderNode();
         } catch (InterruptedException e) {
           Thread.currentThread().interrupt();
           throw e;
@@ -126,7 +119,6 @@ public void cancelElection() throws InterruptedException, KeeperException {
   void runLeaderProcess(boolean weAreReplacement) throws KeeperException, InterruptedException {
     // register as leader - if an ephemeral is already there, wait to see if it goes away
 
-    String parent = ZkMaintenanceUtils.getZkParent(leaderPath);
     try {
       RetryUtil.retryOnException(
           NodeExistsException.class,
@@ -138,30 +130,7 @@ void runLeaderProcess(boolean weAreReplacement) throws KeeperException, Interrup
                   "Creating leader registration node {} after winning as {}",
                   leaderPath,
                   leaderSeqPath);
-
-              // We use a multi operation to get the parent nodes version, which will
-              // be used to make sure we only remove our own leader registration node.
-              // The setData call used to get the parent version is also the trigger to
-              // increment the version. We also do a sanity check that our leaderSeqPath exists.
-              List<CuratorTransactionResult> results =
-                  zkClient.multi(
-                      op -> op.check().withVersion(-1).forPath(leaderSeqPath),
-                      op ->
-                          op.create()
-                              .withMode(CreateMode.EPHEMERAL)
-                              .forPath(leaderPath, Utils.toJSON(leaderProps)),
-                      op -> op.setData().withVersion(-1).forPath(parent, null));
-              leaderZkNodeParentVersion =
-                  results.stream()
-                      .filter(
-                          CuratorTransactionResult.ofTypeAndPath(OperationType.SET_DATA, parent))
-                      .findFirst()
-                      .orElseThrow(
-                          () ->
-                              new RuntimeException(
-                                  "Could not set data for parent path in ZK: " + parent))
-                      .getResultStat()
-                      .getVersion();
+              registerLeaderNode(Utils.toJSON(leaderProps));
             }
           });
     } catch (NoNodeException e) {