jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrea Turli <notificati...@github.com>
Subject Re: [jclouds/jclouds-labs] JCLOUDS-1386 1&1 Baremetal servers (#431)
Date Wed, 21 Mar 2018 16:31:55 GMT
andreaturli requested changes on this pull request.

great start @alibazlamit! I've only some comments, feel free to ask for clarification!

As I don't have access to oneandone it would be nice if you could attach the results of live
tests before merging this. Thanks

> @@ -65,6 +66,7 @@
    private final OneAndOneApi api;
    private final Predicate<Server> waitServerUntilAvailable;
    private final PasswordGenerator.Config passwordGenerator;

it seems not used

> -            //check if the bootable device has enough size to run the appliance(image).
-            float minHddSize = volume.getSize();
-            if (volume.isBootDevice()) {
-               SingleServerAppliance appliance = api.serverApplianceApi().get(image.getId());
-               if (appliance.minHddSize() > volume.getSize()) {
-                  minHddSize = appliance.minHddSize();
-               }
-            }
-            Hdd.CreateHdd hdd = Hdd.CreateHdd.create(minHddSize, volume.isBootDevice());
-            hdds.add(hdd);
-         } catch (Exception ex) {
-            throw Throwables.propagate(ex);
-
+      String imageId = image.getId();
+      HardwareFlavour hardwareModel = isFlavor(hardware.getId());
+      boolean isBaremetal = hardware.getName() == null ? false : hardware.getName().contains(baremetalModelsKey);

I think worth introducing a `OneandoneTemplateOptions extends TemplateOptions` to deal with
specific requirements a user may specify like bare metal or even `hardwareModel` above. wdyt?

>        try {
-         org.apache.jclouds.oneandone.rest.domain.Hardware.CreateHardware hardwareRequest
-                 = org.apache.jclouds.oneandone.rest.domain.Hardware.CreateHardware.create(cores,
1, ram, hdds);
+         if (hardwareModel != null) {

this logic to build the hardwareRequest is quite complex. would it make sense to extract it
in its own method, and add some unit tests, ideally

>     @Override
    public List<HardwareFlavour> listHardwareProfiles() {
-      return api.serverApi().listHardwareFlavours();
+      List<HardwareFlavour> result = new ArrayList<HardwareFlavour>();
+      List<HardwareFlavour> cloudModels = api.serverApi().listHardwareFlavours();
+      for (HardwareFlavour flavor : cloudModels) {
+         result.add(flavor);
+      }
+      List<BareMetalModel> baremetalModels = api.serverApi().listBaremetalModels();
+      for (BareMetalModel model : baremetalModels) {

maybe you can consider a `Function<BareMetalModel, HardwareFlavour>` for better readability
and testability?

> @@ -125,4 +126,28 @@ public boolean apply(Server server) {
                  || server.status().percent() != 0);
       }
    }
+
+   @Provides
+   @Singleton
+   @Named(POLL_PREDICATE_SERVER_SUSPENDED)

why a not using `ComputeServiceProperties.TIMEOUT_NODE_SUSPENDED` so that you can use a quite
adopted pattern like in openstack,digitalocean,packet
```
   @Provides
   @com.google.inject.name.Named(TIMEOUT_NODE_TERMINATED)
   protected Predicate<RegionAndId> provideServerTerminatedPredicate(final NovaApi api,
ComputeServiceConstants.Timeouts timeouts,
                                                                ComputeServiceConstants.PollPeriod
pollPeriod) {
      return retry(new ServerTerminatedPredicate(api), timeouts.nodeTerminated, pollPeriod.pollInitialPeriod,
              pollPeriod.pollMaxPeriod);
   }
```
?

> @@ -53,7 +53,7 @@ public Hardware apply(HardwareFlavour from) {
       }
       final HardwareBuilder builder;
       builder = new HardwareBuilder()
-              .ids(from.id())
+              .ids(from.name())

maybe better to use 
```
.id(from.id())
.name(from.name())
```
if `.ids()` is not what you want here

> @@ -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_SUSPENDED = "jclouds.oneandone.rest.predicate.serverdelete";

oh now I see, it's a predicate to wait for server deletion not really suspension. ok maybe
my comment above is still valid for other predicates though

> +   public static BareMetalModel create(String id, String name, Hardware hardware) {
+      return new AutoValue_BareMetalModel(id, name, hardware);
+   }
+
+   @AutoValue
+   public abstract static class Hardware {
+
+      public abstract double core();
+
+      public abstract double coresPerProcessor();
+
+      public abstract double ram();
+
+      public abstract String unit();
+
+      public abstract Hdd hdds();

out of curiosity, why it's `hdds` if it is not a collection?

-- 
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/431#pullrequestreview-105791803
Mime
View raw message