brooklyncentral / brooklyn

This project has moved and is now part of the ASF
https://github.com/apache/incubator-brooklyn
72 stars 27 forks source link

fix jclouds-byon for non-AWS clouds #1423

Closed ahgittin closed 10 years ago

buildhive commented 10 years ago

Brooklyn Central » brooklyn #2354 SUCCESS This pull request looks good (what's this?)

ahgittin commented 10 years ago

all good comments, addressed in imminent push

buildhive commented 10 years ago

Brooklyn Central » brooklyn #2362 SUCCESS This pull request looks good (what's this?)

grkvlt commented 10 years ago

@ahgittin OK, looks good, happy to merge now :frog:

aledsage commented 10 years ago

@ahgittin when this is used with JcloudsByonLocationResolver such as: brooklyn.location.named.myloc=jcloudsByon:(provider="softlayer",region="ams01",user="root",hosts="50.97.240.38") The "hosts" value gets passed in as id rather than as hostname, so the JcloudsLocation.rebind doesn't use it.

Was your intent that we'd try to use the rawId first as an actual id and then as a hostname or ip address?

aledsage commented 10 years ago

This changes the behaviour for softlayer-bmc. Previously it would accept the serviceProviderResourceId such as "176227". Now you have to use the global identifier, such as "488c90ef-41f3-4fa5-a5a1-36e7b11b5d78".

This is because previously it did computeService.getNode(id) which called through to SoftlayerComputeServiceAdapter.getNode which does: return api.getHardwareApi().getObject(id);

The softlayer API accepts either of the ids, I believe.

However, the NodeMetadata does not have the serviceProviderResourceId currently, e.g.:

`{id=8868ada4-0164-496c-9696-50346ee2496f, providerId=8868ada4-0164-496c-9696-50346ee2496f, name=sap-srv1, location={scope=ZONE, id=sjc01, description=San Jose 1, parent=softlayer-bmc}, group=sap, imageId=CENTOS_6_32, os={family=centos, version=6, description=CentOS / CentOS / 6.5-32, is64Bit=false}, status=RUNNING, loginPort=22, hostname=sap-srv1.saptest.ibm.com, privateAddresses=[10.54.51.7], publicAddresses=[50.97.240.38], hardware={id=1258, providerId=1258, name=CENTOS_6_32, processors=[{cores=0.0, speed=0.0}], ram=0, hypervisor=XenServer, supportsImage=ALWAYS_TRUE}}`

Perhaps we should first call computeService.getNode(id). If that doesn't find it, then try iterating through all the instances?

@andreaturli can have a look at the HardwareToNodeMetadata, but if matching on nodeMetadata.id then we basically have to choose one or the other id format. There is also NodeMetadataBuilder.providerId that we could set, but not convinced it's a good match to map serviceProviderResourceId to that.

buildhive commented 10 years ago

Brooklyn Central » brooklyn #2375 SUCCESS This pull request looks good (what's this?)

andreaturli commented 10 years ago

+1 @aledsage also, from my test, I'm not sure either about the LoginCredentials assigned during the rebinding: this assumes that only the user that is executing brooklyn (that originally launched the node using jclouds) will be able to rebind it. Is it what we want?

ahgittin commented 10 years ago

@andreaturli don't think we need to mess with credentials. normally we run AdminAccess and in any case the consumer can always supply the access details ... if looking them up when required is a big help to someone then it could be added.

the bigger irritant is the plethora of ID's. latest fix matches against IP address, hostname, id, or providerId, optionally with a region prefix. in the case of softlayer id and providerId are both the short number. as @aledsage points out the other SL ID (the long UUID) is not available on the NodeMetadata object so no quick way that I see to get it. in the case of BMC perhaps the conventions around these ID's is reversed.

in any case there should always be an id which will work and normally people can use the ip/hostname which will be easier.

ready for review and merge.

buildhive commented 10 years ago

Brooklyn Central » brooklyn #2377 SUCCESS This pull request looks good (what's this?)

andreaturli commented 10 years ago

lgtm +1