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,20 @@ 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 (see run()'s
+      // finally),
+      // i.e. cases where no Zk reconnect handler will re-drive the election. Our own leader
+      // registration is removed, version-guarded, by OverseerElectionContext.cancelElection() as
+      // part
+      // of the rejoin below -- we no longer delete /overseer_elect/leader here (that used an
+      // always-0
+      // dataVersion guard that could ABA-delete a newer lineage's 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 +565,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 {
@@ -630,6 +624,7 @@ private LeaderStatus amILeader() {
 
     @Override
     public void close() {
+      //      Thread.dumpStack();
       this.isClosed = true;
       IOUtils.closeQuietly(clusterStateUpdaterMetricContext);
     }
@@ -664,6 +659,7 @@ public <T extends Runnable & Closeable> OverseerThread(
     @Override
     public void close() throws IOException {
       thread.close();
+      //      Thread.dumpStack();
       this.isClosed = true;
     }
 

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,23 +60,57 @@ 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);
-
+    // Create the leader registration znode and start the overseer atomically, under the same lock
+    // close() takes, so a concurrent close() can no longer slip in between the create and start and
+    // leave a "zombie leader" znode with no overseer behind it. The registration is a multi op that
+    // also setData's the parent, which bumps (and lets us capture) the parent version so
+    // cancelElection() can later delete only our own registration, ABA-safe. 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 {
+        //        Thread.dumpStack();
+        log.warn(
+            "ZOMBIE LEADER: created leader registration {} -> {} but skipping overseer.start() "
+                + "because the election context was closed underneath us (isClosed={}, shutDown={}). "
+                + "The leader znode now points at an overseer that will never run and will not be "
+                + "removed by cancelElection(); subsequent elections will fail with NodeExists.",
+            leaderPath,
+            id,
+            this.isClosed,
+            shutDown);
       }
     }
   }
 
   @Override
   public void cancelElection() throws InterruptedException, KeeperException {
     super.cancelElection();
+    // Delete our own leader registration, guarded by the parent version we captured at
+    // registration,
+    // so we can never delete a newer lineage's/host's registration (ABA-safe). Mirrors
+    // ShardLeaderElectionContextBase.cancelElection.
+    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();
   }
 
   @Override
   public synchronized void close() {
+    //    Thread.dumpStack();
     this.isClosed = true;
     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;
       }