jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ignasi Barrera <notificati...@github.com>
Subject Re: [jclouds/jclouds] [JCLOUDS-1382] improve usage of Neutron capabilities from Nova (#1178)
Date Thu, 15 Feb 2018 10:43:58 GMT
nacx commented on this pull request.

Thanks @andreaturli!

>  
          try {
-            network = networkApi.create(
-                  Network.createBuilder("jclouds-network-test").external(true).networkType(NetworkType.LOCAL).build());
-            assertNotNull(network);
-
-            ipv4SubnetId = subnetApi.create(Subnet.createBuilder(network.getId(), "198.51.100.0/24").ipVersion(4)
-                  .name("JClouds-Live-IPv4-Subnet").build()).getId();
+            network = networkApi.list().concat().firstMatch(new Predicate<Network>()
{
+               @Override
+               public boolean apply(@Nullable Network input) {
+                  return input.getExternal();

If input is nullable better guard against a NPE

> +         }
+      });
+      return floatingIPOptional;
+   }
+
+   private org.jclouds.openstack.neutron.v2.domain.FloatingIP createFloatingIpUsingNeutron(org.jclouds.openstack.neutron.v2.features.FloatingIPApi
neutronFloatingApi, NodeMetadata node, final String availabilityZone) {
+      String regionId = node.getLocation().getParent().getId();
+
+      Optional<Network> networkOptional = getNetworkApi(regionId).list().concat().firstMatch(new
Predicate<Network>() {
+         @Override
+         public boolean apply(@Nullable Network input) {
+            return input.getExternal() && input.getAvailabilityZone().equals(availabilityZone);
+         }
+      });
+      if (!networkOptional.isPresent())
+         throw new InsufficientResourcesException("Failed to find a suitable external network.");

To be more close to the old behavior, instead of looking for *any* external network, could
we use the existing `poolNames` configuration to filter external networks by name? This way
users will configure the same name `where` they want the floating IPs from, regardless of
the API they are using.

> +      Optional<Network> networkOptional = getNetworkApi(regionId).list().concat().firstMatch(new
Predicate<Network>() {
+         @Override
+         public boolean apply(@Nullable Network input) {
+            return input.getExternal() && input.getAvailabilityZone().equals(availabilityZone);
+         }
+      });
+      if (!networkOptional.isPresent())
+         throw new InsufficientResourcesException("Failed to find a suitable external network.");
+
+      org.jclouds.openstack.neutron.v2.domain.FloatingIP createFloatingIP = org.jclouds.openstack.neutron.v2.domain.FloatingIP.CreateFloatingIP
+              .createBuilder(networkOptional.get().getId())
+              .availabilityZone(networkOptional.get().getAvailabilityZone())
+              .build();
+
+      org.jclouds.openstack.neutron.v2.domain.FloatingIP floatingIP = neutronFloatingApi.create((org.jclouds.openstack.neutron.v2.domain.FloatingIP.CreateFloatingIP)
createFloatingIP);
+      floatingIpCache.asMap().putIfAbsent(RegionAndId.fromSlashEncoded(node.getId()), ImmutableList.of(FloatingIpForServer.create(RegionAndId.fromSlashEncoded(node.getId()),
floatingIP.getId(), floatingIP.getFloatingIpAddress())));

Why put if absent? Shouldn't we override the old value, if any?

> +               Optional<org.jclouds.openstack.neutron.v2.domain.FloatingIP> floatingIPOptional
= tryFindExistingFloatingIp(neutronFloatingApi, availabilityZone);
+               org.jclouds.openstack.neutron.v2.domain.FloatingIP floatingIP;
+               if (floatingIPOptional.isPresent()) {
+                  floatingIP = floatingIPOptional.get();
+               } else {
+                  floatingIP = createFloatingIpUsingNeutron(neutronFloatingApi, node, availabilityZone);
+               }
+
+               org.jclouds.openstack.neutron.v2.domain.FloatingIP ip = neutronFloatingApi.update(floatingIP.getId(),
+                       org.jclouds.openstack.neutron.v2.domain.FloatingIP.UpdateFloatingIP
+                               .updateBuilder()
+                               .portId(optionalPort.get().getId())
+                               .build());
+
+               input.get().getNodeMetadata().set(NodeMetadataBuilder.fromNodeMetadata(node).publicAddresses(ImmutableSet.of(ip.getFloatingIpAddress())).build());
+            }

If the node has no ports associated (although unlikely) we'd better at least log a warning
instead of silently doing nothing.

> +import org.jclouds.openstack.nova.v2_0.domain.FloatingIP;
+
+import com.google.common.base.Function;
+import com.google.inject.assistedinject.Assisted;
+
+public class NeutronFloatingIpToFloatingIp
+      implements Function<org.jclouds.openstack.neutron.v2.domain.FloatingIP, FloatingIP>
{
+
+   public interface Factory {
+      NeutronFloatingIpToFloatingIp create(Location location);
+   }
+
+   private final Location location;
+
+   @Inject
+   public NeutronFloatingIpToFloatingIp(@Assisted Location location) {

Remove the public modifier

> +   }
+
+   private final Location location;
+
+   @Inject
+   public NeutronFloatingIpToFloatingIp(@Assisted Location location) {
+      this.location = location;
+   }
+
+   @Override
+   public FloatingIP apply(@Nullable org.jclouds.openstack.neutron.v2.domain.FloatingIP floatingIP)
{
+      FloatingIP.Builder<?> builder = FloatingIP.builder();
+      builder.id(floatingIP.getId());
+      builder.ip(floatingIP.getFloatingIpAddress());
+      builder.fixedIp(floatingIP.getFixedIpAddress());
+      builder.instanceId(floatingIP.getPortId());

Isn't `instanceId` the ID of the node? In that case, the value should be the `deviceId` of
the port; not the id of the port itself.

> + * limitations under the License.
+ */
+package org.jclouds.openstack.nova.v2_0.domain;
+
+import com.google.auto.value.AutoValue;
+import org.jclouds.json.SerializedNames;
+import org.jclouds.openstack.nova.v2_0.domain.regionscoped.RegionAndId;
+
+@AutoValue
+public abstract class FloatingIpForServer {
+
+   public abstract RegionAndId serverId();
+   public abstract String floatingIpId();
+   public abstract String ip();
+
+   @SerializedNames({"serverId", "floatingIpId", "ip" })

This object is not meant to be serialized to json. Remove this annotation.

> @@ -51,7 +78,66 @@ protected LoggingModule getLoggingModule() {
 
    @Override
    protected Iterable<Module> setupModules() {
-      return ImmutableSet.of(getLoggingModule(), credentialStoreModule, getSshModule());
+      return ImmutableSet.<Module> of(
+              ContextLinking.linkContext(neutronApiContext),
+              getLoggingModule(),
+              credentialStoreModule,
+              getSshModule()
+      );
+   }
+
+   @Override
+   protected TemplateBuilder templateBuilder() {

Remove thsi test with cloudsoft specific stuff?

>        }
    }
 
+   @Override
+   public void testAScriptExecutionAfterBootWithBasicTemplate() throws Exception {
+      super.testAScriptExecutionAfterBootWithBasicTemplate();
+   }

Remove this

> @@ -51,7 +78,66 @@ protected LoggingModule getLoggingModule() {
 
    @Override
    protected Iterable<Module> setupModules() {
-      return ImmutableSet.of(getLoggingModule(), credentialStoreModule, getSshModule());
+      return ImmutableSet.<Module> of(
+              ContextLinking.linkContext(neutronApiContext),
+              getLoggingModule(),
+              credentialStoreModule,
+              getSshModule()
+      );
+   }

With this config we will *always* test nova+neutron, but not the path of only Nova. I think
we should be able to still live test the old path for legacy environments.

-- 
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/pull/1178#pullrequestreview-96796788
Mime
View raw message