nacx commented on this pull request.
Thanks @btrishkin!
> + final DimensionDataCloudControlApi api) {
+ this.nodeNamingConvention = checkNotNull(namingConvention,
"namingConvention").createWithoutPrefix();
+ this.locations = checkNotNull(locations, "locations");
+ this.baseImageToImage = checkNotNull(baseImageToImage,
"baseImageToImage");
+ this.baseImageToHardware = checkNotNull(baseImageToHardware,
"osImageToHardware");
+ this.credentialStore = checkNotNull(credentialStore, "credentialStore
cannot be null");
+ this.api = checkNotNull(api, "api cannot be null");
+ }
+
+ @Override
+ public NodeMetadata apply(final ServerWithExternalIp serverWithExternalIp) {
+ NodeMetadataBuilder builder = new NodeMetadataBuilder();
+ Server server = serverWithExternalIp.server();
+ builder.ids(server.id());
+ builder.name(server.name());
+ builder.hostname(serverWithExternalIp.server().description());
Is this really the hostname? Don't put that here if not.
> + this.nodeNamingConvention = checkNotNull(namingConvention,
> "namingConvention").createWithoutPrefix();
+ this.locations = checkNotNull(locations, "locations");
+ this.baseImageToImage = checkNotNull(baseImageToImage,
"baseImageToImage");
+ this.baseImageToHardware = checkNotNull(baseImageToHardware,
"osImageToHardware");
+ this.credentialStore = checkNotNull(credentialStore, "credentialStore
cannot be null");
+ this.api = checkNotNull(api, "api cannot be null");
+ }
+
+ @Override
+ public NodeMetadata apply(final ServerWithExternalIp serverWithExternalIp) {
+ NodeMetadataBuilder builder = new NodeMetadataBuilder();
+ Server server = serverWithExternalIp.server();
+ builder.ids(server.id());
+ builder.name(server.name());
+ builder.hostname(serverWithExternalIp.server().description());
+ if (server.datacenterId() != null) {
Under which circumstances can this be null?
> + builder.imageId(image.getId());
+ builder.operatingSystem(image.getOperatingSystem());
+ }
+ if (server.state() != null) {
+ builder.status(serverStateToNodeStatus.get(server.state()));
+ }
+
+ String privateAddress = null;
+ if (server.networkInfo() != null && server.networkInfo().primaryNic() !=
null
+ && server.networkInfo().primaryNic().privateIpv4() != null) {
+ privateAddress = server.networkInfo().primaryNic().privateIpv4();
+ builder.privateAddresses(ImmutableSet.of(privateAddress));
+ }
+ if (privateAddress != null && serverWithExternalIp.externalIp() != null)
{
+
builder.publicAddresses(ImmutableSet.of(serverWithExternalIp.externalIp()));
+ }
We should consider not only the primary NIC here. Let's return all IP addresses
of the server.
> + builder.imageId(image.getId());
+ builder.operatingSystem(image.getOperatingSystem());
+ }
+ if (server.state() != null) {
+ builder.status(serverStateToNodeStatus.get(server.state()));
+ }
+
+ String privateAddress = null;
+ if (server.networkInfo() != null && server.networkInfo().primaryNic() !=
null
+ && server.networkInfo().primaryNic().privateIpv4() != null) {
+ privateAddress = server.networkInfo().primaryNic().privateIpv4();
+ builder.privateAddresses(ImmutableSet.of(privateAddress));
+ }
+ if (privateAddress != null && serverWithExternalIp.externalIp() != null)
{
+
builder.publicAddresses(ImmutableSet.of(serverWithExternalIp.externalIp()));
+ }
BTW, why read the external IP only if there is a private IP?
> + String privateAddress = null;
+ if (server.networkInfo() != null && server.networkInfo().primaryNic() !=
null
+ && server.networkInfo().primaryNic().privateIpv4() != null) {
+ privateAddress = server.networkInfo().primaryNic().privateIpv4();
+ builder.privateAddresses(ImmutableSet.of(privateAddress));
+ }
+ if (privateAddress != null && serverWithExternalIp.externalIp() != null)
{
+
builder.publicAddresses(ImmutableSet.of(serverWithExternalIp.externalIp()));
+ }
+
+ // DimensionData does not provide a way to get the credentials.
+ // Try to return them from the credential store
+ Credentials credentials = credentialStore.get("node#" + server.id());
+ if (credentials instanceof LoginCredentials) {
+ builder.credentials(LoginCredentials.class.cast(credentials));
+ }
Remove this. The credentials are [already set by
jclouds](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/strategy/impl/AdaptingComputeServiceStrategies.java#L89-L90).
> + }
+
+ // DimensionData does not provide a way to get the credentials.
+ // Try to return them from the credential store
+ Credentials credentials = credentialStore.get("node#" + server.id());
+ if (credentials instanceof LoginCredentials) {
+ builder.credentials(LoginCredentials.class.cast(credentials));
+ }
+
+ return builder.build();
+ }
+
+ BaseImage getBaseImage(final String sourceImageId) {
+ BaseImage baseImage = api.getServerImageApi().getOsImage(sourceImageId);
+ return baseImage == null ?
api.getServerImageApi().getCustomerImage(sourceImageId) : baseImage;
+ }
This means two additional calls for each existing node, to get the image info.
Instead of doing this, could you inject the image supplier to this class?
jclouds caches the results of the `ComptueService.listImages` call (as it is
usually an expensive one). You could look for the image in the list of cached
images and save these two calls for each node (that would be a significant
amount of extra calls when calling `listNodes` in large environments).
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/428#pullrequestreview-85254258
