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 Thu, 12 Jan 2017 22:04:27 GMT
nacx requested changes on this pull request.

I've added some more comments. Regarding the live test failure, could you share the complete
stacktrace?

> @@ -67,7 +78,8 @@ protected Builder() {
                  .apiMetadata(new OneAndOneApiMetadata())
                  .homepage(URI.create("https://cloudpanel-api.1and1.com"))
                  .console(URI.create("https://account.1and1.com"))
-                 .endpoint("https://cloudpanel-api.1and1.com/v1")
+                 .iso3166Codes("de", "us", "es", "gb")

This should contain the iso codes, not the region id.

> +
+      // provision server
+      final Server server;
+      Double cores = ComputeServiceUtils.getCores(hardware);
+
+      try {
+         List<? extends Processor> processors = hardware.getProcessors();
+         org.apache.jclouds.oneandone.rest.domain.Hardware.CreateHardware hardwareRequest
+                 = org.apache.jclouds.oneandone.rest.domain.Hardware.CreateHardware.create(processors.size(),
cores, hardware.getRam(), hdds);
+         final Server.CreateServer serverRequest = Server.CreateServer.builder()
+                 .name(name)
+                 .hardware(hardwareRequest)
+                 .rsaKey(options.getPublicKey())
+                 .applianceId(image.getId())
+                 .dataCenterId(dataCenterId)
+                 .powerOn(Boolean.TRUE).build();

Set the password if the credentials are password-based?

> +      
+      //if no private key was set use the set/generated password.
+      if (privateKey == null) {
+         serverCredentials = LoginCredentials.builder()
+                 .user(loginUser)
+                 .password(password)
+                 .build();
+      } else {
+         serverCredentials = LoginCredentials.builder()
+                 .user(loginUser)
+                 .privateKey(privateKey)
+                 .build();
+
+      }
+
+      return new NodeAndInitialCredentials<Server>(server, server.id(), serverCredentials);

In this case there is no need to set the credentials manually here. jclouds will populate
the `defaultCredentials` for the image, or the ones provided in the template options automatically.
Just pass `null`.

Regarding the default credentials for the images, do they come with some default user and
a default password? Or are cloud-init images or similar that create users and passwords on-the-fly?

> +   }
+
+   @Override
+   public void rebootNode(String id) {
+      waitServerUntilAvailable.apply(getNode(id));
+      api.serverApi().updateStatus(id, Server.UpdateStatus.create(Types.ServerAction.REBOOT,
Types.ServerActionMethod.HARDWARE));
+   }
+
+   @Override
+   public void resumeNode(String id) {
+      api.serverApi().updateStatus(id, Server.UpdateStatus.create(Types.ServerAction.POWER_ON,
Types.ServerActionMethod.HARDWARE));
+   }
+
+   @Override
+   public void suspendNode(String id) {
+      waitServerUntilRunning.apply(getNode(id));

What if we call this on an already suspended node? Will it block until it times out?
Perhaps it is a better idea to validate the state as a precondition and fail, or not validate
it at all and let the it fail in the server side.

> +
+      public ServerAvailablePredicate(OneAndOneApi api) {
+         this.api = checkNotNull(api, "api must not be null");
+      }
+
+      @Override
+      public boolean apply(Server server) {
+         checkNotNull(server, "Server");
+         server = api.serverApi().get(server.id());
+         if ((server.status().state() != Types.ServerState.POWERED_OFF
+                 && server.status().state() != Types.ServerState.POWERED_ON)
+                 || server.status().percent() != 0) {
+            return false;
+         } else {
+            return true;
+         }

Try to avoid conditionals like `if (condition) return true else return false`. Just `return
condition`.

> +
+         checkNotNull(serverRef, "serverRef");
+         //give time for the operation to actually start
+         Uninterruptibles.sleepUninterruptibly(10, TimeUnit.SECONDS);
+         Server server = api.serverApi().get(serverRef.id());
+
+         if (server == null) {
+            return false;
+         }
+         return server.status().toString().equals(expectedState.toString());
+      }
+
+   }
+
+   @Singleton
+   public static class ComputeConstants {

Get rid of this class and use the existing [PollPeriod](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/reference/ComputeServiceConstants.java#L73-L82)
and [Timeouts](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/reference/ComputeServiceConstants.java#L94-L105).

> +      }).to(OneandoneComputeServiceAdapter.class);
+
+      bind(TemplateBuilderImpl.class).to(ArbitraryCpuRamTemplateBuilderImpl.class);
+
+      bind(new TypeLiteral<Function<Server, NodeMetadata>>() {
+      }).to(ServerToNodeMetadata.class);
+
+      bind(new TypeLiteral<Function<org.apache.jclouds.oneandone.rest.domain.Image,
Image>>() {
+      }).to(ImageToImage.class);
+
+      bind(new TypeLiteral<Function<Hdd, Volume>>() {
+      }).to(HddToVolume.class);
+
+      bind(new TypeLiteral<Function<HardwareFlavour, Hardware>>() {
+      }).to(HardwareFlavourToHardware.class);
+   }

If the public key configured by the user can be injected using the API (as it seems), then
add [this binding](https://github.com/jclouds/jclouds/blob/master/providers/google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/config/GoogleComputeEngineServiceContextModule.java#L121),
so jclouds does not try to upload it as part of a bootstrap ssh script.

> +         HardwareFlavour flavour = api.serverApi().getHardwareFlavour(server.hardware().fixedInstanceSizeId());
+         hardware = fnHardware.apply(flavour);
+
+      } else {
+         //customer hardware
+         double size = 0d;
+         List<Volume> volumes = Lists.newArrayList();
+         List<Hdd> storages = server.hardware().hdds();
+         if (storages != null) {
+            for (Hdd storage : storages) {
+               size += storage.size();
+               volumes.add(fnVolume.apply(storage));
+            }
+         }
+         // Build hardware
+         String id = String.format("cpu=%f,ram=%d,disk=%f", server.hardware().coresPerProcessor(),
(int) server.hardware().ram(), size);

Generate the ID with [this](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/util/AutomaticHardwareIdSpec.java#L60).

> @@ -19,6 +19,7 @@
 public final class OneAndOneProperties {
 
    public static final String POLL_PREDICATE_SERVER = "jclouds.oneandone.rest.predicate.server";
+   public static final String POLL_PREDICATE_SERVER_RUNNING = "jclouds.oneandone.rest.predicate.serveron
  ";

Remove trailing spaces

> +
+   public String getDescription() {
+      return description;
+   }
+
+   public static Location fromValue(String v) {
+      return Location.fromId(v);
+   }
+
+   public static Location fromId(String id) {
+      for (Location location : values()) {
+         if (location.id.equals(id)) {
+            return location;
+         }
+      }
+      return US;

Better throw an `IllegalArgumentException` instead.

-- 
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-16452526
Mime
View raw message