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-labs] Oneandone compute api (#338)
Date Fri, 27 Jan 2017 13:43:19 GMT
nacx requested changes on this pull request.



> @@ -66,6 +69,7 @@ public static Properties defaultProperties() {
       properties.put(POLL_MAX_PERIOD, 2L * 10L);
       properties.put(PROPERTY_SO_TIMEOUT, 60000 * 5);
       properties.put(PROPERTY_CONNECTION_TIMEOUT, 60000 * 5);
+      properties.put(PROPERTY_MAX_RATE_LIMIT_WAIT, 20);

Note that the value is in milliseconds, so under a rate limited environment this is likely
to have no effect.

>           logger.trace(">> provisioning complete for server. returned id='%s'",
server.id());
 
       } catch (Exception ex) {
          logger.error(ex, ">> failed to provision server. rollbacking..");
          throw Throwables.propagate(ex);
       }
 
-      return new NodeAndInitialCredentials<Server>(server, server.id(), null);
+      LoginCredentials serverCredentials = LoginCredentials.builder()
+              .user(loginUser)
+              .password(password)
+              .privateKey(privateKey)
+              .build();

Remove this and don't pass the initial credentials (now it is unused). We can discuss the
details of the node access once everything else is in place.

>     }
 
    @Override
-   public List<HardwareFlavour> listHardwareProfiles() {
-      return api.serverApi().listHardwareFlavours();
+   public List<Hardware> listHardwareProfiles() {

Why it is now a hardcoded list instead of calling the API?

> -         logger.trace(">> found image [%s].", image.name());
-         return image;
+      GenericQueryOptions options = new GenericQueryOptions();
+      options.options(0, 0, null, id, null);
+      try {
+         List<ServerAppliance> list = api.serverApplianceApi().list(options);
+         if (list.size() > 0) {
+            ServerAppliance appliance = list.get(0);
+            List<SingleServerAppliance.AvailableDataCenters> availableDatacenters =
new ArrayList<SingleServerAppliance.AvailableDataCenters>();
+            for (String dcId : appliance.availableDataCenters()) {
+               availableDatacenters.add(SingleServerAppliance.AvailableDataCenters.create(dcId,
""));
+            }
+            SingleServerAppliance image = SingleServerAppliance.create(appliance.id(), appliance.name(),
availableDatacenters, appliance.osInstallationBase(),
+                    appliance.osFamily(), appliance.os(), appliance.osVersion(), appliance.osArchitecture(),
appliance.osImageType(), appliance.minHddSize(),
+                    appliance.type(), appliance.state(), appliance.version(), appliance.categories(),
appliance.eulaUrl());
+            if (image != null) {

This check is redundant.

>        logger.debug("Destroying %s ...", id);
+      try {
+         GenericQueryOptions options = new GenericQueryOptions().options(0, 0, null, node.name()
+ " firewall policy", null);
+         List<FirewallPolicy> firewallRules = api.firewallPolicyApi().list(options);
+         for (FirewallPolicy firewallRule : firewallRules) {
+            api.firewallPolicyApi().delete(firewallRule.id());
+         }
+      } catch (Exception ex) {
+         logger.debug("no firewall policies found for %s ...", id);
+      }

The logic to delete the firewalls should be better moved to a `OneAndOneComputeService` class
that implements the `cleanUpIncidentalResourcesOfDeadNodes` method. Thake the [azurecompute-arm
implementation](https://github.com/jclouds/jclouds-labs/blob/master/azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/AzureComputeService.java#L103)
as an example.

> +            return OsFamily.UBUNTU;
+         }
+
+         if (osFamily.toLowerCase().contains("debian")) {
+            return OsFamily.DEBIAN;
+         }
+
+         if (osFamily.toLowerCase().contains("centos")) {
+            return OsFamily.CENTOS;
+         }
+
+         if (osFamily.toLowerCase().contains("ArchLinux")) {
+            return OsFamily.ARCH;
+         }
+      }
+      return OsFamily.UNRECOGNIZED;

Better do something like https://github.com/jclouds/jclouds-labs/pull/353 to consider all
known OsFamilies.

> +   @Override
+   public Map<?, ListenableFuture<Void>> execute(String group, int count, final
Template template,
+           Set<NodeMetadata> goodNodes, Map<NodeMetadata, Exception> badNodes,
+           Multimap<NodeMetadata, CustomizationResponse> customizationResponses) {
+
+      logger.info(">> looking for a datacenter in %s", template.getLocation().getId());
+
+      // Try to find an existing datacenter in the selected location
+      DataCenter dataCenter = find(api.dataCenterApi().list(), new Predicate<DataCenter>()
{
+         @Override
+         public boolean apply(DataCenter input) {
+            // The location field is not populated when getting the list of datacenters
+            DataCenter details = api.dataCenterApi().get(input.id());
+            return details != null && template.getLocation().getId().equals((details.countryCode().toLowerCase()));
+         }
+      }, null);

Don't use `null` as a default value and let it fail, since it is mandatory.

> +import com.google.inject.Inject;
+import javax.annotation.Resource;
+import javax.inject.Named;
+import static org.jclouds.Constants.PROPERTY_MAX_RATE_LIMIT_WAIT;
+import static org.jclouds.Constants.PROPERTY_MAX_RETRIES;
+import org.jclouds.http.HttpCommand;
+import org.jclouds.http.HttpResponse;
+import org.jclouds.http.HttpRetryHandler;
+import org.jclouds.logging.Logger;
+
+/**
+ * Retry handler that takes into account the provider rate limit and delays the
+ * requests until they are known to succeed.
+ */
+@Beta
+public abstract class AliRateLimitRetryHandler implements HttpRetryHandler {

Remove this class

-- 
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/338#pullrequestreview-18837311
Mime
View raw message