You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Ladislav Thon <no...@github.com> on 2016/07/11 08:43:54 UTC
[jclouds/jclouds-labs] Azure ARM improvements (#298)
@ritazh What is your preferred procedure for suggesting changes to the Azure ARM provider? I understand that it's still heavily work in progress, but I wanted to try it and provide feedback. And I thought that a PR would be the best feedback :-)
Other than these 2 changes, my only objection is that listing all images is painfully slow, because it has to perform O(numLocations * numPublishers * numOffers * numSkus * numVersions) API requests. Even with the default configuration of `"Canonical,RedHat"`, it still takes like 5 minutes for me. I wouldn't mind if listing all images wasn't performed during node creation. (It's also the motivation for one of the commits that replaces a call to `listImages` inside `getImage` with a single direct API request, but sadly, that doesn't fix everything.)
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/298
-- Commit Summary --
* [JCLOUDS-664] make getImage(id) only do a single API request
* [JCLOUDS-664] fill in the private IP addresses in NodeMetadata
-- File Changes --
M azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/AzureComputeServiceAdapter.java (17)
M azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/functions/DeploymentToNodeMetadata.java (14)
M azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/domain/VMDeployment.java (2)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/298.patch
https://github.com/jclouds/jclouds-labs/pull/298.diff
---
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/298
Re: [jclouds/jclouds-labs] [JCLOUDS-664] Azure ARM improvements
(#298)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @Ladicek and @ritazh! Squashed, rebased, and pushed to master as [913e4be9](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/913e4be9).
---
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/298#issuecomment-233206131
Re: [jclouds/jclouds-labs] [JCLOUDS-664] Azure ARM improvements
(#298)
Posted by Ladislav Thon <no...@github.com>.
@Ladicek pushed 1 commit.
d65496f [JCLOUDS-664] unify how getNode and listNodes build the VMDeployment
---
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/298/files/0b440613bcd6531f58f3f046556c0986a1461fb7..d65496f41a2d92aaa8984f86da7dea0ca564631d
Re: [jclouds/jclouds-labs] [JCLOUDS-664] Azure ARM improvements
(#298)
Posted by Ignasi Barrera <no...@github.com>.
Closed #298.
---
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/298#event-725544722
Re: [jclouds/jclouds-labs] [JCLOUDS-664] Azure ARM improvements
(#298)
Posted by Ignasi Barrera <no...@github.com>.
> Even with the default configuration of "Canonical,RedHat", it still takes like 5 minutes for me. I wouldn't mind if listing all images wasn't performed during node creation.
Take into account that this is an expensive operation in many cloud providers, and that's the reason why jclouds caches the images by default during [the session](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/Constants.java#L93-L98), so if you perform several operations using the same context without closing it, the only expensive one should be the first one. The session value can be configured to a higher one in the api metadata if that makes sense.
---
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/298#issuecomment-231683091
Re: [jclouds/jclouds-labs] [JCLOUDS-664] Azure ARM improvements
(#298)
Posted by Ladislav Thon <no...@github.com>.
I've improved the process of obtaining the private IP addresses so that it doesn't depend on how this JClouds provider sets up the resources (similarly to how public IP addresses are obtained). I think this could be merged (?).
@ritazh, thanks! Restricting the set of locations to a single once makes a huge difference. Maybe that option could be added to the provider?
For now, I went with increasing the session timeout as advised by @nacx. Paying the cost only once seems acceptable.
---
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/298#issuecomment-232384668
Re: [jclouds/jclouds-labs] [JCLOUDS-664] Azure ARM improvements
(#298)
Posted by Rita Zhang <no...@github.com>.
Thanks @Ladicek! The update LGTM.
> Restricting the set of locations to a single once makes a huge difference. Maybe that option could be added to the provider?
I can add this as a provider specific property later similar to the `jclouds.azurecompute.arm.publishers` property
---
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/298#issuecomment-232480968
Re: [jclouds/jclouds-labs] [JCLOUDS-664] Azure ARM improvements
(#298)
Posted by Ignasi Barrera <no...@github.com>.
There are already properties to configure the locations. Providers that have a known set of locations configure them via properties instead of using api calls.
If we want to make locations configurable, we should change the provider to use the jclouds configuration properties instead of the api calls. Give me a few minutes and I'll point you to a comment I wrote some time ago explaining how this works, in case you want to consider it :)
---
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/298#issuecomment-232481992
Re: [jclouds/jclouds-labs] [JCLOUDS-664] Azure ARM improvements
(#298)
Posted by Ignasi Barrera <no...@github.com>.
>And in another commit, I converted VMDeployment to use AutoValue. I'm not 100% sure about @SerializedNames, but other than that, I think it should be OK. TBH, VMDeployment is just an internal DTO class that isn't returned by any Azure API nor is it a part of this provider's API, so I think it could have easily stayed as it was, but whatever :-)
We want to unify the way we code our POJOs and domain objects and use AutoValue as it provides out of the box some features we were manually enforcing: immutability, consistent implementations of equals/hashcode, clean builders, etc. It is not meant just to be used to map the provider API model, but the way we want to go to have a proper immutable model. Even when coding internal POJOs, we should try to use it.
You are right regarding the `@SerializedNames` annotation; it is not needed in these classes as they're not going to be (de)serialized, so feel free to remove them if you want. That would be good, as having them there could cause some confusion.
Anyway, code LGTM :) Thanks! Will merge in a while.
---
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/298#issuecomment-232795199
Re: [jclouds/jclouds-labs] [JCLOUDS-664] Azure ARM improvements
(#298)
Posted by Ladislav Thon <no...@github.com>.
Makes sense. I removed the `@SerializedNames` annotation and repushed the last commit.
---
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/298#issuecomment-232868152
Re: [jclouds/jclouds-labs] [JCLOUDS-664] Azure ARM improvements
(#298)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @Ladicek!
>What is your preferred procedure for suggesting changes to the Azure ARM provider? I understand that it's still heavily work in progress, but I wanted to try it and provide feedback. And I thought that a PR would be the best feedback :-)
PRs, design discussions and feedback are really great in this early stage or the provider :)
The code LGTM. I'd only change one thing: could you change the VMDeployment class to use AutoValue, similar to VMImage, VmHardware, etc? This was unnoticed before and now that you are adding fields to the class it would be a good time to fix that. Apart from that, @ritazh good to merge?
---
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/298#issuecomment-232476392
Re: [jclouds/jclouds-labs] [JCLOUDS-664] Azure ARM improvements
(#298)
Posted by Rita Zhang <no...@github.com>.
Hi @Ladicek Sorry for the delayed response and thank you for your contribution.
To speed up `listImages()`, you could set the `jclouds.azurecompute.arm.publishers` property to just `Canonical` or just `RedHat`. For development and testing only, to speed up everything, you could updating the existing code with the following in `AzureComputeServiceAdapter`:
in `listHardwareProfiles()`:
```
VMHardware hwProfile = new VMHardware();
hwProfile.name = "Standard_A5";
hwProfile.numberOfCores = 2;
hwProfile.osDiskSizeInMB = 1047552;
hwProfile.resourceDiskSizeInMB = 138240;
hwProfile.memoryInMB = 14336;
hwProfile.maxDataDiskCount = 4;
hwProfile.location = "westus";
hwProfiles.add(hwProfile);
```
in `listLocations()`:
```
List<Location> locations = new ArrayList<Location>();
Location loc = Location.create("westus", "westus", "westus", 20, 20);
locations.add(loc);
```
---
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/298#issuecomment-232183317
Re: [jclouds/jclouds-labs] [JCLOUDS-664] Azure ARM improvements
(#298)
Posted by Ladislav Thon <no...@github.com>.
I pushed a commit that unifies how `getNode` and `listNodes` build the `VMDeployment`. This actually fixes an omission from the previous commit, where the result of `getNode` does contain the private IP addresses, but the result of `listNodes` doesn't.
And in another commit, I converted `VMDeployment` to use AutoValue. I'm not 100% sure about `@SerializedNames`, but other than that, I think it should be OK. TBH, `VMDeployment` is just an internal DTO class that isn't returned by any Azure API nor is it a part of this provider's API, so I think it could have easily stayed as it was, but whatever :-)
---
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/298#issuecomment-232612092