From 00f5457d6f1a3adb7febd03cc57cc62f0082cc59 Mon Sep 17 00:00:00 2001
From: Valentin Aitken <bostko@gmail.com>
Date: Wed, 28 Sep 2016 00:05:52 +0300
Subject: [PATCH 1/3] Reduce decisions for HostAndPort which will be used in
 JcloudsLocation

Use managementHostAndPort for all customizations made in
- obtainOnce(ConfigBag)
- registerMachineLocation(ConfigBag, NodeMetadata)

Cleanup transformations
---
 .../location/jclouds/JcloudsLocation.java     | 221 +++++++-----------
 .../location/jclouds/JcloudsUtil.java         |   2 +-
 2 files changed, 90 insertions(+), 133 deletions(-)

diff --git a/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java b/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java
index 2fd12a8ec6..f521d8130c 100644
--- a/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java
+++ b/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java
@@ -133,6 +133,7 @@
 import org.apache.brooklyn.util.time.Duration;
 import org.apache.brooklyn.util.time.Time;
 import org.apache.commons.lang3.ArrayUtils;
+import org.apache.commons.lang3.tuple.Pair;
 import org.jclouds.aws.ec2.compute.AWSEC2TemplateOptions;
 import org.jclouds.cloudstack.compute.options.CloudStackTemplateOptions;
 import org.jclouds.compute.ComputeService;
@@ -771,17 +772,19 @@ protected MachineLocation obtainOnce(ConfigBag setup) throws NoMachinesAvailable
                 sshHostAndPortOverride = Optional.absent();
             }
 
+            String vmHostname = getPublicHostname(node, sshHostAndPortOverride, userCredentials, setup);
             LoginCredentials initialCredentials = node.getCredentials();
+            final HostAndPort managementHostAndPort = resolveManagementHostAndPort(computeService, node, vmHostname, sshHostAndPortOverride, setup);
             if (skipJcloudsSshing) {
                 boolean waitForConnectable = (windows) ? waitForWinRmable : waitForSshable;
                 if (waitForConnectable) {
                     if (windows) {
                         // TODO Does jclouds support any windows user setup?
-                        initialCredentials = waitForWinRmAvailable(computeService, node, sshHostAndPortOverride, setup);
+                        initialCredentials = waitForWinRmAvailable(initialCredentials, managementHostAndPort, setup);
                     } else {
-                        initialCredentials = waitForSshable(computeService, node, sshHostAndPortOverride, setup);
+                        initialCredentials = waitForSshable(computeService, node, managementHostAndPort, setup);
                     }
-                    userCredentials = createUser(computeService, node, sshHostAndPortOverride, initialCredentials, setup);
+                    userCredentials = createUser(computeService, node, managementHostAndPort, initialCredentials, setup);
                 }
             }
 
@@ -816,7 +819,7 @@ protected MachineLocation obtainOnce(ConfigBag setup) throws NoMachinesAvailable
 
             // Wait for the VM to be reachable over SSH
             if (waitForSshable && !windows) {
-                waitForSshable(computeService, node, sshHostAndPortOverride, ImmutableList.of(userCredentials), setup);
+                waitForSshable(computeService, node, managementHostAndPort, ImmutableList.of(userCredentials), setup);
             } else {
                 LOG.debug("Skipping ssh check for {} ({}) due to config waitForSshable=false", node, setup.getDescription());
             }
@@ -829,9 +832,9 @@ protected MachineLocation obtainOnce(ConfigBag setup) throws NoMachinesAvailable
 
             // Create a JcloudsSshMachineLocation, and register it
             if (windows) {
-                machineLocation = registerWinRmMachineLocation(computeService, node, userCredentials, sshHostAndPortOverride, setup);
+                machineLocation = registerWinRmMachineLocation(computeService, node, managementHostAndPort, sshHostAndPortOverride, setup);
             } else {
-                machineLocation = registerJcloudsSshMachineLocation(computeService, node, Optional.fromNullable(template), userCredentials, sshHostAndPortOverride, setup);
+                machineLocation = registerJcloudsSshMachineLocation(computeService, node, Optional.fromNullable(template), userCredentials, managementHostAndPort, sshHostAndPortOverride, vmHostname, setup);
             }
 
             PortForwardManager portForwardManager = setup.get(PORT_FORWARDING_MANAGER);
@@ -1893,7 +1896,7 @@ protected WinRmMachineLocation createTemporaryWinRmMachineLocation(HostAndPort h
     /**
      * Create the user immediately - executing ssh commands as required.
      */
-    protected LoginCredentials createUser(ComputeService computeService, NodeMetadata node, Optional<HostAndPort> hostAndPortOverride, LoginCredentials initialCredentials, ConfigBag config) {
+    protected LoginCredentials createUser(ComputeService computeService, NodeMetadata node, HostAndPort managementHostAndPort, LoginCredentials initialCredentials, ConfigBag config) {
         Image image = (node.getImageId() != null) ? computeService.getImage(node.getImageId()) : null;
         UserCreation userCreation = createUserStatements(image, config);
 
@@ -1910,7 +1913,6 @@ protected LoginCredentials createUser(ComputeService computeService, NodeMetadat
 
             if (windows) {
                 LOG.warn("Unable to execute statements on WinRM in JcloudsLocation; skipping for "+node+": "+userCreation.statements);
-                
             } else {
                 List<String> commands = Lists.newArrayList();
                 for (Statement statement : userCreation.statements) {
@@ -1920,9 +1922,7 @@ protected LoginCredentials createUser(ComputeService computeService, NodeMetadat
                 }
 
                 String initialUser = initialCredentials.getUser();
-                String address = hostAndPortOverride.isPresent() ? hostAndPortOverride.get().getHostText() : getFirstReachableAddress(node, config);
-                int port = hostAndPortOverride.isPresent() ? hostAndPortOverride.get().getPort() : node.getLoginPort();
-                
+
                 // TODO Retrying lots of times as workaround for vcloud-director. There the guest customizations
                 // can cause the VM to reboot shortly after it was ssh'able.
                 Map<String,Object> execProps = Maps.newLinkedHashMap();
@@ -1931,16 +1931,15 @@ protected LoginCredentials createUser(ComputeService computeService, NodeMetadat
                 execProps.put(SshTool.PROP_SSH_TRIES_TIMEOUT.getName(), 10*60*1000);
 
                 if (LOG.isDebugEnabled()) {
-                    LOG.debug("VM {}: executing user creation/setup via {}@{}:{}; commands: {}", new Object[] {
-                            config.getDescription(), initialUser, address, port, commands});
+                    LOG.debug("VM {}: executing user creation/setup via {}@{}; commands: {}", new Object[] {
+                            config.getDescription(), initialUser, managementHostAndPort, commands});
                 }
 
-                HostAndPort hostAndPort = hostAndPortOverride.isPresent() ? hostAndPortOverride.get() : HostAndPort.fromParts(address, port);
-                SshMachineLocation sshLoc = createTemporarySshMachineLocation(hostAndPort, initialCredentials, config);
+                SshMachineLocation sshLoc = createTemporarySshMachineLocation(managementHostAndPort, initialCredentials, config);
                 try {
                     // BROOKLYN-188: for SUSE, need to specify the path (for groupadd, useradd, etc)
                     Map<String, ?> env = ImmutableMap.of("PATH", sbinPath());
-                    
+
                     int exitcode = sshLoc.execScript(execProps, "create-user", commands, env);
 
                     if (exitcode != 0) {
@@ -1956,6 +1955,33 @@ protected LoginCredentials createUser(ComputeService computeService, NodeMetadat
         return userCreation.createdUserCredentials;
     }
 
+    protected HostAndPort resolveManagementHostAndPort(ComputeService computeService, NodeMetadata node, String vmHostname, Optional<HostAndPort> hostAndPortOverride, ConfigBag config) {
+        return resolveManagementHostAndPort(computeService, node, vmHostname, hostAndPortOverride, config, false);
+    }
+
+    protected HostAndPort resolveManagementHostAndPort(ComputeService computeService, NodeMetadata node, String vmHostname, Optional<HostAndPort> hostAndPortOverride, ConfigBag config, boolean useOnlyVmHostNameAndHostAndPortOverride) {
+        String pollForFirstReachable = config.get(POLL_FOR_FIRST_REACHABLE_ADDRESS);
+        boolean pollForFirstReachableEnabled = !useOnlyVmHostNameAndHostAndPortOverride && !"false".equalsIgnoreCase(pollForFirstReachable);
+
+        String managementAddress = hostAndPortOverride.isPresent() ? hostAndPortOverride.get().getHostText() :
+                (pollForFirstReachableEnabled ? getFirstReachableAddress(node, config) : vmHostname);
+        int managementPort = hostAndPortOverride.isPresent() ? hostAndPortOverride.get().getPort() : node.getLoginPort();
+        try {
+            Networking.getInetAddressWithFixedName(managementAddress);
+            // fine, it resolves
+        } catch (Exception e) {
+            // occurs if an unresolvable hostname is given as vmHostname, and the machine only has private IP addresses but they are reachable
+            // TODO cleanup use of getPublicHostname so its semantics are clearer, returning reachable hostname or ip, and
+            // do this check/fix there instead of here!
+            Exceptions.propagateIfFatal(e);
+            LOG.debug("Could not resolve reported address '"+managementAddress+"' for "+vmHostname+" ("+config.getDescription()+"/"+node+"), requesting reachable address");
+            if (computeService==null) throw Exceptions.propagate(e);
+            // this has sometimes already been done in waitForReachable (unless skipped) but easy enough to do again
+            managementAddress = getFirstReachableAddress(node, config);
+        }
+        return hostAndPortOverride.isPresent() ? hostAndPortOverride.get() : HostAndPort.fromParts(managementAddress, managementPort);
+    }
+
     /**
      * Setup the TemplateOptions to create the user.
      */
@@ -2268,11 +2294,13 @@ public JcloudsMachineLocation registerMachine(ConfigBag setup) throws NoMachines
 
     protected JcloudsMachineLocation registerMachineLocation(ConfigBag setup, NodeMetadata node) {
         ComputeService computeService = getComputeService(setup);
+        String vmHostname = getPublicHostname(node, Optional.<HostAndPort>absent(), Suppliers.<LoginCredentials>ofInstance(null), setup);
+        HostAndPort managementHostAndPort = resolveManagementHostAndPort(computeService, node, vmHostname, Optional.<HostAndPort>absent(), setup, true);
         if (isWindows(node, setup)) {
-            return registerWinRmMachineLocation(computeService, node, null, Optional.<HostAndPort>absent(), setup);
+            return registerWinRmMachineLocation(computeService, node, managementHostAndPort, Optional.<HostAndPort>absent(), setup);
         } else {
             try {
-                return registerJcloudsSshMachineLocation(computeService, node, Optional.<Template>absent(), null, Optional.<HostAndPort>absent(), setup);
+                return registerJcloudsSshMachineLocation(computeService, node, Optional.<Template>absent(), node.getCredentials(), managementHostAndPort, Optional.<HostAndPort>absent(), vmHostname, setup);
             } catch (IOException e) {
                 throw Exceptions.propagate(e);
             }
@@ -2401,31 +2429,9 @@ public String toString() {
         }
     }
 
-    // -------------- create the SshMachineLocation instance, and connect to it etc ------------------------
-
-    /** @deprecated since 0.7.0 use {@link #registerJcloudsSshMachineLocation(ComputeService, NodeMetadata, LoginCredentials, Optional, ConfigBag)} */
-    @Deprecated
-    protected final JcloudsSshMachineLocation registerJcloudsSshMachineLocation(NodeMetadata node, String vmHostname, Optional<HostAndPort> sshHostAndPort, ConfigBag setup) throws IOException {
-        LOG.warn("Using deprecated registerJcloudsSshMachineLocation: now wants computeService passed", new Throwable("source of deprecated registerJcloudsSshMachineLocation invocation"));
-        return registerJcloudsSshMachineLocation(null, node, Optional.<Template>absent(), null, sshHostAndPort, setup);
-    }
-
-    /**
-     * @deprecated since 0.9.0; see {@link #registerJcloudsSshMachineLocation(ComputeService, NodeMetadata, Optional, LoginCredentials, Optional, ConfigBag)}.
-     *             Marked as final to warn those trying to sub-class. 
-     */
-    @Deprecated
-    protected final JcloudsSshMachineLocation registerJcloudsSshMachineLocation(ComputeService computeService, NodeMetadata node, LoginCredentials userCredentials, Optional<HostAndPort> sshHostAndPort, ConfigBag setup) throws IOException {
-        return registerJcloudsSshMachineLocation(computeService, node, Optional.<Template>absent(), userCredentials, sshHostAndPort, setup);
-    }
-    
-    protected JcloudsSshMachineLocation registerJcloudsSshMachineLocation(ComputeService computeService, NodeMetadata node, Optional<Template> template, LoginCredentials userCredentials, Optional<HostAndPort> sshHostAndPort, ConfigBag setup) throws IOException {
-        if (userCredentials == null)
-            userCredentials = node.getCredentials();
-
-        String vmHostname = getPublicHostname(node, sshHostAndPort, userCredentials, setup);
-
-        JcloudsSshMachineLocation machine = createJcloudsSshMachineLocation(computeService, node, template, vmHostname, sshHostAndPort, userCredentials, setup);
+    protected JcloudsSshMachineLocation registerJcloudsSshMachineLocation(ComputeService computeService, NodeMetadata node, Optional<Template> template, LoginCredentials credentials,
+                                                                          HostAndPort managementHostAndPort, Optional<HostAndPort> sshHostAndPort, String vmHostname, ConfigBag setup) throws IOException {
+        JcloudsSshMachineLocation machine = createJcloudsSshMachineLocation(computeService, node, template, vmHostname, sshHostAndPort, credentials, managementHostAndPort, setup);
         registerJcloudsMachineLocation(node.getId(), machine);
         return machine;
     }
@@ -2436,16 +2442,8 @@ protected void registerJcloudsMachineLocation(String nodeId, JcloudsMachineLocat
         vmInstanceIds.put(machine, nodeId);
     }
 
-    /**
-     * @deprecated since 0.9.0; see {@link #createJcloudsSshMachineLocation(ComputeService, NodeMetadata, Optional, String, Optional, LoginCredentials, ConfigBag)}.
-     *             Marked as final to warn those trying to sub-class. 
-     */
-    @Deprecated
-    protected final JcloudsSshMachineLocation createJcloudsSshMachineLocation(ComputeService computeService, NodeMetadata node, String vmHostname, Optional<HostAndPort> sshHostAndPort, LoginCredentials userCredentials, ConfigBag setup) throws IOException {
-        return createJcloudsSshMachineLocation(computeService, node, Optional.<Template>absent(), vmHostname, sshHostAndPort, userCredentials, setup);
-    }
-    
-    protected JcloudsSshMachineLocation createJcloudsSshMachineLocation(ComputeService computeService, NodeMetadata node, Optional<Template> template, String vmHostname, Optional<HostAndPort> sshHostAndPort, LoginCredentials userCredentials, ConfigBag setup) throws IOException {
+    protected JcloudsSshMachineLocation createJcloudsSshMachineLocation(ComputeService computeService, NodeMetadata node, Optional<Template> template, String vmHostname, Optional<HostAndPort> sshHostAndPort,
+                                                                        LoginCredentials userCredentials, HostAndPort managementHostAndPort, ConfigBag setup) throws IOException {
         Map<?,?> sshConfig = extractSshConfig(setup, node);
         String nodeAvailabilityZone = extractAvailabilityZone(setup, node);
         String nodeRegion = extractRegion(setup, node);
@@ -2454,32 +2452,19 @@ protected JcloudsSshMachineLocation createJcloudsSshMachineLocation(ComputeServi
             nodeRegion = extractProvider(setup, node);
         }
 
-        String address = sshHostAndPort.isPresent() ? sshHostAndPort.get().getHostText() : vmHostname;
-        try {
-            Networking.getInetAddressWithFixedName(address);
-            // fine, it resolves
-        } catch (Exception e) {
-            // occurs if an unresolvable hostname is given as vmHostname, and the machine only has private IP addresses but they are reachable
-            // TODO cleanup use of getPublicHostname so its semantics are clearer, returning reachable hostname or ip, and
-            // do this check/fix there instead of here!
-            Exceptions.propagateIfFatal(e);
-            LOG.debug("Could not resolve reported address '"+address+"' for "+vmHostname+" ("+setup.getDescription()+"/"+node+"), requesting reachable address");
-            if (computeService==null) throw Exceptions.propagate(e);
-            // this has sometimes already been done in waitForReachable (unless skipped) but easy enough to do again
-            address = getFirstReachableAddress(node, setup);
-        }
-
         if (LOG.isDebugEnabled())
             LOG.debug("creating JcloudsSshMachineLocation representation for {}@{} ({}/{}) for {}/{}",
                     new Object[] {
                             getUser(setup),
-                            address,
+                            managementHostAndPort,
                             Sanitizer.sanitize(sshConfig),
                             sshHostAndPort,
                             setup.getDescription(),
                             node
                     });
 
+        String address = managementHostAndPort.getHostText();
+
         if (isManaged()) {
             return getManagementContext().getLocationManager().createLocation(LocationSpec.create(JcloudsSshMachineLocation.class)
                     .configure(sshConfig)
@@ -2530,18 +2515,16 @@ protected JcloudsSshMachineLocation createJcloudsSshMachineLocation(ComputeServi
         }
     }
 
-    protected JcloudsWinRmMachineLocation registerWinRmMachineLocation(ComputeService computeService, NodeMetadata node, LoginCredentials initialCredentials, Optional<HostAndPort> sshHostAndPort, ConfigBag setup) {
-        if (initialCredentials==null)
-            initialCredentials = node.getCredentials();
-
+    protected JcloudsWinRmMachineLocation registerWinRmMachineLocation(ComputeService computeService, NodeMetadata node, HostAndPort managementHostAndPort,
+                                                                       Optional<HostAndPort> sshHostAndPort, ConfigBag setup) {
         String vmHostname = getPublicHostname(node, sshHostAndPort, setup);
 
-        JcloudsWinRmMachineLocation machine = createWinRmMachineLocation(computeService, node, vmHostname, sshHostAndPort, setup);
+        JcloudsWinRmMachineLocation machine = createWinRmMachineLocation(computeService, node, vmHostname, managementHostAndPort, setup);
         registerJcloudsMachineLocation(node.getId(), machine);
         return machine;
     }
 
-    protected JcloudsWinRmMachineLocation createWinRmMachineLocation(ComputeService computeService, NodeMetadata node, String vmHostname, Optional<HostAndPort> sshHostAndPort, ConfigBag setup) {
+    protected JcloudsWinRmMachineLocation createWinRmMachineLocation(ComputeService computeService, NodeMetadata node, String vmHostname, HostAndPort sshHostAndPort, ConfigBag setup) {
         Map<?,?> winrmConfig = extractWinrmConfig(setup, node);
         String nodeAvailabilityZone = extractAvailabilityZone(setup, node);
         String nodeRegion = extractRegion(setup, node);
@@ -2550,15 +2533,13 @@ protected JcloudsWinRmMachineLocation createWinRmMachineLocation(ComputeService
             nodeRegion = extractProvider(setup, node);
         }
         
-        String address = sshHostAndPort.isPresent() ? sshHostAndPort.get().getHostText() : vmHostname;
-
         if (isManaged()) {
             return getManagementContext().getLocationManager().createLocation(LocationSpec.create(JcloudsWinRmMachineLocation.class)
                     .configure(winrmConfig)
                     .configure("jcloudsParent", this)
                     .configure("displayName", vmHostname)
-                    .configure("address", address)
-                    .configure(WinRmMachineLocation.WINRM_CONFIG_PORT, sshHostAndPort.isPresent() ? sshHostAndPort.get().getPort() : node.getLoginPort())
+                    .configure("address", sshHostAndPort.getHostText())
+                    .configure(WinRmMachineLocation.WINRM_CONFIG_PORT, sshHostAndPort.getPort())
                     .configure("user", getUser(setup))
                     .configure(WinRmMachineLocation.USER, setup.get(USER))
                     .configure("node", node)
@@ -2935,11 +2916,7 @@ protected String getFirstReachableAddress(NodeMetadata node, ConfigBag setup) {
         return result;
     }
 
-    protected LoginCredentials waitForWinRmAvailable(final ComputeService computeService, final NodeMetadata node, Optional<HostAndPort> hostAndPortOverride, ConfigBag setup) {
-        return waitForWinRmAvailable(computeService, node, hostAndPortOverride, ImmutableList.of(node.getCredentials()), setup);
-    }
-    
-    protected LoginCredentials waitForWinRmAvailable(final ComputeService computeService, final NodeMetadata node, Optional<HostAndPort> hostAndPortOverride, List<LoginCredentials> credentialsToTry, ConfigBag setup) {
+    protected LoginCredentials waitForWinRmAvailable(LoginCredentials credentialsToTry, final HostAndPort managementHostAndPort, ConfigBag setup) {
         String waitForWinrmAvailable = setup.get(WAIT_FOR_WINRM_AVAILABLE);
         checkArgument(!"false".equalsIgnoreCase(waitForWinrmAvailable), "waitForWinRmAvailable called despite waitForWinRmAvailable=%s", waitForWinrmAvailable);
         Duration timeout = null;
@@ -2953,19 +2930,10 @@ protected LoginCredentials waitForWinRmAvailable(final ComputeService computeSer
         if (timeout == null) {
             timeout = Duration.parse(WAIT_FOR_WINRM_AVAILABLE.getDefaultValue());
         }
-        
-        Set<String> users = Sets.newLinkedHashSet();
-        for (LoginCredentials creds : credentialsToTry) {
-            users.add(creds.getUser());
-        }
-        String user = (users.size() == 1) ? Iterables.getOnlyElement(users) : "{" + Joiner.on(",").join(users) + "}";
-        String vmIp = hostAndPortOverride.isPresent() ? hostAndPortOverride.get().getHostText() : getFirstReachableAddress(node, setup);
-        if (vmIp==null) LOG.warn("Unable to extract IP for "+node+" ("+setup.getDescription()+"): subsequent connection attempt will likely fail");
-        int defaultWinRmPort = getConfig(WinRmMachineLocation.USE_HTTPS_WINRM) ? 5986 : 5985;
-        int vmPort = hostAndPortOverride.isPresent() ? hostAndPortOverride.get().getPortOrDefault(defaultWinRmPort) : defaultWinRmPort;
 
-        String connectionDetails = user + "@" + vmIp + ":" + vmPort;
-        final HostAndPort hostAndPort = hostAndPortOverride.isPresent() ? hostAndPortOverride.get() : HostAndPort.fromParts(vmIp, vmPort);
+        String user = credentialsToTry.getUser();
+
+        String connectionDetails = user + "@" + managementHostAndPort;
         final AtomicReference<LoginCredentials> credsSuccessful = new AtomicReference<LoginCredentials>();
 
         // Don't use config that relates to the final user credentials (those have nothing to do
@@ -2973,21 +2941,18 @@ protected LoginCredentials waitForWinRmAvailable(final ComputeService computeSer
         // The createTemporaryWinRmMachineLocation deals with removing that.
         ConfigBag winrmProps = ConfigBag.newInstanceCopying(setup);
 
-        final Map<WinRmMachineLocation, LoginCredentials> machinesToTry = Maps.newLinkedHashMap();
-        for (LoginCredentials creds : credentialsToTry) {
-            machinesToTry.put(createTemporaryWinRmMachineLocation(hostAndPort, creds, winrmProps), creds);
-        }
+        final Pair<WinRmMachineLocation, LoginCredentials> machinesToTry = Pair.of(createTemporaryWinRmMachineLocation(managementHostAndPort, credentialsToTry, winrmProps), credentialsToTry);
+
         try {
             Callable<Boolean> checker = new Callable<Boolean>() {
                 public Boolean call() {
-                    for (Map.Entry<WinRmMachineLocation, LoginCredentials> entry : machinesToTry.entrySet()) {
-                        final WinRmMachineLocation machine = entry.getKey();
-                        WinRmToolResponse response = machine.executeCommand(
-                                ImmutableMap.of(WinRmTool.PROP_EXEC_TRIES.getName(), 1),
-                                ImmutableList.of("echo testing"));
-                        boolean success = (response.getStatusCode() == 0);
-                        if (success) {
-                            credsSuccessful.set(entry.getValue());
+                    final WinRmMachineLocation machine = machinesToTry.getLeft();
+                    WinRmToolResponse response = machine.executeCommand(
+                            ImmutableMap.of(WinRmTool.PROP_EXEC_TRIES.getName(), 1),
+                            ImmutableList.of("echo testing"));
+                    boolean success = (response.getStatusCode() == 0);
+                    if (success) {
+                            credsSuccessful.set(machinesToTry.getRight());
 
                             String verifyWindowsUp = getConfig(WinRmMachineLocation.WAIT_WINDOWS_TO_START);
                             if (Strings.isBlank(verifyWindowsUp) || verifyWindowsUp.equals("false")) {
@@ -3027,25 +2992,21 @@ public boolean apply(@Nullable WinRmMachineLocation machine) {
                             } else {
                                 return true;
                             }
-                        }
                     }
                     return false;
                 }};
 
-            waitForReachable(checker, connectionDetails, credentialsToTry, setup, timeout);
+            waitForReachable(checker, connectionDetails, ImmutableList.of(credentialsToTry), setup, timeout);
         } finally {
-            for (WinRmMachineLocation machine : machinesToTry.keySet()) {
-                if (getManagementContext().getLocationManager().isManaged(machine)) {
-                    // get benign but unpleasant warnings if we unmanage something already unmanaged
-                    getManagementContext().getLocationManager().unmanage(machine);
-                }
+            if (getManagementContext().getLocationManager().isManaged(machinesToTry.getLeft())) {
+                // get benign but unpleasant warnings if we unmanage something already unmanaged
+                getManagementContext().getLocationManager().unmanage(machinesToTry.getLeft());
             }
         }
-
         return credsSuccessful.get();
     }
 
-    protected LoginCredentials waitForSshable(final ComputeService computeService, final NodeMetadata node, Optional<HostAndPort> hostAndPortOverride, ConfigBag setup) {
+    protected LoginCredentials waitForSshable(final ComputeService computeService, final NodeMetadata node, HostAndPort managementHostAndPort, ConfigBag setup) {
         LoginCredentials nodeCreds = node.getCredentials();
         String nodeUser = nodeCreds.getUser();
         String loginUserOverride = setup.get(LOGIN_USER);
@@ -3071,11 +3032,11 @@ protected LoginCredentials waitForSshable(final ComputeService computeService, f
                 credentialsToTry.add(LoginCredentials.builder(nodeCreds).user(user).build());
             }
         }
-        
-        return waitForSshable(computeService, node, hostAndPortOverride, credentialsToTry, setup);
+
+        return waitForSshable(computeService, node, managementHostAndPort, credentialsToTry, setup);
     }
-    
-    protected LoginCredentials waitForSshable(final ComputeService computeService, final NodeMetadata node, Optional<HostAndPort> hostAndPortOverride, List<LoginCredentials> credentialsToTry, ConfigBag setup) {
+
+    protected LoginCredentials waitForSshable(final ComputeService computeService, final NodeMetadata node, HostAndPort hostAndPort, List<LoginCredentials> credentialsToTry, ConfigBag setup) {
         String waitForSshable = setup.get(WAIT_FOR_SSHABLE);
         checkArgument(!"false".equalsIgnoreCase(waitForSshable), "waitForReachable called despite waitForSshable=%s for %s", waitForSshable, node);
         checkArgument(credentialsToTry.size() > 0, "waitForReachable called without credentials for %s", node);
@@ -3089,21 +3050,17 @@ protected LoginCredentials waitForSshable(final ComputeService computeService, f
         if (timeout == null) {
             timeout = Duration.parse(WAIT_FOR_SSHABLE.getDefaultValue());
         }
-        
+
         Set<String> users = Sets.newLinkedHashSet();
         for (LoginCredentials creds : credentialsToTry) {
             users.add(creds.getUser());
         }
         String user = (users.size() == 1) ? Iterables.getOnlyElement(users) : "{" + Joiner.on(",").join(users) + "}";
-        String vmIp = hostAndPortOverride.isPresent() ? hostAndPortOverride.get().getHostText() : getFirstReachableAddress(node, setup);
-        if (vmIp==null) LOG.warn("Unable to extract IP for "+node+" ("+setup.getDescription()+"): subsequent connection attempt will likely fail");
-        int vmPort = hostAndPortOverride.isPresent() ? hostAndPortOverride.get().getPortOrDefault(22) : getLoginPortOrDefault(node, 22);
 
-        String connectionDetails = user + "@" + vmIp + ":" + vmPort;
-        final HostAndPort hostAndPort = hostAndPortOverride.isPresent() ? hostAndPortOverride.get() : HostAndPort.fromParts(vmIp, vmPort);
+        String connectionDetails = user + "@" + hostAndPort;
         final AtomicReference<LoginCredentials> credsSuccessful = new AtomicReference<LoginCredentials>();
 
-        // Don't use config that relates to the final user credentials (those have nothing to do 
+        // Don't use config that relates to the final user credentials (those have nothing to do
         // with the initial credentials of the VM returned by the cloud provider).
         ConfigBag sshProps = ConfigBag.newInstanceCopying(setup);
         sshProps.remove("password");
@@ -3123,8 +3080,8 @@ public Boolean call() {
                         int exitstatus = machine.execScript(
                                 ImmutableMap.of(
                                         SshTool.PROP_SSH_TRIES_TIMEOUT.getName(), Duration.THIRTY_SECONDS.toMilliseconds(),
-                                        SshTool.PROP_SSH_TRIES.getName(), 1), 
-                                "check-connectivity", 
+                                        SshTool.PROP_SSH_TRIES.getName(), 1),
+                                "check-connectivity",
                                 ImmutableList.of("true"));
                         boolean success = (exitstatus == 0);
                         if (success) {
@@ -3134,7 +3091,7 @@ public Boolean call() {
                     }
                     return false;
                 }};
-    
+
             waitForReachable(checker, connectionDetails, credentialsToTry, setup, timeout);
         } finally {
             for (SshMachineLocation machine : machinesToTry.keySet()) {
@@ -3145,7 +3102,7 @@ public Boolean call() {
                 Streams.closeQuietly(machine);
             }
         }
-        
+
         return credsSuccessful.get();
     }
 
diff --git a/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsUtil.java b/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsUtil.java
index 9d7d2e3fa3..7b75bcc8e8 100644
--- a/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsUtil.java
+++ b/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsUtil.java
@@ -375,7 +375,7 @@ public static String getFirstReachableAddress(NodeMetadata node, Duration timeou
             return result.getHostText();
         } catch (Exception e) {
             Exceptions.propagateIfFatal(e);
-            throw new IllegalStateException("Unable to connect SshClient to "+node+"; check that the node is accessible and that the SSH key exists and is correctly configured, including any passphrase defined", e);
+            throw new IllegalStateException("Unable to choose IP for "+node+"; check whether the node is reachable or whether it meets requirements of the HostAndPort tester: " + socketTester , e);
         } finally {
             executor.shutdownNow();
         }

From 4feaf810f2616ee832e28192fab9abb213841e48 Mon Sep 17 00:00:00 2001
From: Valentin Aitken <bostko@gmail.com>
Date: Tue, 4 Oct 2016 20:09:44 +0300
Subject: [PATCH 2/3] pollForFirstReachableEnabled only if
 WAIT_FOR_WINRM_AVAILABLE or WAIT_FOR_SSHABLE

---
 .../location/jclouds/JcloudsLocation.java         | 15 ++++++++-------
 .../JcloudsReachableAddressStubbedTest.java       |  8 ++++----
 2 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java b/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java
index f521d8130c..422dc66509 100644
--- a/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java
+++ b/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java
@@ -774,9 +774,9 @@ protected MachineLocation obtainOnce(ConfigBag setup) throws NoMachinesAvailable
 
             String vmHostname = getPublicHostname(node, sshHostAndPortOverride, userCredentials, setup);
             LoginCredentials initialCredentials = node.getCredentials();
-            final HostAndPort managementHostAndPort = resolveManagementHostAndPort(computeService, node, vmHostname, sshHostAndPortOverride, setup);
+            boolean waitForConnectable = (windows) ? waitForWinRmable : waitForSshable;
+            final HostAndPort managementHostAndPort = resolveManagementHostAndPort(computeService, node, vmHostname, sshHostAndPortOverride, waitForConnectable, setup);
             if (skipJcloudsSshing) {
-                boolean waitForConnectable = (windows) ? waitForWinRmable : waitForSshable;
                 if (waitForConnectable) {
                     if (windows) {
                         // TODO Does jclouds support any windows user setup?
@@ -1955,13 +1955,13 @@ protected LoginCredentials createUser(ComputeService computeService, NodeMetadat
         return userCreation.createdUserCredentials;
     }
 
-    protected HostAndPort resolveManagementHostAndPort(ComputeService computeService, NodeMetadata node, String vmHostname, Optional<HostAndPort> hostAndPortOverride, ConfigBag config) {
-        return resolveManagementHostAndPort(computeService, node, vmHostname, hostAndPortOverride, config, false);
+    protected HostAndPort resolveManagementHostAndPort(ComputeService computeService, NodeMetadata node, String vmHostname, Optional<HostAndPort> hostAndPortOverride, boolean waitForMakingManagementConnection, ConfigBag config) {
+        return resolveManagementHostAndPort(computeService, node, vmHostname, hostAndPortOverride, config, waitForMakingManagementConnection, false);
     }
 
-    protected HostAndPort resolveManagementHostAndPort(ComputeService computeService, NodeMetadata node, String vmHostname, Optional<HostAndPort> hostAndPortOverride, ConfigBag config, boolean useOnlyVmHostNameAndHostAndPortOverride) {
+    protected HostAndPort resolveManagementHostAndPort(ComputeService computeService, NodeMetadata node, String vmHostname, Optional<HostAndPort> hostAndPortOverride, ConfigBag config, boolean waitForMakingManagementConnection, boolean useOnlyVmHostNameAndHostAndPortOverride) {
         String pollForFirstReachable = config.get(POLL_FOR_FIRST_REACHABLE_ADDRESS);
-        boolean pollForFirstReachableEnabled = !useOnlyVmHostNameAndHostAndPortOverride && !"false".equalsIgnoreCase(pollForFirstReachable);
+        boolean pollForFirstReachableEnabled = !useOnlyVmHostNameAndHostAndPortOverride && !"false".equalsIgnoreCase(pollForFirstReachable) && waitForMakingManagementConnection;
 
         String managementAddress = hostAndPortOverride.isPresent() ? hostAndPortOverride.get().getHostText() :
                 (pollForFirstReachableEnabled ? getFirstReachableAddress(node, config) : vmHostname);
@@ -2295,7 +2295,8 @@ public JcloudsMachineLocation registerMachine(ConfigBag setup) throws NoMachines
     protected JcloudsMachineLocation registerMachineLocation(ConfigBag setup, NodeMetadata node) {
         ComputeService computeService = getComputeService(setup);
         String vmHostname = getPublicHostname(node, Optional.<HostAndPort>absent(), Suppliers.<LoginCredentials>ofInstance(null), setup);
-        HostAndPort managementHostAndPort = resolveManagementHostAndPort(computeService, node, vmHostname, Optional.<HostAndPort>absent(), setup, true);
+        boolean waitForMakingManagementConnection = isWindows(node, setup) ? !"false".equalsIgnoreCase(setup.get(WAIT_FOR_WINRM_AVAILABLE)) : !"false".equalsIgnoreCase(setup.get(WAIT_FOR_SSHABLE));
+        HostAndPort managementHostAndPort = resolveManagementHostAndPort(computeService, node, vmHostname, Optional.<HostAndPort>absent(), setup, waitForMakingManagementConnection, true);
         if (isWindows(node, setup)) {
             return registerWinRmMachineLocation(computeService, node, managementHostAndPort, Optional.<HostAndPort>absent(), setup);
         } else {
diff --git a/locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/networking/JcloudsReachableAddressStubbedTest.java b/locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/networking/JcloudsReachableAddressStubbedTest.java
index 2ba966bc6c..8f78d9443b 100644
--- a/locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/networking/JcloudsReachableAddressStubbedTest.java
+++ b/locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/networking/JcloudsReachableAddressStubbedTest.java
@@ -183,7 +183,7 @@ public void testMachineUsesVanillaPublicAddress() throws Exception {
      * Only one public and one private; private is reachable;
      * With waitForSshable=true; pollForFirstReachableAddress=true; and custom reachable-predicate
      */
-    @Test(enabled = false, groups = "WIP")
+    @Test
     public void testMachineUsesVanillaPrivateAddress() throws Exception {
         initNodeCreatorAndJcloudsLocation(ImmutableList.of("1.1.1.1"), ImmutableList.of("2.1.1.1"), ImmutableMap.of());
         reachableIp = "2.1.1.1";
@@ -197,7 +197,7 @@ public void testMachineUsesVanillaPrivateAddress() throws Exception {
         assertEquals(RecordingSshTool.getLastExecCmd().constructorProps.get(SshTool.PROP_HOST.getName()), reachableIp);
 
         assertEquals(machine.getAddress().getHostAddress(), reachableIp);
-        assertEquals(machine.getHostname(), reachableIp);
+        assertEquals(machine.getHostname(), "1.1.1.1");
         assertEquals(machine.getSubnetIp(), reachableIp);
     }
 
@@ -205,7 +205,7 @@ public void testMachineUsesVanillaPrivateAddress() throws Exception {
      * Multiple public addresses; chooses the reachable one;
      * With waitForSshable=true; pollForFirstReachableAddress=true; and custom reachable-predicate
      */
-    @Test(enabled = false, groups = "WIP")
+    @Test
     public void testMachineUsesReachablePublicAddress() throws Exception {
         initNodeCreatorAndJcloudsLocation(ImmutableList.of("1.1.1.1", "1.1.1.2", "1.1.1.2"), ImmutableList.of("2.1.1.1"), ImmutableMap.of());
         reachableIp = "1.1.1.2";
@@ -219,7 +219,7 @@ public void testMachineUsesReachablePublicAddress() throws Exception {
         assertEquals(RecordingSshTool.getLastExecCmd().constructorProps.get(SshTool.PROP_HOST.getName()), reachableIp);
 
         assertEquals(machine.getAddress().getHostAddress(), reachableIp);
-        assertEquals(machine.getHostname(), reachableIp);
+        assertEquals(machine.getHostname(), "1.1.1.1");
         assertEquals(machine.getSubnetIp(), "2.1.1.1");
     }
 

From 401e4bb2504440712c368a8728d98122e8c32ea6 Mon Sep 17 00:00:00 2001
From: Aled Sage <aled.sage@gmail.com>
Date: Tue, 4 Oct 2016 22:37:55 +0100
Subject: [PATCH 3/3] Fixing/testing JcloudsLocation choice of address

---
 locations/jclouds/pom.xml                     |   7 +
 .../location/jclouds/JcloudsLocation.java     | 236 ++++++++++++++----
 ...ByonLocationResolverSoftlayerLiveTest.java |   8 +-
 .../JcloudsReachableAddressStubbedTest.java   |  90 +++++--
 .../internal/winrm/RecordingWinRmTool.java    |  14 +-
 5 files changed, 277 insertions(+), 78 deletions(-)

diff --git a/locations/jclouds/pom.xml b/locations/jclouds/pom.xml
index 9bf5021be6..3984a0ccdc 100644
--- a/locations/jclouds/pom.xml
+++ b/locations/jclouds/pom.xml
@@ -201,6 +201,13 @@
             <classifier>tests</classifier>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>org.apache.brooklyn</groupId>
+            <artifactId>brooklyn-software-winrm</artifactId>
+            <version>${project.version}</version>
+            <classifier>tests</classifier>
+            <scope>test</scope>
+        </dependency>
         <dependency>
             <groupId>org.mockito</groupId>
             <artifactId>mockito-all</artifactId>
diff --git a/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java b/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java
index 422dc66509..1429a54d12 100644
--- a/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java
+++ b/locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java
@@ -747,6 +747,8 @@ protected MachineLocation obtainOnce(ConfigBag setup) throws NoMachinesAvailable
                 throw new IllegalStateException("No nodes returned by jclouds create-nodes in " + setup.getDescription());
 
             boolean windows = isWindows(node, setup);
+            boolean waitForConnectable = (windows) ? waitForWinRmable : waitForSshable;
+            
             if (windows) {
                 int newLoginPort = node.getLoginPort() == 22 ? (getConfig(WinRmMachineLocation.USE_HTTPS_WINRM) ? 5986 : 5985) : node.getLoginPort();
                 String newLoginUser = "root".equals(node.getCredentials().getUser()) ? "Administrator" : node.getCredentials().getUser();
@@ -772,10 +774,16 @@ protected MachineLocation obtainOnce(ConfigBag setup) throws NoMachinesAvailable
                 sshHostAndPortOverride = Optional.absent();
             }
 
-            String vmHostname = getPublicHostname(node, sshHostAndPortOverride, userCredentials, setup);
             LoginCredentials initialCredentials = node.getCredentials();
-            boolean waitForConnectable = (windows) ? waitForWinRmable : waitForSshable;
-            final HostAndPort managementHostAndPort = resolveManagementHostAndPort(computeService, node, vmHostname, sshHostAndPortOverride, waitForConnectable, setup);
+            
+            final HostAndPort managementHostAndPort = resolveManagementHostAndPort(
+                    computeService, node, Optional.fromNullable(userCredentials), sshHostAndPortOverride, setup,
+                    new ResolveOptions()
+                            .windows(windows)
+                            .expectConnectable(waitForConnectable)
+                            .pollForFirstReachableAddress(!"false".equalsIgnoreCase(setup.get(POLL_FOR_FIRST_REACHABLE_ADDRESS)))
+                            .propagatePollForReachableFailure(true));
+            
             if (skipJcloudsSshing) {
                 if (waitForConnectable) {
                     if (windows) {
@@ -832,9 +840,9 @@ protected MachineLocation obtainOnce(ConfigBag setup) throws NoMachinesAvailable
 
             // Create a JcloudsSshMachineLocation, and register it
             if (windows) {
-                machineLocation = registerWinRmMachineLocation(computeService, node, managementHostAndPort, sshHostAndPortOverride, setup);
+                machineLocation = registerWinRmMachineLocation(computeService, node, managementHostAndPort, setup);
             } else {
-                machineLocation = registerJcloudsSshMachineLocation(computeService, node, Optional.fromNullable(template), userCredentials, managementHostAndPort, sshHostAndPortOverride, vmHostname, setup);
+                machineLocation = registerJcloudsSshMachineLocation(computeService, node, Optional.fromNullable(template), userCredentials, managementHostAndPort, setup);
             }
 
             PortForwardManager portForwardManager = setup.get(PORT_FORWARDING_MANAGER);
@@ -1955,31 +1963,102 @@ protected LoginCredentials createUser(ComputeService computeService, NodeMetadat
         return userCreation.createdUserCredentials;
     }
 
-    protected HostAndPort resolveManagementHostAndPort(ComputeService computeService, NodeMetadata node, String vmHostname, Optional<HostAndPort> hostAndPortOverride, boolean waitForMakingManagementConnection, ConfigBag config) {
-        return resolveManagementHostAndPort(computeService, node, vmHostname, hostAndPortOverride, config, waitForMakingManagementConnection, false);
+    private static class ResolveOptions {
+        boolean pollForFirstReachableAddress;
+        boolean expectConnectable;
+        boolean isWindows;
+        boolean propagatePollForReachableFailure;
+        
+        ResolveOptions pollForFirstReachableAddress(boolean val) {
+            pollForFirstReachableAddress = val;
+            return this;
+        }
+        ResolveOptions expectConnectable(boolean val) {
+            expectConnectable = val;
+            return this;
+        }
+        ResolveOptions windows(boolean val) {
+            isWindows = val;
+            return this;
+        }
+        public ResolveOptions propagatePollForReachableFailure(boolean val) {
+            this.propagatePollForReachableFailure = val;
+            return this;
+        }
     }
+    
+    /**
+     * Infers the hostAndPort to use for subsequent creation of the  
+     * {@link JcloudsSshMachineLocation} or {@link JcloudsWinRmMachineLocation}.
+     * This is expected to be the login host:port, for connecting to the VM
+     * via ssh or WinRM.
+     * 
+     * However, some VMs provisioned will not be sshable or reachable at all. 
+     * In such cases, this method will likely return the first address returned by 
+     * jclouds.
+     * 
+     * For AWS, if we are allowed to SSH to the VM to find out its DNS name, then we'll
+     * return that fully qualified name (which we expect to be reachable from inside
+     * and outside the AWS region).
+     */
+    private HostAndPort resolveManagementHostAndPort(ComputeService computeService, NodeMetadata node, Optional<LoginCredentials> userCredentials, Optional<HostAndPort> hostAndPortOverride, ConfigBag config, ResolveOptions options) {
+        Boolean lookupAwsHostname = config.get(LOOKUP_AWS_HOSTNAME);
+        String provider = config.get(CLOUD_PROVIDER);
+        if (provider == null) provider= getProvider();
+        int defaultPort;
+        if (options.isWindows) {
+            defaultPort = config.get(WinRmMachineLocation.USE_HTTPS_WINRM) ? 5986 : 5985;
+        } else {
+            defaultPort = node.getLoginPort();
+        }
 
-    protected HostAndPort resolveManagementHostAndPort(ComputeService computeService, NodeMetadata node, String vmHostname, Optional<HostAndPort> hostAndPortOverride, ConfigBag config, boolean waitForMakingManagementConnection, boolean useOnlyVmHostNameAndHostAndPortOverride) {
-        String pollForFirstReachable = config.get(POLL_FOR_FIRST_REACHABLE_ADDRESS);
-        boolean pollForFirstReachableEnabled = !useOnlyVmHostNameAndHostAndPortOverride && !"false".equalsIgnoreCase(pollForFirstReachable) && waitForMakingManagementConnection;
+        if (hostAndPortOverride.isPresent()) {
+            // Don't try to resolve it; just use it
+            int port = hostAndPortOverride.get().hasPort() ? hostAndPortOverride.get().getPort() : defaultPort;
+            return HostAndPort.fromParts(hostAndPortOverride.get().getHostText(), port);
+        }
+        if (options.expectConnectable && userCredentials.isPresent() && "aws-ec2".equals(provider) && Boolean.TRUE.equals(lookupAwsHostname)) {
+            // Treat AWS as a special case because the DNS fully qualified hostname in AWS is 
+            // (normally?!) a good way to refer to the VM from both inside and outside of the 
+            // region.
+            Maybe<String> result = getHostnameAws(node, hostAndPortOverride, Suppliers.ofInstance(userCredentials.get()), config);
+            if (result.isPresent()) {
+                return HostAndPort.fromParts(result.get(), defaultPort);
+            }
+        }
+        if (options.expectConnectable && options.pollForFirstReachableAddress) {
+            try {
+                String firstReachableAddress = getFirstReachableAddress(node, config);
+                return HostAndPort.fromParts(firstReachableAddress, defaultPort);
+            } catch (RuntimeException e) {
+                if (options.propagatePollForReachableFailure) {
+                    throw Exceptions.propagate(e);
+                } else {
+                    LOG.warn("No reachable address ("+config.getDescription()+"/"+node+"); falling back to any advertised address; may cause future failures");
+                }
+            }
+        }
 
-        String managementAddress = hostAndPortOverride.isPresent() ? hostAndPortOverride.get().getHostText() :
-                (pollForFirstReachableEnabled ? getFirstReachableAddress(node, config) : vmHostname);
-        int managementPort = hostAndPortOverride.isPresent() ? hostAndPortOverride.get().getPort() : node.getLoginPort();
+        Iterable<String> addresses = Iterables.concat(node.getPublicAddresses(), node.getPrivateAddresses());
+        for (String address : addresses) {
+            if (isAddressResolvable(address)) {
+                return HostAndPort.fromParts(address, defaultPort);
+            }
+        }
+        
+        LOG.warn("No resolvable address in "+addresses+" ("+config.getDescription()+"/"+node+"); using first; may cause future failures");
+        String host = Iterables.get(addresses, 0);
+        return HostAndPort.fromParts(host, defaultPort);
+    }
+
+    private boolean isAddressResolvable(String addr) {
         try {
-            Networking.getInetAddressWithFixedName(managementAddress);
-            // fine, it resolves
-        } catch (Exception e) {
-            // occurs if an unresolvable hostname is given as vmHostname, and the machine only has private IP addresses but they are reachable
-            // TODO cleanup use of getPublicHostname so its semantics are clearer, returning reachable hostname or ip, and
-            // do this check/fix there instead of here!
+            Networking.getInetAddressWithFixedName(addr);
+            return true; // fine, it resolves
+        } catch (RuntimeException e) {
             Exceptions.propagateIfFatal(e);
-            LOG.debug("Could not resolve reported address '"+managementAddress+"' for "+vmHostname+" ("+config.getDescription()+"/"+node+"), requesting reachable address");
-            if (computeService==null) throw Exceptions.propagate(e);
-            // this has sometimes already been done in waitForReachable (unless skipped) but easy enough to do again
-            managementAddress = getFirstReachableAddress(node, config);
+            return false;
         }
-        return hostAndPortOverride.isPresent() ? hostAndPortOverride.get() : HostAndPort.fromParts(managementAddress, managementPort);
     }
 
     /**
@@ -2294,14 +2373,22 @@ public JcloudsMachineLocation registerMachine(ConfigBag setup) throws NoMachines
 
     protected JcloudsMachineLocation registerMachineLocation(ConfigBag setup, NodeMetadata node) {
         ComputeService computeService = getComputeService(setup);
-        String vmHostname = getPublicHostname(node, Optional.<HostAndPort>absent(), Suppliers.<LoginCredentials>ofInstance(null), setup);
-        boolean waitForMakingManagementConnection = isWindows(node, setup) ? !"false".equalsIgnoreCase(setup.get(WAIT_FOR_WINRM_AVAILABLE)) : !"false".equalsIgnoreCase(setup.get(WAIT_FOR_SSHABLE));
-        HostAndPort managementHostAndPort = resolveManagementHostAndPort(computeService, node, vmHostname, Optional.<HostAndPort>absent(), setup, waitForMakingManagementConnection, true);
-        if (isWindows(node, setup)) {
-            return registerWinRmMachineLocation(computeService, node, managementHostAndPort, Optional.<HostAndPort>absent(), setup);
+        boolean windows = isWindows(node, setup);
+        
+        HostAndPort managementHostAndPort = resolveManagementHostAndPort(
+                computeService, node, Optional.<LoginCredentials>absent(), Optional.<HostAndPort>absent(), setup,
+                new ResolveOptions()
+                        .windows(windows)
+                        .expectConnectable(true)
+                        .propagatePollForReachableFailure(false)
+                        .pollForFirstReachableAddress(!"false".equalsIgnoreCase(setup.get(POLL_FOR_FIRST_REACHABLE_ADDRESS))));
+        
+
+        if (windows) {
+            return registerWinRmMachineLocation(computeService, node, managementHostAndPort, setup);
         } else {
             try {
-                return registerJcloudsSshMachineLocation(computeService, node, Optional.<Template>absent(), node.getCredentials(), managementHostAndPort, Optional.<HostAndPort>absent(), vmHostname, setup);
+                return registerJcloudsSshMachineLocation(computeService, node, Optional.<Template>absent(), node.getCredentials(), managementHostAndPort, setup);
             } catch (IOException e) {
                 throw Exceptions.propagate(e);
             }
@@ -2431,8 +2518,8 @@ public String toString() {
     }
 
     protected JcloudsSshMachineLocation registerJcloudsSshMachineLocation(ComputeService computeService, NodeMetadata node, Optional<Template> template, LoginCredentials credentials,
-                                                                          HostAndPort managementHostAndPort, Optional<HostAndPort> sshHostAndPort, String vmHostname, ConfigBag setup) throws IOException {
-        JcloudsSshMachineLocation machine = createJcloudsSshMachineLocation(computeService, node, template, vmHostname, sshHostAndPort, credentials, managementHostAndPort, setup);
+                                                                          HostAndPort managementHostAndPort, ConfigBag setup) throws IOException {
+        JcloudsSshMachineLocation machine = createJcloudsSshMachineLocation(computeService, node, template, credentials, managementHostAndPort, setup);
         registerJcloudsMachineLocation(node.getId(), machine);
         return machine;
     }
@@ -2443,7 +2530,7 @@ protected void registerJcloudsMachineLocation(String nodeId, JcloudsMachineLocat
         vmInstanceIds.put(machine, nodeId);
     }
 
-    protected JcloudsSshMachineLocation createJcloudsSshMachineLocation(ComputeService computeService, NodeMetadata node, Optional<Template> template, String vmHostname, Optional<HostAndPort> sshHostAndPort,
+    protected JcloudsSshMachineLocation createJcloudsSshMachineLocation(ComputeService computeService, NodeMetadata node, Optional<Template> template,
                                                                         LoginCredentials userCredentials, HostAndPort managementHostAndPort, ConfigBag setup) throws IOException {
         Map<?,?> sshConfig = extractSshConfig(setup, node);
         String nodeAvailabilityZone = extractAvailabilityZone(setup, node);
@@ -2454,24 +2541,30 @@ protected JcloudsSshMachineLocation createJcloudsSshMachineLocation(ComputeServi
         }
 
         if (LOG.isDebugEnabled())
-            LOG.debug("creating JcloudsSshMachineLocation representation for {}@{} ({}/{}) for {}/{}",
+            LOG.debug("creating JcloudsSshMachineLocation representation for {}@{} ({}) for {}/{}",
                     new Object[] {
                             getUser(setup),
                             managementHostAndPort,
                             Sanitizer.sanitize(sshConfig),
-                            sshHostAndPort,
                             setup.getDescription(),
                             node
                     });
 
         String address = managementHostAndPort.getHostText();
-
+        int port = managementHostAndPort.hasPort() ? managementHostAndPort.getPort() : node.getLoginPort();
+        
+        // The display name will be one of the IPs of the VM (preferring public if there are any).
+        // If the managementHostAndPort matches any of the IP contenders, then prefer that.
+        // (Don't just use the managementHostAndPort, because that could be using DNAT so could be
+        // a shared IP address, for example).
+        String displayName = getPublicHostnameGeneric(node, setup, Optional.of(address));
+        
         if (isManaged()) {
             return getManagementContext().getLocationManager().createLocation(LocationSpec.create(JcloudsSshMachineLocation.class)
                     .configure(sshConfig)
-                    .configure("displayName", vmHostname)
+                    .configure("displayName", displayName)
                     .configure("address", address)
-                    .configure(JcloudsSshMachineLocation.SSH_PORT, sshHostAndPort.isPresent() ? sshHostAndPort.get().getPort() : node.getLoginPort())
+                    .configure(JcloudsSshMachineLocation.SSH_PORT, port)
                     .configure("user", userCredentials.getUser())
                     .configure(SshMachineLocation.PASSWORD.getName(), sshConfig.get(SshMachineLocation.PASSWORD.getName()) != null ?
                             sshConfig.get(SshMachineLocation.PASSWORD.getName()) :
@@ -2494,9 +2587,9 @@ protected JcloudsSshMachineLocation createJcloudsSshMachineLocation(ComputeServi
             LOG.warn("Using deprecated JcloudsSshMachineLocation constructor because "+this+" is not managed");
             return new JcloudsSshMachineLocation(MutableMap.builder()
                     .putAll(sshConfig)
-                    .put("displayName", vmHostname)
+                    .put("displayName", displayName)
                     .put("address", address)
-                    .put("port", sshHostAndPort.isPresent() ? sshHostAndPort.get().getPort() : node.getLoginPort())
+                    .put("port", port)
                     .put("user", userCredentials.getUser())
                     .putIfNotNull(SshMachineLocation.PASSWORD.getName(), sshConfig.get(SshMachineLocation.PASSWORD.getName()) != null ?
                             SshMachineLocation.PASSWORD.getName() :
@@ -2517,15 +2610,13 @@ protected JcloudsSshMachineLocation createJcloudsSshMachineLocation(ComputeServi
     }
 
     protected JcloudsWinRmMachineLocation registerWinRmMachineLocation(ComputeService computeService, NodeMetadata node, HostAndPort managementHostAndPort,
-                                                                       Optional<HostAndPort> sshHostAndPort, ConfigBag setup) {
-        String vmHostname = getPublicHostname(node, sshHostAndPort, setup);
-
-        JcloudsWinRmMachineLocation machine = createWinRmMachineLocation(computeService, node, vmHostname, managementHostAndPort, setup);
+                                                                       ConfigBag setup) {
+        JcloudsWinRmMachineLocation machine = createWinRmMachineLocation(computeService, node, managementHostAndPort, setup);
         registerJcloudsMachineLocation(node.getId(), machine);
         return machine;
     }
 
-    protected JcloudsWinRmMachineLocation createWinRmMachineLocation(ComputeService computeService, NodeMetadata node, String vmHostname, HostAndPort sshHostAndPort, ConfigBag setup) {
+    protected JcloudsWinRmMachineLocation createWinRmMachineLocation(ComputeService computeService, NodeMetadata node, HostAndPort winrmHostAndPort, ConfigBag setup) {
         Map<?,?> winrmConfig = extractWinrmConfig(setup, node);
         String nodeAvailabilityZone = extractAvailabilityZone(setup, node);
         String nodeRegion = extractRegion(setup, node);
@@ -2534,13 +2625,16 @@ protected JcloudsWinRmMachineLocation createWinRmMachineLocation(ComputeService
             nodeRegion = extractProvider(setup, node);
         }
         
+        String address = winrmHostAndPort.getHostText();
+        String displayName = getPublicHostnameGeneric(node, setup, Optional.of(address));
+
         if (isManaged()) {
             return getManagementContext().getLocationManager().createLocation(LocationSpec.create(JcloudsWinRmMachineLocation.class)
                     .configure(winrmConfig)
                     .configure("jcloudsParent", this)
-                    .configure("displayName", vmHostname)
-                    .configure("address", sshHostAndPort.getHostText())
-                    .configure(WinRmMachineLocation.WINRM_CONFIG_PORT, sshHostAndPort.getPort())
+                    .configure("displayName", displayName)
+                    .configure("address", address)
+                    .configure(WinRmMachineLocation.WINRM_CONFIG_PORT, winrmHostAndPort.getPort())
                     .configure("user", getUser(setup))
                     .configure(WinRmMachineLocation.USER, setup.get(USER))
                     .configure("node", node)
@@ -3238,7 +3332,8 @@ protected String getPublicHostname(NodeMetadata node, Optional<HostAndPort> sshH
             if (result.isPresent()) return result.get();
         }
 
-        return getPublicHostnameGeneric(node, setup);
+        Optional<String> preferredAddress = sshHostAndPort.isPresent() ? Optional.of(sshHostAndPort.get().getHostText()) : Optional.<String>absent();
+        return getPublicHostnameGeneric(node, setup, preferredAddress);
     }
 
     /**
@@ -3267,10 +3362,21 @@ protected String getPrivateHostname(NodeMetadata node, Optional<HostAndPort> ssh
             if (result.isPresent()) return result.get();
         }
 
-        return getPrivateHostnameGeneric(node, setup);
+        Optional<String> preferredAddress = sshHostAndPort.isPresent() ? Optional.of(sshHostAndPort.get().getHostText()) : Optional.<String>absent();
+        return getPrivateHostnameGeneric(node, setup, preferredAddress);
     }
 
     private String getPublicHostnameGeneric(NodeMetadata node, @Nullable ConfigBag setup) {
+        return getPublicHostnameGeneric(node, setup, Optional.<String>absent());
+    }
+    
+    /**
+     * The preferredAddress is returned if it is one of the best choices (e.g. if publicAddresses 
+     * contains it, or if publicAddresses.isEmpty but the privateAddresses contains it).
+     * 
+     * Otherwise, returns the first publicAddress (if any), or failing that the first privateAddress.
+     */
+    private String getPublicHostnameGeneric(NodeMetadata node, @Nullable ConfigBag setup, Optional<String> preferredAddress) {
         // JcloudsUtil.getFirstReachableAddress() (probably) already succeeded so at least one of the provided
         // public and private IPs is reachable. Prefer the public IP. Don't use hostname as a fallback
         // from the public address - if public address is missing why would hostname resolve to a 
@@ -3283,25 +3389,45 @@ private String getPublicHostnameGeneric(NodeMetadata node, @Nullable ConfigBag s
         // TODO Choose an IP once and stick to it - multiple places call JcloudsUtil.getFirstReachableAddress(),
         // could even get different IP on each call.
         if (groovyTruth(node.getPublicAddresses())) {
+            if (preferredAddress.isPresent() && node.getPublicAddresses().contains(preferredAddress.get())) {
+                return preferredAddress.get();
+            }
             return node.getPublicAddresses().iterator().next();
         } else if (groovyTruth(node.getPrivateAddresses())) {
+            if (preferredAddress.isPresent() && node.getPrivateAddresses().contains(preferredAddress.get())) {
+                return preferredAddress.get();
+            }
             return node.getPrivateAddresses().iterator().next();
         } else {
             return null;
         }
     }
 
-    private String getPrivateHostnameGeneric(NodeMetadata node, @Nullable ConfigBag setup) {
+    /**
+     * The preferredAddress is returned if it is one of the best choices (e.g. if non-local privateAddresses 
+     * contains it, or if privateAddresses.isEmpty but the publicAddresses contains it).
+     * 
+     * Otherwise, returns the first publicAddress (if any), or failing that the first privateAddress.
+     */
+    private String getPrivateHostnameGeneric(NodeMetadata node, @Nullable ConfigBag setup, Optional<String> preferredAddress) {
         //prefer the private address to the hostname because hostname is sometimes wrong/abbreviated
         //(see that javadoc; also e.g. on rackspace/cloudstack, the hostname is not registered with any DNS).
         //Don't return local-only address (e.g. never 127.0.0.1)
-        if (groovyTruth(node.getPrivateAddresses())) {
-            for (String p : node.getPrivateAddresses()) {
-                if (Networking.isLocalOnly(p)) continue;
-                return p;
+        Iterable<String> privateAddresses = Iterables.filter(node.getPrivateAddresses(), new Predicate<String>() {
+            @Override public boolean apply(String input) {
+                return input != null && !Networking.isLocalOnly(input);
+            }});
+        if (!Iterables.isEmpty(privateAddresses)) {
+            if (preferredAddress.isPresent() && Iterables.contains(privateAddresses, preferredAddress.get())) {
+                return preferredAddress.get();
             }
+            return Iterables.get(privateAddresses, 0);
         }
+        
         if (groovyTruth(node.getPublicAddresses())) {
+            if (preferredAddress.isPresent() && node.getPublicAddresses().contains(preferredAddress.get())) {
+                return preferredAddress.get();
+            }
             return node.getPublicAddresses().iterator().next();
         } else if (groovyTruth(node.getHostname())) {
             return node.getHostname();
diff --git a/locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/JcloudsByonLocationResolverSoftlayerLiveTest.java b/locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/JcloudsByonLocationResolverSoftlayerLiveTest.java
index 648e71e12f..048ad0bb17 100644
--- a/locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/JcloudsByonLocationResolverSoftlayerLiveTest.java
+++ b/locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/JcloudsByonLocationResolverSoftlayerLiveTest.java
@@ -44,14 +44,14 @@ public class JcloudsByonLocationResolverSoftlayerLiveTest extends AbstractJcloud
     private String slVmHostname;
     
     private LocalManagementContext classManagementContext;
-    private JcloudsLocation classEc2Loc;
+    private JcloudsLocation classLoc;
     private JcloudsSshMachineLocation classVm;
 
     @BeforeClass(groups="Live")
     public void setUpClass() throws Exception {
         classManagementContext = newManagementContext();
-        classEc2Loc = (JcloudsLocation) classManagementContext.getLocationRegistry().getLocationManaged(SOFTLAYER_LOCATION_SPEC);
-        classVm = (JcloudsSshMachineLocation)classEc2Loc.obtain(MutableMap.<String,Object>builder()
+        classLoc = (JcloudsLocation) classManagementContext.getLocationRegistry().getLocationManaged(SOFTLAYER_LOCATION_SPEC);
+        classVm = (JcloudsSshMachineLocation)classLoc.obtain(MutableMap.<String,Object>builder()
                 .put("inboundPorts", ImmutableList.of(22))
                 .build());
         slVmUser = classVm.getUser();
@@ -64,7 +64,7 @@ public void setUpClass() throws Exception {
     public void tearDownClass() throws Exception {
         try {
             if (classVm != null) {
-                classEc2Loc.release(classVm);
+                classLoc.release(classVm);
             }
         } finally {
             if (classManagementContext != null) classManagementContext.terminate();
diff --git a/locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/networking/JcloudsReachableAddressStubbedTest.java b/locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/networking/JcloudsReachableAddressStubbedTest.java
index 8f78d9443b..9a5f16866e 100644
--- a/locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/networking/JcloudsReachableAddressStubbedTest.java
+++ b/locations/jclouds/src/test/java/org/apache/brooklyn/location/jclouds/networking/JcloudsReachableAddressStubbedTest.java
@@ -18,9 +18,7 @@
  */
 package org.apache.brooklyn.location.jclouds.networking;
 
-import static com.google.common.base.Preconditions.checkNotNull;
 import static org.testng.Assert.assertEquals;
-import static org.testng.Assert.assertNotNull;
 import static org.testng.Assert.assertTrue;
 
 import java.io.IOException;
@@ -38,19 +36,24 @@
 import org.apache.brooklyn.location.jclouds.JcloudsLocationConfig;
 import org.apache.brooklyn.location.jclouds.JcloudsSshMachineLocation;
 import org.apache.brooklyn.location.jclouds.JcloudsStubTemplateBuilder;
+import org.apache.brooklyn.location.jclouds.JcloudsWinRmMachineLocation;
 import org.apache.brooklyn.location.jclouds.StubbedComputeServiceRegistry;
 import org.apache.brooklyn.location.jclouds.StubbedComputeServiceRegistry.AbstractNodeCreator;
 import org.apache.brooklyn.location.jclouds.networking.JcloudsPortForwardingStubbedLiveTest.RecordingJcloudsPortForwarderExtension;
 import org.apache.brooklyn.location.ssh.SshMachineLocation;
+import org.apache.brooklyn.location.winrm.WinRmMachineLocation;
 import org.apache.brooklyn.test.Asserts;
 import org.apache.brooklyn.util.core.internal.ssh.RecordingSshTool;
 import org.apache.brooklyn.util.core.internal.ssh.RecordingSshTool.CustomResponse;
 import org.apache.brooklyn.util.core.internal.ssh.RecordingSshTool.CustomResponseGenerator;
 import org.apache.brooklyn.util.core.internal.ssh.RecordingSshTool.ExecParams;
 import org.apache.brooklyn.util.core.internal.ssh.SshTool;
+import org.apache.brooklyn.util.core.internal.winrm.RecordingWinRmTool;
+import org.apache.brooklyn.util.core.internal.winrm.WinRmTool;
 import org.jclouds.compute.domain.NodeMetadata;
 import org.jclouds.compute.domain.NodeMetadata.Status;
 import org.jclouds.compute.domain.NodeMetadataBuilder;
+import org.jclouds.compute.domain.OsFamily;
 import org.jclouds.compute.domain.Template;
 import org.jclouds.domain.LoginCredentials;
 import org.slf4j.Logger;
@@ -94,6 +97,7 @@ public void setUp() throws Exception {
         addressChooser = new AddressChooser();
         customResponseGenerator = new CustomResponseGeneratorImpl();
         RecordingSshTool.clear();
+        RecordingWinRmTool.clear();
         super.setUp();
     }
     
@@ -104,6 +108,7 @@ public void tearDown() throws Exception {
             super.tearDown();
         } finally {
             RecordingSshTool.clear();
+            RecordingWinRmTool.clear();
         }
     }
 
@@ -150,18 +155,11 @@ protected void initNodeCreatorAndJcloudsLocation(List<String> publicAddresses, L
                         .build());
     }
 
-    protected JcloudsSshMachineLocation obtainMachine(Map<?, ?> conf) throws Exception {
-        assertNotNull(jcloudsLocation);
-        JcloudsSshMachineLocation result = (JcloudsSshMachineLocation)jcloudsLocation.obtain(conf);
-        machines.add(checkNotNull(result, "result"));
-        return result;
-    }
-    
     /**
      * Only one public and one private; public is reachable;
      * With waitForSshable=true; pollForFirstReachableAddress=true; and custom reachable-predicate
      */
-    @Test(groups = {"Live", "Live-sanity"})
+    @Test
     public void testMachineUsesVanillaPublicAddress() throws Exception {
         initNodeCreatorAndJcloudsLocation(ImmutableList.of("1.1.1.1"), ImmutableList.of("2.1.1.1"), ImmutableMap.of());
         reachableIp = "1.1.1.1";
@@ -179,6 +177,27 @@ public void testMachineUsesVanillaPublicAddress() throws Exception {
         assertEquals(machine.getSubnetIp(), "2.1.1.1");
     }
 
+    /**
+     * Similar to {@link #testMachineUsesVanillaPublicAddress()}, except for a windows machine.
+     */
+    @Test
+    public void testWindowsMachineUsesVanillaPublicAddress() throws Exception {
+        initNodeCreatorAndJcloudsLocation(ImmutableList.of("1.1.1.1"), ImmutableList.of("2.1.1.1"), ImmutableMap.of());
+        reachableIp = "1.1.1.1";
+
+        JcloudsWinRmMachineLocation machine = newWinrmMachine(ImmutableMap.<ConfigKey<?>,Object>builder()
+                .put(JcloudsLocationConfig.WAIT_FOR_WINRM_AVAILABLE, Asserts.DEFAULT_LONG_TIMEOUT.toString())
+                .put(JcloudsLocation.POLL_FOR_FIRST_REACHABLE_ADDRESS, Asserts.DEFAULT_LONG_TIMEOUT.toString())
+                .build());
+        
+        addressChooser.assertCalled();
+        assertEquals(RecordingWinRmTool.getLastExec().constructorProps.get(WinRmTool.PROP_HOST.getName()), reachableIp);
+
+        assertEquals(machine.getAddress().getHostAddress(), reachableIp);
+        assertEquals(machine.getHostname(), reachableIp);
+        assertEquals(machine.getSubnetIp(), "2.1.1.1");
+    }
+
     /**
      * Only one public and one private; private is reachable;
      * With waitForSshable=true; pollForFirstReachableAddress=true; and custom reachable-predicate
@@ -197,7 +216,7 @@ public void testMachineUsesVanillaPrivateAddress() throws Exception {
         assertEquals(RecordingSshTool.getLastExecCmd().constructorProps.get(SshTool.PROP_HOST.getName()), reachableIp);
 
         assertEquals(machine.getAddress().getHostAddress(), reachableIp);
-        assertEquals(machine.getHostname(), "1.1.1.1");
+        assertEquals(machine.getHostname(), "1.1.1.1"); // preferes public, even if that was not reachable
         assertEquals(machine.getSubnetIp(), reachableIp);
     }
 
@@ -219,7 +238,7 @@ public void testMachineUsesReachablePublicAddress() throws Exception {
         assertEquals(RecordingSshTool.getLastExecCmd().constructorProps.get(SshTool.PROP_HOST.getName()), reachableIp);
 
         assertEquals(machine.getAddress().getHostAddress(), reachableIp);
-        assertEquals(machine.getHostname(), "1.1.1.1");
+        assertEquals(machine.getHostname(), reachableIp);
         assertEquals(machine.getSubnetIp(), "2.1.1.1");
     }
 
@@ -227,7 +246,7 @@ public void testMachineUsesReachablePublicAddress() throws Exception {
      * Multiple private addresses; chooses the reachable one;
      * With waitForSshable=true; pollForFirstReachableAddress=true; and custom reachable-predicate
      */
-    @Test(enabled = false, groups = "WIP")
+    @Test
     public void testMachineUsesReachablePrivateAddress() throws Exception {
         initNodeCreatorAndJcloudsLocation(ImmutableList.of("1.1.1.1"), ImmutableList.of("2.1.1.1", "2.1.1.2", "2.1.1.2"), ImmutableMap.of());
         reachableIp = "2.1.1.2";
@@ -241,8 +260,8 @@ public void testMachineUsesReachablePrivateAddress() throws Exception {
         assertEquals(RecordingSshTool.getLastExecCmd().constructorProps.get(SshTool.PROP_HOST.getName()), reachableIp);
 
         assertEquals(machine.getAddress().getHostAddress(), reachableIp);
-        assertEquals(machine.getHostname(), reachableIp);
-        assertEquals(machine.getSubnetIp(), reachableIp);
+        assertEquals(machine.getHostname(), "1.1.1.1"); // prefers public, even if that was not reachable
+        assertEquals(machine.getSubnetIp(), "2.1.1.1"); // TODO uses first, rather than "reachable"; is that ok?
     }
 
     /**
@@ -313,6 +332,34 @@ public void testReachabilityChecksWithPortForwarding() throws Exception {
         assertEquals(machine.getSubnetIp(), "2.1.1.1");
     }
     
+    /**
+     * Similar to {@link #testMachineUsesVanillaPublicAddress()}, except for a windows machine.
+     */
+    @Test
+    public void testWindowsReachabilityChecksWithPortForwarding() throws Exception {
+        initNodeCreatorAndJcloudsLocation(ImmutableList.of("1.1.1.1"), ImmutableList.of("2.1.1.1"), ImmutableMap.of());
+        reachableIp = "1.2.3.4";
+                
+        PortForwardManager pfm = new PortForwardManagerImpl();
+        RecordingJcloudsPortForwarderExtension portForwarder = new RecordingJcloudsPortForwarderExtension(pfm);
+        
+        JcloudsWinRmMachineLocation machine = newWinrmMachine(ImmutableMap.<ConfigKey<?>,Object>builder()
+                .put(JcloudsLocationConfig.WAIT_FOR_WINRM_AVAILABLE, Asserts.DEFAULT_LONG_TIMEOUT.toString())
+                .put(JcloudsLocation.POLL_FOR_FIRST_REACHABLE_ADDRESS, Asserts.DEFAULT_LONG_TIMEOUT.toString())
+                .put(JcloudsLocation.USE_PORT_FORWARDING, true)
+                .put(JcloudsLocation.PORT_FORWARDER, portForwarder)
+                .build());
+        
+        addressChooser.assertNotCalled();
+        assertEquals(RecordingWinRmTool.getLastExec().constructorProps.get(WinRmTool.PROP_HOST.getName()), reachableIp);
+        assertEquals(RecordingWinRmTool.getLastExec().constructorProps.get(WinRmTool.PROP_PORT.getName()), 12345);
+
+        assertEquals(machine.getAddress().getHostAddress(), "1.2.3.4");
+        assertEquals(machine.getPort(), 12345);
+        assertEquals(machine.getHostname(), "1.2.3.4"); // TODO Different impl from JcloudsSshMachineLocation!
+        assertEquals(machine.getSubnetIp(), "2.1.1.1");
+    }
+    
     @Test
     public void testMachineUsesFirstPublicAddress() throws Exception {
         initNodeCreatorAndJcloudsLocation(ImmutableList.of("10.10.10.1", "10.10.10.2"), ImmutableList.of("1.1.1.1", "1.1.1.2", "1.1.1.2"), ImmutableMap.of());
@@ -338,6 +385,19 @@ protected JcloudsSshMachineLocation newMachine(Map<? extends ConfigKey<?>, ?> ad
                 .build());
     }
     
+    protected JcloudsWinRmMachineLocation newWinrmMachine() throws Exception {
+        return newWinrmMachine(ImmutableMap.<ConfigKey<?>, Object>of());
+    }
+    
+    protected JcloudsWinRmMachineLocation newWinrmMachine(Map<? extends ConfigKey<?>, ?> additionalConfig) throws Exception {
+        return obtainWinrmMachine(ImmutableMap.<ConfigKey<?>,Object>builder()
+                .put(WinRmMachineLocation.WINRM_TOOL_CLASS, RecordingWinRmTool.class.getName())
+                .put(JcloudsLocation.POLL_FOR_FIRST_REACHABLE_ADDRESS_PREDICATE, addressChooser)
+                .put(JcloudsLocation.OS_FAMILY_OVERRIDE, OsFamily.WINDOWS)
+                .putAll(additionalConfig)
+                .build());
+    }
+    
     protected class AddressChooser implements Predicate<HostAndPort> {
         final List<HostAndPort> calls = Lists.newCopyOnWriteArrayList();
         
diff --git a/software/winrm/src/test/java/org/apache/brooklyn/util/core/internal/winrm/RecordingWinRmTool.java b/software/winrm/src/test/java/org/apache/brooklyn/util/core/internal/winrm/RecordingWinRmTool.java
index 0795880605..6d39fab4bc 100644
--- a/software/winrm/src/test/java/org/apache/brooklyn/util/core/internal/winrm/RecordingWinRmTool.java
+++ b/software/winrm/src/test/java/org/apache/brooklyn/util/core/internal/winrm/RecordingWinRmTool.java
@@ -48,10 +48,12 @@ public enum ExecType {
     
     public static class ExecParams {
         public final ExecType type;
+        public final Map<?, ?> constructorProps;
         public final List<String> commands;
         
-        public ExecParams(ExecType type, List<String> commands) {
+        public ExecParams(ExecType type, Map<?, ?> constructorProps, List<String> commands) {
             this.type = type;
+            this.constructorProps = constructorProps;
             this.commands = commands;
         }
         
@@ -59,6 +61,7 @@ public ExecParams(ExecType type, List<String> commands) {
         public String toString() {
             return Objects.toStringHelper(this)
                     .add("type", type)
+                    .add("constructorProps", constructorProps)
                     .add("commands", commands)
                     .toString();
         }
@@ -118,28 +121,31 @@ public static List<ExecParams> getExecs() {
     public static ExecParams getLastExec() {
         return execs.get(execs.size()-1);
     }
+
+    private final Map<?, ?> ownConstructorProps;
     
     public RecordingWinRmTool(Map<?,?> props) {
         constructorProps.add(props);
+        ownConstructorProps = props;
     }
     
     @Override
     public WinRmToolResponse executeCommand(List<String> commands) {
-        ExecParams execParams = new ExecParams(ExecType.COMMAND, commands);
+        ExecParams execParams = new ExecParams(ExecType.COMMAND, ownConstructorProps, commands);
         execs.add(execParams);
         return generateResponse(execParams);
     }
 
     @Override
     public WinRmToolResponse executePs(List<String> commands) {
-        ExecParams execParams = new ExecParams(ExecType.POWER_SHELL, commands);
+        ExecParams execParams = new ExecParams(ExecType.POWER_SHELL, ownConstructorProps, commands);
         execs.add(execParams);
         return generateResponse(execParams);
     }
 
     @Override
     public WinRmToolResponse copyToServer(InputStream source, String destination) {
-        execs.add(new ExecParams(ExecType.COPY_TO_SERVER, ImmutableList.of(new String(Streams.readFully(source)))));
+        execs.add(new ExecParams(ExecType.COPY_TO_SERVER, ownConstructorProps, ImmutableList.of(new String(Streams.readFully(source)))));
         return new WinRmToolResponse("", "", 0);
     }