Merge pull request #281 from smukil/fix_public_private_IP_switch

Use IPAddresses from HostSupplier rather than from AbstractTokenMapSu…
Netflix · Aug 30, 2019 · d6f3674 · d6f3674
2 parents b32df0a + e01c1c6
commit d6f3674
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 3 deletions.
diff --git a/dyno-core/src/main/java/com/netflix/dyno/connectionpool/impl/HostsUpdater.java b/dyno-core/src/main/java/com/netflix/dyno/connectionpool/impl/HostsUpdater.java
@@ -16,6 +16,7 @@
 package com.netflix.dyno.connectionpool.impl;
 
 import com.netflix.dyno.connectionpool.Host;
+import com.netflix.dyno.connectionpool.HostBuilder;
 import com.netflix.dyno.connectionpool.HostSupplier;
 import com.netflix.dyno.connectionpool.TokenMapSupplier;
 import com.netflix.dyno.connectionpool.exception.DynoException;
@@ -116,12 +117,50 @@ public HostStatusTracker refreshHosts() {
                     throw new DynoException("Could not find " + hostFromHostSupplier.getHostName() + " in token map supplier.");
                 }
 
-                hostsUpFromHostSupplier.add(Host.clone(hostFromTokenMapSupplier).setStatus(Host.Status.Up));
+                // XXX: There's slight subtlety here. We get the hostname and IPAddress from 'hostFromHostSupplier'
+                // which is derived from the HostSupplier, whereas we get everything else from the 'hostFromTokenMapSupplier'
+                // which is basically derived from the AbstractTokenMapSupplier.
+                // The reason for this subtlety is due to the fact that the host supplier returns the preferred IPAddress
+                // and the AbstractTokenMapSupplier may return an alternative IPAddress (public IP vs. private IP) that
+                // we may not be able to access. (The same applies to the following 'else' case.)
+                //
+                // Why does the AbstractTokenMapSupplier return public IPs?
+                // This is because the AbstractTokenMapSupplier derives its values from dynomite-manager which
+                // returns the topology seen by it and the server processes, and they use Public IPs to communicate with
+                // each other.
+                // TODO: Can we ensure that both the HostSupplier and AbstractTokenMapSupplier agree on the same values?
+                HostBuilder upHostBuilder = new HostBuilder()
+                        .setHostname(hostFromHostSupplier.getHostName())
+                        .setIpAddress(hostFromHostSupplier.getIpAddress())
+                        .setStatus(Host.Status.Up);
+
+                upHostBuilder.setPort(hostFromTokenMapSupplier.getPort())
+                        .setSecurePort(hostFromTokenMapSupplier.getSecurePort())
+                        .setDatastorePort(hostFromTokenMapSupplier.getDatastorePort())
+                        .setRack(hostFromTokenMapSupplier.getRack())
+                        .setDatacenter(hostFromTokenMapSupplier.getDatacenter())
+                        .setHashtag(hostFromHostSupplier.getHashtag())
+                        .setPassword(hostFromTokenMapSupplier.getPassword());
+
+                hostsUpFromHostSupplier.add(upHostBuilder.createHost());
                 allHostSetFromTokenMapSupplier.remove(hostFromTokenMapSupplier);
             } else {
                 Host hostFromTokenMapSupplier = allHostSetFromTokenMapSupplier.get(hostFromHostSupplier);
 
-                hostsDownFromHostSupplier.add(Host.clone(hostFromTokenMapSupplier).setStatus(Host.Status.Down));
+                HostBuilder downHostBuilder = new HostBuilder()
+                        .setHostname(hostFromHostSupplier.getHostName())
+                        .setIpAddress(hostFromHostSupplier.getIpAddress())
+                        .setStatus(Host.Status.Down);
+
+                downHostBuilder.setPort(hostFromTokenMapSupplier.getPort())
+                        .setSecurePort(hostFromTokenMapSupplier.getSecurePort())
+                        .setDatastorePort(hostFromTokenMapSupplier.getDatastorePort())
+                        .setRack(hostFromTokenMapSupplier.getRack())
+                        .setDatacenter(hostFromTokenMapSupplier.getDatacenter())
+                        .setHashtag(hostFromHostSupplier.getHashtag())
+                        .setPassword(hostFromTokenMapSupplier.getPassword());
+
+                hostsDownFromHostSupplier.add(downHostBuilder.createHost());
                 allHostSetFromTokenMapSupplier.remove(hostFromTokenMapSupplier);
             }
         }

diff --git a/dyno-recipes/src/test/java/com/netflix/dyno/recipes/lock/LocalRedisLockTest.java b/dyno-recipes/src/test/java/com/netflix/dyno/recipes/lock/LocalRedisLockTest.java
@@ -25,7 +25,15 @@ public void setUp() throws IOException {
         redisServer = new RedisServer(REDIS_PORT);
         redisServer.start();
         Assume.assumeFalse(System.getProperty("os.name").toLowerCase().startsWith("win"));
-        host = new HostBuilder().setHostname("localhost").setDatastorePort(REDIS_PORT).setPort(REDIS_PORT).setRack(REDIS_RACK).setStatus(Host.Status.Up).createHost();
+        host = new HostBuilder()
+                .setHostname("localhost")
+                .setIpAddress("127.0.0.1")
+                .setDatastorePort(REDIS_PORT)
+                .setPort(REDIS_PORT)
+                .setRack(REDIS_RACK)
+                .setStatus(Host.Status.Up)
+                .createHost();
+
         tokenMapSupplier = new TokenMapSupplierImpl(host);
         dynoLockClient = constructDynoLockClient();
     }