New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ConnectivityResolver #529
ConnectivityResolver #529
Conversation
b782468
to
bd4e71f
Compare
Will have a look through this now, however, on the points for opinion above, my 2¢ worth is:
|
bd4e71f
to
01da14a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great in general; haven't quite finished going through this yet but here are some comments in the mean time.
import com.google.common.net.HostAndPort; | ||
|
||
/** | ||
* The default location network info customizer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest adding in some text here to describe what it does - just a copy of the description from the PR would be great.
"mode", "Operation mode", NetworkMode.PREFER_PUBLIC); | ||
|
||
@Beta | ||
public static final ConfigKey<Boolean> TEST_CREDENTIALS = ConfigKeys.newBooleanConfigKey( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe validate_credentials
- test_credentials
sounds like it means "credentials for a test"
/** @deprecated Since 0.11.0. Use {@link #waitForSshableGuessCredentials} instead. */ | ||
@Deprecated | ||
protected LoginCredentials waitForSshable(ComputeService computeService, NodeMetadata node, HostAndPort managementHostAndPort, ConfigBag setup) { | ||
return waitForSshable(computeService, node, managementHostAndPort, setup); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not recursive? Should be waitForSshableGuessCredentials
I suppose. This isn't hit in the unit tests, can we add one that will hit it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot.
* Holds parameters to be used by a {@link LocationNetworkInfoCustomizer}. | ||
*/ | ||
@Beta | ||
public class ManagementAddressResolveOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, I think this is much preferable to managing these settings as separate local variables as previously.
See comment on where it is used.
@@ -625,6 +630,30 @@ public MachineLocation obtain(Map<?,?> flags) throws NoMachinesAvailableExceptio | |||
} | |||
} | |||
|
|||
protected ManagementAddressResolveOptions getManagementAddressResolveOptions( | |||
NodeMetadata node, ConfigBag setup, Optional<HostAndPort> sshHostAndPortOverride, Optional<LoginCredentials> userCredentials) { | |||
boolean waitForSshable = !"false".equalsIgnoreCase(setup.get(WAIT_FOR_SSHABLE)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could push the knowledge about the configuration keys into the constructor of the class itself, and just make this
protected ManagementAddressResolveOptions getManagementAddressResolveOptions(
NodeMetadata x, ConfigBag setup, Optional<HostAndPort> x, Optional<LoginCredentials> x) {
return new ManagementAddressResolveOptions(setup)
.hostAndPortOverride(sshHostAndPortOverride)
.initialCredentials(node.getCredentials())
.userCredentials(userCredentials));
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The config keys it needs are only available as arguments to certain methods (obtain
, primarily) so we can't pull it up to the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think I misunderstood you here. Were you referring to the constructor for ManagementAddressResolveOptions
?
|
||
public static final ConfigKey<Boolean> PUBLISH_NETWORKS = ConfigKeys.newBooleanConfigKey( | ||
"publishNetworks", | ||
"Indicates that the customiser should publish addresses as sensors on each entity", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep consistency of spelling, customizer
.
|
||
private Duration pollTimeout; | ||
private boolean waitForSshable; | ||
private boolean pollForFirstReachableAddress; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also add POLL_FOR_FIRST_REACHABLE_ADDRESS_PREDICATE
and POLL_FOR_FIRST_REACHABLE_ADDRESS_PREDICATE_TYPE
, keeping all access to these closely related keys together.
private boolean expectReachable; | ||
private boolean isWindows; | ||
private boolean propagatePollForReachableFailure; | ||
private LoginCredentials initialCredentials; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could add JcloudsLocation.CUSTOM_CREDENTIALS
protected Iterable<HostAndPort> getManagementCandidates( | ||
JcloudsLocation location, NodeMetadata node, ConfigBag config, ManagementAddressResolveOptions options) { | ||
final Optional<HostAndPort> hostAndPortOverride = options.getHostAndPortOverride(); | ||
boolean lookupAwsHostname = Boolean.TRUE.equals(config.get(JcloudsLocation.LOOKUP_AWS_HOSTNAME)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to factor out the AWS specifics into a separate tool; I know this has come from the existing JcloudsLocation code but as this is a new class maybe there is an opportunity to avoid repeating this pattern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p.s. I don't mean this PR requires this change as such; but it might be worth adding a "TODO" to highlight the point.
String host = Iterables.get(addresses, 0); | ||
return HostAndPort.fromParts(host, defaultPort); | ||
} | ||
|
||
private boolean isAddressResolvable(String addr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be deleted now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from a code review standpoint, some minor points made previously. Haven't tested it yet but will do so now.
} | ||
} | ||
|
||
protected Predicate<? super HostAndPort> getReachableAddressesPredicate(ConfigBag setup) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could maybe be folded into the options class; similar thought to note here
@@ -2610,24 +2525,32 @@ public boolean apply(@Nullable WinRmMachineLocation machine) { | |||
return credsSuccessful.get(); | |||
} | |||
|
|||
protected LoginCredentials waitForSshable(final ComputeService computeService, final NodeMetadata node, HostAndPort managementHostAndPort, ConfigBag setup) { | |||
LoginCredentials nodeCreds = node.getCredentials(); | |||
protected LoginCredentials waitForSshableGuessCredentials(final ComputeService computeService, final NodeMetadata node, HostAndPort managementHostAndPort, ConfigBag setup) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new method isn't being called, but the newly deprecated method at line 2564 still is, at JcloudsLocation.java:811. I guess you meant here to replace the calls to waitForSshable(final ComputeService computeService, final NodeMetadata node, HostAndPort hostAndPort, Iterable<LoginCredentials> credentialsToTry, ConfigBag setup)
with calls to waitForSshableGuessCredentials
. But shouldn't waitForSshableGuessCredentials
be calling waitForSshable( HostAndPort hostAndPort, Iterable<LoginCredentials> credentialsToTry, ConfigBag setup)
at line 2533 rather than the deprecated five param version?
} | ||
|
||
// 1. Were they configured by the user? | ||
LoginCredentials customCredentials = setup.get(JcloudsLocation.CUSTOM_CREDENTIALS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth some validation here for non-blank?
// 2. Can they be extracted from the setup+node? | ||
if (userCredentials == null || | ||
(!userCredentials.getOptionalPassword().isPresent() && !userCredentials.getOptionalPrivateKey().isPresent())) { | ||
// We either don't have any userCredentials, or it is missing both a password/key. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like a misconfiguration, so it's probably worth putting a WARN in the log.
import static org.testng.Assert.assertEquals; | ||
import static org.testng.Assert.assertFalse; | ||
import static org.testng.Assert.assertTrue; | ||
import static org.testng.Assert.fail; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused
I tested this successfully: put a brooklyn on ibm-bluebox-lon-pub, and used it to deploy the blueprint
i.e. both servers are deployed to the same location as brooklyn, which is configured to create a public address ( The server 'custom' above was assigned a host address on the private network, while the 'default' one was assigned it on the public network:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, most comments are minor, with caveat that the comments that I think are worth addressing are those around the calling structure of waitForSshable
, in particular removing the infinite recursion, and the deletable method here. Any suggested refactorings aren't needed as such and the PR can be merged without them.
01da14a
to
960f092
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't get too far today - will continue tomorrow.
} | ||
|
||
// Will normally have come from port forwarding. | ||
if (hostAndPortOverride.isPresent()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's port forwarding then presumably it'd be the external interface. What if the user has specified PREFER_PRIVATE
?
// 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. | ||
// TODO This is a bit weird: if the statement below is true then getHostnameAws will find the first | ||
// reachable address, which repeats if case after this one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it logins to the machine to find which IP to login to :).
Note that this is different from the next case. It similarly fetches the first reachable IP, but then connects to it and executes a curl
command, returning the hostname
value.
Would make more sense to move this after the code which already figured out the correct IPs and use them to fetch the hostname. I'd expect this to be available from EC2 API so in future could improve it to fetch it directly through jclouds.
For amazon it would return the hostname
. Wonder how it interacts with PREFER_PRIVATE
. Seems the return value gets coerced to InetAddress
(which will resolve the hostname from the POV of the Brooklyn instance) and set on the MachineLocation
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest skipping if NetworkMode
is set. If it's not set then it acts similarly to auto NetworkMode
. When outside of Amazon it will resolve to the public and when inside will resolve to the private IP. Could cause problems when Brooklyn & managed machines are inside Amazon but on different (non-reachable) subnets. Wonder how DNS resolving behaves then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really happy to see JcloudsLocation
broken apart in smaller chunks! Changes look good. Definitely there's more to untangle in JcloudsLocation
, but not in scope for this PR. Take what you consider important from following comments, leaving the rest for future changes, TODO comments.
|
||
|
||
// TODO: This should log a message if timeout is null and the method blocks for an unreasonably long time - | ||
// it probably means someone called .get() and forgot to submit the task. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should differentiate between not queued to a parent task (because the user forgot the submit it) and not yet submitted to an executor because a previous task failed which is a valid case.
return this; | ||
} | ||
|
||
public ManagementAddressResolveOptions publishNetworks(boolean publishNetworks) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setter is publishNetworks
, while getter is publishNetworkSensor
.
return Boolean.TRUE.equals(config().get(PUBLISH_NETWORKS)); | ||
} | ||
|
||
// TODO: Separate this into second part? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what this comment means?
if (entity.sensors().get(sensor) == null) { | ||
entity.sensors().set(sensor, address); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a particular usage of the sensors in mind? I'm wondering whether an array in a single sensor will be more useful. It can be processed by transformers and such. Having separate keys with unknown count makes any address > host.address.xxx.0
useless.
Is it possible that we have more than one hostname per machine? That's re the todo hostnames
comment.
boolean skipJcloudsSshing = Boolean.FALSE.equals(setup.get(USE_JCLOUDS_SSH_INIT)) || usePortForwarding; | ||
boolean windows = isWindows(node, setup); | ||
boolean waitForConnectable = windows ? waitForWinRmable : waitForSshable; | ||
String pollForFirstReachable = setup.get(POLL_FOR_FIRST_REACHABLE_ADDRESS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description for POLL_FOR_FIRST_REACHABLE_ADDRESS
needs updating in light of the NetworkMode
changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the reachability check doing credentials check I think POLL_FOR_FIRST_REACHABLE_ADDRESS
and WAIT_FOR_XXX_AVAILABLE
pretty much overlap now. Suggest (at the very least) adding a TODO somewhere to deprecate one of them.
if (hit.hasNext()) HostAndPort.fromHost(hit.next()); | ||
} | ||
if (hapChoice == null) { | ||
throw new IllegalStateException("Exhausted all options when determining address for " + location); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would happen only when jclouds returns no IP addresses at all so could amend with "jclouds did not return any IP addresses matching " + getMode()
* @see JcloudsLocationConfig#LOCATION_NETWORK_INFO_CUSTOMIZER | ||
*/ | ||
@Beta | ||
public interface LocationNetworkInfoCustomizer extends JcloudsLocationCustomizer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think this should be a JcloudsLocationCustomizer
. Yes it customises the behaviour of the location. My thinking is that JcloudsLocationCustomizer
is not properly named though - it's too generic and anything altering the behaviour of the location matches it. Something better describing it would be LocationLifecycleCallback
.
To back the above - you are not implementing any of the methods of the customiser.
The primary task of this interface is to return the addresses we can ssh into. A name more fitting in this description would be LocationConnectivityResolver
, LocationAddressResolver
.
@@ -429,6 +429,11 @@ protected CloudMachineNamer getCloudMachineNamer(ConfigBag config) { | |||
return result; | |||
} | |||
|
|||
public LocationNetworkInfoCustomizer getLocationNetworkInfoCustomizer(ConfigBag setup) { | |||
LocationNetworkInfoCustomizer configured = setup.get(LOCATION_NETWORK_INFO_CUSTOMIZER); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a config key is suitable for this. Just FYI there is the extension
API as well. We could improve it in future and use it to customise the behaviour of the location with pluggable components, leaving the keys for (declarative) configuration.
// store the credentials, in case they have changed | ||
putIfPresentButDifferent(setup, JcloudsLocationConfig.PASSWORD, userCredentials.getOptionalPassword().orNull()); | ||
putIfPresentButDifferent(setup, JcloudsLocationConfig.PRIVATE_KEY_DATA, userCredentials.getOptionalPrivateKey().orNull()); | ||
|
||
// Wait for the VM to be reachable over SSH | ||
// TODO: this has already been tested by locationnetworkinfocustomizer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard it with with a !CHECK_CREDENTIALS
then?
@@ -2476,37 +2382,10 @@ protected String getFirstReachableAddress(NodeMetadata node, ConfigBag setup) { | |||
String result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't think the method getFirstReachableAddress
is still referenced. Mark it deprecated.
|
ff12c71
to
b9c27e8
Compare
650b06d
to
728fb46
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; one wee typo.
Tests succeed as previously #529 (comment)
for ename in custom default ; do printf "%s " ${ename} ; br app app1 ent ${ename} sensor host.address ; done
custom 10.101.1.49
default 169.50.81.120
My gut feeling is there is a noticeable pause while it does the connectivity tests, compared to previous deployments, but nothing dramatic.
* and the elements in the Iterable are ordered according to their position in sockets. | ||
* @return An Iterable containing present values for the elements of sockets that are reachable | ||
* according to {@link #socketTester} and absent values for those not. Checks are concurrent. | ||
* The iterable returned is ordered according sockets. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo "according to"
Rather than only PROP_SSH_TRIES.
LocationNetworkInfoCustomizer is a customiser that gives users much more control over which addresses and credentials are used when connecting to instances provisioned by JcloudsLocation.
728fb46
to
3fd64c8
Compare
looks good, will merge |
ConnectivityResolver `ConnectivityResolver` is a special customiser used by `JcloudsLocation` to determine the address and credentials with which a location should be contacted. By default it should behave as the previous logic in `JcloudsLocation`, but it exposes options to have Brooklyn prefer to contact VMs on private addresses and can be injected on a per-entity basis. For example: ```yaml services: - type: server location: the-same-private-network-as-brooklyn brooklyn.initializers: - type: org.apache.brooklyn.location.jclouds.DefaultConnectivityResolver brooklyn.config: mode: ONLY_PRIVATE - type: server location: another-cloud # implicit use of the default network info customizer. ``` Would result in the first entity being managed on the instance's private address (and deployment failing if this was not possible) and the second being managed on its public address. Graceful fallback is possible by replacing `ONLY_PRIVATE` with `PREFER_PRIVATE`. There are `PUBLIC` variants of each of these. These changes were prompted by a thread on the Brooklyn mailing list last December: http://markmail.org/thread/xpr6nsyimv7goewy. Some of the items discussed in that thread have been left for future work. It depends on PRs #497 and #524. ~~I'd like opinions on:~~ * ~~naming: "BasicLocationNetworkInfoCustomizer" is cumbersome.~~ * ~~whether the class should extend `BasicJcloudsLocationCustomizer`. I can see it being useful to anyone that wants to subclass BLNIC but at the moment it's totally unused.~~ * ~~whether we should include the `publishNetworks` option right now. It controls the publication of `host.address.{public,private}.n` sensors. The idea is good but the feature feels a bit half-baked right now.~~ * ~~whether `ManagementAddressResolveOptions` is the right way to convey information between the location and the customiser. Again, it's a bit ungainly.~~
ConnectivityResolver
is a special customiser used byJcloudsLocation
to determine the address and credentials with which a location should be contacted. By default it should behave as the previous logic inJcloudsLocation
, but it exposes options to have Brooklyn prefer to contact VMs on private addresses and can be injected on a per-entity basis. For example:Would result in the first entity being managed on the instance's private address (and deployment failing if this was not possible) and the second being managed on its public address. Graceful fallback is possible by replacing
ONLY_PRIVATE
withPREFER_PRIVATE
. There arePUBLIC
variants of each of these.These changes were prompted by a thread on the Brooklyn mailing list last December: http://markmail.org/thread/xpr6nsyimv7goewy. Some of the items discussed in that thread have been left for future work. It depends on PRs #497 and #524.
I'd like opinions on:naming: "BasicLocationNetworkInfoCustomizer" is cumbersome.whether the class should extendBasicJcloudsLocationCustomizer
. I can see it being useful to anyone that wants to subclass BLNIC but at the moment it's totally unused.whether we should include thepublishNetworks
option right now. It controls the publication ofhost.address.{public,private}.n
sensors. The idea is good but the feature feels a bit half-baked right now.whetherManagementAddressResolveOptions
is the right way to convey information between the location and the customiser. Again, it's a bit ungainly.