You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by andreaturli <gi...@git.apache.org> on 2018/02/21 11:14:10 UTC
[GitHub] brooklyn-server pull request #949: Improve JcloudsWinrmingLiveTest coverage
GitHub user andreaturli opened a pull request:
https://github.com/apache/brooklyn-server/pull/949
Improve JcloudsWinrmingLiveTest coverage
- add Azure ARM
- improve error message for DefaultConnectivityResolver
---
When `DefaultConnectivityResolver.getReachableAddresses` now fails when no reachable address are found with this error message
```
2018-02-21 11:47:15,502 ERROR None of the addresses [168.63.120.198, 10.0.0.4] of node {id=brp4hwrr/brp4hwrr-9f7, providerId=/subscriptions/012e832d-7b27-4c30-9f21-22cdd9159d12/resourceGroups/brp4hwrr/providers/Microsoft.Compute/virtualMachines/brp4hwrr-9f7, name=brp4hwrr-9f7, location={scope=REGION, id=westeurope, description=West Europe, parent=azurecompute-arm, iso3166Codes=[NL]}, group=brp4hwrr, imageId=westeurope/MicrosoftWindowsServer/WindowsServer/2016-Datacenter, os={family=windows, version=2016-Datacenter, description=2016-Datacenter, is64Bit=true}, status=RUNNING[ProvisioningState/succeeded,PowerState/running], loginPort=5985, hostname=brp4hwrr-9f7, privateAddresses=[10.0.0.4], publicAddresses=[168.63.120.198], hardware={id=westeurope/Standard_B1s, providerId=Standard_B1s, name=Standard_B1s, location={scope=REGION, id=westeurope, description=West Europe, parent=azurecompute-arm, iso3166Codes=[NL]}, processors=[{cores=1.0, speed=2.0}], ram=1024, volumes=[{type=LOCAL, siz
e=2048.0, bootDevice=false, durable=false}, {type=LOCAL, size=1047552.0, bootDevice=false, durable=false}], supportsImage=Predicates.alwaysTrue(), userMetadata={maxDataDiskCount=2}}, loginUser=jclouds, userMetadata={Name=brp4hwrr-idg4, brooklyn-user=andrea, jclouds_group=brp4hwrr}} are reachable in mode PREFER_PUBLIC
```
instead of the IllegalStateException
```
jclouds did not return any IP addresses matching " + getNetworkMode() + " in " + location
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/andreaturli/brooklyn-server improvements/win-arm-messages
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/949.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #949
----
commit e5b70df0185f685d5541dc7303742c85849cecd1
Author: andreaturli <an...@...>
Date: 2018-02-21T11:10:32Z
Improve JcloudsWinrmingLiveTest coverage
- add Azure ARM
- improve error message for DefaultConnectivityResolver
----
---
[GitHub] brooklyn-server issue #949: Improve JcloudsWinrmingLiveTest coverage
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on the issue:
https://github.com/apache/brooklyn-server/pull/949
@andreaturli looks good. I've left two minor comments that needn't stop this being merged if you wish.
---
[GitHub] brooklyn-server pull request #949: Improve JcloudsWinrmingLiveTest coverage
Posted by andreaturli <gi...@git.apache.org>.
Github user andreaturli commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/949#discussion_r169926335
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/DefaultConnectivityResolver.java ---
@@ -342,7 +343,12 @@ void publishNetworks(NodeMetadata node, Entity entity) {
protected Iterable<HostAndPort> getReachableAddresses(NodeMetadata node, Predicate<? super HostAndPort> reachablePredicate, Duration timeout) {
if (timeout == null) timeout = Duration.FIVE_MINUTES;
Iterable<String> candidates = getResolvableAddressesWithMode(node);
- return JcloudsUtil.getReachableAddresses(candidates, node.getLoginPort(), timeout, reachablePredicate);
+ Iterable<HostAndPort> result = JcloudsUtil.getReachableAddresses(candidates, node.getLoginPort(), timeout, reachablePredicate);
+ if (FluentIterable.from(result).filter(Predicates.equalTo(Optional.absent())).isEmpty()) {
+ LOG.error("None of the addresses {} of node {} are reachable in mode {}", new Object[]{Iterables.toString(candidates), node, getNetworkMode()});
+ throw new IllegalStateException("Could not find any reachable address for node: " + node + " in mode: " + getNetworkMode());
--- End diff --
I think it is to avoid 185 to be executed, but we could change 215 as well
---
[GitHub] brooklyn-server issue #949: Improve JcloudsWinrmingLiveTest coverage
Posted by andreaturli <gi...@git.apache.org>.
Github user andreaturli commented on the issue:
https://github.com/apache/brooklyn-server/pull/949
@sjcorbett is it ok for you now?
---
[GitHub] brooklyn-server pull request #949: Improve JcloudsWinrmingLiveTest coverage
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/949#discussion_r169928209
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/DefaultConnectivityResolver.java ---
@@ -342,7 +343,12 @@ void publishNetworks(NodeMetadata node, Entity entity) {
protected Iterable<HostAndPort> getReachableAddresses(NodeMetadata node, Predicate<? super HostAndPort> reachablePredicate, Duration timeout) {
if (timeout == null) timeout = Duration.FIVE_MINUTES;
Iterable<String> candidates = getResolvableAddressesWithMode(node);
- return JcloudsUtil.getReachableAddresses(candidates, node.getLoginPort(), timeout, reachablePredicate);
+ Iterable<HostAndPort> result = JcloudsUtil.getReachableAddresses(candidates, node.getLoginPort(), timeout, reachablePredicate);
+ if (FluentIterable.from(result).filter(Predicates.equalTo(Optional.absent())).isEmpty()) {
+ LOG.error("None of the addresses {} of node {} are reachable in mode {}", new Object[]{Iterables.toString(candidates), node, getNetworkMode()});
+ throw new IllegalStateException("Could not find any reachable address for node: " + node + " in mode: " + getNetworkMode());
--- End diff --
If you want to avoid that I think it would be cleaner to wrap 185-203 in an `if (!Iterables.isEmpty(managementCandidates))`
---
[GitHub] brooklyn-server pull request #949: Improve JcloudsWinrmingLiveTest coverage
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/949#discussion_r170685601
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/DefaultConnectivityResolver.java ---
@@ -212,8 +217,10 @@ public ManagementAddressResolveResult resolve(
}
if (hapChoice == null) {
- throw new IllegalStateException("jclouds did not return any IP addresses matching " + getNetworkMode() + " in " + location);
+ LOG.error("None of the addresses of node {} are reachable in mode {}", new Object[]{node, getNetworkMode()});
+ throw new IllegalStateException("Could not find any reachable address for node: " + node + " in mode: " + getNetworkMode());
--- End diff --
Strictly speaking Brooklyn didn't look for a "reachable" address if ConnectivityResolverOptions.pollForReachableAddresses was false. How about "Could not determine management address for node..."?
---
[GitHub] brooklyn-server pull request #949: Improve JcloudsWinrmingLiveTest coverage
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/949#discussion_r169910351
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/DefaultConnectivityResolver.java ---
@@ -342,7 +343,12 @@ void publishNetworks(NodeMetadata node, Entity entity) {
protected Iterable<HostAndPort> getReachableAddresses(NodeMetadata node, Predicate<? super HostAndPort> reachablePredicate, Duration timeout) {
if (timeout == null) timeout = Duration.FIVE_MINUTES;
Iterable<String> candidates = getResolvableAddressesWithMode(node);
- return JcloudsUtil.getReachableAddresses(candidates, node.getLoginPort(), timeout, reachablePredicate);
+ Iterable<HostAndPort> result = JcloudsUtil.getReachableAddresses(candidates, node.getLoginPort(), timeout, reachablePredicate);
+ if (FluentIterable.from(result).filter(Predicates.equalTo(Optional.absent())).isEmpty()) {
+ LOG.error("None of the addresses {} of node {} are reachable in mode {}", new Object[]{Iterables.toString(candidates), node, getNetworkMode()});
+ throw new IllegalStateException("Could not find any reachable address for node: " + node + " in mode: " + getNetworkMode());
--- End diff --
Why not change the existing message on line 215?
---
[GitHub] brooklyn-server pull request #949: Improve JcloudsWinrmingLiveTest coverage
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/949
---
[GitHub] brooklyn-server pull request #949: Improve JcloudsWinrmingLiveTest coverage
Posted by andreaturli <gi...@git.apache.org>.
Github user andreaturli commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/949#discussion_r169930310
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/DefaultConnectivityResolver.java ---
@@ -342,7 +343,12 @@ void publishNetworks(NodeMetadata node, Entity entity) {
protected Iterable<HostAndPort> getReachableAddresses(NodeMetadata node, Predicate<? super HostAndPort> reachablePredicate, Duration timeout) {
if (timeout == null) timeout = Duration.FIVE_MINUTES;
Iterable<String> candidates = getResolvableAddressesWithMode(node);
- return JcloudsUtil.getReachableAddresses(candidates, node.getLoginPort(), timeout, reachablePredicate);
+ Iterable<HostAndPort> result = JcloudsUtil.getReachableAddresses(candidates, node.getLoginPort(), timeout, reachablePredicate);
+ if (FluentIterable.from(result).filter(Predicates.equalTo(Optional.absent())).isEmpty()) {
+ LOG.error("None of the addresses {} of node {} are reachable in mode {}", new Object[]{Iterables.toString(candidates), node, getNetworkMode()});
+ throw new IllegalStateException("Could not find any reachable address for node: " + node + " in mode: " + getNetworkMode());
--- End diff --
ok, although we need to check for `!FluentIterable.from(managementCandidates).filter(Predicates.equalTo(Optional.absent())).isEmpty()` as managementCandidates can't be empty
---
[GitHub] brooklyn-server pull request #949: Improve JcloudsWinrmingLiveTest coverage
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/949#discussion_r170685134
--- Diff: locations/jclouds/src/main/java/org/apache/brooklyn/location/jclouds/DefaultConnectivityResolver.java ---
@@ -181,24 +183,27 @@ public ManagementAddressResolveResult resolve(
LoginCredentials credChoice = null;
final Iterable<HostAndPort> managementCandidates = getManagementCandidates(location, node, config, options);
- final Iterable<LoginCredentials> credentialCandidates = getCredentialCandidates(location, node, options, config);
-
- // Try each pair of address and credential until one succeeds.
- if (shouldCheckCredentials() && options.pollForReachableAddresses()) {
- for (HostAndPort hap : managementCandidates) {
- for (LoginCredentials cred : credentialCandidates) {
- LOG.trace("Testing host={} with credential={}", hap, cred);
- if (checkCredential(location, hap, cred, config, options.isWindows())) {
- hapChoice = hap;
- credChoice = cred;
- break;
+ Iterable<LoginCredentials> credentialCandidates = Lists.newArrayList();
--- End diff --
You could use `Collections.emptyList()` or `ImmutableList.of()` here to save the redundant creation of an object. You could also assign it in an `else` block for the list so its use is a little clearer.
---