Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/355#discussion_r80890063
--- Diff:
locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/JcloudsLocation.java
---
@@ -3314,15 +3314,12 @@ private String
getPublicHostnameGeneric(NodeMetadata node, @Nullable ConfigBag s
// public IP? It is sometimes wrong/abbreviated, resolving to the
wrong IP, also e.g. on
// rackspace, the hostname lacks the domain.
//
- // TODO If POLL_FOR_FIRST_REACHABLE_ADDRESS=false, then won't have
checked if any node is reachable.
- // TODO Some of the private addresses might not be reachable,
should check connectivity before
- // making a choice.
// 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())) {
return node.getPublicAddresses().iterator().next();
} else if (groovyTruth(node.getPrivateAddresses())) {
- return node.getPrivateAddresses().iterator().next();
+ return getFirstReachableAddress(node, setup);
--- End diff --
TL;DR: Tricky. I don't think we should solve the problem like this. I need
to think more about this!
The comment above says `JcloudsUtil.getFirstReachableAddress() (probably)
already succeeded so at least one of the provided public and private IPs is
reachable`.
The "probably" is particularly scary. Also, we may need to worry about this
being called some time after provisioning, where the VM might have been
terminated behind Brooklyn's back. In that case, none of the addresses would be
reachable. The `getFirstReachableAddress` throws an exception (and might take
quite some time to do that, depending on the firewalls used within a given
cloud). We really don't want `getPublicHostname()` to throw an exception.
Also, the code now looks strange: why is it calling
`getFirstReachableAddress` for the private addresses but not the public
addresses?
The comment you deleted is also very worrying: if
`POLL_FOR_FIRST_REACHABLE_ADDRESS=false` then we shouldn't be calling
`getFirstReachableAddress` here. Reasons one might set that include if the VM
being provisioned requires special DNAT access (so none of the IP addresses are
directly accessible), or if the VM being provisioned is locked down so Brooklyn
won't be able to reach it ever.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
