ZOOKEEPER-3758: Leader reachability check fails with single address

Since ZooKeeper 3.6.0 we can specify multiple addresses for each ZooKeeper server instance
(this can increase availability when multiple physical network interfaces can be used parallel
in the cluster). ZooKeeper will perform ICMP ECHO requests or try to establish a TCP connection
on port 7 (Echo) of the destination host in order to find the reachable addresses. This should
happen only if the user provide multiple addresses in the configuration, in a single address is
used then ZooKeeper shouldn’t send any ICMP requests.

This works as we expected for the leader election connections, but in this Jira issue we found
a bug when the reachability check was performed even with a single address when the Follower
tries to connect to the newly elected Leader.

The fix follows the same approach we discussed for the election protocol before (see
ZOOKEEPER-3698). We avoid the reachability check for single addresses. Also when we have
multiple addresses and none of them can be reached, we still start to connect to all addresses
in parallel.

Author: Mate Szalay-Beko <[email protected]>

Reviewers: Enrico Olivelli <[email protected]>, Norbert Kalmar <[email protected]>

Closes apache#1288 from symat/ZOOKEEPER-3758

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/Learner.java

@@ -303,7 +303,9 @@ protected void connectToLeader(MultipleAddresses multiAddr, String hostname) thr
         this.leaderAddr = multiAddr;
         Set<InetSocketAddress> addresses;
         if (self.isMultiAddressReachabilityCheckEnabled()) {
-            addresses = multiAddr.getAllReachableAddresses();
+            // even if none of the addresses are reachable, we want to try to establish connection
+            // see ZOOKEEPER-3758
+            addresses = multiAddr.getAllReachableAddressesOrAll();
         } else {
             addresses = multiAddr.getAllAddresses();
         }

zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/MultipleAddresses.java

@@ -139,6 +139,24 @@ public Set<InetSocketAddress> getAllReachableAddresses() {
           .collect(Collectors.toSet());
     }
 
+    /**
+     * Returns a set of all reachable addresses. If none is reachable than returns all addresses.
+     *
+     * @return all reachable addresses, or all addresses if none is reachable.
+     */
+    public Set<InetSocketAddress> getAllReachableAddressesOrAll() {
+        // if there is only a single address provided then we don't need to do any reachability check
+        if (addresses.size() == 1) {
+            return getAllAddresses();
+        }
+
+        Set<InetSocketAddress> allReachable = getAllReachableAddresses();
+        if (allReachable.isEmpty()) {
+            return getAllAddresses();
+        }
+        return allReachable;
+    }
+
     /**
      * Returns a reachable address or an arbitrary one, if none is reachable. It throws an exception
      * if there are no addresses registered. The function is nondeterministic in the sense that the

zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/MultipleAddressesTest.java

@@ -187,6 +187,32 @@ public void testGetAllReachableAddresses() throws Exception {
         Assert.assertEquals(reachableHosts, multipleAddresses.getAllReachableAddresses());
     }
 
+    @Test
+    public void testGetAllReachableAddressesOrAllWhenSomeReachable() throws Exception {
+        InetSocketAddress reachableHost1 = new InetSocketAddress("127.0.0.1", 1234);
+        InetSocketAddress reachableHost2 = new InetSocketAddress("127.0.0.1", 2345);
+        InetSocketAddress unreachableHost1 = new InetSocketAddress("unreachable1.address.zookeeper.apache.com", 1234);
+        InetSocketAddress unreachableHost2 = new InetSocketAddress("unreachable2.address.zookeeper.apache.com", 1234);
+
+        MultipleAddresses multipleAddresses = new MultipleAddresses(
+          Arrays.asList(unreachableHost1, unreachableHost2, reachableHost1, reachableHost2));
+
+        Set<InetSocketAddress> reachableHosts = new HashSet<>(Arrays.asList(reachableHost1, reachableHost2));
+        Assert.assertEquals(reachableHosts, multipleAddresses.getAllReachableAddressesOrAll());
+    }
+
+    @Test
+    public void testGetAllReachableAddressesOrAllWhenNoneReachable() throws Exception {
+        InetSocketAddress unreachableHost1 = new InetSocketAddress("unreachable1.address.zookeeper.apache.com", 1234);
+        InetSocketAddress unreachableHost2 = new InetSocketAddress("unreachable2.address.zookeeper.apache.com", 1234);
+        InetSocketAddress unreachableHost3 = new InetSocketAddress("unreachable3.address.zookeeper.apache.com", 1234);
+        List<InetSocketAddress> allUnreachableAddresses = Arrays.asList(unreachableHost1, unreachableHost2, unreachableHost3);
+
+        MultipleAddresses multipleAddresses = new MultipleAddresses(allUnreachableAddresses);
+
+        Assert.assertEquals(new HashSet<>(allUnreachableAddresses), multipleAddresses.getAllReachableAddressesOrAll());
+    }
+
     @Test
     public void testEquals() {
         List<InetSocketAddress> addresses = getAddressList();