Merge pull request #16557 from vespa-engine/jonmv/properly-detect-leaving-zk-servers

Properly detect leaving ZK servers by comparing full spec

2 files changed, 26 insertions, 15 deletions

diff --git a/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Reconfigurer.java b/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Reconfigurer.java
index 33191718bfb..4ef73ec2374 100644
--- a/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Reconfigurer.java
+++ b/zookeeper-server/zookeeper-server-common/src/main/java/com/yahoo/vespa/zookeeper/Reconfigurer.java
@@ -86,12 +86,12 @@ public class Reconfigurer extends AbstractComponent {
         if (newConfig.server().size() == 1) shutdownAndDie(server, Duration.ZERO);
 
         List<String> newServers = difference(servers(newConfig), servers(activeConfig));
-        String leavingServers = String.join(",", difference(serverIds(activeConfig), serverIds(newConfig)));
-        String joiningServers = String.join(",", newServers);
-        leavingServers = leavingServers.isEmpty() ? null : leavingServers;
-        joiningServers = joiningServers.isEmpty() ? null : joiningServers;
-        log.log(Level.INFO, "Will reconfigure ZooKeeper cluster. \nJoining servers: " + joiningServers +
-                            "\nleaving servers: " + leavingServers +
+        String leavingServerIds = String.join(",", serverIdsDifference(activeConfig, newConfig));
+        String joiningServersSpec = String.join(",", newServers);
+        leavingServerIds = leavingServerIds.isEmpty() ? null : leavingServerIds;
+        joiningServersSpec = joiningServersSpec.isEmpty() ? null : joiningServersSpec;
+        log.log(Level.INFO, "Will reconfigure ZooKeeper cluster. \nJoining servers: " + joiningServersSpec +
+                            "\nleaving servers: " + leavingServerIds +
                             "\nServers in active config:" + servers(activeConfig) +
                             "\nServers in new config:" + servers(newConfig));
         String connectionSpec = localConnectionSpec(activeConfig);
@@ -102,7 +102,7 @@ public class Reconfigurer extends AbstractComponent {
         for (int attempt = 1; now.isBefore(end); attempt++) {
             try {
                 Instant reconfigStarted = Instant.now();
-                vespaZooKeeperAdmin.reconfigure(connectionSpec, joiningServers, leavingServers);
+                vespaZooKeeperAdmin.reconfigure(connectionSpec, joiningServersSpec, leavingServerIds);
                 Instant reconfigEnded = Instant.now();
                 log.log(Level.INFO, "Reconfiguration completed in " +
                                     Duration.between(reconfigTriggered, reconfigEnded) +
@@ -141,11 +141,10 @@ public class Reconfigurer extends AbstractComponent {
         return HostName.getLocalhost() + ":" + config.clientPort();
     }
 
-    private static List<String> serverIds(ZookeeperServerConfig config) {
-        return config.server().stream()
-                     .map(ZookeeperServerConfig.Server::id)
-                     .map(String::valueOf)
-                     .collect(Collectors.toList());
+    private static List<String> serverIdsDifference(ZookeeperServerConfig oldConfig, ZookeeperServerConfig newConfig) {
+        return difference(servers(oldConfig), servers(newConfig)).stream()
+                                                                 .map(server -> server.substring(0, server.indexOf('=')))
+                                                                 .collect(Collectors.toList());
     }
 
     private static List<String> servers(ZookeeperServerConfig config) {
diff --git a/zookeeper-server/zookeeper-server-common/src/test/java/com/yahoo/vespa/zookeeper/ReconfigurerTest.java b/zookeeper-server/zookeeper-server-common/src/test/java/com/yahoo/vespa/zookeeper/ReconfigurerTest.java
index a1b98a23bd0..5cee0de2b6e 100644
--- a/zookeeper-server/zookeeper-server-common/src/test/java/com/yahoo/vespa/zookeeper/ReconfigurerTest.java
+++ b/zookeeper-server/zookeeper-server-common/src/test/java/com/yahoo/vespa/zookeeper/ReconfigurerTest.java
@@ -13,7 +13,7 @@ import java.io.File;
 import java.io.IOException;
 import java.nio.file.Path;
 import java.time.Duration;
-import java.util.stream.IntStream;
+import java.util.Arrays;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
@@ -68,6 +68,14 @@ public class ReconfigurerTest {
         assertNull("No servers are joining", reconfigurer.joiningServers());
         assertEquals("3,4", reconfigurer.leavingServers());
         assertSame(nextConfig, reconfigurer.activeConfig());
+
+        // Cluster loses node1, but node3 joins. Indices are shuffled.
+        nextConfig = createConfig(3, true, 1);
+        reconfigurer.startOrReconfigure(nextConfig);
+        assertEquals(3, reconfigurer.reconfigurations());
+        assertEquals("1=node2:2182:2183;2181,2=node3:2182:2183;2181", reconfigurer.joiningServers());
+        assertEquals("1,2", reconfigurer.leavingServers());
+        assertSame(nextConfig, reconfigurer.activeConfig());
     }
 
     @Test
@@ -107,11 +115,15 @@ public class ReconfigurerTest {
        reconfigurer.shutdown();
     }
 
-    private ZookeeperServerConfig createConfig(int numberOfServers, boolean dynamicReconfiguration) {
+    private ZookeeperServerConfig createConfig(int numberOfServers, boolean dynamicReconfiguration, int... skipIndices) {
+        Arrays.sort(skipIndices);
         ZookeeperServerConfig.Builder builder = new ZookeeperServerConfig.Builder();
         builder.zooKeeperConfigFile(cfgFile.getAbsolutePath());
         builder.myidFile(idFile.getAbsolutePath());
-        IntStream.range(0, numberOfServers).forEach(i -> builder.server(newServer(i, "node" + i)));
+        for (int i = 0, index = 0; i < numberOfServers; i++, index++) {
+            while (Arrays.binarySearch(skipIndices, index) >= 0) index++;
+            builder.server(newServer(i, "node" + index));
+        }
         builder.myid(0);
         builder.dynamicReconfiguration(dynamicReconfiguration);
         return builder.build();