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 2014/01/09 01:05:05 UTC

[jclouds-labs] JCLOUDS-330 & JCLOUDS-416: Use network options in Abiquo (#47)

@andreaturli This should allow you to specify the networks that a deployed VM should get the IPs from. You can specify the networks to use with:

```java
templateBuilder.options(networks(&quot;networkId1&quot;, &quot;networkId2&quot;, ... ))
```

If not specified, the current behavior is maintained: jclouds will try to assign a public ip, and fallback to one in the default network for the selected virtual datacenter.

Could you give this a try before we merge the pull request?
You can merge this Pull Request by running:

  git pull https://github.com/nacx/jclouds-labs networkIds

Or you can view, comment on it, or merge it online at:

  https://github.com/jclouds/jclouds-labs/pull/47

-- Commit Summary --

  * JCLOUDS-330/JCLOUDS-416: Use network options in Abiquo

-- File Changes --

    M abiquo/src/main/java/org/jclouds/abiquo/compute/strategy/AbiquoComputeServiceAdapter.java (79)

-- Patch Links --

https://github.com/jclouds/jclouds-labs/pull/47.patch
https://github.com/jclouds/jclouds-labs/pull/47.diff

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/47

Re: [jclouds-labs] JCLOUDS-330 & JCLOUDS-416: Use network options in Abiquo (#47)

Posted by Andrew Phillips <no...@github.com>.
> I plan to open a PR today with an alternative implementation of the TemplateBuilderImpl 

Curious! Looking forward to that...

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/47#issuecomment-32171037

Re: [jclouds-labs] JCLOUDS-330 & JCLOUDS-416: Use network options in Abiquo (#47)

Posted by Ignasi Barrera <no...@github.com>.
The problem is the current design of the ComputeService, which I'm not happy with. I don't really like how it uses the locations and how the Abiquo concepts are mapped to the jclouds compute concepts. Even to code the strategy tests, the compute service needs to be implemented in a more reasonable way.

I'll open a thread in the ML explaining the current implementation and asking for opinion, and see if we can come up with a better approach than the current one.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/47#issuecomment-32075715

Re: [jclouds-labs] JCLOUDS-330 & JCLOUDS-416: Use network options in Abiquo (#47)

Posted by Andrew Phillips <no...@github.com>.
> +    * assigned.
> +    */
> +   private void configureNetworking(VirtualMachine vm, VirtualApplianceCachingTemplate template, Datacenter datacenter,
> +         TemplateOptions options) {
> +
> +      if (!options.getNetworks().isEmpty()) {
> +         List<Ip<?, ?>> ips = Lists.newArrayList();
> +         Enterprise enterprise = adminService.getCurrentEnterprise();
> +         Iterable<ExternalNetwork> externalNetworks = enterprise.listExternalNetworks(datacenter);
> +
> +         for (String networkId : options.getNetworks()) {
> +            Network<?> network = template.getVirtualDatacenter().getPrivateNetwork(Ints.tryParse(networkId));
> +            if (network == null) {
> +               // If the given network is not a private network, it should be an
> +               // external one
> +               network = find(externalNetworks, networkId(networkId));

`networkId(networkId)` looks a bit weird? `hasNetworkId`? `networkIdIs`?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/47/files#r8755710

Re: [jclouds-labs] JCLOUDS-330 & JCLOUDS-416: Use network options in Abiquo (#47)

Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #122](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/122/) SUCCESS
This pull request looks good

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/47#issuecomment-32075830

Re: [jclouds-labs] JCLOUDS-330 & JCLOUDS-416: Use network options in Abiquo (#47)

Posted by Andrew Phillips <no...@github.com>.
> +   /**
> +    * Configures the networkking for the created virtual machine.
> +    * <p>
> +    * If the template options have been configured with a set of network
> +    * identifiers, jclouds will assign the virtual machine one IP address of
> +    * each network.
> +    * <p>
> +    * If no network ids have been defined, jclouds will try to assign a public
> +    * IP address. If no public IP addresses are available in the user account,
> +    * then an IP address in the virtual datacenter's default network will be
> +    * assigned.
> +    */
> +   private void configureNetworking(VirtualMachine vm, VirtualApplianceCachingTemplate template, Datacenter datacenter,
> +         TemplateOptions options) {
> +
> +      if (!options.getNetworks().isEmpty()) {

Maybe change this around and escape earlier here?
```
if (options.getNetworks().isEmpty()) {
  // maybe logging here?
  return;
}
// rest here
```

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/47/files#r8755510

Re: [jclouds-labs] JCLOUDS-330 & JCLOUDS-416: Use network options in Abiquo (#47)

Posted by Andrew Phillips <no...@github.com>.
> +
> +            // Get the first available ip
> +            Ip<?, ?> availableIp = get(network.listUnusedIps(), 0);
> +            logger.debug(">> Found available %s ip %s", availableIp.getNetworkType().name().toLowerCase(),
> +                  availableIp.getIp());
> +            ips.add(availableIp);
> +         }
> +
> +         // Assign all ips to the virtual machine
> +         vm.setNics(ips);
> +      } else {
> +         Optional<PublicIp> publicIp = tryFind(template.getVirtualDatacenter().listPurchasedPublicIps(),
> +               IpPredicates.<PublicIp> notUsed());
> +         if (publicIp.isPresent()) {
> +            logger.debug(">> Found available public ip %s", publicIp.get().getIp());
> +            vm.setNics(Lists.<Ip<?, ?>> newArrayList(publicIp.get()));

`ImmutableList.of(...)`?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/47/files#r8755814

Re: [jclouds-labs] JCLOUDS-330 & JCLOUDS-416: Use network options in Abiquo (#47)

Posted by Andrew Phillips <no...@github.com>.
> @@ -269,6 +313,15 @@ public boolean apply(final VirtualMachine input) {
>        };
>     }
>  
> +   private static Predicate<Network<?>> networkId(final String id) {
> +      return new Predicate<Network<?>>() {
> +         @Override
> +         public boolean apply(final Network<?> input) {
> +            return Integer.valueOf(id).equals(input.getId());

Rather than calling `Integer.valueOf(id)` every time, make it a `final` variable in the method?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/47/files#r8755870

Re: [jclouds-labs] JCLOUDS-330 & JCLOUDS-416: Use network options in Abiquo (#47)

Posted by Ignasi Barrera <no...@github.com>.
@demobox Addressed comments. Unfortunately the ComputeServiceAdapterLiveTest is still not working in Abiquo (and this is one of the main reasons I've still never proposed to promote the provider to the main repo). There are some bits that I have to figure out. Once the test is properly configured, the corresponding network tests will be added there.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/47#issuecomment-32074875

Re: [jclouds-labs] JCLOUDS-330 & JCLOUDS-416: Use network options in Abiquo (#47)

Posted by Andrew Phillips <no...@github.com>.
Missing tests for the new logic?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/47#issuecomment-31931624

Re: [jclouds-labs] JCLOUDS-330 & JCLOUDS-416: Use network options in Abiquo (#47)

Posted by Andrew Phillips <no...@github.com>.
> @@ -260,6 +255,55 @@ public boolean apply(VirtualMachine machine) {
>        });
>     }
>  
> +   /**
> +    * Configures the networkking for the created virtual machine.

"networking"

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/47/files#r8754214

Re: [jclouds-labs] JCLOUDS-330 & JCLOUDS-416: Use network options in Abiquo (#47)

Posted by Andrew Phillips <no...@github.com>.
> @@ -260,6 +255,55 @@ public boolean apply(VirtualMachine machine) {
>        });
>     }
>  
> +   /**
> +    * Configures the networkking for the created virtual machine.
> +    * <p>
> +    * If the template options have been configured with a set of network
> +    * identifiers, jclouds will assign the virtual machine one IP address of
> +    * each network.
> +    * <p>
> +    * If no network ids have been defined, jclouds will try to assign a public
> +    * IP address. If no public IP addresses are available in the user account,
> +    * then an IP address in the virtual datacenter's default network will be
> +    * assigned.

Turn this into
* a
* bulleted
* list
?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/47/files#r8754221

Re: [jclouds-labs] JCLOUDS-330 & JCLOUDS-416: Use network options in Abiquo (#47)

Posted by Andrew Phillips <no...@github.com>.
> Unfortunately the ComputeServiceAdapterLiveTest is still not working in Abiquo

Would this be worth pulling out into a strategy with a unit test, or something like that?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/47#issuecomment-32075214

Re: [jclouds-labs] JCLOUDS-330 & JCLOUDS-416: Use network options in Abiquo (#47)

Posted by Andrew Phillips <no...@github.com>.
> I'll open a thread in the ML explaining the current implementation and asking for opinion, and see if we can come
> up with a better approach than the current one.

OK, thanks! This one is good to go then, as far as I'm concerned.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/47#issuecomment-32108074

Re: [jclouds-labs] JCLOUDS-330 & JCLOUDS-416: Use network options in Abiquo (#47)

Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-pull-requests #120](https://jclouds.ci.cloudbees.com/job/jclouds-labs-pull-requests/120/) SUCCESS
This pull request looks good

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/47#issuecomment-31891086

Re: [jclouds-labs] JCLOUDS-330 & JCLOUDS-416: Use network options in Abiquo (#47)

Posted by Ignasi Barrera <no...@github.com>.
Thanks @demobox! Squashed & merged.

I plan to open a PR today with an alternative implementation of the TemplateBuilderImpl that will make jclouds work better with providers that don't have hardware Profiles (like Abiquo) as a starting point to the Compute Service cleanup.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/47#issuecomment-32153117

Re: [jclouds-labs] JCLOUDS-330 & JCLOUDS-416: Use network options in Abiquo (#47)

Posted by Andrew Phillips <no...@github.com>.
> +    * Configures the networkking for the created virtual machine.
> +    * <p>
> +    * If the template options have been configured with a set of network
> +    * identifiers, jclouds will assign the virtual machine one IP address of
> +    * each network.
> +    * <p>
> +    * If no network ids have been defined, jclouds will try to assign a public
> +    * IP address. If no public IP addresses are available in the user account,
> +    * then an IP address in the virtual datacenter's default network will be
> +    * assigned.
> +    */
> +   private void configureNetworking(VirtualMachine vm, VirtualApplianceCachingTemplate template, Datacenter datacenter,
> +         TemplateOptions options) {
> +
> +      if (!options.getNetworks().isEmpty()) {
> +         List<Ip<?, ?>> ips = Lists.newArrayList();

`ImmutableList.builder()`?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/47/files#r8755515

Re: [jclouds-labs] JCLOUDS-330 & JCLOUDS-416: Use network options in Abiquo (#47)

Posted by Andrew Phillips <no...@github.com>.
> +      if (!options.getNetworks().isEmpty()) {
> +         List<Ip<?, ?>> ips = Lists.newArrayList();
> +         Enterprise enterprise = adminService.getCurrentEnterprise();
> +         Iterable<ExternalNetwork> externalNetworks = enterprise.listExternalNetworks(datacenter);
> +
> +         for (String networkId : options.getNetworks()) {
> +            Network<?> network = template.getVirtualDatacenter().getPrivateNetwork(Ints.tryParse(networkId));
> +            if (network == null) {
> +               // If the given network is not a private network, it should be an
> +               // external one
> +               network = find(externalNetworks, networkId(networkId));
> +            }
> +
> +            // Get the first available ip
> +            Ip<?, ?> availableIp = get(network.listUnusedIps(), 0);
> +            logger.debug(">> Found available %s ip %s", availableIp.getNetworkType().name().toLowerCase(),

Why `toLowerCase()`? For debugging, wouldn't it be easier to see the actual name?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/47/files#r8755799

Re: [jclouds-labs] JCLOUDS-330 & JCLOUDS-416: Use network options in Abiquo (#47)

Posted by Andrew Phillips <no...@github.com>.
> @@ -269,6 +313,15 @@ public boolean apply(final VirtualMachine input) {
>        };
>     }
>  
> +   private static Predicate<Network<?>> networkId(final String id) {

Inline this? It's only used in one place.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/47/files#r8755862

Re: [jclouds-labs] JCLOUDS-330 & JCLOUDS-416: Use network options in Abiquo (#47)

Posted by Andrew Phillips <no...@github.com>.
> +    */
> +   private void configureNetworking(VirtualMachine vm, VirtualApplianceCachingTemplate template, Datacenter datacenter,
> +         TemplateOptions options) {
> +
> +      if (!options.getNetworks().isEmpty()) {
> +         List<Ip<?, ?>> ips = Lists.newArrayList();
> +         Enterprise enterprise = adminService.getCurrentEnterprise();
> +         Iterable<ExternalNetwork> externalNetworks = enterprise.listExternalNetworks(datacenter);
> +
> +         for (String networkId : options.getNetworks()) {
> +            Network<?> network = template.getVirtualDatacenter().getPrivateNetwork(Ints.tryParse(networkId));
> +            if (network == null) {
> +               // If the given network is not a private network, it should be an
> +               // external one
> +               network = find(externalNetworks, networkId(networkId));
> +            }

Is the `NoSuchElementException` that will be thrown here what we want? Or convert to `IllegalArg` or `IllegalState`?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/47/files#r8755771

Re: [jclouds-labs] JCLOUDS-330 & JCLOUDS-416: Use network options in Abiquo (#47)

Posted by Andrew Phillips <no...@github.com>.
> +
> +      if (!options.getNetworks().isEmpty()) {
> +         List<Ip<?, ?>> ips = Lists.newArrayList();
> +         Enterprise enterprise = adminService.getCurrentEnterprise();
> +         Iterable<ExternalNetwork> externalNetworks = enterprise.listExternalNetworks(datacenter);
> +
> +         for (String networkId : options.getNetworks()) {
> +            Network<?> network = template.getVirtualDatacenter().getPrivateNetwork(Ints.tryParse(networkId));
> +            if (network == null) {
> +               // If the given network is not a private network, it should be an
> +               // external one
> +               network = find(externalNetworks, networkId(networkId));
> +            }
> +
> +            // Get the first available ip
> +            Ip<?, ?> availableIp = get(network.listUnusedIps(), 0);

Is `network.listUnusedIps()` guaranteed to be non-empty?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/47/files#r8755783