You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by Aled Sage <no...@github.com> on 2013/09/10 13:33:21 UTC
[jclouds] Fix SoftLayer ram to use MB (#147)
Fixes https://issues.apache.org/jira/browse/JCLOUDS-269
Note I couldn't regression test all of the live tests because I need the fix in https://github.com/jclouds/jclouds/pull/77 to be merged, for those to work.
You can merge this Pull Request by running:
git pull https://github.com/aledsage/jclouds fix/softlayer-ram-in-MB
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/147
-- Commit Summary --
* SoftLayer: report ram as MB rather than GB
* Fix softlayer getAssignableLocations: missing NL
-- File Changes --
M providers/softlayer/src/main/java/org/jclouds/softlayer/SoftLayerProviderMetadata.java (2)
M providers/softlayer/src/main/java/org/jclouds/softlayer/compute/functions/ProductItemsToHardware.java (7)
M providers/softlayer/src/test/java/org/jclouds/softlayer/compute/SoftLayerTemplateBuilderLiveTest.java (14)
M providers/softlayer/src/test/java/org/jclouds/softlayer/compute/functions/ProductItemsToHardwareTest.java (4)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/147.patch
https://github.com/jclouds/jclouds/pull/147.diff
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by Andrew Phillips <no...@github.com>.
> @aledsage Thanks for the report; I pushed a fix to master and 1.6.x.
@andrewgaul: Double commit? Or were you referring to https://github.com/jclouds/jclouds/pull/148 here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147#issuecomment-24500585
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by Andrew Phillips <no...@github.com>.
> Shall I e-mail jclouds dev and jclouds users, then you merge it?
I think we can happily submit and merge both PRs and _then_ notify. After all, we can always revert if there's some huge outcry...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147#issuecomment-24302485
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by Andrew Phillips <no...@github.com>.
Looks good to me - just waiting on BuildHive et al. now.
> Not sure what you meant by "rebase-n-squash": I can't squash as there's only one commit now.
I guess "squash of one" is simply a noop ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147#issuecomment-24498229
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #671](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/671/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147#issuecomment-24155355
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by Andrew Phillips <no...@github.com>.
@aledsage: Just because the NL change also resulted in one comment: could you split that out into a separate PR and rebase-n-squash this one? I think we should be ready to go on fixing JCLOUDS-269, at least.
@nacx: Would this work for you?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147#issuecomment-24492433
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by Aled Sage <no...@github.com>.
@demobox I've remove the NL fix from this pull request (so note that the associated live test will again fail; will submit that separately soon).
I've rebased against 1.6.x latest.
Not sure what you meant by "rebase-n-squash": I can't squash as there's only one commit now.
Good to merge now, I hope.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147#issuecomment-24496931
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #210](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/210/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147#issuecomment-24155294
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by Ignasi Barrera <no...@github.com>.
Yes, it does :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147#issuecomment-24492885
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #225](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/225/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147#issuecomment-24499441
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by Andrew Phillips <no...@github.com>.
Committed to [master](https://git-wip-us.apache.org/repos/asf?p=incubator-jclouds.git;a=commit;h=404870c705f2ed55956e3638f7ddeaad20a8a703) and [1.6.x](https://git-wip-us.apache.org/repos/asf?p=incubator-jclouds.git;a=commit;h=4f772c4d222dbdaf50865db9fbc1a110339e46d1)
Thanks, @aledsage and @nacx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147#issuecomment-24500031
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by Aled Sage <no...@github.com>.
Thanks for comments @demobox
Agree don't like breaking backwards compatibility for those wanted 16GB not getting the smallest, but feels like the right thing moving forwards. I could add a warning if the value is less than 100 or something like that (but haven't seen that pattern in other jclouds places)?
If user requests 1536MB... when we're collecting the "pricing info" for hardware profiles, getting back 8 and 16, we'll convert those to 1*1024 and 2*1024 for the actual `Hardware` instance. The `TemplateBuilderImpl.hardwareRamPredicate` will do `2048 > 1536` to return true as an acceptable hardware profile. So I think that's fine.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147#issuecomment-24241277
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by Aled Sage <no...@github.com>.
Note that the unrelated `TransientBlobIntegrationTest.testPutIncorrectContentMD5` is failing for me after rebase with `mvn clean install`. I've asked on IRC about it:
Andrew Gaul: guessing it should `SkipException` in same way as FilesystemBlobIntegrationTest?
If that's the right fix, then happy to do that in a separate pull request...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147#issuecomment-24497083
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by Andrew Phillips <no...@github.com>.
Closed #147.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by David Toy <no...@github.com>.
> @@ -91,7 +91,7 @@ protected Builder() {
> .apiMetadata(new SoftLayerApiMetadata())
> .homepage(URI.create("http://www.softlayer.com"))
> .console(URI.create("https://manage.softlayer.com"))
> - .iso3166Codes("SG","US-CA","US-TX","US-VA","US-WA","US-TX")
> + .iso3166Codes("SG","US-CA","US-TX","US-VA","US-WA","US-TX", "NL")
"NL-NH" is probably most accurate for Amsterdam.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147/files#r6321586
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds #398](https://buildhive.cloudbees.com/job/jclouds/job/jclouds/398/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147#issuecomment-24155009
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by Andrew Gaul <no...@github.com>.
@aledsage Thanks for the report; I pushed a fix to master and 1.6.x.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147#issuecomment-24499884
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by Andrew Phillips <no...@github.com>.
Code itself looks good - just wondering about the breaking change and what users would be doing right now. I'm guessing the only way to use SoftLayer code currently is to know that RAM is actually in GB, and now if you just continue you'll end up requesting a machine with 1Mb RAM.
Also: what should happen if a user now requests 1536Mb? Does that work in SoftLayer? Do we need to convert to the nearest Gb?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147#issuecomment-24221214
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by Ignasi <ig...@gmail.com>.
My main worries were about silently breaking the code of existing users,
who may already jave that kind of ugly conditionals.
If there is confidence about that won't happen (nobody has expressed an
opinion against in the ML, so we can assume that), then I'm ok with having
this change in 1.6.x too.
Thanks for clarifying Aled!
El 16/09/2013 01:47, "Aled Sage" <no...@github.com> escribió:
> We certainly need it urgently in the 1.6.x branch. If it's not in until
> 1.7 release, then we'll work around it with an ugly `if
> (provider.equals("softlayer")` block to change MBs passed in to the GBs it
> currently expects. Anyone else who's using generic jclouds code to run
> against softlayer + other clouds will have to do the same (or have
> inconsistent units in their config files or some such).
>
> My gut feel is that pain for new users will be greater than the pain to
> existing users of fixing it - new users will all get it wrong if they
> follow the standard jclouds examples (i.e. they use MBs); existing users
> have been alerted via the mailing list (and it can be in the release notes).
>
> Also note that https://github.com/jclouds/jclouds/pull/77 (which fixes
> https://issues.apache.org/jira/browse/JCLOUDS-163) means softlayer is
> probably unusable in jclouds 1.6.x currently, so there might not be
> "existing users" for 1.6.x.
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/jclouds/jclouds/pull/147#issuecomment-24483258
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by Aled Sage <no...@github.com>.
We certainly need it urgently in the 1.6.x branch. If it's not in until 1.7 release, then we'll work around it with an ugly `if (provider.equals("softlayer")` block to change MBs passed in to the GBs it currently expects. Anyone else who's using generic jclouds code to run against softlayer + other clouds will have to do the same (or have inconsistent units in their config files or some such).
My gut feel is that pain for new users will be greater than the pain to existing users of fixing it - new users will all get it wrong if they follow the standard jclouds examples (i.e. they use MBs); existing users have been alerted via the mailing list (and it can be in the release notes).
Also note that https://github.com/jclouds/jclouds/pull/77 (which fixes https://issues.apache.org/jira/browse/JCLOUDS-163) means softlayer is probably unusable in jclouds 1.6.x currently, so there might not be "existing users" for 1.6.x.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147#issuecomment-24483258
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by Andrew Phillips <no...@github.com>.
> I could add a warning if the value is less than 100 or something like that (but haven't seen that pattern in other
> jclouds places)?
Agree - not something we really do elsewhere, from what I know, so feels a bit weird. Perhaps just an email to dev@ and users@ to announce the change?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147#issuecomment-24242240
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by Andrew Phillips <no...@github.com>.
@aledsage Given [Ignasi's comment](http://markmail.org/message/3hjk7eyaa66junkd). would it make sense to apply this to master only? Or do you need this for 1.6.3 urgently?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147#issuecomment-24473841
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by Andrew Gaul <no...@github.com>.
Two comments, two replies.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147#issuecomment-24501254
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #686](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/686/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147#issuecomment-24499701
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by Aled Sage <no...@github.com>.
Shall I e-mail jclouds dev and jclouds users, then you merge it? And then I submit the same PR against master branch? I'll write that e-mail now...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147#issuecomment-24301864
Re: [jclouds] Fix SoftLayer ram to use MB (#147)
Posted by Aled Sage <no...@github.com>.
@jdtoy thanks, but I'll leave it as `"NL"` for this pull request if that's ok.
I think in another part of the softlayer integration code we're also extracting the datacenter's iso3166 automatically from the json returned by softlayer, which gives us `"country":"NL"`. There's a test that asserts the extracted string is contained within the set of iso3166 codes hard-coded in the softlayer metadata (which you spotted). So changing the hard-coded value would break the test, until we did some more work...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/147#issuecomment-24351557