You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by Ignasi Barrera <no...@github.com> on 2013/08/23 11:10:45 UTC
[jclouds-labs] JCLOUDS-205: Do not create duplicated groups in Abiquo
(#25)
Refactored the `AbiquoComputeServiceAdapter` to avoid creating more than one group with the same name, when creating multiple nodes concurrently.
There is a little room for improvement in this pull request, and I'd appreciate some advice. Currently I've overridden the [CreateNodesWithGroupEncodedIntoNameThenAddToSet#execute](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/strategy/impl/CreateNodesWithGroupEncodedIntoNameThenAddToSet.java#L118-L128) method to create the group before starting the creation of the nodes. This works fine, but there are two (not expensive, though) api calls that are going to be repeated for each deployed node, and I'd like to save them.
In order to do that, I'd need to pass a reference to the retrieved objects to the `createNodeWithGroupEncodedIntoName` method. This would require to also override the [CreateNodesWithGroupEncodedIntoNameThenAddToSet#createNodeInGroupWithNameAndTemplate](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/strategy/impl/CreateNodesWithGroupEncodedIntoNameThenAddToSet.java#L166-L169) method, and the [CreateNodesWithGroupEncodedIntoNameThenAddToSet#AddNode](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/strategy/impl/CreateNodesWithGroupEncodedIntoNameThenAddToSet.java#L75-L83) class, so I can invoke the new method with the additional objects. This has the counterpart that the new method would not be part of the [CreateNodeWithGroupEncodedIntoName](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/strategy/CreateNodeWithGroupEncodedIntoName.java) strategy int
erface.
So:
* Do not override those method and assume those repeated api calls.
* Implement the mentioned changes even if we end up calling a method that is not part of the strategy interface (which shouldn't be a problem and shouldn't cause side-effects).
* Does anyone have better/smarter ideas?
Thx!
You can merge this Pull Request by running:
git pull https://github.com/nacx/jclouds-labs 205-creategroup
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs/pull/25
-- Commit Summary --
* JCLOUDS-205: Do not create duplicated groups in Abiquo
-- File Changes --
M abiquo/src/main/java/org/jclouds/abiquo/compute/config/AbiquoComputeServiceContextModule.java (5)
M abiquo/src/main/java/org/jclouds/abiquo/compute/strategy/AbiquoComputeServiceAdapter.java (61)
A abiquo/src/main/java/org/jclouds/abiquo/compute/strategy/CreateGroupBeforeCreatingNodes.java (107)
M abiquo/src/test/java/org/jclouds/abiquo/compute/AbiquoComputeServiceLiveTest.java (20)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/25.patch
https://github.com/jclouds/jclouds-labs/pull/25.diff
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by Ignasi Barrera <no...@github.com>.
> + public boolean apply(VirtualAppliance input) {
> + return input.getName().equals(group);
> + }
> + });
> +
> + // Create the group if still does not exist
> + if (!vapp.isPresent()) {
> + logger.debug(">> Creating group %s", group);
> + VirtualAppliance newVapp = VirtualAppliance.builder(context, vdc).name(group).build();
> + newVapp.save();
> + logger.debug("<< group(%s) created", newVapp.getId());
> + } else {
> + logger.debug(">> Using existing group(%s)", vapp.get().getId());
> + }
> +
> + return super.execute(group, count, template, goodNodes, badNodes, customizationResponses);
Thanks for the idea @demobox. I've been trying to implement this approach, but the Template object is immutable and there is no way to inject new options to it. Building a new Template seems to be not an option, as it would require using the template builder and that will go through the entire image selection process again, which is not good.
One possible option would be to expose a `virtualDatacenter` and `virtualAppliance` method in the `AbiquoTemplateOptions`, but I'd prefer to avoid that. The virtual datacenter should be configured through the location of the selected hardware profile, and the virtual appliance should be read from the group name. Exposing those methods could end in an inconsistent configuration, and will make it too easy to do "the bad thing".
The more I think about it, the more I believe the cleanest approach might be the second point mentioned in the PR description. WDYT?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/25/files#r6073514
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -53,28 +53,13 @@ public AbiquoComputeServiceLiveTest() {
> }
>
> @Override
> - public void setServiceDefaults() {
It is just hardcoded stuff that shouldn't be here if we want to properly be able to run the ComputeServiceAdapter tests.
They are still not fully working, though, but definitely this shouldn't be hardcoded here. I can put the changes in a separate PR if that makes more sense.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/25/files#r5998656
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by Ignasi Barrera <no...@github.com>.
Renamed the `VirtualApplianceCachingTemplate` builder methods accordingly, squashed the commits and merged. Thx @demobox !
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/25#issuecomment-23570683
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by Andrew Phillips <no...@github.com>.
> +import com.google.common.collect.Multimap;
> +import com.google.common.util.concurrent.ListenableFuture;
> +import com.google.common.util.concurrent.ListeningExecutorService;
> +
> +/**
> + * Creates the group before concurrently creating the nodes, to avoid creating
> + * more than one group with the same name.
> + *
> + * @author Ignasi Barrera
> + */
> +@Singleton
> +public class CreateGroupBeforeCreatingNodes extends CreateNodesWithGroupEncodedIntoNameThenAddToSet {
> +
> + private final ApiContext<AbiquoApi> context;
> +
> + private final CloudService cloudService;
`protected` in case someone needs to override this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/25/files#r5995321
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by Andrew Phillips <no...@github.com>.
> + public boolean apply(VirtualAppliance input) {
> + return input.getName().equals(group);
> + }
> + });
> +
> + // Create the group if still does not exist
> + if (!vapp.isPresent()) {
> + logger.debug(">> Creating group %s", group);
> + VirtualAppliance newVapp = VirtualAppliance.builder(context, vdc).name(group).build();
> + newVapp.save();
> + logger.debug("<< group(%s) created", newVapp.getId());
> + } else {
> + logger.debug(">> Using existing group(%s)", vapp.get().getId());
> + }
> +
> + return super.execute(group, count, template, goodNodes, badNodes, customizationResponses);
OK, so the only other thing I can imagine here is that you put the `vapp` and `vdc` in the template somehow, and use that object to pass them through to the eventual `createNode...` call.
I don't know what promises we make about the template object and how evil we consider that to be, but since that's the "recipe" for making the object and `vapp` and `vdc` seem to be part of that recipe (albeit not things the user knows about) the would seem to be the closest possible fit..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/25/files#r5995240
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by Andrew Phillips <no...@github.com>.
> @@ -199,8 +207,7 @@ public VirtualMachineTemplateInVirtualDatacenter apply(final VirtualDatacenter v
>
> @Override
> public VirtualMachineTemplate getImage(final String id) {
> - Enterprise enterprise = adminService.getCurrentEnterprise();
> - return find(enterprise.listTemplates(), new Predicate<VirtualMachineTemplate>() {
> + return find(listImages(), new Predicate<VirtualMachineTemplate>() {
How is this change related to the rest of the PR? Or is this just cleanup?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/25/files#r5995089
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by Ignasi Barrera <no...@github.com>.
@demobox I added the changes and it works fine. Thanks for the idea! If the latest changes look good to you I'll proceed with the merge.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/25#issuecomment-23545005
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #52](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/52/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/25#issuecomment-23545438
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by Ignasi Barrera <no...@github.com>.
> }
> - }, null);
> + });
That's right. The current code actually does that "createIfDoesotExist", but I noticed that there were some failures when concurrently creating nodes, so the purpose of this PR is to remove that race condition.
In the current code, the two API calls are already executed for each node, but if we can save them, the better!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/25/files#r5998612
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by Andrew Phillips <no...@github.com>.
> }
> - }, null);
> + });
One simple option would be to say that, if you have to make those two calls _anyway_ for each node, why not do the "if !group.exist then create" logic _here_? Of course, race conditions etc. make that nasty. And I guess the main aim is to say we don't _need_ to do these two calls for every node, since we already _know_ what the `vdc` and `vapp` should be..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/25/files#r5995132
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by Andrew Phillips <no...@github.com>.
> +
> + // Create the group if still does not exist
> + VirtualAppliance newVapp = null;
> + if (!vapp.isPresent()) {
> + logger.debug(">> Creating group %s", group);
> + newVapp = VirtualAppliance.builder(context, vdc).name(group).build();
> + newVapp.save();
> + logger.debug("<< group(%s) created", newVapp.getId());
> + } else {
> + logger.debug(">> Using existing group(%s)", vapp.get().getId());
> + }
> +
> + VirtualApplianceCachingTemplate abiquoTemplate = VirtualApplianceCachingTemplate //
> + .from(template) //
> + .with(vdc) //
> + .with(vapp.or(newVapp)) //
Sorry, I was a bit sloppy in my suggestion there ;-) Probably `withVdc`/`withVapp` or `withVirtualDatacenter`/`withVirtualAppliance` is a little clearer. But since this is internal, it's a minor - feel free to ignore and go ahead without changing.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/25/files#r6083941
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by Andrew Phillips <no...@github.com>.
> + public boolean apply(VirtualAppliance input) {
> + return input.getName().equals(group);
> + }
> + });
> +
> + // Create the group if still does not exist
> + if (!vapp.isPresent()) {
> + logger.debug(">> Creating group %s", group);
> + VirtualAppliance newVapp = VirtualAppliance.builder(context, vdc).name(group).build();
> + newVapp.save();
> + logger.debug("<< group(%s) created", newVapp.getId());
> + } else {
> + logger.debug(">> Using existing group(%s)", vapp.get().getId());
> + }
> +
> + return super.execute(group, count, template, goodNodes, badNodes, customizationResponses);
> The more I think about it, the more I believe the cleanest approach might be the second point mentioned in the PR description.
> WDYT?
Worth a try, for sure, but the fact that you have to override so many methods hints at the fact that you're trying to do something "differently from the intended way", not that that's a problem ;-)
Alternatively, how about something like
```
VappCachingAbiquoTemplate extends AbiquoTemplateOptions { // package or protected scope, so not accessible to users
// extra fields to store what we need
}
VappCachingAbiquoTemplate cachingTemplate = VappCachingAbiquoTemplate.from(template).with(vApp).with(otherStuffWeNeed);
return super.execute(group, count, cachingTemplate, goodNodes, badNodes, customizationResponses);
...
// at the use-site, cast to VappCachingAbiquoTemplate to get the value out. You could even handle the case where it's **not** a
// VappCachingAbiquoTemplate (if that use case makes sense) by getting the vApp etc. again
```
?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/25/files#r6074763
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -199,8 +207,7 @@ public VirtualMachineTemplateInVirtualDatacenter apply(final VirtualDatacenter v
>
> @Override
> public VirtualMachineTemplate getImage(final String id) {
> - Enterprise enterprise = adminService.getCurrentEnterprise();
> - return find(enterprise.listTemplates(), new Predicate<VirtualMachineTemplate>() {
> + return find(listImages(), new Predicate<VirtualMachineTemplate>() {
It is just cleanup to keep this method aligned with the listImages one, as the code they use is the same. Just avoid duplicating code.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/25/files#r5998621
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by Andrew Phillips <no...@github.com>.
> @@ -53,28 +53,13 @@ public AbiquoComputeServiceLiveTest() {
> }
>
> @Override
> - public void setServiceDefaults() {
The changes in this file are also unrelated cleanup?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/25/files#r5995298
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #362](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/362/) UNSTABLE
Looks like there's a problem with this pull request
[(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-labs/pull/25#issuecomment-23571127
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by Andrew Phillips <no...@github.com>.
> jclouds » jclouds-labs #361 UNSTABLE
These are all related to [JCLOUDS-257](https://issues.apache.org/jira/browse/JCLOUDS-257), not to this issue.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/25#issuecomment-23554849
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by Andrew Phillips <no...@github.com>.
> @@ -53,28 +53,13 @@ public AbiquoComputeServiceLiveTest() {
> }
>
> @Override
> - public void setServiceDefaults() {
> I can put the changes in a separate PR if that makes more sense.
I think they're fine here, just wanted to know if/how they were related to the rest.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/25/files#r6007099
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #53](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/53/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/25#issuecomment-23571175
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by Ignasi Barrera <no...@github.com>.
> + public boolean apply(VirtualAppliance input) {
> + return input.getName().equals(group);
> + }
> + });
> +
> + // Create the group if still does not exist
> + if (!vapp.isPresent()) {
> + logger.debug(">> Creating group %s", group);
> + VirtualAppliance newVapp = VirtualAppliance.builder(context, vdc).name(group).build();
> + newVapp.save();
> + logger.debug("<< group(%s) created", newVapp.getId());
> + } else {
> + logger.debug(">> Using existing group(%s)", vapp.get().getId());
> + }
> +
> + return super.execute(group, count, template, goodNodes, badNodes, customizationResponses);
Cool! That should do the trick and keep it simple and clean. Will give it a try and update the PR. Thx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/25/files#r6074932
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #48](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/48/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/25#issuecomment-23152676
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #361](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/361/) UNSTABLE
Looks like there's a problem with this pull request
[(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-labs/pull/25#issuecomment-23545668
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #51](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/51/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/25#issuecomment-23545369
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #360](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/360/) UNSTABLE
Looks like there's a problem with this pull request
[(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-labs/pull/25#issuecomment-23545265
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by Andrew Phillips <no...@github.com>.
> Renamed the VirtualApplianceCachingTemplate builder methods accordingly, squashed the commits and merged.
Nice, thanks @nacx !
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/25#issuecomment-23572472
Re: [jclouds-labs] JCLOUDS-205: Do not create duplicated groups in
Abiquo (#25)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs #327](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs/327/) 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-labs/pull/25#issuecomment-23152501